Merge lp:~thumper/launchpad/simplistic-official-links-speedup into lp:launchpad

[Tim Penhey]

Be more efficient generating the bazaar identities for the branch listings.

[Tim Penhey (thumper)]

= Summary =

This branch reduces the query count on branch listings that contain package
branches.

== Proposed fix ==

Get the branch listing browser classes to use the query optimiser to load all
the suite source package links for the branches, and make those available
through the decorated Branch class.

Move the package development focus check into the model code for branch, and
change the bazaar_identity function to us is_dev_focus for package branches.

Also determine the development focus in the browser class.

== Pre-implementation notes ==

Distribution.currentseries is an abomination and had to be worked around in
the browser class.

== Implementation details ==

I've deferred some possible refactoring of the dev focus checks.

== Tests ==

test_linked_to_development_package
TestDevelopmentFocusPackageBranches
code/stories
TestBzrIdentity

== Demo and Q/A ==

Add the mint distro using the code test helper, and go to look at:
https://code.launchpad.dev/mint/+branches?batch=150

and look for lp://dev/mint/twisted

 reviewer mwhudson

[Michael Hudson-Doyle (mwhudson)]

Generally, I like this branch. It makes the handling of product and
package more consistent, which has to be a good thing. And it reduces
the query counts!

Did you think about adding tests that check how many queries we're issuing?

