launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/garbo-archivepermission-duplicates into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1025441 in Launchpad itself: "Please clean up the duplicates from archivepermission"
  https://bugs.launchpad.net/launchpad/+bug/1025441

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/garbo-archivepermission-duplicates/+merge/115554

== Summary ==

There are some duplicated rows in ArchivePermission left over from an old (fixed) bug.  These get in the way because Archive.removePackagesetUploader uses .one().

== Proposed fix ==

Add a BulkPruner-based garbo job to get rid of the duplicates.  Previous analysis showed that the only problems have non-NULL packageset and duplicate person, archive, packageset, permission, so we'll get rid of all but the first id for all such groups.

== LOC Rationale ==

+99, but it will all be removed again once the job has completed.

== Tests ==

bin/test -vvct test_DuplicateArchivePermissionPruner

== Demo and Q/A ==

dogfood's database is already in an appropriately broken state.  Dump the archivepermission table, run the garbo job, and dump it again, and then verify that all the removed IDs (there should be 190, I believe) correspond to duplicates and that exactly one of each duplicated group remains.
-- 
https://code.launchpad.net/~cjwatson/launchpad/garbo-archivepermission-duplicates/+merge/115554
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/garbo-archivepermission-duplicates into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-07-17 06:21:47 +0000
+++ database/schema/security.cfg	2012-07-18 13:53:21 +0000
@@ -2201,6 +2201,7 @@
 [garbo]
 groups=script,read
 public.account                          = SELECT, DELETE
+public.archivepermission                = SELECT, DELETE
 public.answercontact                    = SELECT, DELETE
 public.branch                           = SELECT, UPDATE
 public.branchjob                        = SELECT, DELETE

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-07-10 06:57:47 +0000
+++ lib/lp/scripts/garbo.py	2012-07-18 13:53:21 +0000
@@ -93,6 +93,7 @@
     MAIN_STORE,
     MASTER_FLAVOR,
     )
+from lp.soyuz.model.archivepermission import ArchivePermission
 from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.model.potmsgset import POTMsgSet
 from lp.translations.model.potranslation import POTranslation
@@ -990,6 +991,23 @@
         transaction.commit()
 
 
+class DuplicateArchivePermissionPruner(BulkPruner):
+    """Cleans up duplicate ArchivePermission rows created by bug 887185."""
+    target_table_class = ArchivePermission
+    ids_to_prune_query = """
+        SELECT id FROM (
+            SELECT id, rank() OVER w AS rank
+            FROM ArchivePermission
+            WHERE packageset IS NOT NULL
+            WINDOW w AS (
+                PARTITION BY person, archive, packageset, permission
+                ORDER BY id
+                )
+            ) AS whatever
+        WHERE rank > 1
+        """
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -1267,6 +1285,7 @@
         BugWatchActivityPruner,
         CodeImportEventPruner,
         CodeImportResultPruner,
+        DuplicateArchivePermissionPruner,
         HWSubmissionEmailLinker,
         LoginTokenPruner,
         ObsoleteBugAttachmentPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-07-10 07:08:54 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-07-18 13:53:21 +0000
@@ -97,6 +97,9 @@
     MASTER_FLAVOR,
     )
 from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.soyuz.enums import ArchivePermissionType
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
+from lp.soyuz.model.archivepermission import ArchivePermission
 from lp.testing import (
     person_logged_in,
     TestCase,
@@ -1016,6 +1019,82 @@
         self.runHourly()
         self.assertNotEqual(old_update, naked_bug.heat_last_updated)
 
+    def test_DuplicateArchivePermissionPruner_removes_duplicate_rows(self):
+        # DuplicateArchivePermissionPruner removes duplicated packageset
+        # permissions.
+        switch_dbuser('testadmin')
+        archive = self.factory.makeArchive()
+        person = self.factory.makePerson()
+        packageset = self.factory.makePackageset()
+        for _ in range(3):
+            ArchivePermission(
+                archive=archive, person=person, packageset=packageset,
+                permission=ArchivePermissionType.UPLOAD)
+        ap_set = getUtility(IArchivePermissionSet)
+        self.assertEqual(
+            3, ap_set.uploadersForPackageset(archive, packageset).count())
+        self.runDaily()
+        self.assertEqual(
+            1, ap_set.uploadersForPackageset(archive, packageset).count())
+
+    def test_DuplicateArchivePermissionPruner_skips_unique_rows(self):
+        # DuplicateArchivePermissionPruner leaves unique packageset
+        # permissions alone.
+        switch_dbuser('testadmin')
+        archive_one = self.factory.makeArchive()
+        archive_two = self.factory.makeArchive()
+        person_one = self.factory.makePerson()
+        person_two = self.factory.makePerson()
+        packageset_one = self.factory.makePackageset()
+        packageset_two = self.factory.makePackageset()
+        for archive, person, packageset in (
+            (archive_one, person_one, packageset_one),
+            (archive_two, person_one, packageset_one),
+            (archive_one, person_two, packageset_one),
+            (archive_one, person_one, packageset_two),
+            ):
+            ArchivePermission(
+                archive=archive, person=person, packageset=packageset,
+                permission=ArchivePermissionType.UPLOAD)
+        ap_set = getUtility(IArchivePermissionSet)
+        self.assertEqual(
+            2,
+            ap_set.uploadersForPackageset(archive_one, packageset_one).count())
+        self.assertEqual(
+            1,
+            ap_set.uploadersForPackageset(archive_two, packageset_one).count())
+        self.assertEqual(
+            1,
+            ap_set.uploadersForPackageset(archive_one, packageset_two).count())
+        self.runDaily()
+        self.assertEqual(
+            2,
+            ap_set.uploadersForPackageset(archive_one, packageset_one).count())
+        self.assertEqual(
+            1,
+            ap_set.uploadersForPackageset(archive_two, packageset_one).count())
+        self.assertEqual(
+            1,
+            ap_set.uploadersForPackageset(archive_one, packageset_two).count())
+
+    def test_DuplicateArchivePermissionPruner_skips_non_packagesets(self):
+        # DuplicateArchivePermissionPruner leaves non-packageset permissions
+        # alone.
+        switch_dbuser('testadmin')
+        archive = self.factory.makeArchive()
+        person = self.factory.makePerson()
+        component = self.factory.makeComponent()
+        for _ in range(3):
+            ArchivePermission(
+                archive=archive, person=person, component=component,
+                permission=ArchivePermissionType.UPLOAD)
+        ap_set = getUtility(IArchivePermissionSet)
+        self.assertEqual(
+            3, ap_set.uploadersForComponent(archive, component).count())
+        self.runDaily()
+        self.assertEqual(
+            3, ap_set.uploadersForComponent(archive, component).count())
+
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer