launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/remove-unused-sharing-garbo into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1036189 in Launchpad itself: "Unused sharing policies litter the db tables"
  https://bugs.launchpad.net/launchpad/+bug/1036189

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/remove-unused-sharing-garbo/+merge/120698

Add a daily garbo job that will trawl all products and remove all access policies that are unused. I've tried to minimize the queries used, and make use of the accesspolicy methods.
-- 
https://code.launchpad.net/~stevenk/launchpad/remove-unused-sharing-garbo/+merge/120698
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/remove-unused-sharing-garbo into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-08-17 07:09:39 +0000
+++ database/schema/security.cfg	2012-08-22 05:07:34 +0000
@@ -2219,6 +2219,8 @@
 
 [garbo]
 groups=script,read
+public.accesspolicy                     = SELECT, DELETE
+public.accesspolicygrant                = SELECT, DELETE
 public.account                          = SELECT, DELETE
 public.answercontact                    = SELECT, DELETE
 public.branch                           = SELECT, UPDATE

=== modified file 'lib/lp/code/model/branchnamespace.py'
--- lib/lp/code/model/branchnamespace.py	2012-08-20 13:26:21 +0000
+++ lib/lp/code/model/branchnamespace.py	2012-08-22 05:07:34 +0000
@@ -8,6 +8,7 @@
     'BranchNamespaceSet',
     'PackageNamespace',
     'PersonalNamespace',
+    'POLICY_ALLOWED_TYPES',
     'ProductNamespace',
     ]
 

=== modified file 'lib/lp/registry/interfaces/accesspolicy.py'
--- lib/lp/registry/interfaces/accesspolicy.py	2012-07-19 03:18:37 +0000
+++ lib/lp/registry/interfaces/accesspolicy.py	2012-08-22 05:07:34 +0000
@@ -229,6 +229,9 @@
             pairs.
         """
 
+    def revokeByPolicy(policies):
+        """Revoke all `IAccessPolicyGrant` for the policies."""
+
 
 class IAccessPolicyGrantFlatSource(Interface):
     """Experimental query utility to search through the flattened schema."""

=== modified file 'lib/lp/registry/model/accesspolicy.py'
--- lib/lp/registry/model/accesspolicy.py	2012-08-06 03:47:42 +0000
+++ lib/lp/registry/model/accesspolicy.py	2012-08-22 05:07:34 +0000
@@ -378,6 +378,11 @@
         """See `IAccessPolicyGrantSource`."""
         cls.find(grants).remove()
 
+    @classmethod
+    def revokeByPolicy(cls, policies):
+        """See `IAccessPolicyGrantSource`."""
+        cls.findByPolicy(policies).remove()
+
 
 class AccessPolicyGrantFlat(StormBase):
     __storm_table__ = 'AccessPolicyGrantFlat'

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-08-17 07:18:49 +0000
+++ lib/lp/scripts/garbo.py	2012-08-22 05:07:34 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'DailyDatabaseGarbageCollector',
+    'FrequentDatabaseGarbageCollector',
     'HourlyDatabaseGarbageCollector',
     ]
 
@@ -46,6 +47,9 @@
 
 from lp.answers.model.answercontact import AnswerContact
 from lp.bugs.interfaces.bug import IBugSet
+from lp.bugs.interfaces.bugtarget import (
+    POLICY_ALLOWED_TYPES as BUG_POLICY_ALLOWED_TYPES,
+    )
 from lp.bugs.model.bug import Bug
 from lp.bugs.model.bugattachment import BugAttachment
 from lp.bugs.model.bugnotification import BugNotification
@@ -56,6 +60,9 @@
     )
 from lp.code.enums import BranchVisibilityRule
 from lp.code.interfaces.revision import IRevisionSet
+from lp.code.model.branchnamespace import (
+    POLICY_ALLOWED_TYPES as BRANCH_POLICY_ALLOWED_TYPES,
+    )
 from lp.code.model.branchvisibilitypolicy import BranchVisibilityTeamPolicy
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportresult import CodeImportResult
@@ -64,6 +71,11 @@
     RevisionCache,
     )
 from lp.hardwaredb.model.hwdb import HWSubmission
+from lp.registry.interfaces.accesspolicy import (
+    IAccessPolicyArtifactSource,
+    IAccessPolicyGrantSource,
+    IAccessPolicySource,
+    )
 from lp.registry.model.commercialsubscription import CommercialSubscription
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
@@ -1054,6 +1066,45 @@
         transaction.commit()
 
 
+class UnusedSharingPolicyPruner(TunableLoop):
+
+    maximum_chunk_size = 5000
+
+    def __init__(self, log, abort_time=None):
+        super(UnusedSharingPolicyPruner, self).__init__(log, abort_time)
+        self.start_at = 1
+        self.store = IMasterStore(Product)
+
+    def findProducts(self):
+        return self.store.find(
+            Product, Product.id >= self.start_at).order_by(Product.id)
+
+    def isDone(self):
+        return self.findProducts().is_empty()
+
+    def __call__(self, chunk_size):
+        products = list(self.findProducts()[:chunk_size])
+        for product in products:
+            allowed_bug_policies = set(
+                BUG_POLICY_ALLOWED_TYPES.get(product.bug_sharing_policy, []))
+            allowed_branch_policies = set(
+                BRANCH_POLICY_ALLOWED_TYPES.get(
+                    product.branch_sharing_policy, []))
+            access_polices = set(
+                getUtility(IAccessPolicySource).findByPillar([product]))
+            candidate_aps = access_polices.difference(
+                allowed_bug_policies, allowed_branch_policies)
+            apas = getUtility(IAccessPolicyArtifactSource).findByPolicy(
+                candidate_aps)
+            used_aps = set([apa.policy for apa in apas])
+            unused_aps = candidate_aps - used_aps
+            getUtility(IAccessPolicyGrantSource).revokeByPolicy(unused_aps)
+            for ap in unused_aps:
+                self.store.remove(ap)
+        self.start_at = products[-1].id + 1
+        transaction.commit()
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -1340,8 +1391,9 @@
         ScrubPOFileTranslator,
         SuggestiveTemplatesCacheUpdater,
         POTranslationPruner,
+        UnlinkedAccountPruner,
         UnusedPOTMsgSetPruner,
-        UnlinkedAccountPruner,
+        UnusedSharingPolicyPruner,
         ]
     experimental_tunable_loops = [
         PersonPruner,

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2012-08-17 07:18:49 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2012-08-22 05:07:34 +0000
@@ -59,6 +59,7 @@
     BranchSharingPolicy,
     BugSharingPolicy,
     )
+from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.model.product import Product
@@ -1083,6 +1084,21 @@
             self.assertEqual(
                 BugSharingPolicy.PUBLIC, product.bug_sharing_policy)
 
+    def test_UnusedSharingPolicyPruner(self):
+        # UnusedSharingPolicyPruner destroys the unused sharing details.
+        switch_dbuser('testadmin')
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
+        ap = self.factory.makeAccessPolicy(pillar=product)
+        self.factory.makeAccessPolicyArtifact(policy=ap)
+        self.factory.makeAccessPolicyGrant(policy=ap)
+        all_aps = getUtility(IAccessPolicySource).findByPillar([product])
+        # There are 3 because two are created implicitly when the product is.
+        self.assertEqual(3, all_aps.count())
+        self.runDaily()
+        all_aps = getUtility(IAccessPolicySource).findByPillar([product])
+        self.assertEqual([ap], list(all_aps))
+
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer