launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05264
[Merge] lp:~jtv/launchpad/bug-870130 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-870130 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #870130 in Launchpad itself: "shortlist error requesting recipe build"
https://bugs.launchpad.net/launchpad/+bug/870130
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-870130/+merge/79691
= Summary =
We were getting some errors from the API; they're blocking important Ubuntu work.
The problem is that some interfaces have collection-type attributes that get too big to be included in the API objects.
== Proposed fix ==
Mark these attributes as doNotSnapshot.
== Pre-implementation notes ==
I checked that this is not considered a break in backward compatibility of the API. Graham confirmed: theoretically some client programs may expect to see these attributes snapshotted, but the standing decision is to consider that a bug in the client.
== Implementation details ==
This is the standard solution for these problems. To be honest I'm not sure why we made snapshotting the default in the first place.
While I was at it, I also added a test to verify that Product really does implement its numerous interfaces. This may already be tested somewhere but if so, I didn't find it.
== Tests ==
{{{
./bin/test -vvc lp.code.model.tests.test_sourcepackagerecipe
./bin/test -vvc lp.registry.tests.test_product
}}}
== Demo and Q/A ==
The bug report has instrutions on reproducing the bug. It should be gone now.
In addition, loading a Product through the API should still work.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/model/tests/test_sourcepackagerecipe.py
lib/lp/code/model/sourcepackagerecipebuild.py
lib/lp/code/interfaces/sourcepackagerecipe.py
lib/lp/code/interfaces/hasrecipes.py
lib/lp/registry/tests/test_product.py
--
https://code.launchpad.net/~jtv/launchpad/bug-870130/+merge/79691
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-870130 into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/hasrecipes.py'
--- lib/lp/code/interfaces/hasrecipes.py 2011-02-23 01:24:09 +0000
+++ lib/lp/code/interfaces/hasrecipes.py 2011-10-18 13:53:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Interface definitions for IHasRecipes."""
@@ -9,23 +9,22 @@
]
-from zope.interface import (
- Interface,
- )
-
+from lazr.lifecycle.snapshot import doNotSnapshot
from lazr.restful.declarations import exported
from lazr.restful.fields import (
CollectionField,
Reference,
)
+from zope.interface import Interface
from canonical.launchpad import _
+
class IHasRecipes(Interface):
"""An object that has recipes."""
- recipes = exported(
+ recipes = exported(doNotSnapshot(
CollectionField(
title=_("All recipes associated with the object."),
value_type=Reference(schema=Interface),
- readonly=True))
+ readonly=True)))
=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py 2011-05-09 17:46:54 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py 2011-10-18 13:53:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213,F0401
@@ -19,6 +19,7 @@
from textwrap import dedent
+from lazr.lifecycle.snapshot import doNotSnapshot
from lazr.restful.declarations import (
call_with,
export_as_webservice_entry,
@@ -82,7 +83,7 @@
deb_version_template = exported(
TextLine(
title=_('deb-version template'), readonly=True,
- description = _(
+ description=_(
'The template that will be used to generate a deb version.')))
def getReferencedBranches():
@@ -104,31 +105,31 @@
recipe_text = exported(Text(readonly=True))
- pending_builds = exported(
+ pending_builds = exported(doNotSnapshot(
CollectionField(
title=_("The pending builds of this recipe."),
description=_('Pending builds of this recipe, sorted in '
'descending order of creation.'),
value_type=Reference(schema=Interface),
- readonly=True))
+ readonly=True)))
- completed_builds = exported(
+ completed_builds = exported(doNotSnapshot(
CollectionField(
title=_("The completed builds of this recipe."),
description=_('Completed builds of this recipe, sorted in '
'descending order of finishing (or starting if not'
'completed successfully).'),
value_type=Reference(schema=Interface),
- readonly=True))
+ readonly=True)))
- builds = exported(
+ builds = exported(doNotSnapshot(
CollectionField(
title=_("All builds of this recipe."),
description=_('All builds of this recipe, sorted in '
'descending order of finishing (or starting if not'
'completed successfully).'),
value_type=Reference(schema=Interface),
- readonly=True))
+ readonly=True)))
last_build = exported(
Reference(
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py 2011-07-14 04:18:59 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py 2011-10-18 13:53:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=F0401,E1002
@@ -25,9 +25,7 @@
Storm,
)
from storm.store import Store
-from zope.component import (
- getUtility,
- )
+from zope.component import getUtility
from zope.interface import (
classProvides,
implements,
@@ -67,9 +65,9 @@
from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.services.job.model.job import Job
+from lp.soyuz.interfaces.archive import CannotUploadToArchive
from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
from lp.soyuz.model.buildfarmbuildjob import BuildFarmBuildJob
-from lp.soyuz.interfaces.archive import CannotUploadToArchive
from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
@@ -99,9 +97,10 @@
@property
def binary_builds(self):
"""See `ISourcePackageRecipeBuild`."""
- return Store.of(self).find(BinaryPackageBuild,
+ return Store.of(self).find(
+ BinaryPackageBuild,
BinaryPackageBuild.source_package_release ==
- SourcePackageRelease.id,
+ SourcePackageRelease.id,
SourcePackageRelease.source_package_recipe_build == self.id)
@property
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py 2011-09-26 06:30:07 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py 2011-10-18 13:53:28 +0000
@@ -12,6 +12,7 @@
import textwrap
from bzrlib.plugins.builder.recipe import ForbiddenInstructionError
+from lazr.lifecycle.event import ObjectModifiedEvent
from pytz import UTC
from storm.locals import Store
import transaction
@@ -20,7 +21,6 @@
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
-from lazr.lifecycle.event import ObjectModifiedEvent
from canonical.database.constants import UTC_NOW
from canonical.launchpad.webapp.authorization import check_permission
from canonical.launchpad.webapp.testing import verifyObject
@@ -40,6 +40,7 @@
from lp.code.interfaces.sourcepackagerecipe import (
ISourcePackageRecipe,
ISourcePackageRecipeSource,
+ ISourcePackageRecipeView,
MINIMAL_RECIPE_TEXT,
)
from lp.code.interfaces.sourcepackagerecipebuild import (
@@ -76,6 +77,7 @@
TestCaseWithFactory,
ws_object,
)
+from lp.testing.matchers import DoesNotSnapshot
class TestSourcePackageRecipe(TestCaseWithFactory):
@@ -92,6 +94,16 @@
recipe = self.factory.makeSourcePackageRecipe()
verifyObject(ISourcePackageRecipe, recipe)
+ def test_avoids_problematic_snapshots(self):
+ problematic_properties = [
+ 'builds',
+ 'completed_builds',
+ 'pending_builds',
+ ]
+ self.assertThat(
+ self.factory.makeSourcePackageRecipe(),
+ DoesNotSnapshot(problematic_properties, ISourcePackageRecipeView))
+
def makeRecipeComponents(self, branches=()):
"""Return a dict of values that can be used to make a recipe.
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py 2011-10-12 11:10:39 +0000
+++ lib/lp/registry/tests/test_product.py 2011-10-18 13:53:28 +0000
@@ -6,10 +6,15 @@
from cStringIO import StringIO
import datetime
-from lazr.lifecycle.snapshot import Snapshot
import pytz
import transaction
+from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.interfaces.launchpad import (
+ IHasIcon,
+ IHasLogo,
+ IHasMugshot,
+ )
from canonical.launchpad.testing.pages import (
find_main_content,
get_feedback_messages,
@@ -18,8 +23,17 @@
from canonical.testing.layers import (
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
+ ZopelessDatabaseLayer,
)
+from lp.answers.interfaces.faqtarget import IFAQTarget
from lp.app.enums import ServiceUsage
+from lp.app.interfaces.launchpad import (
+ ILaunchpadUsage,
+ IServiceUsage,
+ )
+from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
+from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
+from lp.bugs.interfaces.bugtarget import IHasBugHeat
from lp.registry.interfaces.product import (
IProduct,
License,
@@ -39,7 +53,14 @@
TestCaseWithFactory,
WebServiceTestCase,
)
+from lp.testing.matchers import (
+ DoesNotSnapshot,
+ Provides,
+ )
from lp.translations.enums import TranslationPermission
+from lp.translations.interfaces.customlanguagecode import (
+ IHasCustomLanguageCodes,
+ )
class TestProduct(TestCaseWithFactory):
@@ -52,6 +73,21 @@
product = self.factory.makeProduct()
self.assertEqual("Project", product.pillar_category)
+ def test_implements_interfaces(self):
+ # Product fully implements its interfaces.
+ product = removeSecurityProxy(self.factory.makeProduct())
+ self.assertThat(product, Provides(IProduct))
+ self.assertThat(product, Provides(IBugSummaryDimension))
+ self.assertThat(product, Provides(IFAQTarget))
+ self.assertThat(product, Provides(IHasBugHeat))
+ self.assertThat(product, Provides(IHasBugSupervisor))
+ self.assertThat(product, Provides(IHasCustomLanguageCodes))
+ self.assertThat(product, Provides(IHasIcon))
+ self.assertThat(product, Provides(IHasLogo))
+ self.assertThat(product, Provides(IHasMugshot))
+ self.assertThat(product, Provides(ILaunchpadUsage))
+ self.assertThat(product, Provides(IServiceUsage))
+
def test_deactivation_failure(self):
# Ensure that a product cannot be deactivated if
# it is linked to source packages.
@@ -306,29 +342,30 @@
class ProductSnapshotTestCase(TestCaseWithFactory):
- """A TestCase for product snapshots."""
-
- layer = DatabaseFunctionalLayer
+ """Test product snapshots.
+
+ Some attributes of a product should not be included in snapshots,
+ typically because they are either too costly to fetch unless there's
+ a real need, or because they get too big and trigger a shortlist
+ overflow error.
+
+ To stop an attribute from being snapshotted, wrap its declaration in
+ the interface in `doNotSnapshot`.
+ """
+
+ layer = ZopelessDatabaseLayer
def setUp(self):
super(ProductSnapshotTestCase, self).setUp()
self.product = self.factory.makeProduct(name="shamwow")
- def test_snapshot(self):
- """Snapshots of products should not include marked attribues.
-
- Wrap an export with 'doNotSnapshot' to force the snapshot to not
- include that attribute.
- """
- snapshot = Snapshot(self.product, providing=IProduct)
+ def test_excluded_from_snapshot(self):
omitted = [
'series',
+ 'recipes',
'releases',
]
- for attribute in omitted:
- self.assertFalse(
- hasattr(snapshot, attribute),
- "Snapshot should not include %s." % attribute)
+ self.assertThat(self.product, DoesNotSnapshot(omitted, IProduct))
class BugSupervisorTestCase(TestCaseWithFactory):
