Merge lp:~adeuring/launchpad/bug-507642 into lp:launchpad

[Abel Deuring]

This branch removes the properties IBug.subscriptions,
IBug.users_affected IBug.users_unaffected,
IBug.users_affected_with_dupes and IBug.messages from snapshots
of IBug objects.

It is at present impossible to (un)subscribe from bug 1 via AJAX.
(other manipulations of bug 1 via the webservice API like marking
oneself as being affected by this bug don't work either.) Attempts
to do that lead to OOPSes as reported in bug 507642, bug 505999,
bug 534339. They are caused because a snapshot of the bug involves
making shortlist instances of the above mentioned properties in
snapshot_sql_result(), and the hard limit for the shortlist is set
to 1000, while bug 1 has meanwhile more than 1000 comments.

It would have been enough to mark IBug.messages with doNotSnapshot(),
but bug 1 has ca 300 subscribers and ca 200 people feel affected by
it, and I think that these numbers are close enough to the hard limit
to remove the related properties from the snapshot too.

In order to trigger the failure, we need a bug with "too many" comments,
subscribers or affected people. Instead of creating 1000 bug comments
or people just for the tests, I changed the hard limit for the
shortlist() calls for the tests.

main test:

./bin/test -t test_bugs_webservice

possibly affected tests where snapshots of IBug objects are involved:

./bin/test -t test_bugtask -t test_bugnotification -t test_bugchanges

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/webapp/snapshot.py
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/tests/test_bugs_webservice.py

== Pylint notices ==

lib/canonical/launchpad/webapp/snapshot.py
    11: [F0401] Unable to import 'lazr.lifecycle.interfaces' (No module named lifecycle)

lib/lp/bugs/interfaces/bug.py
    36: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    52: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    58: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    59: [F0401] Unable to import 'lazr.restful.interface' (No module named restful)
    506: [C0322, IBug.addAttachment] Operator not preceded by a space
    comment=Text(), filename=TextLine(), is_patch=Bool(),
    ^
    content_type=TextLine(), description=Text())
    @export_factory_operation(IBugAttachment, [])
    def addAttachment(owner, data, comment, filename, is_patch=False,
    content_type=None, description=None):
    653: [C0322, IBug.getNominations] Operator not preceded by a space
    nominations=List(
    ^
    title=_("Nominations to search through."),
    value_type=Reference(schema=Interface), # IBugNomination
    required=False))
    @operation_returns_collection_of(Interface) # IBugNomination
    @export_read_operation()
    def getNominations(target=None, nominations=None):
    733: [C0322, IBug.markUserAffected] Operator not preceded by a space
    required=False, default=True))
    ^
    @call_with(user=REQUEST_USER)
    @export_write_operation()
    def markUserAffected(user, affected=True):
    748: [C0322, IBug.setCommentVisibility] Operator not preceded by a space
    required=True),
    ^
    visible=Bool(title=_('Show this comment?'), required=True))
    @call_with(user=REQUEST_USER)
    @export_write_operation()
    def setCommentVisibility(user, comment_number, visible):
    760: [C0322, IBug.linkHWSubmission] Operator not preceded by a space
    Interface, title=_('A HWDB submission'), required=True))
    ^
    @export_write_operation()
    def linkHWSubmission(submission):
    767: [C0322, IBug.unlinkHWSubmission] Operator not preceded by a space
    Interface, title=_('A HWDB submission'), required=True))
    ^
    @export_write_operation()
    def unlinkHWSubmission(submission):

[Abel Deuring (adeuring)]

Hi Jeroen,

I added an explicit test that the collections are labeled as "do not snapshot". But I did not run the "full test" of too many subscritpions with the real hard limit, as you suggested: Doing this takes ca 80 second on my machine,even when factory.makePersonNoCimmit() is used. BTW, according to top(1), most time is spent in Python, not in Postgres during this test.

[Jeroen T. Vermeulen (jtv)]

Went through this on IRC. Changes:

 * Don't test the entire integration chain for each affected attribute (exceeding the hard limit on snapshots, then simulating a post). Instead, test once that exceeding the hardlimit on one attribute no longer breaks things; then for each of the attributes test that snapshotting is disabled.

 * Move things out of setUp where appropriate; generate all those bug subscribers or whatever using factory.makePersonNoCommit and there's no need for database commits (after the login).

 * The test docstring was basically one huge sentence, obscuring the essentials.

 * The range(5) should be range(<current limit> + 1)

Go land that baby!

review: Approve (code)

=== modified file 'lib/canonical/launchpad/webapp/snapshot.py'
--- lib/canonical/launchpad/webapp/snapshot.py	2009-12-03 21:30:02 +0000
+++ lib/canonical/launchpad/webapp/snapshot.py	2010-03-10 11:35:40 +0000
@@ -12,6 +12,8 @@
 
 from canonical.launchpad.helpers import shortlist
 
+HARD_LIMIT_FOR_SNAPSHOT = 1000
+
 @implementer(ISnapshotValueFactory)
 @adapter(IResultSet) # And ISQLObjectResultSet.
 def snapshot_sql_result(value):
@@ -19,4 +21,5 @@
     # SelectResults, which doesn't really help the Snapshot
     # object. We therefore list()ify the values; this isn't
     # perfect but allows deltas to be generated reliably.
