Merge lp:~lifeless/launchpad/bug-279513 into lp:launchpad

[Robert Collins]

[r=jcsackett,wgrant][bug=279513] Delete Distribution.getCurrentSourceReleases as not modelling user needs well.

Distribution.getCurrentSourceReleases returns arbitrary uninteresting versions - when folk speak about 'ubuntu' they mean 'ubuntu.currentseries' (and in fact the currentseries attribute explains this). This branch just deletes the inappropriate Distribution.getCurrentSourceReleases and changes the 3 callers to indirect via the currentseries attribute.

We could alternatively make Distribution.getCurrentSourceReleases batch indirect via the relevant distro series, but I think the overall deletion of code is more of a win. We may have some tests that need updating, but I'll wait for ec2 to complain.

[William Grant (wgrant)]

I don't like the DistributionSourcePackage.currentrelease change, but it's probably cleaner than the alternative. Otherwise this is fine.

review: Approve (code*)

[j.c.sackett (jcsackett)]

This looks good to land.

review: Approve

[Robert Collins (lifeless)]

I'm abandoning this - its a bit of a rabbit warren, and the perf wins are relatively small compared to the initial work. We are really broken right now, but this is something to do when things are fast, not right now.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-03-01 05:05:26 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-03-02 05:19:17 +0000
@@ -250,7 +250,6 @@
 from lp.bugs.interfaces.malone import IMaloneApplication
 from lp.registry.interfaces.distribution import (
     IDistribution,
-    IDistributionSet,
     )
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
@@ -267,6 +266,7 @@
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.model.sourcepackage import SourcePackage
 from lp.registry.vocabularies import MilestoneVocabulary
 from lp.services.fields import PersonChoice
 from lp.services.propertycache import cachedproperty
@@ -3152,32 +3152,45 @@
         self.many_bugtasks = len(self.bugtasks) >= 10
         self.cached_milestone_source = CachedMilestoneSourceFactory()
         self.user_is_subscribed = self.context.isSubscribed(self.user)
-        distro_packages = defaultdict(list)
         distro_series_packages = defaultdict(list)
         for bugtask in self.bugtasks:
             target = bugtask.target
             if IDistributionSourcePackage.providedBy(target):
-                distro_packages[target.distribution].append(
-                    target.sourcepackagename)
+                distro_series = target.distribution.currentseries
+                if distro_series is not None:
+                    distro_series_packages[distro_series].append(
+                        target.sourcepackagename)
             if ISourcePackage.providedBy(target):
                 distro_series_packages[target.distroseries].append(
                     target.sourcepackagename)
-        distro_set = getUtility(IDistributionSet)
-        self.target_releases = dict(distro_set.getCurrentSourceReleases(
-            distro_packages))
         distro_series_set = getUtility(IDistroSeriesSet)
-        self.target_releases.update(
-            distro_series_set.getCurrentSourceReleases(distro_series_packages))
+        self.target_releases = distro_series_set.getCurrentSourceReleases(
+            distro_series_packages)
+
+    def _get_target_release(self, target):
+        """Get a target release for target."""
+        error = None
+        if not (IDistributionSourcePackage.providedBy(target) or
+                ISourcePackage.providedBy(target)):
+            return None, None
+        distribution = target.distribution
+        if IDistributionSourcePackage.providedBy(target):
+            target = SourcePackage(target.sourcepackagename,
+                distribution.currentseries)
+        if target.distroseries is not None:
+            current_release = self.target_releases.get(target)
+        else:
+            current_release = None
+        if current_release is None:
+            error = "No current release for this source package in %s" % (
+                distribution.displayname)
+        return current_release, error
 
     def getTargetLinkTitle(self, target):
         """Return text to put as the title for the link to the target."""
-        if not (IDistributionSourcePackage.providedBy(target) or
-                ISourcePackage.providedBy(target)):
-            return None
-        current_release = self.target_releases.get(target)
+        current_release, result = self._get_target_release(target)
         if current_release is None:
