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

Proposed by Abel Deuring
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/bug-507642
Merge into: lp:launchpad
Diff against target: 162 lines (+67/-12)
3 files modified
lib/canonical/launchpad/webapp/snapshot.py (+4/-1)
lib/lp/bugs/interfaces/bug.py (+11/-10)
lib/lp/bugs/tests/test_bugs_webservice.py (+52/-1)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-507642
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+20978@code.launchpad.net

Description of the change

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):

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

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.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/webapp/snapshot.py'
2--- lib/canonical/launchpad/webapp/snapshot.py 2009-12-03 21:30:02 +0000
3+++ lib/canonical/launchpad/webapp/snapshot.py 2010-03-10 11:35:40 +0000
4@@ -12,6 +12,8 @@
5
6 from canonical.launchpad.helpers import shortlist
7
8+HARD_LIMIT_FOR_SNAPSHOT = 1000
9+
10 @implementer(ISnapshotValueFactory)
11 @adapter(IResultSet) # And ISQLObjectResultSet.
12 def snapshot_sql_result(value):
13@@ -19,4 +21,5 @@
14 # SelectResults, which doesn't really help the Snapshot
15 # object. We therefore list()ify the values; this isn't
16 # perfect but allows deltas to be generated reliably.
17- return shortlist(value, longest_expected=100, hardlimit=1000)
18+ return shortlist(
19+ value, longest_expected=100, hardlimit=HARD_LIMIT_FOR_SNAPSHOT)
20
21=== modified file 'lib/lp/bugs/interfaces/bug.py'
22--- lib/lp/bugs/interfaces/bug.py 2010-03-08 09:02:25 +0000
23+++ lib/lp/bugs/interfaces/bug.py 2010-03-10 11:35:40 +0000
24@@ -33,6 +33,7 @@
25 from canonical.launchpad import _
26 from canonical.launchpad.fields import (
27 BugField, ContentNameField, DuplicateBug, PublicPersonChoice, Tag, Title)
28+from lazr.lifecycle.snapshot import doNotSnapshot
29 from lp.bugs.interfaces.bugattachment import IBugAttachment
30 from lp.bugs.interfaces.bugtask import (
31 BugTaskImportance, BugTaskStatus, IBugTask)
32@@ -237,10 +238,10 @@
33 readonly=True))
34 cve_links = Attribute('Links between this bug and CVE entries.')
35 subscriptions = exported(
36- CollectionField(
37+ doNotSnapshot(CollectionField(
38 title=_('Subscriptions.'),
39 value_type=Reference(schema=Interface),
40- readonly=True))
41+ readonly=True)))
42 duplicates = exported(
43 CollectionField(
44 title=_('MultiJoin of the bugs which are dups of this one'),
45@@ -303,23 +304,23 @@
46 # general master bug.
47 Int(title=_('The number of users unaffected by this bug'),
48 required=True, readonly=True))
49- users_affected = exported(CollectionField(
50+ users_affected = exported(doNotSnapshot(CollectionField(
51 title=_('Users affected (not including duplicates)'),
52 value_type=Reference(schema=IPerson),
53- readonly=True))
54- users_unaffected = exported(CollectionField(
55+ readonly=True)))
56+ users_unaffected = exported(doNotSnapshot(CollectionField(
57 title=_('Users explicitly marked as unaffected '
58 '(not including duplicates)'),
59 value_type=Reference(schema=IPerson),
60- readonly=True))
61+ readonly=True)))
62 users_affected_count_with_dupes = exported(
63 Int(title=_('The number of users affected by this bug '
64 '(including duplicates)'),
65 required=True, readonly=True))
66- users_affected_with_dupes = exported(CollectionField(
67+ users_affected_with_dupes = exported(doNotSnapshot(CollectionField(
68 title=_('Users affected (including duplicates)'),
69 value_type=Reference(schema=IPerson),
70- readonly=True))
71+ readonly=True)))
72
73 heat = exported(
74 Int(title=_("The 'heat' of the bug"),
75@@ -332,11 +333,11 @@
76 "The number of comments on this bug, not including the initial "
77 "comment.")
78
79- messages = CollectionField(
80+ messages = doNotSnapshot(CollectionField(
81 title=_("The messages related to this object, in reverse "
82 "order of creation (so newest first)."),
83 readonly=True,
84- value_type=Reference(schema=IMessage))
85+ value_type=Reference(schema=IMessage)))
86
87 indexed_messages = exported(
88 CollectionField(
89
90=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
91--- lib/lp/bugs/tests/test_bugs_webservice.py 2009-12-08 19:08:15 +0000
92+++ lib/lp/bugs/tests/test_bugs_webservice.py 2010-03-10 11:35:40 +0000
93@@ -12,11 +12,14 @@
94 from simplejson import dumps
95
96 from zope.component import getMultiAdapter
97+from lazr.lifecycle.interfaces import IDoNotSnapshot
98
99 from lp.bugs.browser.bugtask import get_comments_for_bugtask
100-from canonical.launchpad.ftests import login
101+from lp.bugs.interfaces.bug import IBug
102+from canonical.launchpad.ftests import login, logout
103 from lp.testing import TestCaseWithFactory
104 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
105+from canonical.launchpad.webapp import snapshot
106 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
107 from canonical.testing import DatabaseFunctionalLayer
108
109@@ -152,5 +155,53 @@
110 self.failUnlessEqual(None, latest_message['parent_link'])
111
112
113+class TestPostBugWithLargeCollections(TestCaseWithFactory):
114+ """Ensure that large IBug collections cause OOPSes on POSTs for IBug.
115+
116+ When a POST request on a bug is processed, a snapshot of the bug
117+ is created. This can lead to OOPSes as described in bugs 507642,
118+ 505999, 534339: A snapshot of a database collection field is a
119+ shortlist() copy of the data and the creation of the snapshot fails
120+ if a collection contains more elements than the hard limit of the
121+ sortlist().
122+
123+ Hence we do not include properties in the snapshot that may have
124+ a large number of elements: messages, bug subscriptions and
125+ (un)affected users.
126+ """
127+ layer = DatabaseFunctionalLayer
128+
129+ def test_no_snapshots_for_large_collections(self):
130+ # Ensure that no snapshots are made of the properties comments,
131+ # bug subscriptions and (un)affected users.
132+ for field_name in (
133+ 'subscriptions', 'users_affected', 'users_unaffected',
134+ 'users_affected_with_dupes', 'messages'):
135+ self.failUnless(
136+ IDoNotSnapshot.providedBy(IBug[field_name]),
137+ 'IBug.%s should not be included in snapshots, see bug 507642.'
138+ % field_name)
139+
140+ def test_many_subscribers(self):
141+ # Many subscriptions do not cause an OOPS for IBug POSTs.
142+ bug = self.factory.makeBug()
143+ webservice = LaunchpadWebServiceCaller(
144+ 'launchpad-library', 'salgado-change-anything')
145+ real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT
146+ snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3
147+ try:
148+ login('foo.bar@canonical.com')
149+ for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):
150+ person = self.factory.makePersonNoCommit()
151+ bug.subscribe(person, person)
152+ logout()
153+ response = webservice.named_post(
154+ '/bugs/%d' % bug.id, 'subscribe',
155+ person='http://api.launchpad.dev/beta/~name12')
156+ self.failUnlessEqual(200, response.status)
157+ finally:
158+ snapshot.HARD_LIMIT_FOR_SNAPSHOT = real_hard_limit_for_snapshot
159+
160+
161 def test_suite():
162 return unittest.TestLoader().loadTestsFromName(__name__)