Merge lp:~jcsackett/launchpad/shortlist-696913 into lp:launchpad

[j.c.sackett]

[r=benji,edwin-grubbs][ui=none][bug=696913] Adds doNotSnapshot to fields involved in OOPSes with ShortlistTooBigErrors.

Summary
=======

Because we are currently snapshotting most fields in pillars when snapshot is called for. However, certain collections don't need to be snapshotted, and cause an error because the number of objects in the collection exceed hardcoded limits.

This branch adds doNotSnapshot to those collections.

Proposed Fix
============

Add doNotSnapshot to the offending fields. A better solution would involve altering Snapshot to ignore (or be able to ignore) CollectionFields, but as this bug is holding up other work, the simple fix should be landed first. The better solution has been filed as bug 701715.

Preimplementation Talk
======================

Spoke with Curtis Hovey about the problem and the interactions of Snapshot and the various fields. Came up with both the simple fix and the more comprehensive one.

Implementation
==============

lib/lp/blueprints/interfaces/specificationtarget.py
lib/lp/registry/interfaces/milestones.py
lib/lp/registry/interfaces/productseries.py
-------------------------------------------

Added doNotSnapshot to the CollectionFields in IHasSpecifications and IHasMilestones. doNotSnapshot had to be added to IProductSeries milestone CollectionFields directly, b/c the interface redefines those fields, but they still connect to probable causes of the Shortlist error.

lib/lp/blueprints/tests/test_hasspecifications.py
lib/lp/registry/tests/test_milestone.py
lib/lp/registry/tests/test_productseries.py
-------------------------------------------

Added tests.

Test
====

bin/test -t test_productseries
bin/test -t test_milestone
bin/test -t test_hasspecifications

QA
==

On staging, hit the +edit or +configure* links for Launchpad. It should not error with shortlist

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/interfaces/specificationtarget.py
  lib/lp/blueprints/tests/test_hasspecifications.py
  lib/lp/registry/interfaces/milestone.py
  lib/lp/registry/interfaces/productseries.py
  lib/lp/registry/tests/test_milestone.py
  lib/lp/registry/tests/test_productseries.py

---------------------------------------------------

[Jeroen T. Vermeulen (jtv)]

Hi Jon,

I'm glad to see you address this nuisance. You'll make all our lives easier. Danilo was planning to look into it as well this week, since we were getting some of the errors on Translations, but this may just save him the time.

The big question I have though is: could this change confuse clients written to our 1.0 API? We made a promise to keep that API stable. Or do we consider it a plain bug in the client if it can't cope with a collection no longer being snapshotted?

I'm voting "needs fixing," but please don't take that as a vote of no confidence in your approach. It's merely that I'm requesting some changes to the test code that would probably warrant a new review round.

> === modified file 'lib/lp/blueprints/tests/test_hasspecifications.py'
> --- lib/lp/blueprints/tests/test_hasspecifications.py 2010-11-29 18:04:07 +0000
> +++ lib/lp/blueprints/tests/test_hasspecifications.py 2011-01-12 13:35:47 +0000

> @@ -178,3 +184,49 @@
> product=product, name="spec3")
> self.assertNamesOfSpecificationsAre(
> ["spec1"], person.valid_specifications)
> +
> +
> +class HasSpecificationsSnapshotTestCase(TestCaseWithFactory):
> + """A TestCase for snapshots of specification targets."""
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(HasSpecificationsSnapshotTestCase, self).setUp()

This method looks redundant. Might as well leave it out AFAICS.

> + def check_omissions(self, target, providing):

What omissions are being checked, exactly?

The word "omission" has strong negative connotations: it generally implies that something is missing that shouldn't be. If this method is meant to check that collections are _properly omitted_ from the snapshot, maybe there's some better word.

> + """snapshots of objects with IHasSpecification should not snapshot
> + marked attributes.

Please start your sentence with a capital letter!

This is a statement of fact, but the first line of a docstring is expected to describe what the method actually does for the caller.

> + wrap an export with 'donotsnapshot' to force the snapshot to not
> + include that attribute.

Please start your sentence with a capital letter!

Is the imperative an instruction to this method's caller? Or a description of what the method does in the imperative form? Or general advice to programmers?

> + """
> + snapshot = Snapshot(target, providing=providing)

