Merge lp:~twom/launchpad/precache-gitrepository-branch-queries into lp:launchpad

[Tom Wardill]

Precache gitrepository voting summary data

[Colin Watson (cjwatson)]

review: Needs Fixing

[Colin Watson (cjwatson)]

review: Needs Fixing

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2018-07-16 00:49:00 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2018-09-06 16:01:23 +0000
@@ -266,7 +266,7 @@
         if (self.context.queue_status ==
             BranchMergeProposalStatus.NEEDS_REVIEW):
             enabled = True
-            if (self.context.votes.count()) > 0:
+            if len(self.context.votes) > 0:
                 text = 'Request another review'
         return Link('+request-review', text, icon='add', enabled=enabled)
 
@@ -890,8 +890,15 @@
     @cachedproperty
     def reviews(self):
         """Return the decorated votes for the proposal."""
-        users_vote = self.context.getUsersVoteReference(self.user)
-        return [DecoratedCodeReviewVoteReference(vote, self.user, users_vote)
+
+        # This would use getUsersVoteReference, but we need to
+        # be able to cache the property. We don't need to normalize
+        # the review types.
+        users_vote = sorted((uv for uv in self.context.votes
+                             if uv.reviewer == self.user),
+                            key=operator.attrgetter('date_created'))
+        final_vote = users_vote[0] if users_vote else None
+        return [DecoratedCodeReviewVoteReference(vote, self.user, final_vote)
                 for vote in self.context.votes
                 if check_permission('launchpad.LimitedView', vote.reviewer)]
 

=== modified file 'lib/lp/code/browser/tests/test_gitrepository.py'
--- lib/lp/code/browser/tests/test_gitrepository.py	2018-08-31 11:35:52 +0000
+++ lib/lp/code/browser/tests/test_gitrepository.py	2018-09-06 16:01:23 +0000
@@ -50,7 +50,10 @@
     record_two_runs,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.matchers import (
     Contains,
     HasQueryCount,
@@ -96,7 +99,7 @@
 
 class TestGitRepositoryView(BrowserTestCase):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_clone_instructions(self):
         repository = self.factory.makeGitRepository()
@@ -267,7 +270,7 @@
                 self.assertIsNotNone(
                     find_tag_by_id(browser.contents, 'landing-candidates'))
 
-    def test_landing_candidate_count(self):
+    def test_landing_candidates_count(self):
         source_repository = self.factory.makeGitRepository()
         view = create_initialized_view(source_repository, '+index')
 
@@ -275,19 +278,44 @@
         self.assertEqual('1 branch', view._getBranchCountText(1))
         self.assertEqual('2 branches', view._getBranchCountText(2))
 
+    def test_landing_candidates_query_count(self):
+        repository = self.factory.makeGitRepository()
+        git_refs = self.factory.makeGitRefs(
+            repository,
+            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
+
+        def login_and_view():
+            with FeatureFixture({"code.git.show_repository_mps": "on"}):
+                with person_logged_in(repository.owner):
+                    browser = self.getViewBrowser(repository)
+                    self.assertIsNotNone(
+                        find_tag_by_id(browser.contents, 'landing-candidates'))
+
+        def create_merge_proposal():
+            bmp = self.factory.makeBranchMergeProposalForGit(
+                target_ref=git_refs[0],
+                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+            self.factory.makePreviewDiff(merge_proposal=bmp)
+
+        recorder1, recorder2 = record_two_runs(
+            login_and_view,
+            create_merge_proposal,
+            2)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_view_with_landing_targets(self):
         product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
         target_repository = self.factory.makeGitRepository(target=product)
         source_repository = self.factory.makeGitRepository(target=product)
-        target_git_refs = self.factory.makeGitRefs(
+        [target_git_ref] = self.factory.makeGitRefs(
             target_repository,
-            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
-        source_git_refs = self.factory.makeGitRefs(
+            paths=["refs/heads/master"])
+        [source_git_ref] = self.factory.makeGitRefs(
             source_repository,
             paths=["refs/heads/master"])
         self.factory.makeBranchMergeProposalForGit(
-            target_ref=target_git_refs[0],
-            source_ref=source_git_refs[0],
+            target_ref=target_git_ref,
+            source_ref=source_git_ref,
             set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
         with FeatureFixture({"code.git.show_repository_mps": "on"}):
             with person_logged_in(target_repository.owner):
@@ -296,19 +324,49 @@
                 self.assertIsNotNone(
                     find_tag_by_id(browser.contents, 'landing-targets'))
 
+    def test_landing_targets_query_count(self):
+        product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
+        target_repository = self.factory.makeGitRepository(target=product)
+        source_repository = self.factory.makeGitRepository(target=product)
+
+        def create_merge_proposal():
+            [target_git_ref] = self.factory.makeGitRefs(
+                target_repository)
+            [source_git_ref] = self.factory.makeGitRefs(
+                source_repository)
+            bmp = self.factory.makeBranchMergeProposalForGit(
+                target_ref=target_git_ref,
+                source_ref=source_git_ref,
+                set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+            self.factory.makePreviewDiff(merge_proposal=bmp)
+
+        def login_and_view():
+            with FeatureFixture({"code.git.show_repository_mps": "on"}):
+                with person_logged_in(target_repository.owner):
+                    browser = self.getViewBrowser(
+                        source_repository, user=source_repository.owner)
+                    self.assertIsNotNone(
+                        find_tag_by_id(browser.contents, 'landing-targets'))
+
+        recorder1, recorder2 = record_two_runs(
+            login_and_view,
+            create_merge_proposal,
+            2)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_view_with_inactive_landing_targets(self):
         product = self.factory.makeProduct(name="foo", vcs=VCSType.GIT)
         target_repository = self.factory.makeGitRepository(target=product)
         source_repository = self.factory.makeGitRepository(target=product)
-        target_git_refs = self.factory.makeGitRefs(
+        [target_git_ref] = self.factory.makeGitRefs(
             target_repository,
-            paths=["refs/heads/master", "refs/heads/1.0", "refs/tags/1.1"])
-        source_git_refs = self.factory.makeGitRefs(
+            paths=["refs/heads/master"])
+        [source_git_ref] = self.factory.makeGitRefs(
             source_repository,
             paths=["refs/heads/master"])
         self.factory.makeBranchMergeProposalForGit(
-            target_ref=target_git_refs[0],
-            source_ref=source_git_refs[0],
+            target_ref=target_git_ref,
+            source_ref=source_git_ref,
             set_state=BranchMergeProposalStatus.MERGED)
         with FeatureFixture({"code.git.show_repository_mps": "on"}):
             with person_logged_in(target_repository.owner):

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2018-04-25 12:01:33 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2018-09-06 16:01:23 +0000
@@ -21,7 +21,6 @@
 from sqlobject import (
     ForeignKey,
     IntCol,
-    SQLMultipleJoin,
     StringCol,
     )
 from storm.expr import (
@@ -123,6 +122,7 @@
 from lp.services.helpers import shortlist
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
+from lp.services.librarian.model import LibraryFileAlias
 from lp.services.mail.sendmail import validate_message
 from lp.services.propertycache import (
     cachedproperty,
@@ -610,8 +610,11 @@
     def preview_diff(self):
         return self._preview_diffs.last()
 
-    votes = SQLMultipleJoin(
-        'CodeReviewVoteReference', joinColumn='branch_merge_proposal')
+    @cachedproperty
+    def votes(self):
+        return list(Store.of(self).find(
+            CodeReviewVoteReference,
+            CodeReviewVoteReference.branch_merge_proposal == self))
 
     def getNotificationRecipients(self, min_level):
         """See IBranchMergeProposal.getNotificationRecipients"""
@@ -1275,7 +1278,7 @@
         return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
 
     @staticmethod
-    def preloadDataForBMPs(branch_merge_proposals, user):
+    def preloadDataForBMPs(branch_merge_proposals, user, include_votes=True):
         # Utility to load the data related to a list of bmps.
         # Circular imports.
         from lp.code.model.branch import Branch
@@ -1330,7 +1333,7 @@
                 PreviewDiff.branch_merge_proposal_id,
                 Desc(PreviewDiff.date_created)).config(
                     distinct=[PreviewDiff.branch_merge_proposal_id])
-        load_related(Diff, preview_diffs, ['diff_id'])
+        diffs = load_related(Diff, preview_diffs, ['diff_id'])
         preview_diff_map = {}
         for previewdiff in preview_diffs:
             preview_diff_map[previewdiff.branch_merge_proposal_id] = (
@@ -1367,6 +1370,18 @@
         if repositories:
             GenericGitCollection.preloadDataForRepositories(repositories)
 
+        # if we need to include a vote summary, we should precache
+        # that data too
+        if include_votes:
+            vote_list = list(load_referencing(
+                CodeReviewVoteReference, branch_merge_proposals,
+                ['branch_merge_proposalID']))
+            for mp in branch_merge_proposals:
+                get_property_cache(mp).votes = vote_list
+
+            # we also provide a summary of diffs, so load them
+            load_related(LibraryFileAlias, diffs, ['diff_textID'])
+
 
 @implementer(IBranchMergeProposalGetter)
 class BranchMergeProposalGetter:

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2018-08-31 13:14:39 +0000
+++ lib/lp/code/model/gitrepository.py	2018-09-06 16:01:23 +0000
@@ -922,7 +922,10 @@
 
     def getPrecachedLandingCandidates(self, user):
         """See `IGitRepository`."""
-        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        loader = partial(
+            BranchMergeProposal.preloadDataForBMPs,
+            user=user,
+            include_votes=True)
         return DecoratedResultSet(
             self.landing_candidates, pre_iter_hook=loader)