← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~lifeless/launchpad/bug-618849 into lp:launchpad/devel

 

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-618849 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #618849 Timeout accessing bugs' attachments using the API
  https://bugs.launchpad.net/bugs/618849


Reduce the query count for bug/attachments API calls: inject an IIndexedMessage into the bug attachment when we query, rather than doing O(N^2) work later. Also, to make my test easier to write, fixed up the LibraryFileAlias lookups to be eager loaded.

Net result - 23 SQL calls to answer the API (down from 38 for a bug with 2 attachments when I started testing this). There is a lurking surprise when indexed_messages becomes too expensive to query; I think we probably want the index in the DB rather than calculating at runtime, however that is a separate bridge to cross in the future.

The implementation is a little convoluted due to:
 - needing to index a *non db class* as the answer for bugattachment.message (https://answers.edge.launchpad.net/lazr.restful/+question/123212)
 - lazr.restful not being able to replace an unexported attribute. (bug 625102)

The former point is also potentially solvable via lazr restful, but I'm not sure its a bug, so its just a question for now.

I've done some driveby cleanups to sample data in the test I modified too.
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-618849 into lp:launchpad/devel.
=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/browser/bug.py	2010-08-29 07:05:59 +0000
@@ -519,7 +519,7 @@
                 'file': ProxiedLibraryFileAlias(
                     attachment.libraryfile, attachment),
                 }
-            for attachment in self.context.attachments
+            for attachment in self.context.attachments_unpopulated
             if attachment.type != BugAttachmentType.PATCH]
 
     @property
@@ -531,7 +531,7 @@
                 'file': ProxiedLibraryFileAlias(
                     attachment.libraryfile, attachment),
                 }
-            for attachment in self.context.attachments
+            for attachment in self.context.attachments_unpopulated
             if attachment.type == BugAttachmentType.PATCH]
 
 
@@ -877,15 +877,17 @@
         if bug.security_related:
             text.append('security: yes')
 
+        patches = []
         text.append('attachments: ')
-        for attachment in bug.attachments:
+        for attachment in bug.attachments_unpopulated:
             if attachment.type != BugAttachmentType.PATCH:
                 text.append(' %s' % self.attachment_text(attachment))
+            else:
+                patches.append(attachment)
 
         text.append('patches: ')