It's probably just my shallow understanding of Zope, but I would have expected "providing=IHasSpecifications" to work here.

> + omitted = [
> + 'all_specifications',
> + 'valid_specifications',
> + ]
> + for attribute in omitted:
> + self.assertFalse(
> + hasattr(snapshot, attribute),
> + "snapshot should not include %s." % attribute)

Please start your sentence with a capital letter!

> === modified file 'lib/lp/registry/tests/test_milestone.py'
> --- lib/lp/registry/tests/test_milestone.py 2010-10-04 19:50:45 +0000
> +++ lib/lp/registry/tests/test_milestone.py 2011-01-12 13:35:47 +0000

> @@ -81,6 +91,48 @@
...

review: Needs Fixing

[j.c.sackett (jcsackett)]

> Danilo was planning to look into it as well this week, since we were getting
> some of the errors on Translations, but this may just save him the time.

Danilo and I actually talked about this yesterday; my branch incorporates the fix he had as well.

> The big question I have though is: could this change confuse clients written
> to our 1.0 API? We made a promise to keep that API stable. Or do we consider
> it a plain bug in the client if it can't cope with a collection no longer
> being snapshotted?

I don't believe this will be an issue--CollectionFields at large can still be snapshotted; these particular fields just are no longer included. I don't see how that would effect a client, but I may be missing something?

> > === modified file 'lib/lp/blueprints/tests/test_hasspecifications.py'
> > --- lib/lp/blueprints/tests/test_hasspecifications.py 2010-11-29 18:04:07
> +0000
> > +++ lib/lp/blueprints/tests/test_hasspecifications.py 2011-01-12 13:35:47
> +0000
>
> > @@ -178,3 +184,49 @@
> > product=product, name="spec3")
> > self.assertNamesOfSpecificationsAre(
> > ["spec1"], person.valid_specifications)
> > +
> > +
> > +class HasSpecificationsSnapshotTestCase(TestCaseWithFactory):
> > + """A TestCase for snapshots of specification targets."""
> > +
> > + layer = DatabaseFunctionalLayer
> > +
> > + def setUp(self):
> > + super(HasSpecificationsSnapshotTestCase, self).setUp()
>
> This method looks redundant. Might as well leave it out AFAICS.

For TestCaseWithFactory descendants, you have to construct it this way. I'm not certain why, but in the past these classes fail otherwise. You might also see code that calls it in the parent setUp directly in the in __init__ instead.

> > + def check_omissions(self, target, providing):
>
> What omissions are being checked, exactly?
>
> The word "omission" has strong negative connotations: it generally implies
> that something is missing that shouldn't be. If this method is meant to check
> that collections are _properly omitted_ from the snapshot, maybe there's some
> better word.

It's checking to make sure that the correct fields are omitted from the snapshot. I suppose I can use skipped instead, if omitted is too negative.

> > + snapshot = Snapshot(target, providing=providing)
>
> It's probably just my shallow understanding of Zope, but I would have expected
> "providing=IHasSpecifications" to work here.

It might, but the test is checking the workflow for a snapshot of the interface normally snapshotted, so providing can be (and is, in each test) IProduct, IDistribution, etc. I thought it made for a better test this way, but doing it the way you suggest would decouple it from other interfaces, so I'll make that change.

> > === modified file 'lib/lp/registry/tests/test_milestone.py'
> > --- lib/lp/registry/tests/test_milestone.py 2010-10-04 19:50:45 +0000
> > +++ lib/lp/registry/tests/test_milestone.py 2011-01-12 13:35:47 +0000
>
> > @@ -81,6 +91,48 @@
> > [1, 2, 3])
> >
> >
> > +class HasMilestonesSnapshotTestCase(TestCaseWithFactory):
> > + """A TestCase for snapshots of pillars with milestones."""
> > +
>...

[j.c.sackett (jcsackett)]

I have made the changes outlined above.

I don't believe that this will be an API incompatibility.

[Benji York (benji)]

I believe you've addressed (and well) all of Jeroen's comments. I have
only two minor tweaks to suggest:

The use of "list()" to construct an empty list on line 274 of your diff
is a bit non-idiomatic. The list literal is more common.

