launchpad-reviewers team mailing list archive

[Andrey Fedoseev <mp+428250@xxxxxxxxxxxxxxxxxx>]

Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:close-account into launchpad:master.

Commit message:
close-account: handle the referenced deleted PPAs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/428250

If a deleted PPA is not referenced, or the references allows cascade deletion, it gets purged. Otherwise, it is ignored.

Non-deleted PPAs still block account closing.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:close-account into launchpad:master.

diff --git a/lib/lp/registry/scripts/closeaccount.py b/lib/lp/registry/scripts/closeaccount.py
index 8f24d6f..7a21c56 100644
--- a/lib/lp/registry/scripts/closeaccount.py
+++ b/lib/lp/registry/scripts/closeaccount.py
@@ -11,7 +11,6 @@ __all__ = [
 from typing import List, Tuple
 
 import six
-from storm.exceptions import IntegrityError
 from storm.expr import And, LeftJoin, Lower, Or
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -409,26 +408,60 @@ def close_account(username, log):
         (person.id,),
     )
 
-    # Purge deleted PPAs.  This is safe because the archive can only be in
-    # the DELETED status if the publisher has removed it from disk and set
-    # all its publications to DELETED.
-    # XXX cjwatson 2019-08-09: This will fail if anything non-trivial has
-    # been done in this person's PPAs; and it's not obvious what to do in
-    # more complicated cases such as builds having been copied out
-    # elsewhere.  It's good enough for some simple cases, though.
-    try:
+    # Purge deleted PPAs that aren't referenced anywhere.
+    # This is safe because the archive can only be in the DELETED status if
+    # the publisher has removed it from disk and set all its publications
+    # to DELETED.
+    # Deleted PPAs that are still referenced are ignored.
+    deleted_ppa_ids = set(
         store.find(
-            Archive,
+            Archive.id,
             Archive.owner == person,
             Archive.status == ArchiveStatus.DELETED,
-        ).remove()
-    except IntegrityError:
-        raise LaunchpadScriptFailure(
-            "Can't delete non-trivial PPAs for user %s" % person_name
         )
+    )
+    ppa_references = list(postgresql.listReferences(cur, "archive", "id"))
+    referenced_ppa_ids = set()
+    for ppa_id in deleted_ppa_ids:
+        for src_tab, src_col, *_, delete_action in ppa_references:
+            if delete_action == "c":
+                # cascade deletion is enabled, so the reference may be ignored
+                continue
+            result = store.execute(
+                """
+                SELECT COUNT(*) FROM %(src_tab)s WHERE %(src_col)s = ?
+                """
+                % {
+                    "src_tab": src_tab,
+                    "src_col": src_col,
+                },
+                (ppa_id,),
+            )
+            count = result.get_one()[0]
+            if count:
+                referenced_ppa_ids.add(ppa_id)
+                reference = "{}.{}".format(src_tab, src_col)
+                log.warning(
+                    "PPA %d is still referenced by %d %s values"
+                    % (ppa_id, count, reference)
+                )
+
+    non_referenced_ppa_ids = deleted_ppa_ids - referenced_ppa_ids
+    if non_referenced_ppa_ids:
+        store.find(Archive, Archive.id.is_in(non_referenced_ppa_ids)).remove()
 
     reference_counts = []  # type: List[Tuple[str, int]]
 
+    # Check for non-deleted PPAs
+    count = store.find(
+        Archive,
+        Archive.owner == person,
+        Archive.status != ArchiveStatus.DELETED,
+    ).count()
+    if count:
+        reference_counts.append(("archive.owner", count))
+    skip.add(("archive", "owner"))
+
     # Check for active related projects, and skip inactive ones.
     for col in "bug_supervisor", "driver", "owner":
         # Raw SQL because otherwise using Product._owner while displaying it
@@ -549,7 +582,7 @@ def close_account(username, log):
     # by this point.  If not, it's safer to bail out.  It's OK if this
     # doesn't work in all conceivable situations, since some of them may
     # require careful thought and decisions by a human administrator.
-    for src_tab, src_col, ref_tab, ref_col, updact, delact in references:
+    for src_tab, src_col, *_ in references:
         if (src_tab, src_col) in skip:
             continue
         result = store.execute(
diff --git a/lib/lp/registry/scripts/tests/test_closeaccount.py b/lib/lp/registry/scripts/tests/test_closeaccount.py
index 120328c..7a13f9d 100644
--- a/lib/lp/registry/scripts/tests/test_closeaccount.py
+++ b/lib/lp/registry/scripts/tests/test_closeaccount.py
@@ -965,11 +965,10 @@ class TestCloseAccount(TestCaseWithFactory):
         )
         self.assertNotRemoved(account_id, person_id)
 
-    def test_fails_on_deleted_ppa_with_builds(self):
-        # XXX cjwatson 2019-08-09: A PPA that has ever had builds can't
-        # currently be purged.  It's not clear what to do about this case.
+    def test_ignores_deleted_ppa_with_builds(self):
         person = self.factory.makePerson()
         ppa = self.factory.makeArchive(owner=person)
+        ppa_id = ppa.id
         self.factory.makeBinaryPackageBuild(archive=ppa)
         ppa.delete(person)
         Publisher(
@@ -979,15 +978,11 @@ class TestCloseAccount(TestCaseWithFactory):
         account_id = person.account.id
         script = self.makeScript([person.name])
         with dbuser("launchpad"):
-            self.assertRaisesWithContent(
-                LaunchpadScriptFailure,
-                "Can't delete non-trivial PPAs for user %s" % person.name,
-                self.runScript,
-                script,
-            )
-        self.assertNotRemoved(account_id, person_id)
+            self.runScript(script)
+        self.assertRemoved(account_id, person_id)
+        self.assertEqual(ppa, Store.of(ppa).get(Archive, ppa_id))
 
-    def test_handles_empty_deleted_ppa(self):
+    def test_purges_empty_deleted_ppa(self):
         person = self.factory.makePerson()
         ppa = self.factory.makeArchive(owner=person)
         ppa_id = ppa.id