launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02692
[Merge] lp:~lifeless/launchpad/bug-636158 into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/bug-636158 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-636158/+merge/50534
This batches lookups of the current source releases in bugtasks; its work towards improving the high query count shown in bug 636158.
Its entirely refactoring: I've added no new functionality, just shifted per-object code to per-set code. As such, I haven't added any tests; they wouldn't add any value at this point.
In the slightly longer term, we can get rid of the Distro[Set].getCurrentSourceReleases methods when we fix bug 279513. With those methods gone, the tests for DistroSeries.getCurrentSourceReleases can be easily adjusted to test DistroSeriesSet.getCurrentSourceReleases *instead*, and we can then test that DistroSeries.getCurrentSourceReleases calls into the set correctly. I think that this should wait for now.
I'm a little unhappy about the sql OR list construction - do we have a helper for that I could be using? The rest of the duplication between these two locations I can tolerate given we can remove it all when we fix bug 279513.
--
https://code.launchpad.net/~lifeless/launchpad/bug-636158/+merge/50534
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-636158 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-02-18 02:58:59 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-02-20 23:01:38 +0000
@@ -43,6 +43,7 @@
]
import cgi
+from collections import defaultdict
from datetime import (
datetime,
timedelta,
@@ -247,11 +248,17 @@
from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
from lp.bugs.interfaces.cve import ICveSet
from lp.bugs.interfaces.malone import IMaloneApplication
-from lp.registry.interfaces.distribution import IDistribution
+from lp.registry.interfaces.distribution import (
+ IDistribution,
+ IDistributionSet,
+ )
from lp.registry.interfaces.distributionsourcepackage import (
IDistributionSourcePackage,
)
-from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseries import (
+ IDistroSeries,
+ IDistroSeriesSet,
+ )
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -3144,26 +3151,22 @@
self.many_bugtasks = len(self.bugtasks) >= 10
self.cached_milestone_source = CachedMilestoneSourceFactory()
self.user_is_subscribed = self.context.isSubscribed(self.user)
- distro_packages = {}
+ distro_packages = defaultdict(list)
+ distro_series_packages = defaultdict(list)
for bugtask in self.bugtasks:
target = bugtask.target
if IDistributionSourcePackage.providedBy(target):
- distro_packages.setdefault(target.distribution, [])
distro_packages[target.distribution].append(
target.sourcepackagename)
if ISourcePackage.providedBy(target):
- distro_packages.setdefault(target.distroseries, [])
- distro_packages[target.distroseries].append(
+ distro_series_packages[target.distroseries].append(
target.sourcepackagename)
- # Set up a mapping from a target to its current release, using
- # only a few DB queries. It would be easier to use the packages'
- # currentrelease attributes, but that causes many DB queries to
- # be issued.
- self.target_releases = {}
- for distro_or_series, package_names in distro_packages.items():
- releases = distro_or_series.getCurrentSourceReleases(
- package_names)
- self.target_releases.update(releases)
+ 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))
def getTargetLinkTitle(self, target):
"""Return text to put as the title for the link to the target."""
=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py 2011-01-21 08:30:55 +0000
+++ lib/lp/registry/interfaces/distribution.py 2011-02-20 23:01:38 +0000
@@ -692,6 +692,15 @@
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/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py 2011-01-21 08:12:29 +0000
+++ lib/lp/registry/interfaces/distroseries.py 2011-02-20 23:01:38 +0000
@@ -916,6 +916,15 @@
`PackagePublishingPocket`.
"""
+ def getCurrentSourceReleases(distro_series_source_packagenames):
+ """Lookup many distroseries source package releases.
+
+ :param distro_series_to_source_packagenames: A dictionary with
+ its keys being `IDistroSeries` and its values a list of
+ `ISourcePackageName`.
+ :return: A dict as per `IDistroSeries.getCurrentSourceReleases`
+ """
+
def search(distribution=None, released=None, orderBy=None):
"""Search the set of distro seriess.
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2011-02-10 16:50:17 +0000
+++ lib/lp/registry/model/distribution.py 2011-02-20 23:01:38 +0000
@@ -10,6 +10,7 @@
'DistributionSet',
]
+from operator import attrgetter
from sqlobject import (
BoolCol,
@@ -33,6 +34,7 @@
alsoProvides,
implements,
)
+from zope.security.proxy import removeSecurityProxy
from canonical.database.constants import UTC_NOW
from canonical.database.datetimecol import UtcDateTimeCol
@@ -775,34 +777,8 @@
def getCurrentSourceReleases(self, source_package_names):
"""See `IDistribution`."""
- source_package_ids = [
- package_name.id for package_name in source_package_names]
- releases = SourcePackageRelease.select("""
- SourcePackageName.id IN %s AND
- SourcePackageRelease.id =
- SourcePackagePublishingHistory.sourcepackagerelease AND
- SourcePackagePublishingHistory.id = (
- SELECT max(spph.id)
- FROM SourcePackagePublishingHistory spph,
- SourcePackageRelease spr, SourcePackageName spn,
- DistroSeries ds
- WHERE
- spn.id = SourcePackageName.id AND
- spr.sourcepackagename = spn.id AND
- spph.sourcepackagerelease = spr.id AND
- spph.archive IN %s AND
- spph.status IN %s AND
- spph.distroseries = ds.id AND
- ds.distribution = %s)
- """ % sqlvalues(
- source_package_ids, self.all_distro_archive_ids,
- active_publishing_status, self),
- clauseTables=[
- 'SourcePackageName', 'SourcePackagePublishingHistory'])
- return dict(
- (self.getSourcePackage(release.sourcepackagename),
- DistributionSourcePackageRelease(self, release))
- for release in releases)
+ return getUtility(IDistributionSet).getCurrentSourceReleases(
+ {self:source_package_names})
@property
def has_any_specifications(self):
@@ -1925,3 +1901,59 @@
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
+ DistroSeries.distribution = %s
+ """ % sqlvalues(source_package_ids, archives, distro.id)
+ series_clauses.append(clause)
+ distro_lookup[distro.id] = distro
+ if not len(series_clauses):
+ return {}
+ if len(series_clauses) > 1:
+ combined_clause = series_clauses[0]
+ else:
+ combined_clause = "(" + " OR ".join(series_clauses) + ")"
+ # The complex join constructs a table of just current releases for the
+ # desired SPNs.
+ origin = SQL("""
+ SourcePackageRelease
+ JOIN (
+ SELECT
+ spr.id as sourcepackagerelease, MAX(spph.id),
+ DistroSeries.distribution as distribution
+ FROM SourcePackagePublishingHistory spph,
+ SourcePackageRelease spr,
+ DistroSeries
+ WHERE
+ spph.sourcepackagerelease = spr.id AND
+ spph.distroseries = DistroSeries.id AND
+ spph.status IN %s AND
+ %s
+ GROUP BY spr.id, DistroSeries.distribution)
+ AS spph ON SourcePackageRelease.id = spph.sourcepackagerelease
+ """ % (sqlvalues(active_publishing_status)+ (combined_clause,)))
+ releases = IStore(Distribution).using(origin).find(
+ (SourcePackageRelease, SQL("distribution")))
+ 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/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2011-01-21 08:12:29 +0000
+++ lib/lp/registry/model/distroseries.py 2011-02-20 23:01:38 +0000
@@ -15,6 +15,7 @@
import collections
from cStringIO import StringIO
import logging
+from operator import attrgetter
import apt_pkg
from sqlobject import (
@@ -38,6 +39,7 @@
from zope.component import getUtility
from zope.interface import implements
from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
from canonical.database.constants import (
DEFAULT,
@@ -981,32 +983,8 @@
def getCurrentSourceReleases(self, source_package_names):
"""See `IDistroSeries`."""
- source_package_ids = [
- package_name.id for package_name in source_package_names]
- # Construct a table of just current releases for the desired SPNs.
- origin = SQL("""
- SourcePackageRelease
- JOIN (
- SELECT
- spr.id as sourcepackagerelease, MAX(spph.id)
- FROM SourcePackagePublishingHistory spph,
- SourcePackageRelease spr
- WHERE
- spr.sourcepackagename IN %s AND
- spph.sourcepackagerelease = spr.id AND
- spph.archive IN %s AND
- spph.status IN %s AND
- spph.distroseries = %s
- GROUP BY spr.id)
- AS spph ON SourcePackageRelease.id = spph.sourcepackagerelease
- """ % sqlvalues(
- source_package_ids, self.distribution.all_distro_archive_ids,
- active_publishing_status, self))
- releases = IStore(self).using(origin).find(SourcePackageRelease)
- return dict(
- (self.getSourcePackage(release.sourcepackagename),
- DistroSeriesSourcePackageRelease(self, release))
- for release in releases)
+ return getUtility(IDistroSeriesSet).getCurrentSourceReleases(
+ {self:source_package_names})
def getTranslatableSourcePackages(self):
"""See `IDistroSeries`."""
@@ -2001,6 +1979,61 @@
series = distribution.getSeries(series_name)
return series, pocket
+ def getCurrentSourceReleases(self, distro_series_source_packagenames):
+ """See `IDistroSeriesSet`."""
+ # Builds one query for all the distro_series_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 = []
+ distroseries_lookup = {}
+ for distroseries, package_names in \
+ distro_series_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(
+ distroseries.distribution.all_distro_archive_ids)
+ clause = """spr.sourcepackagename IN %s AND
+ spph.archive IN %s AND
+ spph.distroseries = %s
+ """ % sqlvalues(source_package_ids, archives, distroseries.id)
+ series_clauses.append(clause)
+ distroseries_lookup[distroseries.id] = distroseries
+ if not len(series_clauses):
+ return {}
+ if len(series_clauses) > 1:
+ combined_clause = series_clauses[0]
+ else:
+ combined_clause = "(" + " OR ".join(series_clauses) + ")"
+ # The complex join constructs a table of just current releases for the
+ # desired SPNs.
+ origin = SQL("""
+ SourcePackageRelease
+ JOIN (
+ SELECT
+ spr.id as sourcepackagerelease, MAX(spph.id),
+ spph.distroseries as distroseries
+ FROM SourcePackagePublishingHistory spph,
+ SourcePackageRelease spr
+ WHERE
+ spph.sourcepackagerelease = spr.id AND
+ spph.status IN %s AND
+ %s
+ GROUP BY spr.id, spph.distroseries)
+ AS spph ON SourcePackageRelease.id = spph.sourcepackagerelease
+ """ % (sqlvalues(active_publishing_status)+ (combined_clause,)))
+ releases = IStore(DistroSeries).using(origin).find(
+ (SourcePackageRelease, SQL("distroseries")))
+ result = {}
+ for sp_release, distroseries_id in releases:
+ distroseries = distroseries_lookup[distroseries_id]
+ sourcepackage = distroseries.getSourcePackage(
+ sp_release.sourcepackagename)
+ result[sourcepackage] = DistroSeriesSourcePackageRelease(
+ distroseries, sp_release)
+ return result
+
def search(self, distribution=None, isreleased=None, orderBy=None):
"""See `IDistroSeriesSet`."""
where_clause = ""
=== modified file 'lib/lp/soyuz/model/distroseriessourcepackagerelease.py'
--- lib/lp/soyuz/model/distroseriessourcepackagerelease.py 2011-01-12 13:36:38 +0000
+++ lib/lp/soyuz/model/distroseriessourcepackagerelease.py 2011-02-20 23:01:38 +0000
@@ -15,6 +15,7 @@
from lazr.delegates import delegates
from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
from canonical.database.sqlbase import sqlvalues
from lp.soyuz.interfaces.distroseriessourcepackagerelease import (
@@ -182,13 +183,16 @@
@property
def publishing_history(self):
"""See `IDistroSeriesSourcePackage`."""
+ # sqlvalues bails on security proxied objects.
+ archive_ids = removeSecurityProxy(
+ self.distroseries.distribution.all_distro_archive_ids)
return SourcePackagePublishingHistory.select("""
distroseries = %s AND
archive IN %s AND
sourcepackagerelease = %s
""" % sqlvalues(
self.distroseries,
- self.distroseries.distribution.all_distro_archive_ids,
+ archive_ids,
self.sourcepackagerelease),
orderBy='-datecreated')