The style guide (https://dev.launchpad.net/PythonStyleGuide#Truth%20conditionals)
prefers "if len(mismatches) == 0:" over "if mismatches == []:" for the
expression on line 279.

With those two changes your incremental diff looks great.

review: Approve (code*)

[j.c.sackett (jcsackett)]

I'll make those two tweaks.

Also, I have confirmed with Leonard Richardson that this branch shouldn't cause API incompatability.

3:05 PM leonardr
jcsackett: when an entry is changed, we take a snapshot before making the change, and send the snapshot along with the new object in an ObjectModifiedEvent

3:05 PM leonardr
so that anyone who cares about the change can see what changed

3:05 PM
afaik that's the only place where lazr.restful uses snapshots

3:05 PM leonardr
and you can't change a collection field

3:05 PM
so unless i'm missing something, this should not change the behavior of the web service

[Edwin Grubbs (edwin-grubbs)]

> I believe you've addressed (and well) all of Jeroen's comments. I have
> only two minor tweaks to suggest:
>
> The use of "list()" to construct an empty list on line 274 of your diff
> is a bit non-idiomatic. The list literal is more common.
>
> The style guide
> (https://dev.launchpad.net/PythonStyleGuide#Truth%20conditionals)
> prefers "if len(mismatches) == 0:" over "if mismatches == []:" for the
> expression on line 279.
>
> With those two changes your incremental diff looks great.

Hi JC,

I agree with Benji's comments. Also, the test_hasspecifications.py has a couple of minor issues that are breaking it.

Looks good.

-Edwin

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

Now that's team work! Benji, good review.

=== modified file 'lib/lp/blueprints/interfaces/specificationtarget.py'
--- lib/lp/blueprints/interfaces/specificationtarget.py	2010-11-25 19:09:11 +0000
+++ lib/lp/blueprints/interfaces/specificationtarget.py	2011-01-13 14:37:26 +0000
@@ -19,6 +19,7 @@
     )
 from zope.schema import TextLine
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     exported,
     export_as_webservice_entry,
@@ -42,21 +43,21 @@
     associated with them, and you can use this interface to query those.
     """
 
-    all_specifications = exported(
+    all_specifications = exported(doNotSnapshot(
         CollectionField(
             title=_("All specifications"),
             value_type=Reference(schema=Interface),  # ISpecification, really.
             readonly=True,
             description=_(
                 'A list of all specifications, regardless of status or '
-                'approval or completion, for this object.')),
+                'approval or completion, for this object.'))),
         ('devel', dict(exported=True)), exported=False)
 
     has_any_specifications = Attribute(
         'A true or false indicator of whether or not this object has any '
         'specifications associated with it, regardless of their status.')
 
-    valid_specifications = exported(
+    valid_specifications = exported(doNotSnapshot(
         CollectionField(
             title=_("Valid specifications"),
             value_type=Reference(schema=Interface),  # ISpecification, really.
@@ -64,7 +65,7 @@
             description=_(
                 'All specifications that are not obsolete. When called from '
                 'an ISpecificationGoal it will also exclude the ones that '
-                'have not been accepted for that goal')),
+                'have not been accepted for that goal'))),
         ('devel', dict(exported=True)), exported=False)
 
     latest_specifications = Attribute(

=== modified file 'lib/lp/blueprints/tests/test_hasspecifications.py'
--- lib/lp/blueprints/tests/test_hasspecifications.py	2010-11-29 18:04:07 +0000
+++ lib/lp/blueprints/tests/test_hasspecifications.py	2011-01-13 14:37:26 +0000
@@ -5,12 +5,15 @@
 
 __metaclass__ = type
 
-
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.blueprints.interfaces.specification import (
     SpecificationDefinitionStatus,
     )
+from lp.blueprints.interfaces.specificationtarget import (
+    IHasSpecifications,
+    )
 from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import DoesNotSnapshot
 
 
 class HasSpecificationsTests(TestCaseWithFactory):
@@ -178,3 +181,37 @@
             product=product, name="spec3")
         self.assertNamesOfSpecificationsAre(
             ["spec1"], person.valid_specifications)
+
+
+class HasSpecificationsSnapshotTestCase(TestCaseWithFactory):
+    """A TestCase for snapshots of specification targets."""
+
+    layer = DatabaseFunctionalLayer
+
+    def check_skipped(self, target):
+        """Asserts that fields marked doNotSnapshot are skipped."""
+        skipped = [
+            'all_specifications',
+            'valid_specifications',
+            ]
+        self.assertThat(target, DoesNotSnapshot(skipped, IHasSpecifications))
+
+    def test_product(self):
+        product = self.factory.makeProduct()
+        self.check_skipped(product)
+
+    def test_distribution(self):
+        distribution = self.factory.makeDistribution()
+        self.check_skipped(distribution)
+
+    def test_productseries(self):
+        productseries = self.factory.makeProductSeries()
+        self.check_skipped(productseries)
+
+    def test_distroseries(self):
+        distroseries = self.factory.makeDistroSeries()
+        self.check_skipped(distroseries)
+
+    def test_projectgroup(self):
+        projectgroup = self.factory.makeProject()
+        self.check_skipped(projectgroup)

=== modified file 'lib/lp/registry/interfaces/milestone.py'
--- lib/lp/registry/interfaces/milestone.py	2010-12-01 22:26:50 +0000
+++ lib/lp/registry/interfaces/milestone.py	2011-01-13 14:37:26 +0000
@@ -15,6 +15,7 @@
     'IProjectGroupMilestone',
     ]
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     call_with,
     export_as_webservice_entry,
@@ -39,7 +40,6 @@
 from zope.schema import (
     Bool,
     Choice,
-    Date,
     Int,
     TextLine,
     )
@@ -255,18 +255,18 @@
 
     has_milestones = Bool(title=_("Whether the object has any milestones."))
 
-    milestones = exported(
+    milestones = exported(doNotSnapshot(
         CollectionField(
             title=_("The visible and active milestones associated with this "
                     "object, ordered by date expected."),
-            value_type=Reference(schema=IMilestone)),
+            value_type=Reference(schema=IMilestone))),
         exported_as='active_milestones')
 
-    all_milestones = exported(
+    all_milestones = exported(doNotSnapshot(
         CollectionField(
             title=_("All milestones associated with this object, ordered by "
                     "date expected."),
-            value_type=Reference(schema=IMilestone)))
+            value_type=Reference(schema=IMilestone))))
 
 
 class ICanGetMilestonesDirectly(Interface):

=== modified file 'lib/lp/registry/interfaces/productseries.py'
--- lib/lp/registry/interfaces/productseries.py	2010-11-27 05:40:38 +0000
+++ lib/lp/registry/interfaces/productseries.py	2011-01-13 14:37:26 +0000
@@ -16,6 +16,7 @@
     'ITimelineProductSeries',
     ]
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     export_as_webservice_entry,
     export_factory_operation,
@@ -212,20 +213,20 @@
     sourcepackages = Attribute(_("List of distribution packages for this "
         "product series"))
 
-    milestones = exported(
+    milestones = exported(doNotSnapshot(
         CollectionField(
             title=_("The visible milestones associated with this "
                     "project series, ordered by date expected."),
             readonly=True,
-            value_type=Reference(schema=IMilestone)),
+            value_type=Reference(schema=IMilestone))),
         exported_as='active_milestones')
 
-    all_milestones = exported(
+    all_milestones = exported(doNotSnapshot(
         CollectionField(
             title=_("All milestones associated with this project series, "
                     "ordered by date expected."),
             readonly=True,
-            value_type=Reference(schema=IMilestone)))
+            value_type=Reference(schema=IMilestone))))
 
     branch = exported(
         ReferenceChoice(

=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/tests/test_milestone.py	2011-01-13 14:37:26 +0000
@@ -14,12 +14,19 @@
     login,
     logout,
     )
-from canonical.testing.layers import LaunchpadFunctionalLayer
-
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.registry.interfaces.milestone import IMilestoneSet
+from lp.registry.interfaces.milestone import (
+    IHasMilestones,
+    IMilestoneSet,
+    )
 from lp.registry.interfaces.product import IProductSet
+from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import DoesNotSnapshot
 
 
 class MilestoneTest(unittest.TestCase):
@@ -81,10 +88,31 @@
             [1, 2, 3])
 
 
-def test_suite():
-    """Return the test suite for the tests in this module."""
-    return unittest.TestLoader().loadTestsFromName(__name__)
-
-
-if __name__ == '__main__':
-    unittest.main()
+class HasMilestonesSnapshotTestCase(TestCaseWithFactory):
+    """A TestCase for snapshots of pillars with milestones."""
+
+    layer = DatabaseFunctionalLayer
+
+    def check_skipped(self, target):
+        """Asserts that fields marked doNotSnapshot are skipped."""
+        skipped = [
+            'milestones',
+            'all_milestones',
+            ]
+        self.assertThat(target, DoesNotSnapshot(skipped, IHasMilestones))
+
+    def test_product(self):
+        product = self.factory.makeProduct()
+        self.check_skipped(product)
+
+    def test_distribution(self):
+        distribution = self.factory.makeDistribution()
+        self.check_skipped(distribution)
+
+    def test_distroseries(self):
+        distroseries = self.factory.makeDistroSeries()
+        self.check_skipped(distroseries)
+
+    def test_projectgroup(self):
+        projectgroup = self.factory.makeProject()
+        self.check_skipped(projectgroup)

=== modified file 'lib/lp/registry/tests/test_productseries.py'
--- lib/lp/registry/tests/test_productseries.py	2010-10-18 20:40:05 +0000
+++ lib/lp/registry/tests/test_productseries.py	2011-01-13 14:37:26 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-from unittest import TestLoader
-
 from zope.component import getUtility
 
 from canonical.launchpad.ftests import login
@@ -17,8 +15,12 @@
     )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
-from lp.registry.interfaces.productseries import IProductSeriesSet
+from lp.registry.interfaces.productseries import (
+    IProductSeries,
+    IProductSeriesSet,
+    )
 from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import DoesNotSnapshot
 from lp.translations.interfaces.translations import (
     TranslationsBranchImportMode,
     )
@@ -301,5 +303,18 @@
             self.productseries.getLatestRelease())
 
 
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)
+class ProductSeriesSnapshotTestCase(TestCaseWithFactory):
+    """A TestCase for snapshots of productseries."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_productseries(self):
+        """Asserts that fields marked doNotSnapshot are skipped."""
+        productseries = self.factory.makeProductSeries()
+        skipped = [
+            'milestones',
+            'all_milestones',
+            ]
+        self.assertThat(
+            productseries,
+            DoesNotSnapshot(skipped, IProductSeries))

=== modified file 'lib/lp/testing/matchers.py'
--- lib/lp/testing/matchers.py	2010-12-13 21:40:56 +0000
+++ lib/lp/testing/matchers.py	2011-01-13 14:37:26 +0000
@@ -13,6 +13,7 @@
     'ProvidesAndIsProxied',
     ]
 