-        for attachment in bug.attachments:
-            if attachment.type == BugAttachmentType.PATCH:
-                text.append(' %s' % self.attachment_text(attachment))
+        for attachment in patches:
+            text.append(' %s' % self.attachment_text(attachment))
 
         text.append('tags: %s' % ' '.join(bug.tags))
 

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2010-08-22 18:31:30 +0000
+++ lib/lp/bugs/browser/bugtask.py	2010-08-29 07:05:59 +0000
@@ -310,7 +310,7 @@
     """
     chunks = bugtask.bug.getMessageChunks()
     comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)
-    for attachment in bugtask.bug.attachments:
+    for attachment in bugtask.bug.attachments_unpopulated:
         message_id = attachment.message.id
         # All attachments are related to a message, so we can be
         # sure that the BugComment is already created.

=== modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
--- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2010-02-23 15:34:15 +0000
+++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt	2010-08-29 07:05:59 +0000
@@ -284,7 +284,7 @@
 The attachments got added, with the charsets preserved, and the one that
 didn't specify a description got an autogenerated one.
 
-    >>> for attachment in filebug_view.added_bug.attachments:
+    >>> for attachment in filebug_view.added_bug.attachments_unpopulated:
     ...     print "Filename: %s" % attachment.libraryfile.filename
     ...     print "Content type: %s" % attachment.libraryfile.mimetype
     ...     print "Description: %s" % attachment.title

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/interfaces/bug.py	2010-08-29 07:05:59 +0000
@@ -279,6 +279,10 @@
         CollectionField(
             title=_("MultiJoin of bugs which are dupes of this one."),
             value_type=BugField(), readonly=True))
+    attachments_unpopulated = CollectionField(
+            title=_("List of bug attachments."),
+            value_type=Reference(schema=IBugAttachment),
+            readonly=True)
     attachments = exported(
         CollectionField(
             title=_("List of bug attachments."),

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-08-25 14:41:20 +0000
+++ lib/lp/bugs/model/bug.py	2010-08-29 07:05:59 +0000
@@ -72,6 +72,7 @@
 
 from canonical.cachedproperty import (
     cachedproperty,
+    cache_property,
     clear_property,
     )
 from canonical.config import config
@@ -82,6 +83,7 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
 from canonical.launchpad.database.librarian import LibraryFileAlias
 from canonical.launchpad.database.message import (
     Message,
@@ -1476,7 +1478,9 @@
                 self.who_made_private = None
                 self.date_made_private = None
 
-            for attachment in self.attachments:
+            # XXX: This should be a bulk update. RBC 20100827
+            # bug=https://bugs.edge.launchpad.net/storm/+bug/625071
+            for attachment in self.attachments_unpopulated:
                 attachment.libraryfile.restricted = private
 
             # Correct the heat for the bug immediately, so that we don't have
@@ -1728,19 +1732,56 @@
             task.target.recalculateBugHeatCache()
         store.flush()
 
-    @property
-    def attachments(self):
-        """See `IBug`."""
-        # We omit those bug attachments that do not have a
-        # LibraryFileContent record in order to avoid OOPSes as
-        # mentioned in bug 542274. These bug attachments will be
-        # deleted anyway during the next garbo_daily run.
+    def _attachments_query(self):
+        """Helper for the attachments* properties."""
+        # bug attachments with no LibraryFileContent have been deleted - the
+        # garbo_daily run will remove the LibraryFileAlias asynchronously.
+        # See bug 542274 for more details.
         store = Store.of(self)
         return store.find(
-            BugAttachment, BugAttachment.bug == self,
+            (BugAttachment, LibraryFileAlias),
+            BugAttachment.bug == self,
             BugAttachment.libraryfile == LibraryFileAlias.id,
             LibraryFileAlias.content != None).order_by(BugAttachment.id)
 
+    @property
+    def attachments(self):
+        """See `IBug`.
+        
+        This property does eager loading of the index_messages so that the API
+        which wants the message_link for the attachment can answer that without
+        O(N^2) overhead. As such it is moderately expensive to call (it
+        currently retrieves all messages before any attachments, and does this
+        which attachments is evaluated, not when the resultset is processed).
+        """
+        message_to_indexed = {}
+        for message in self.indexed_messages:
+            message_to_indexed[message.id] = message
+        def set_indexed_message(row):
+            attachment = row[0]
+            # row[1] - the LibraryFileAlias is now in the storm cache and
+            # will be found without a query when dereferenced.
+            indexed_message = message_to_indexed.get(attachment._messageID)
+            if indexed_message is not None:
+                cache_property(attachment, '_message_cached', indexed_message)
+            return attachment
+        rawresults = self._attachments_query()
+        return DecoratedResultSet(rawresults, set_indexed_message)
+
+    @property
+    def attachments_unpopulated(self):
+        """See `IBug`.
+        
+        This version dos not pre-lookup messages and LFA's.
+        
+        The regular 'attachments' property does prepopulation because it is
+        exposed in the API.
+        """
+        # Grab the attachment only; the LFA will be eager loaded.
+        return DecoratedResultSet(
+            self._attachments_query(),
+            operator.itemgetter(0))
+
 
 class BugSet:
     """See BugSet."""

=== modified file 'lib/lp/bugs/model/bugattachment.py'
--- lib/lp/bugs/model/bugattachment.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/model/bugattachment.py	2010-08-29 07:05:59 +0000
@@ -19,6 +19,7 @@
 from zope.event import notify
 from zope.interface import implements
 
+from canonical.cachedproperty import cachedproperty
 from canonical.database.enumcol import EnumCol
 from canonical.database.sqlbase import SQLBase
 from lp.app.errors import NotFoundError
@@ -46,9 +47,18 @@
         foreignKey='LibraryFileAlias', dbName='libraryfile', notNull=True)
     data = ForeignKey(
         foreignKey='LibraryFileAlias', dbName='libraryfile', notNull=True)
-    message = ForeignKey(
+    _message = ForeignKey(
         foreignKey='Message', dbName='message', notNull=True)
 
+    @cachedproperty('_message_cached')
+    def message(self):
+        """This is a property to allow message to be an IIndexedMessage.
+        
+        This is needed for the bug/attachments API calls, which seem to be the
+        only users of .message.
+        """
+        return self._message
+
     @property
     def is_patch(self):
         """See IBugAttachment."""
@@ -99,7 +109,7 @@
             attach_type = IBugAttachment['type'].default
         attachment = BugAttachment(
             bug=bug, libraryfile=filealias, type=attach_type, title=title,
-            message=message)
+            _message=message)
         # canonial_url(attachment) (called by notification subscribers
         # to generate the download URL of the attachments) blows up if
         # attachment.id is not (yet) set.

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2010-08-29 07:05:59 +0000
@@ -10,6 +10,10 @@
 from BeautifulSoup import BeautifulSoup
 from lazr.lifecycle.interfaces import IDoNotSnapshot
 from simplejson import dumps
+from testtools.matchers import (
+    Equals,
+    LessThan,
+    )
 from zope.component import getMultiAdapter
 
 from canonical.launchpad.ftests import (
@@ -19,10 +23,19 @@
 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
+from canonical.testing import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
 from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import HasQueryCount
+from lp.testing.sampledata import (
+    ADMIN_EMAIL,
+    USER_EMAIL,
+    )
+from lp.testing._webservice import QueryCollector
 
 
 class TestBugDescriptionRepresentation(TestCaseWithFactory):
@@ -31,7 +44,7 @@
 
     def setUp(self):
         TestCaseWithFactory.setUp(self)
-        login('foo.bar@xxxxxxxxxxxxx')
+        login(ADMIN_EMAIL)
         # Make two bugs, one whose description points to the other, so it will
         # get turned into a HTML link.
         self.bug_one = self.factory.makeBug(title="generic")
@@ -128,12 +141,38 @@
             rendered_comment, self.expected_comment_html)
 
 
+class TestBugAttachments(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_attachments_query_counts_constant(self):
+        login(USER_EMAIL)
+        self.bug = self.factory.makeBug()
+        self.factory.makeBugAttachment(self.bug)
+        self.factory.makeBugAttachment(self.bug)
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        url = '/bugs/%d/attachments' % self.bug.id
+        response = webservice.get(url)
+        self.assertThat(collector, HasQueryCount(LessThan(24)))
+        with_2_count = collector.count
+        self.failUnlessEqual(response.status, 200)
+        login(USER_EMAIL)
+        self.factory.makeBugAttachment(self.bug)
+        logout()
+        response = webservice.get(url)
+        self.assertThat(collector, HasQueryCount(Equals(with_2_count)))
+
+
 class TestBugMessages(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        super(TestBugMessages, self).setUp('test@xxxxxxxxxxxxx')
+        super(TestBugMessages, self).setUp(USER_EMAIL)
         self.bug = self.factory.makeBug()
         self.message1 = self.factory.makeMessage()
         self.message2 = self.factory.makeMessage(parent=self.message1)
@@ -191,7 +230,7 @@
         real_hard_limit_for_snapshot = snapshot.HARD_LIMIT_FOR_SNAPSHOT
         snapshot.HARD_LIMIT_FOR_SNAPSHOT = 3
         try:
-            login('foo.bar@xxxxxxxxxxxxx')
+            login(ADMIN_EMAIL)
             for count in range(snapshot.HARD_LIMIT_FOR_SNAPSHOT + 1):
                 person = self.factory.makePerson()
                 bug.subscribe(person, person)

=== modified file 'lib/lp/testing/sampledata.py'
--- lib/lp/testing/sampledata.py	2010-08-26 12:33:03 +0000
+++ lib/lp/testing/sampledata.py	2010-08-29 07:05:59 +0000
@@ -53,7 +53,7 @@
 VCS_IMPORTS_MEMBER_EMAIL = 'david.allouche@xxxxxxxxxxxxx'
 COMMERCIAL_ADMIN_EMAIL = 'commercial-member@xxxxxxxxxxxxx'
 ADMIN_EMAIL = 'foo.bar@xxxxxxxxxxxxx'
-SAMPLE_PERSON_EMAIL = 'test@xxxxxxxxxxxxx'
+SAMPLE_PERSON_EMAIL = USER_EMAIL
 # A user that is an admin of ubuntu-team, which has upload rights
 # to Ubuntu.
 UBUNTU_DEVELOPER_ADMIN_NAME = 'name16'