launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/sharing-details-page-branch-queries-1021622 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1021622 in Launchpad itself: "PillarPersonSharingView uses O(n) queries for branch access"
  https://bugs.launchpad.net/launchpad/+bug/1021622

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-page-branch-queries-1021622/+merge/114577

== Implementation ==

The sharing details page was using O(n) queries to load the branches it wanted to display. This is because once the branches were loaded by IBranchCollection.getGranches(), a query was used by the zope security check each time it called branch.visibleByUser().

So I implemented a caching mechanism similar to that used for bugs (I copied the known_viewers property including docstring). There is a known_viewers set, and the getBranches() call populates the _known_viewers.

== Tests ==

Revert the TestProductSharingDetailsView.test_view_query_count() to check for the correct number of queries (12) instead of the temporarily inflated number used to make the test pass.

Add a new test test_getBranches_caches_viewers() test_branchcollection.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/branch.py
  lib/lp/code/model/branchcollection.py
  lib/lp/code/model/tests/test_branchcollection.py
  lib/lp/registry/browser/tests/test_pillar_sharing.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/sharing-details-page-branch-queries-1021622/+merge/114577
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sharing-details-page-branch-queries-1021622 into lp:launchpad.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-07-11 09:43:04 +0000
+++ lib/lp/code/model/branch.py	2012-07-12 07:31:24 +0000
@@ -1276,12 +1276,30 @@
         job.celeryRunOnCommit()
         return job
 
+    @cachedproperty
+    def _known_viewers(self):
+        """A set of known persons able to view this branch.
+
+        This method must return an empty set or branch searches will trigger
+        late evaluation. Any 'should be set on load' properties must be done by
+        the branch search.
+
+        If you are tempted to change this method, don't. Instead see
+        visibleByUser which defines the just-in-time policy for branch
+        visibility, and IBranchCollection which honours visibility rules.
+        """
+        return set()
+
     def visibleByUser(self, user, checked_branches=None):
         """See `IBranch`."""
         if checked_branches is None:
             checked_branches = []
         if self.information_type in PUBLIC_INFORMATION_TYPES:
             can_access = True
+        elif user is None:
+            can_access = False
+        elif user.id in self._known_viewers:
+            can_access = True
         else:
             can_access = not getUtility(IAllBranches).withIds(
                 self.id).visibleByUser(user).is_empty()

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2012-07-09 10:35:00 +0000
+++ lib/lp/code/model/branchcollection.py	2012-07-12 07:31:24 +0000
@@ -295,8 +295,6 @@
         tables = [Branch] + list(all_tables)
         expressions = self._getBranchExpressions()
         resultset = self.store.using(*tables).find(Branch, *expressions)
-        if not eager_load:
-            return resultset
 
         def do_eager_load(rows):
             branch_ids = set(branch.id for branch in rows)
@@ -309,7 +307,17 @@
             load_related(Person, rows,
                 ['ownerID', 'registrantID', 'reviewerID'])
             load_referencing(BugBranch, rows, ['branchID'])
-        return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
+
+        def cache_permission(branch):
+            if self._user:
+                get_property_cache(branch)._known_viewers = (
+                    set([self._user.id]))
+            return branch
+
+        eager_load_hook = do_eager_load if eager_load else None
+        return DecoratedResultSet(
+            resultset, pre_iter_hook=eager_load_hook,
+            result_decorator=cache_permission)
 
     def getMergeProposals(self, statuses=None, for_branches=None,
                           target_branch=None, merged_revnos=None,

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2012-07-05 04:59:52 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2012-07-12 07:31:24 +0000
@@ -9,10 +9,12 @@
 
 import pytz
 from storm.store import EmptyResultSet
+from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.app.interfaces.services import IService
 from lp.code.enums import (
     BranchLifecycleStatus,
     BranchMergeProposalStatus,
@@ -41,9 +43,11 @@
 from lp.testing import (
     person_logged_in,
     run_with_login,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestBranchCollectionAdaptation(TestCaseWithFactory):
@@ -132,6 +136,24 @@
             self.store, [Branch.product == branch.product])
         self.assertEqual([branch], list(collection.getBranches()))
 
+    def test_getBranches_caches_viewers(self):
+        # getBranches() caches the user as a known viewer so that
+        # branch.visibleByUser() does not have to hit the database.
+        collection = GenericBranchCollection(self.store)
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
+        branch = self.factory.makeProductBranch(
+            owner=owner,
+            product=product,
+            information_type=InformationType.USERDATA)
+        someone = self.factory.makePerson()
+        getUtility(IService, 'sharing').ensureAccessGrants(
+            [someone], owner, branches=[branch], ignore_permissions=True)
+        [branch] = list(collection.visibleByUser(someone).getBranches())
+        with StormStatementRecorder() as recorder:
+            self.assertTrue(branch.visibleByUser(someone))
+            self.assertThat(recorder, HasQueryCount(Equals(0)))
+
     def test_count(self):
         # The 'count' property of a collection is the number of elements in
         # the collection.

=== modified file 'lib/lp/registry/browser/tests/test_pillar_sharing.py'
--- lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-07-06 07:39:40 +0000
+++ lib/lp/registry/browser/tests/test_pillar_sharing.py	2012-07-12 07:31:24 +0000
@@ -241,7 +241,7 @@
             IStore(self.pillar).invalidate()
             with StormStatementRecorder() as recorder:
                 create_initialized_view(pillarperson, '+index')
-            self.assertThat(recorder, HasQueryCount(LessThan(26)))
+            self.assertThat(recorder, HasQueryCount(LessThan(12)))
 
     def test_view_write_enabled_without_feature_flag(self):
         # Test that sharing_write_enabled is not set without the feature flag.