launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug744888 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #744888 in Launchpad itself: "Muting bug 1 times out"
  https://bugs.launchpad.net/launchpad/+bug/744888

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug744888/+merge/57499

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
-- 
https://code.launchpad.net/~gary/launchpad/bug744888/+merge/57499
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug744888 into lp:launchpad.

=== 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 14:26:44 +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 14:26:44 +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 14:26:44 +0000
@@ -5,9 +5,18 @@
 
 __metaclass__ = type
 
+from lazr.lifecycle.snapshot import Snapshot
+from zope.interface import providedBy
+
+from canonical.launchpad.webapp.adapter import (
+    get_request_statements,
+    set_request_started,
+    clear_request_started,
+    )
 from canonical.testing.layers import DatabaseFunctionalLayer
 
 from lp.bugs.enum import BugNotificationLevel
+from lp.bugs.interfaces.bug import IBug
 from lp.testing import (
     feature_flags,
     person_logged_in,
@@ -55,6 +64,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 +92,71 @@
             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):
+            # We need to do this to enable SQL logging.
+            set_request_started(enable_timeout=False)
+            try:
+                snapshot = Snapshot(self.bug, providing=providedBy(self.bug))
+                sql_statements = get_request_statements()
+            finally:
+                clear_request_started()
+        # 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 (start, stop, dbname, sql) in sql_statements:
+            # Yes, we are doing dumb parsing of SQL.  Hopefully this is not
+            # too fragile.  See comment at start of test for why we are
+            # doing this bizarre thing.
+            # This gets the string between "SELECT" and "FROM".  It should
+            # handle "WITH" being used.  We use the SELECT phrase rather than
+            # the FROM phrase because the FROM might be a subquery, and this
+            # seemed simpler.
+            sql = sql.lower()
+            select_sql = sql.partition('select ')[2].partition(' from ')[0]
+            # This gets the field names with a simple split.
+            select_fields = select_sql.split(', ')
+            # Now we verify that the Message table is not referenced. 
+            # Of course, if the Message table is aliased, it won't catch
+            # it.  This shouldn't be a common problem for the way we
+            # currently generate our SQL, and for the kind of problem we are
+            # trying to prevent (that is, getting 1001 messages for a
+            # bug snapshot).
+            self.assertEqual(
+                [field_name for field_name in select_fields
+                 if field_name.startswith('message.')],
+                [])