launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03640
[Merge] lp:~sinzui/launchpad/product-release-traversal into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/product-release-traversal into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #736011 in Launchpad itself: "ProductRelease:+rdf timeout"
https://bugs.launchpad.net/launchpad/+bug/736011
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/product-release-traversal/+merge/61168
Traversal to a product release must be a single query.
Launchpad bug: https://bugs.launchpad.net/bugs/736011
Pre-implementation: lifeless
ProductRelease:+rdf timeout is caused by the unneeded loading of hundreds
of milestones during traversal to locate the match release version. This
issue is broader than +rdf, +index will also timeout if the release is deep
into a list of hundreds or milestones.
ProductSeriesNavigation.traverse() calls ProductSeries.getRelease(), which
iterates over all releases (and their parent milestone) to locate the
matching version.
--------------------------------------------------------------------
RULES
* ProductSeriesNavigation.traverse() could call
ProductReleaseSet.getBySeriesAndVersion() to get the release in a
single query.
* ProductReleaseSet.getBySeriesAndVersion() has been broken for 2 years;
update the query to return the release for the matching milestone.name.
QA
* View https://launchpad.net/libpng/main/0.89c
* Verify it does not timeout.
* View https://launchpad.net/libpng/main/1.4.0beta35-no-config
* Verify it does not timeout.
LINT
lib/lp/registry/model/productrelease.py
lib/lp/registry/model/productseries.py
lib/lp/registry/tests/test_productrelease.py
lib/lp/registry/tests/test_productseries.py
^ There is lint in model/productseries.py that I can address after the review.
TEST
./bin/test -vv \
-m lp.registry.tests.test_productrelease \
-t -t registry.*ProductSeries.*TestCase
IMPLEMENTATION
Updated ProductReleaseSet.getBySeriesAndVersion to query the release by
milestone name.
lib/lp/registry/model/productrelease.py
lib/lp/registry/tests/test_productrelease.py
Updated ProductSeries.getRelease() to use ProductReleaseSet's
getBySeriesAndVersion instead of iterating over all the releases. Updated
the releases property to cache the milestone most uses of a release require
access to its milestone.
lib/lp/registry/model/productseries.py
lib/lp/registry/tests/test_productseries.py
--
https://code.launchpad.net/~sinzui/launchpad/product-release-traversal/+merge/61168
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/product-release-traversal into lp:launchpad.
=== modified file 'lib/lp/registry/model/productrelease.py'
--- lib/lp/registry/model/productrelease.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/productrelease.py 2011-05-16 20:07:32 +0000
@@ -14,7 +14,6 @@
from StringIO import StringIO
from sqlobject import (
- AND,
ForeignKey,
SQLMultipleJoin,
StringCol,
@@ -35,11 +34,7 @@
sqlvalues,
)
from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
-from canonical.launchpad.webapp.interfaces import (
- DEFAULT_FLAVOR,
- IStoreSelector,
- MAIN_STORE,
- )
+from canonical.launchpad.interfaces.lpstorm import IStore
from lp.app.errors import NotFoundError
from lp.registry.interfaces.person import (
validate_person,
@@ -243,12 +238,21 @@
def getBySeriesAndVersion(self, productseries, version, default=None):
"""See `IProductReleaseSet`."""
- query = AND(ProductRelease.q.version==version,
- ProductRelease.q.productseriesID==productseries.id)
- productrelease = ProductRelease.selectOne(query)
- if productrelease is None:
- return default
- return productrelease
+ # Local import of Milestone to avoid circular imports.
+ from lp.registry.model.milestone import Milestone
+ store = IStore(productseries)
+ # The Milestone is cached too because most uses of a ProductRelease
+ # need it.
+ result = store.find(
+ (ProductRelease, Milestone),
+ Milestone.productseries == productseries,
+ ProductRelease.milestone == Milestone.id,
+ Milestone.name == version)
+ found = result.one()
+ if found is None:
+ return None
+ product_release, milestone = found
+ return product_release
def getReleasesForSeries(self, series):
"""See `IProductReleaseSet`."""
@@ -256,7 +260,7 @@
from lp.registry.model.milestone import Milestone
if len(list(series)) == 0:
return EmptyResultSet()
- store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+ store = IStore(series)
series_ids = [s.id for s in series]
result = store.find(
ProductRelease,
=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py 2011-04-20 20:53:36 +0000
+++ lib/lp/registry/model/productseries.py 2011-05-16 20:07:32 +0000
@@ -40,6 +40,9 @@
SQLBase,
sqlvalues,
)
+from canonical.launchpad.components.decoratedresultset import (
+ DecoratedResultSet,
+ )
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.webapp.publisher import canonical_url
from canonical.launchpad.webapp.sorting import sorted_dotted_numbers
@@ -74,6 +77,7 @@
)
from lp.registry.interfaces.packaging import PackagingType
from lp.registry.interfaces.person import validate_person
+from lp.registry.interfaces.productrelease import IProductReleaseSet
from lp.registry.interfaces.productseries import (
IProductSeries,
IProductSeriesSet,
@@ -202,11 +206,19 @@
def releases(self):
"""See `IProductSeries`."""
store = Store.of(self)
+
+ # The Milestone is cached too because most uses of a ProductRelease
+ # need it. The decorated resultset returns just the ProductRelease.
+ def decorate(row):
+ product_release, milestone = row
+ return product_release
+
result = store.find(
- ProductRelease,
- And(Milestone.productseries == self,
- ProductRelease.milestone == Milestone.id))
- return result.order_by(Desc('datereleased'))
+ (ProductRelease, Milestone),
+ Milestone.productseries == self,
+ ProductRelease.milestone == Milestone.id)
+ result = result.order_by(Desc('datereleased'))
+ return DecoratedResultSet(result, decorate)
@property
def release_files(self):
@@ -458,10 +470,8 @@
return None
def getRelease(self, version):
- for release in self.releases:
- if release.version == version:
- return release
- return None
+ return getUtility(IProductReleaseSet).getBySeriesAndVersion(
+ self, version)
def getPackage(self, distroseries):
"""See IProductSeries."""
=== added file 'lib/lp/registry/tests/test_productrelease.py'
--- lib/lp/registry/tests/test_productrelease.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_productrelease.py 2011-05-16 20:07:32 +0000
@@ -0,0 +1,47 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test product releases and product release set."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.registry.interfaces.productrelease import IProductReleaseSet
+from lp.testing import TestCaseWithFactory
+
+
+class ProductReleaseSetTestcase(TestCaseWithFactory):
+ """Tests for ProductReleaseSet."""
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(ProductReleaseSetTestcase, self).setUp()
+ self.product_release_set = getUtility(IProductReleaseSet)
+
+ def test_getBySeriesAndVersion_match(self):
+ # The release is returned when there is a matching release version.
+ milestone = self.factory.makeMilestone(name='0.0.1')
+ release = self.factory.makeProductRelease(milestone=milestone)
+ found = self.product_release_set.getBySeriesAndVersion(
+ milestone.series_target, '0.0.1')
+ self.assertEqual(release, found)
+
+ def test_getBySeriesAndVersion_none(self):
+ # None is returned when there is no matching release version.
+ milestone = self.factory.makeMilestone(name='0.0.1')
+ found = self.product_release_set.getBySeriesAndVersion(
+ milestone.series_target, '0.0.1')
+ self.assertEqual(None, found)
+
+ def test_getBySeriesAndVersion_caches_milestone(self):
+ # The release's milestone was cached when the release was retrieved.
+ milestone = self.factory.makeMilestone(name='0.0.1')
+ self.factory.makeProductRelease(milestone=milestone)
+ series = milestone.series_target
+ IStore(series).invalidate()
+ release = self.product_release_set.getBySeriesAndVersion(
+ series, '0.0.1')
+ self.assertStatementCount(0, getattr, release, 'milestone')
=== modified file 'lib/lp/registry/tests/test_productseries.py'
--- lib/lp/registry/tests/test_productseries.py 2011-04-20 20:53:36 +0000
+++ lib/lp/registry/tests/test_productseries.py 2011-05-16 20:07:32 +0000
@@ -10,6 +10,7 @@
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.ftests import login
+from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.testing.layers import (
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
@@ -32,6 +33,53 @@
)
+class ProductSeriesReleasesTestCase(TestCaseWithFactory):
+ """Test for ProductSeries.release property."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_releases(self):
+ # The release property returns an iterator of releases ordered
+ # by date_released from youngest to oldest.
+ series = self.factory.makeProductSeries()
+ milestone = self.factory.makeMilestone(
+ name='0.0.1', productseries=series)
+ release_1 = self.factory.makeProductRelease(milestone=milestone)
+ milestone = self.factory.makeMilestone(
+ name='0.0.2', productseries=series)
+ release_2 = self.factory.makeProductRelease(milestone=milestone)
+ self.assertEqual(
+ [release_2, release_1], list(series.releases))
+
+ def test_releases_caches_milestone(self):
+ # The release's milestone was cached when the release was retrieved.
+ milestone = self.factory.makeMilestone(name='0.0.1')
+ self.factory.makeProductRelease(milestone=milestone)
+ series = milestone.series_target
+ IStore(series).invalidate()
+ [release] = [release for release in series.releases]
+ self.assertStatementCount(0, getattr, release, 'milestone')
+
+
+class ProductSeriesGetReleaseTestCase(TestCaseWithFactory):
+ """Test for ProductSeries.getRelease()."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_getRelease_match(self):
+ # The release is returned when there is a matching release version.
+ milestone = self.factory.makeMilestone(name='0.0.1')
+ release = self.factory.makeProductRelease(milestone=milestone)
+ series = milestone.series_target
+ self.assertEqual(release, series.getRelease('0.0.1'))
+
+ def test_getRelease_None(self):
+ # None is returned when there is no matching release version.
+ milestone = self.factory.makeMilestone(name='0.0.1')
+ series = milestone.series_target
+ self.assertEqual(None, series.getRelease('0.0.1'))
+
+
class TestProductSeriesSetPackaging(TestCaseWithFactory):
"""Test for ProductSeries.setPackaging()."""