+from lazr.lifecycle.snapshot import Snapshot
 from testtools.content import Content
 from testtools.content_type import UTF8_TEXT
 from testtools.matchers import (
@@ -141,7 +142,7 @@
         result = []
         for query in self.query_collector.queries:
             result.append(unicode(query).encode('utf8'))
-        return {'queries': Content(UTF8_TEXT, lambda:['\n'.join(result)])}
+        return {'queries': Content(UTF8_TEXT, lambda: ['\n'.join(result)])}
 
 
 class IsNotProxied(Mismatch):
@@ -265,3 +266,39 @@
                 mismatches.append(mismatch)
         if mismatches:
             return MismatchesAll(mismatches)
+
+
+class WasSnapshotted(Mismatch):
+
+    def __init__(self, matchee, attribute):
+        self.matchee = matchee
+        self.attribute = attribute
+
+    def describe(self):
+        return "Snapshot of %s should not include %s" % (
+            self.matchee, self.attribute)
+
+
+class DoesNotSnapshot(Matcher):
+    """Checks that certain fields are skipped on Snapshots."""
+
+    def __init__(self, attr_list, interface, error_msg=None):
+        self.attr_list = attr_list
+        self.interface = interface
+        self.error_msg = error_msg
+
+    def __str__(self):
+        return "Does not include %s when Snapshot is provided %s." % (
+            ', '.join(self.attr_list), self.interface)
+
+    def match(self, matchee):
+        snapshot = Snapshot(matchee, providing=self.interface)
+        mismatches = []
+        for attribute in self.attr_list:
+            if hasattr(snapshot, attribute):
+                mismatches.append(WasSnapshotted(matchee, attribute))
+
+        if len(mismatches) == 0:
+            return None
+        else:
+            return MismatchesAll(mismatches)