-            return "No current release for this source package in %s" % (
-                target.distribution.displayname)
+            return result
         uploader = current_release.creator
         maintainer = current_release.maintainer
         return (
 
=== modified file 'lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt'
--- lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt	2010-08-22 18:31:30 +0000
+++ lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt	2011-03-02 05:19:17 +0000
@@ -89,6 +89,8 @@
     >>> print view.getTargetLinkTitle(published_distro)
     None
     >>> view = set_up_view(published_evolution)
+    >>> from lp.registry.interfaces.series import SeriesStatus
+    >>> published_distro.currentseries.status = SeriesStatus.FROZEN
     >>> print view.getTargetLinkTitle(published_evolution)
     Latest release: 1.0, uploaded to universe on 2008-07-18 09:30:15+00:00
     by Foo Bar (name16), maintained by No Privileges Person (no-priv)
@@ -192,7 +194,7 @@
     >>> view = BugTasksAndNominationsView(bug, None)
     >>> view.initialize()
     >>> for bug_target in bug_targets:
-    ...     release = view.target_releases.get(bug_target)
+    ...     release, _ = view._get_target_release(bug_target)
     ...     if release is None:
     ...         version = 'No releases'
     ...     else:
@@ -201,7 +203,7 @@
     product: No releases
     product/product-series: No releases
     published-distro: No releases
-    evolution (Published Distro): 2.0
+    evolution (Published Distro): 1.0
     Published-distro Published-distro-series: No releases
     evolution (Published-distro Published-distro-series): 1.0
     unpublished-distro: No releases
 
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-index.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-index.txt	2010-05-19 05:47:50 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-index.txt	2011-03-02 05:19:17 +0000
@@ -61,11 +61,23 @@
     1
 
 If the context is a distribution package, the package name has a
-tooltip containing the package details.
+tooltip containing the package details. The detailed tests for the tooltip
+contents are in lib/lp/bugs/browser/tests/bugtask-target-link-titles.txt so we
+just need to check that one is set - that the layers are wired together.
+
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.distribution import IDistributionSet
+    >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
+    >>> mozilla = ubuntu.getSourcePackage('mozilla-firefox')
+    >>> mozilla_release = factory.makeSourcePackageRelease(
+    ...     sourcepackagename=mozilla.sourcepackagename,
+    ...     distroseries=mozilla.distribution.currentseries,
+    ...     archive=mozilla.distribution.main_archive, version='0.9')
+
 
     >>> print anon_browser.getLink('mozilla-firefox (Ubuntu)').attrs['title']
-    Latest release: 0.9, uploaded to main on 2004-09-27 11:57:13+00:00...
-    by Mark Shuttleworth (mark), maintained by Mark Shuttleworth (mark)
+    Latest release: 0.9, uploaded to main on ...
+    by ..., maintained by ...
 
     >>> print anon_browser.getLink('mozilla-firefox (Debian)').attrs['title']
     No current release for this source package in Debian
 
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2011-02-05 06:11:48 +0000
+++ lib/lp/registry/browser/product.py	2011-03-02 05:19:17 +0000
@@ -2038,9 +2038,9 @@
         # Set the source_package_release attribute on the licenses
         # widget, so that the source package's copyright info can be
         # displayed.
-        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        ubuntu_dev = getUtility(ILaunchpadCelebrities).ubuntu.currentseries
         if self.source_package_name is not None:
-            release_list = ubuntu.getCurrentSourceReleases(
+            release_list = ubuntu_dev.getCurrentSourceReleases(
                 [self.source_package_name])
             if len(release_list) != 0:
                 self.widgets['licenses'].source_package_release = (
 
=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2011-02-24 15:30:54 +0000
+++ lib/lp/registry/interfaces/distribution.py	2011-03-02 05:19:17 +0000
@@ -417,16 +417,6 @@
         Receives a sourcepackagerelease.
         """
 
-    def getCurrentSourceReleases(source_package_names):
-        """Get the current release of a list of source packages.
-
-        :param source_package_names: a list of `ISourcePackageName`
-            instances.
-
-        :return: a dict where the key is a `IDistributionSourcePackage`
-            and the value is a `IDistributionSourcePackageRelease`.
-        """
-
     def getDistroSeriesAndPocket(distroseriesname):
         """Return a (distroseries,pocket) tuple which is the given textual
         distroseriesname in this distribution."""
@@ -692,15 +682,6 @@
             members, owner, mugshot=None, logo=None, icon=None):
         """Create a new distribution."""
 
-    def getCurrentSourceReleases(distro_to_source_packagenames):
-        """Lookup many distribution source package releases.
-
-        :param distro_to_source_packagenames: A dictionary with
-            its keys being `IDistribution` and its values a list of
-            `ISourcePackageName`.
-        :return: A dict as per `IDistribution.getCurrentSourceReleases`
-        """
-
 
 class NoSuchDistribution(NameLookupFailed):
     """Raised when we try to find a distribution that doesn't exist."""
 
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-03-01 05:05:26 +0000
+++ lib/lp/registry/model/distribution.py	2011-03-02 05:19:17 +0000
@@ -775,11 +775,6 @@
         """See `IDistribution`."""
         return DistributionSourcePackageRelease(self, sourcepackagerelease)
 
-    def getCurrentSourceReleases(self, source_package_names):
-        """See `IDistribution`."""
-        return getUtility(IDistributionSet).getCurrentSourceReleases(
-            {self:source_package_names})
-
     @property
     def has_any_specifications(self):
         """See `IHasSpecifications`."""
@@ -1900,56 +1895,3 @@
         getUtility(IArchiveSet).new(distribution=distro,
             owner=owner, purpose=ArchivePurpose.PRIMARY)
         return distro
-
-    def getCurrentSourceReleases(self, distro_source_packagenames):
-        """See `IDistributionSet`."""
-        # Builds one query for all the distro_source_packagenames.
-        # This may need tuning: its possible that grouping by the common
-        # archives may yield better efficiency: the current code is
-        # just a direct push-down of the previous in-python lookup to SQL.
-        series_clauses = []
-        distro_lookup = {}
-        for distro, package_names in distro_source_packagenames.items():
-            source_package_ids = map(attrgetter('id'), package_names)
-            # all_distro_archive_ids is just a list of ints, but it gets
-            # wrapped anyway - and sqlvalues goes boom.
-            archives = removeSecurityProxy(
-                distro.all_distro_archive_ids)
-            clause = """(spr.sourcepackagename IN %s AND
-                spph.archive IN %s AND
-                ds.distribution = %s)
-                """ % sqlvalues(source_package_ids, archives, distro.id)
-            series_clauses.append(clause)
-            distro_lookup[distro.id] = distro
-        if not len(series_clauses):
-            return {}
-        combined_clause = "(" + " OR ".join(series_clauses) + ")"
-
-        releases = IStore(SourcePackageRelease).find(
-            (SourcePackageRelease, Distribution.id), SQL("""
-                (SourcePackageRelease.id, Distribution.id) IN (
-                    SELECT DISTINCT ON (
-                        spr.sourcepackagename, ds.distribution)
-                        spr.id, ds.distribution
-                    FROM
-                        SourcePackageRelease AS spr,
-                        SourcePackagePublishingHistory AS spph,
-                        DistroSeries AS ds
-                    WHERE
-                        spph.sourcepackagerelease = spr.id
-                        AND spph.distroseries = ds.id
-                        AND spph.status IN %s
-                        AND %s
-                    ORDER BY
-                        spr.sourcepackagename, ds.distribution, spph.id DESC
-                    )
-                """
-                % (sqlvalues(active_publishing_status) + (combined_clause,))))
-        result = {}
-        for sp_release, distro_id in releases:
-            distro = distro_lookup[distro_id]
-            sourcepackage = distro.getSourcePackage(
-                sp_release.sourcepackagename)
-            result[sourcepackage] = DistributionSourcePackageRelease(
-                distro, sp_release)
-        return result
 
=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py	2011-01-21 08:12:29 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py	2011-03-02 05:19:17 +0000
@@ -298,9 +298,11 @@
     @property
     def currentrelease(self):
         """See `IDistributionSourcePackage`."""
-        releases = self.distribution.getCurrentSourceReleases(
+        releases = self.distribution.currentseries.getCurrentSourceReleases(
             [self.sourcepackagename])
-        return releases.get(self)
+        if releases:
+            return releases.values()[0]
+        return None
 
     def bugtasks(self, quantity=None):
         """See `IDistributionSourcePackage`."""
 
=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2010-10-24 12:46:23 +0000
+++ lib/lp/registry/tests/test_distribution.py	2011-03-02 05:19:17 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 
 from lazr.lifecycle.snapshot import Snapshot
-from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
@@ -15,13 +14,6 @@
 from lp.registry.errors import NoSuchDistroSeries
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.series import SeriesStatus
-from lp.registry.tests.test_distroseries import (
-    TestDistroSeriesCurrentSourceReleases,
-    )
-from lp.services.propertycache import get_property_cache
-from lp.soyuz.interfaces.distributionsourcepackagerelease import (
-    IDistributionSourcePackageRelease,
-    )
 from lp.testing import TestCaseWithFactory
 
 
@@ -48,64 +40,6 @@
         self.assertEqual("'\\u0170-distro'", displayname)
 
 
-class TestDistributionCurrentSourceReleases(
-    TestDistroSeriesCurrentSourceReleases):
-    """Test for Distribution.getCurrentSourceReleases().
-
-    This works in the same way as
-    DistroSeries.getCurrentSourceReleases() works, except that we look
-    for the latest published source across multiple distro series.
-    """
-
-    layer = LaunchpadFunctionalLayer
-    release_interface = IDistributionSourcePackageRelease
-
-    @property
-    def test_target(self):
-        return self.distribution
-
-    def test_which_distroseries_does_not_matter(self):
-        # When checking for the current release, we only care about the
-        # version numbers. We don't care whether the version is
-        # published in a earlier or later series.
-        self.current_series = self.factory.makeDistroRelease(
-            self.distribution, '1.0', status=SeriesStatus.CURRENT)
-        self.publisher.getPubSource(
-            version='0.9', distroseries=self.current_series)
-        self.publisher.getPubSource(
-            version='1.0', distroseries=self.development_series)
-        self.assertCurrentVersion('1.0')
-
-        self.publisher.getPubSource(
-            version='1.1', distroseries=self.current_series)
-        self.assertCurrentVersion('1.1')
-
-    def test_distribution_series_cache(self):
-        distribution = removeSecurityProxy(
-            self.factory.makeDistribution('foo'))
-
-        cache = get_property_cache(distribution)
-
-        # Not yet cached.
-        self.assertNotIn("series", cache)
-
-        # Now cached.
-        series = distribution.series
-        self.assertIs(series, cache.series)
-
-        # Cache cleared.
-        distribution.newSeries(
-            name='bar', displayname='Bar', title='Bar', summary='',
-            description='', version='1', parent_series=None,
-            owner=self.factory.makePerson())
-        self.assertNotIn("series", cache)
-
-        # New cached value.
-        series = distribution.series
-        self.assertEqual(1, len(series))
-        self.assertIs(series, cache.series)
-
-
 class SeriesByStatusTests(TestCaseWithFactory):
     """Test IDistribution.getSeriesByStatus().
     """
 
=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2010-10-26 15:47:24 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2011-03-02 05:19:17 +0000
@@ -152,13 +152,13 @@
     def test_get_multiple(self):
         # getCurrentSourceReleases() allows you to get information about
         # the current release for multiple packages at the same time.
-        # This is done using a single DB query, making it more efficient
-        # than using IDistributionSource.currentrelease.
+        # This is done using a set queries, making it more efficient than
+        # looking up each package separately.
         self.publisher.getPubSource(version='0.9', sourcename='foo')
         self.publisher.getPubSource(version='1.0', sourcename='bar')
-        foo_package = self.distribution.getSourcePackage('foo')
-        bar_package = self.distribution.getSourcePackage('bar')
-        releases = self.distribution.getCurrentSourceReleases(
+        foo_package = self.publisher.distroseries.getSourcePackage('foo')
+        bar_package = self.publisher.distroseries.getSourcePackage('bar')
+        releases = self.publisher.distroseries.getCurrentSourceReleases(
             [foo_package.sourcepackagename, bar_package.sourcepackagename])
         self.assertEqual(releases[foo_package].version, '0.9')
         self.assertEqual(releases[bar_package].version, '1.0')
 
=== modified file 'lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt'
--- lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt	2010-11-01 15:46:48 +0000
+++ lib/lp/soyuz/doc/closing-bugs-from-changelogs.txt	2011-03-02 05:19:17 +0000
@@ -209,7 +209,10 @@
 is "NEW".
 
     >>> cdrkit_ubuntu = ubuntu.getSourcePackage('cdrkit')
-    >>> cdrkit_release = cdrkit_ubuntu.currentrelease.sourcepackagerelease
+    >>> cdrkit_release = factory.makeSourcePackageRelease(
+    ...     sourcepackagename=cdrkit_ubuntu.sourcepackagename,
+    ...     distroseries=cdrkit_ubuntu.distribution['warty'],
+    ...     archive=cdrkit_ubuntu.distribution.main_archive, version='1.0')
 
     >>> cdrkit_bug_id = cdrkit_ubuntu.createBug(bug_params).id
 
 
=== modified file 'lib/lp/soyuz/stories/soyuz/xx-distributionsourcepackagerelease-pages.txt'
--- lib/lp/soyuz/stories/soyuz/xx-distributionsourcepackagerelease-pages.txt	2011-01-12 23:07:40 +0000
+++ lib/lp/soyuz/stories/soyuz/xx-distributionsourcepackagerelease-pages.txt	2011-03-02 05:19:17 +0000
@@ -12,6 +12,7 @@
     >>> stp = SoyuzTestPublisher()
     >>> login('[e-mail]')
     >>> stp.prepareBreezyAutotest()
+    >>> stp.distroseries = stp.ubuntutest.currentseries
     >>> source = stp.getPubSource('testing-dspr', version='1.0')
     >>> source.setPublished()
     >>> binaries = stp.getPubBinaries(pub_source=source)