Merge lp:~gary/launchpad/bug744888 into lp:launchpad

[Gary Poster]

[r=abentley][bug=744888] Snapshots of a bug should not get all of its messages. webservice bug.mute() should work.

This branch hopefully fixes the timeout when muting bug 1 via the webservice. Thanks to investigation by Graham and Robert, the problem appeared to be the fact that, when the webservice made snapshots made snapshots, we got all of the bugs comments ("messages"), and this extra weight pushed us over the timeout. We did that because we snapshotted "messages" and "attachments". Bug 1 has over 1400 comments as of this writing. Robert suggested that simply indicating that these attributes should not be included in snapshots should be sufficient to fix the problem. My branch implements that solution. An ec2 test run seems to indicate that it won't cause any problems.

The most questionable aspect of this is the test. I'll leave the comments to speak for themselves.

Along the way, when I was testing the problem, I discovered that the bug's mute method in the web API was not working as expected. This was getting in the way of my work. I introduced a fix for that in this branch, as well as a test to show that it and unmute both work as the webservice exposes them (unmute already did).

Thank you!

Gary

[Aaron Bentley (abentley)]

If StormStatementRecorder works for your test, I recommend using that instead.

Please change the SQL check as discussed on IRC:
<abentley> gary_poster: I wonder whether there's risk of your SQL check hitting false negatives? Would it be crazy to just look for the string 'message' in the entire statement?
<gary_poster> abentley: looking for message: well...there are also "bugmessage"s in theory...it's obviously a balance between possible false negatives and false positives.
<gary_poster> I guess we could try being a bit more aggressive. I'd be OK with simply splitting on whitespace and making sure there are no strings that start with "message" (which would include tables). If people find that too restrictive in the future they can adjust the test.

Aside from that, looks fine.

review: Approve

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-04-05 17:53:52 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-04-13 21:05:25 +0000
@@ -294,11 +294,11 @@
             title=_("List of bug attachments."),
             value_type=Reference(schema=IBugAttachment),
             readonly=True)
-    attachments = exported(
+    attachments = doNotSnapshot(exported(
         CollectionField(
             title=_("List of bug attachments."),
             value_type=Reference(schema=IBugAttachment),
-            readonly=True))
+            readonly=True)))
     questions = Attribute("List of questions related to this bug.")
     specifications = Attribute("List of related specifications.")
     linked_branches = exported(
@@ -394,13 +394,13 @@
             readonly=True,
             value_type=Reference(schema=IMessage)))
 
-    indexed_messages = exported(
+    indexed_messages = doNotSnapshot(exported(
         CollectionField(
             title=_("The messages related to this object, in reverse "
                     "order of creation (so newest first)."),
             readonly=True,
             value_type=Reference(schema=IMessage)),
-        exported_as='messages')
+        exported_as='messages'))
 
     def _indexed_messages(include_content=False, include_parents=False):
         """Low level query for getting bug messages.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-04-12 13:17:34 +0000
+++ lib/lp/bugs/model/bug.py	2011-04-13 21:05:25 +0000
@@ -850,6 +850,9 @@
 
     def mute(self, person, muted_by):
         """See `IBug`."""
+        if person is None:
+            # This may be a webservice request.
+            person = muted_by
         # If there's an existing subscription, update it.
         store = Store.of(self)
         subscriptions = store.find(

=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py	2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/tests/test_bug.py	2011-04-13 21:05:25 +0000
@@ -5,16 +5,19 @@
 
 __metaclass__ = type
 
+from lazr.lifecycle.snapshot import Snapshot
+from zope.interface import providedBy
+
 from canonical.testing.layers import DatabaseFunctionalLayer
 
 from lp.bugs.enum import BugNotificationLevel
 from lp.testing import (
-    feature_flags,
     person_logged_in,
-    set_feature_flag,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 
+
 class TestBugSubscriptionMethods(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
@@ -55,6 +58,15 @@
                 BugNotificationLevel.NOTHING,
                 muted_subscription.bug_notification_level)
 
+    def test_mute_mutes_muter(self):
+        # When exposed in the web API, the mute method regards the
+        # first, `person` argument as optional, and the second
+        # `muted_by` argument is supplied from the request.  In this
+        # case, the person should be the muter.
+        with person_logged_in(self.person):
+            self.bug.mute(None, self.person)
+            self.assertTrue(self.bug.isMuted(self.person))
+
     def test_mute_mutes_user_with_existing_subscription(self):
         # Bug.mute() will update an existing subscription so that it
         # becomes muted.
@@ -74,3 +86,56 @@
             self.assertTrue(self.bug.isMuted(self.person))
             self.bug.unmute(self.person, self.person)
             self.assertFalse(self.bug.isMuted(self.person))
+
+    def test_unmute_mutes_unmuter(self):
+        # When exposed in the web API, the unmute method regards the
+        # first, `person` argument as optional, and the second
+        # `unmuted_by` argument is supplied from the request.  In this
+        # case, the person should be the muter.
+        with person_logged_in(self.person):
+            self.bug.mute(self.person, self.person)
+            self.bug.unmute(None, self.person)
+            self.assertFalse(self.bug.isMuted(self.person))
+
+
+class TestBugSnapshotting(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugSnapshotting, self).setUp()
+        self.bug = self.factory.makeBug()
+        self.person = self.factory.makePerson()
+
+    def test_bug_snapshot_does_not_include_messages(self):
+        # A snapshot of a bug does not include its messages or
+        # attachments (which get the messages from the database).  If it
+        # does, the webservice can become unusable if changes are made
+        # to bugs with many comments, such as bug 1. See, for instance,
+        # bug 744888.  This test is primarily to keep the problem from
+        # slipping in again.  To do so, we resort to somewhat
+        # extraordinary measures.  In addition to verifying that the
+        # snapshot does not have the attributes that currently trigger
+        # the problem, we also actually look at the SQL that is
+        # generated by creating the snapshot.  With this, we can verify
+        # that the Message table is not included.  This is ugly, but
+        # this has a chance of fighting against future eager loading
+        # optimizations that might trigger the problem again.
+        with person_logged_in(self.person):
+            with StormStatementRecorder() as recorder:
+                snapshot = Snapshot(self.bug, providing=providedBy(self.bug))
+            sql_statements = recorder.statements
+        # This uses "self" as a marker to show that the attribute does not
+        # exist.  We do not use hasattr because it eats exceptions.
+        #self.assertTrue(getattr(snapshot, 'messages', self) is self)
+        #self.assertTrue(getattr(snapshot, 'attachments', self) is self)
+        for sql in sql_statements:
+            # We are going to be aggressive about looking for the problem in
+            # the SQL.  We'll split the SQL up by whitespace, and then look
+            # for strings that start with "message".  If that is too
+            # aggressive in the future from some reason, please do adjust the
+            # test appropriately.
+            sql_tokens = sql.lower().split()
+            self.assertEqual(
+                [token for token in sql_tokens
+                 if token.startswith('message')],
+                [])