-    return shortlist(value, longest_expected=100, hardlimit=1000)
+    return shortlist(
+        value, longest_expected=100, hardlimit=HARD_LIMIT_FOR_SNAPSHOT)

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2010-03-08 09:02:25 +0000
+++ lib/lp/bugs/interfaces/bug.py	2010-03-10 11:35:40 +0000
@@ -33,6 +33,7 @@
 from canonical.launchpad import _
 from canonical.launchpad.fields import (
     BugField, ContentNameField, DuplicateBug, PublicPersonChoice, Tag, Title)
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lp.bugs.interfaces.bugattachment import IBugAttachment
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance, BugTaskStatus, IBugTask)
@@ -237,10 +238,10 @@
             readonly=True))
     cve_links = Attribute('Links between this bug and CVE entries.')
     subscriptions = exported(
-        CollectionField(
+        doNotSnapshot(CollectionField(
             title=_('Subscriptions.'),
             value_type=Reference(schema=Interface),
-            readonly=True))
+            readonly=True)))
     duplicates = exported(
         CollectionField(
             title=_('MultiJoin of the bugs which are dups of this one'),
@@ -303,23 +304,23 @@
         # general master bug.
         Int(title=_('The number of users unaffected by this bug'),
             required=True, readonly=True))
-    users_affected = exported(CollectionField(
+    users_affected = exported(doNotSnapshot(CollectionField(
             title=_('Users affected (not including duplicates)'),
             value_type=Reference(schema=IPerson),
-            readonly=True))
-    users_unaffected = exported(CollectionField(
+            readonly=True)))
+    users_unaffected = exported(doNotSnapshot(CollectionField(
             title=_('Users explicitly marked as unaffected '
                     '(not including duplicates)'),
             value_type=Reference(schema=IPerson),
-            readonly=True))
+            readonly=True)))
     users_affected_count_with_dupes = exported(
       Int(title=_('The number of users affected by this bug '
                   '(including duplicates)'),
           required=True, readonly=True))
-    users_affected_with_dupes = exported(CollectionField(
+    users_affected_with_dupes = exported(doNotSnapshot(CollectionField(
             title=_('Users affected (including duplicates)'),
             value_type=Reference(schema=IPerson),
-            readonly=True))
+            readonly=True)))
 
     heat = exported(
         Int(title=_("The 'heat' of the bug"),
@@ -332,11 +333,11 @@
         "The number of comments on this bug, not including the initial "
         "comment.")
 
-    messages = CollectionField(
+    messages = doNotSnapshot(CollectionField(
             title=_("The messages related to this object, in reverse "
                     "order of creation (so newest first)."),
             readonly=True,
-            value_type=Reference(schema=IMessage))
+            value_type=Reference(schema=IMessage)))
 
     indexed_messages = exported(
         CollectionField(

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2009-12-08 19:08:15 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2010-03-10 11:35:40 +0000
@@ -12,11 +12,14 @@
 from simplejson import dumps
 
 from zope.component import getMultiAdapter
+from lazr.lifecycle.interfaces import IDoNotSnapshot
 
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
-from canonical.launchpad.ftests import login
+from lp.bugs.interfaces.bug import IBug
+from canonical.launchpad.ftests import login, logout
 from lp.testing import TestCaseWithFactory
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
+from canonical.launchpad.webapp import snapshot
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing import DatabaseFunctionalLayer
 
@@ -152,5 +155,53 @@
         self.failUnlessEqual(None, latest_message['parent_link'])
 
 
+class TestPostBugWithLargeCollections(TestCaseWithFactory):
+    """Ensure that large IBug collections cause OOPSes on POSTs for IBug.
+
+    When a POST request on a bug is processed, a snapshot of the bug
+    is created. This can lead to OOPSes as described in bugs 507642,
+    505999, 534339: A snapshot of a database collection field is a
+    shortlist() copy of the data and the creation of the snapshot fails
+    if a collection contains more elements than the hard limit of the
+    sortlist().
+
+    Hence we do not include properties in the snapshot that may have
+    a large number of elements: messages, bug subscriptions and
+    (un)affected users.
+    """
+    layer = DatabaseFunctionalLayer
+
+    def test_no_snapshots_for_large_collections(self):
+        # Ensure that no snapshots are made of the properties comments,
+        # bug subscriptions and (un)affected users.
+        for field_name in (
+            'subscriptions', 'users_affected', 'users_unaffected',
+            'users_affected_with_dupes', 'messages'):
+            self.failUnless(
+                IDoNotSnapshot.providedBy(IBug[field_name]),
+                'IBug.%s should not be included in snapshots, see bug 507642.'
+                % field_name)
+
+    def test_many_subscribers(self):
+        # Many subscriptions do not cause an OOPS for IBug POSTs.
+        bug = self.factory.makeBug()
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT
+        snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3
+        try:
+            login('[e-mail]')
+            for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):
+                person = self.factory.makePersonNoCommit()
+                bug.subscribe(person, person)
+            logout()
+            response = webservice.named_post(
+                '/bugs/%d' % bug.id, 'subscribe',
+                person='http://api.launchpad.dev/beta/~name12')
+            self.failUnlessEqual(200, response.status)
+        finally:
+            snapshot.HARD_LIMIT_FOR_SNAPSHOT = real_hard_limit_for_snapshot
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)