Merge lp:~cjwatson/launchpad/garbo-archivepermission-duplicates into lp:launchpad

[Colin Watson]

Remove duplicate rows from ArchivePermission, created due to bug 887185.

== 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.

[Francesco Banconi (frankban)]

Thanks Colin, this branch looks good.
It seems the tests contain some repeated code (to create ArchivePermissions), but I think that's ok since the job will be removed.
Tests pass, and you said on IRC that you checked the SQL fragment by hand on dogfood.

review: Approve (code*)

[Raphaël Badin (rvb)]

Thanks for the cleanup!

The test code could probably be more compact, but, like Francesco noted, this doesn't matter much since this will be ditched eventually.

I didn't know about window functions, I've always used regular aggregate functions to do that kind of stuff. Thanks Colin!

review: Approve

[Colin Watson (cjwatson)]

Right. Some of the repeated code was necessary to deliberately avoid
guards against duplication in the ArchivePermission model; the rest was
just so that the tests were more or less regular.

=== 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