> === modified file 'lib/lp/code/browser/branchlisting.py'
> --- lib/lp/code/browser/branchlisting.py 2009-09-22 18:45:02 +0000
> +++ lib/lp/code/browser/branchlisting.py 2009-10-21 02:06:48 +0000
> @@ -87,6 +87,7 @@
> ProductDownloadFileMixin, SortSeriesMixin)
> from lp.registry.interfaces.distroseries import DistroSeriesStatus
> from lp.registry.interfaces.person import IPerson, IPersonSet
> +from lp.registry.interfaces.pocket import PackagePublishingPocket
> from lp.registry.interfaces.product import IProduct
> from lp.registry.interfaces.sourcepackage import ISourcePackageFactory
> from lp.registry.model.sourcepackage import SourcePackage
> @@ -153,21 +154,26 @@
> delegates(IBranch, 'context')
>
> def __init__(self, branch, last_commit, now, show_bug_badge,
> - show_blueprint_badge, is_dev_focus,
> - associated_product_series, show_mp_badge):
> + show_blueprint_badge, show_mp_badge,
> + associated_product_series, suite_source_packages, is_dev_focus):
> BranchBadges.__init__(self, branch)
> self.last_commit = last_commit
> self.show_bug_badge = show_bug_badge
> self.show_blueprint_badge = show_blueprint_badge
> self.show_merge_proposals = show_mp_badge
> self._now = now
> + self.associated_product_series = associated_product_series
> + self.suite_source_packages = suite_source_packages
> self.is_development_focus = is_dev_focus
> - self.associated_product_series = associated_product_series
>
> def associatedProductSeries(self):
> """Override the IBranch.associatedProductSeries."""
> return self.associated_product_series
>
> + def associatedSuiteSourcePackages(self):
> + """Override the IBranch.associatedSuiteSourcePackages."""
> + return self.suite_source_packages
> +
> @property
> def active_series(self):
> return [series for series in self.associated_product_series
> @@ -314,7 +320,7 @@
> # Requires the following attributes:
> # visible_branches_for_view
> def __init__(self, user):
> - self._dev_series_map = {}
> + self._distro_series_map = {}
> self._now = datetime.now(pytz.UTC)
> self.view_user = user
>
> @@ -373,21 +379,63 @@
> series.insert(0, dev_focus)
> return series
>
> - def getDevFocusBranch(self, branch):
> - """Get the development focus branch that relates to `branch`."""
> - # XXX: 2009-07-02 TimPenhey spec=package-branches We need to determine
> - # here what constitutes a dev focus branch for package branches, and
> - # that perhaps this should also refer to targets instead of using
> - # product.
> - if branch.product is None:
> - return None
> + @cachedproperty
> + def official_package_links_map(self):
> + """Return a map of ...

review: Approve

[Tim Penhey (thumper)]

On Wed, 21 Oct 2009 16:22:06 Michael Hudson wrote:
> Review: Approve
> Generally, I like this branch. It makes the handling of product and
> package more consistent, which has to be a good thing. And it reduces
> the query counts!
>
> Did you think about adding tests that check how many queries we're issuing?

I brought this up at the team lead sprints, and it was somewhat discouraged.

Explicitly checking at some levels about the exact number of queries being
used is ok, but not checking less than a certain level.

Probably better to graph and track query counts on key pages.

> > === modified file 'lib/lp/code/browser/branchlisting.py'
> > --- lib/lp/code/browser/branchlisting.py 2009-09-22 18:45:02 +0000
> > +++ lib/lp/code/browser/branchlisting.py 2009-10-21 02:06:48 +0000
> > + @cachedproperty
> > + def official_package_links_map(self):
> > + """Return a map of branch id to a list of package links.
>
> I guess it's a bit hard, English being English, but I found this hard
> to grok. s/of/from/ would help a bit, I think.

Fair enough, changed.

> > + While this code is still valid with package branches is it a
> > query + that isn't needed.
> > + """
>
> Here though I think you have a confusing thinko: s/package/product/
> surely? I'm not quite sure what you mean at a higher level -- are you
> saying that this will be a waste on a page that we know only has
> product branches? In which case an XXX comment would seem more
> appropiate than a mention in the docstring.
>
> Oh, I see: you copied this from product_series_map.__doc__ :-) Please
> fix up both.

Done.

> > === modified file 'lib/lp/code/model/tests/test_branch.py'
> > --- lib/lp/code/model/tests/test_branch.py 2009-10-15 05:45:57 +0000
> > +++ lib/lp/code/model/tests/test_branch.py 2009-10-21 02:06:48 +0000
> > @@ -486,6 +486,23 @@
> > login(ANONYMOUS)
> > self.assertBzrIdentity(branch, linked_branch.bzr_path)
> >
> > + def test_linked_to_development_package(self):
> > + # If a branch is linked to the RELEASE pocket of the current
> > + # distroseries for the source package, then the bzr identity is
> > the + # distribution name followed by the sourcepackage name. +
> > branch = self.factory.makePackageBranch()
> > + pocket = PackagePublishingPocket.RELEASE
> > + linked_branch = ICanHasLinkedBranch(
> > + branch.sourcepackage.getSuiteSourcePackage(pocket))
> > + registrant = getUtility(
> > + ILaunchpadCelebrities).ubuntu_branches.teamowner
> > + login_person(registrant)
> > + linked_branch.setBranch(branch, registrant)
> > + logout()
> > + login(ANONYMOUS)
> > + self.assertBzrIdentity(branch, '%s/%s' % (
> > + branch.distribution.name, branch.sourcepackagename.name))
>
> As pointed out in IRC, this test is basically the same as the following.

Yep, deleted.

> Cheers,
> mwh

thanks

[Jonathan Lange (jml)]

On Thu, Oct 22, 2009 at 12:57 AM, Tim Penhey <email address hidden> wrote:
> On Wed, 21 Oct 2009 16:22:06 Michael Hudson wrote:
>> Review: Approve
>> Generally, I like this branch.  It makes the handling of product and
>> package more consistent, which has to be a good thing.  And it reduces
>> the query counts!
>>
>> Did you think about adding tests that check how many queries we're issuing?
>
> I brought this up at the team lead sprints, and it was somewhat discouraged.
>

I don't recall it being discouraged. I just remember Bjorn & Martin
Pool saying to check for equality rather than less than.

jml

=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py	2009-09-22 18:45:02 +0000
+++ lib/lp/code/browser/branchlisting.py	2009-10-21 23:58:14 +0000
@@ -87,6 +87,7 @@
     ProductDownloadFileMixin, SortSeriesMixin)
 from lp.registry.interfaces.distroseries import DistroSeriesStatus
 from lp.registry.interfaces.person import IPerson, IPersonSet
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.sourcepackage import ISourcePackageFactory
 from lp.registry.model.sourcepackage import SourcePackage
@@ -153,21 +154,26 @@
     delegates(IBranch, 'context')
 
     def __init__(self, branch, last_commit, now, show_bug_badge,
-                 show_blueprint_badge, is_dev_focus,
-                 associated_product_series, show_mp_badge):
+                 show_blueprint_badge, show_mp_badge,
+                 associated_product_series, suite_source_packages, is_dev_focus):
         BranchBadges.__init__(self, branch)
         self.last_commit = last_commit
         self.show_bug_badge = show_bug_badge
         self.show_blueprint_badge = show_blueprint_badge
         self.show_merge_proposals = show_mp_badge
         self._now = now
+        self.associated_product_series = associated_product_series
+        self.suite_source_packages = suite_source_packages
         self.is_development_focus = is_dev_focus
-        self.associated_product_series = associated_product_series
 
     def associatedProductSeries(self):
         """Override the IBranch.associatedProductSeries."""
         return self.associated_product_series
 
+    def associatedSuiteSourcePackages(self):
+        """Override the IBranch.associatedSuiteSourcePackages."""
+        return self.suite_source_packages
+
     @property
     def active_series(self):
         return [series for series in self.associated_product_series
@@ -314,7 +320,7 @@
     # Requires the following attributes:
     #   visible_branches_for_view
     def __init__(self, user):
-        self._dev_series_map = {}
+        self._distro_series_map = {}
         self._now = datetime.now(pytz.UTC)
         self.view_user = user
 
@@ -345,11 +351,7 @@
 
     @cachedproperty
     def product_series_map(self):
-        """Return a map of branch id to a list of product series.
-
-        While this code is still valid with package branches is it a query
-        that isn't needed.
-        """
+        """Return a map from branch id to a list of product series."""
         series_resultset = self._query_optimiser.getProductSeriesForBranches(
             self._visible_branch_ids)
         result = {}
@@ -373,21 +375,59 @@
                 series.insert(0, dev_focus)
         return series
 
-    def getDevFocusBranch(self, branch):
-        """Get the development focus branch that relates to `branch`."""
-        # XXX: 2009-07-02 TimPenhey spec=package-branches We need to determine
-        # here what constitutes a dev focus branch for package branches, and
-        # that perhaps this should also refer to targets instead of using
-        # product.
-        if branch.product is None:
-            return None
+    @cachedproperty
+    def official_package_links_map(self):
+        """Return a map from branch id to a list of package links."""
+        links = self._query_optimiser.getOfficialSourcePackageLinksForBranches(
+            self._visible_branch_ids)
+        result = {}
+        for link in links:
+            result.setdefault(link.branch.id, []).append(link)
+        return result
+
+    def getSuiteSourcePackages(self, branch):
+        """Get the associated SuiteSourcePackages for the branch.
+
+        If there is more than one, they are sorted by pocket.
+        """
+        links = [link.suite_sourcepackage for link in
+                 self.official_package_links_map.get(branch.id, [])]
+        return sorted(links, key=attrgetter('pocket'))
+
+    def getDistroDevelSeries(self, distribution):
+        """Since distribution.currentseries hits the DB every time, cache it."""
+        self._distro_series_map = {}
         try:
-            return self._dev_series_map[branch.product]
+            return self._distro_series_map[distribution]
         except KeyError:
-            result = branch.product.development_focus.branch
-            self._dev_series_map[branch.product] = result
+            result = distribution.currentseries
+            self._distro_series_map[distribution] = result
             return result
 
+    def isBranchDevFocus(self, branch,
+                         associated_product_series, suite_source_packages):
+        """Is the branch the development focus?
+
+        For product branches this means that the branch is linked to the
+        development focus series.
+
+        For package branches this means that the branch is linked to the
+        release pocket of the development series.
+        """
+        # Refactor this code to work for model.branch too?
+        # Do we care if a non-product branch is linked to the product series?
+        # Do we care if a non-package branch is linked to the package?
+        # A) not right now.
+        for series in associated_product_series:
+            if series.product.development_focus == series:
+                return True
+        for ssp in suite_source_packages:
+            if (ssp.pocket == PackagePublishingPocket.RELEASE and
+                ssp.distroseries == self.getDistroDevelSeries(
+                    ssp.distribution)):
+                return True
+        return False
+
     @cachedproperty
     def branch_ids_with_bug_links(self):
         """Return a set of branch ids that should show bug badges."""
@@ -436,11 +476,13 @@
         show_blueprint_badge = branch.id in self.branch_ids_with_spec_links
         show_mp_badge = branch.id in self.branch_ids_with_merge_proposals
         associated_product_series = self.getProductSeries(branch)
-        is_dev_focus = (self.getDevFocusBranch(branch) == branch)
+        suite_source_packages = self.getSuiteSourcePackages(branch)
+        is_dev_focus = self.isBranchDevFocus(
+            branch, associated_product_series, suite_source_packages)
         return BranchListingItem(
             branch, last_commit, self._now, show_bug_badge,
-            show_blueprint_badge, is_dev_focus,
-            associated_product_series, show_mp_badge)
+            show_blueprint_badge, show_mp_badge,
+            associated_product_series, suite_source_packages, is_dev_focus)
 
     def decoratedBranches(self, branches):
         """Return the decorated branches for the branches passed in."""
 
=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2009-10-01 11:21:31 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2009-10-21 23:58:14 +0000
@@ -307,6 +307,29 @@
         self.assertGroupBranchesEqual(expected, series)
 
 
+class TestDevelopmentFocusPackageBranches(TestCaseWithFactory):
+    """Make sure that the bzr_identity of the branches are correct."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_package_development_focus(self):
+        # Check the bzr_identity of a development focus package branch.
+        branch = self.factory.makePackageBranch()
+        series_set = removeSecurityProxy(getUtility(IMakeOfficialBranchLinks))
+        sspb = series_set.new(
+            branch.distroseries, PackagePublishingPocket.RELEASE,
+            branch.sourcepackagename, branch, branch.owner)
+        identity = "lp://dev/%s/%s" % (
+            branch.distribution.name, branch.sourcepackagename.name)
+        self.assertEqual(identity, branch.bzr_identity)
+        # Now confirm that we get the same through the view.
+        view = create_initialized_view(branch.distribution, name='+branches')
+        # There is only one branch.
+        batch = view.branches()
+        [view_branch] = batch.branches
+        self.assertEqual(identity, view_branch.bzr_identity)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)
 
 
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2009-10-15 05:45:57 +0000
+++ lib/lp/code/interfaces/branch.py	2009-10-21 23:58:14 +0000
@@ -68,7 +68,6 @@
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.branchtarget import IHasBranchTarget
 from lp.code.interfaces.hasbranches import IHasMergeProposals
-from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from canonical.launchpad.interfaces.launchpad import (
     ILaunchpadCelebrities, IPrivacy)
 from lp.registry.interfaces.role import IHasOwner
@@ -1230,10 +1229,11 @@
             'series': use_series.name}
 
     if branch.sourcepackage is not None:
-        distro_package = branch.sourcepackage.distribution_sourcepackage
-        linked_branch = ICanHasLinkedBranch(distro_package)
-        if linked_branch.branch == branch:
-            return lp_prefix + linked_branch.bzr_path
+        if is_dev_focus:
+            return "%(prefix)s%(distro)s/%(packagename)s" % {
+            'prefix': lp_prefix,
+            'distro': branch.distroseries.distribution.name,
+            'packagename': branch.sourcepackagename.name}
         suite_sourcepackages = branch.associatedSuiteSourcePackages()
         # Take the first link if there is one.
         if len(suite_sourcepackages) > 0:
 
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2009-10-06 16:24:20 +0000
+++ lib/lp/code/model/branch.py	2009-10-21 23:58:14 +0000
@@ -68,6 +68,7 @@
 from lp.code.interfaces.branchnamespace import IBranchNamespacePolicy
 from lp.code.interfaces.branchpuller import IBranchPuller
 from lp.code.interfaces.branchtarget import IBranchTarget
+from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.code.interfaces.seriessourcepackagebranch import (
     IFindOfficialBranchLinks)
 from lp.registry.interfaces.person import (
@@ -441,11 +442,14 @@
     @property
     def bzr_identity(self):
         """See `IBranch`."""
-        # XXX: JonathanLange 2009-03-19 spec=package-branches bug=345740: This
-        # should not dispatch on product is None.
+        # Should probably put this into the branch target.
         if self.product is not None:
             series_branch = self.product.development_focus.branch
             is_dev_focus = (series_branch == self)
+        elif self.distroseries is not None:
+            distro_package = self.sourcepackage.distribution_sourcepackage
+            linked_branch = ICanHasLinkedBranch(distro_package)
+            is_dev_focus = (linked_branch.branch == self)
         else:
             is_dev_focus = False
         return bazaar_identity(self, is_dev_focus)