launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-750949 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #750949 in Launchpad itself: "BugTask:+index times out with lots of attachments"
  https://bugs.launchpad.net/launchpad/+bug/750949

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-750949/+merge/56295

Bug comments display the size of any associated attachments, which requires the LibraryFileContent. This branch expands the LFA preloading to LFCs too, and adds a test.

Ahhh, the test. Browsing to a bug page caused canonical_url(attachment) to die, which I eventually tracked down to BugAttachmentURL depending on a value in ILaunchBag. While one could argue that BugAttachmentURL should be fixed, how it should be fixed is not entirely clear. So, I opted to avoid hours more of future confusion by clearing LaunchBag at the end of each request. It's already cleared at the start, so it won't affect the webapp. Some tests touch LaunchBag inappropriately, but that's mostly to get the current logged in user, not anything from a browser request.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-750949/+merge/56295
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-750949 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2011-03-29 00:11:57 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2011-04-05 06:57:34 +0000
@@ -708,6 +708,8 @@
 
         da.clear_request_started()
 
+        getUtility(IOpenLaunchBag).clear()
+
         # Maintain operational statistics.
         if getattr(request, '_wants_retry', False):
             OpStats.stats['retries'] += 1

=== modified file 'lib/lp/bugs/browser/bugattachment.py'
--- lib/lp/bugs/browser/bugattachment.py	2011-03-28 00:43:40 +0000
+++ lib/lp/bugs/browser/bugattachment.py	2011-04-05 06:57:34 +0000
@@ -92,6 +92,9 @@
     usedfor = IBugAttachmentSet
 
 
+# Despite declaring compliance with ICanonicalUrlData, the LaunchBag
+# dependency means this tends towards the "not canonical at all" end of
+# the canonicalness scale. Beware.
 class BugAttachmentURL:
     """Bug URL creation rules."""
     implements(ICanonicalUrlData)

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-04-05 06:57:34 +0000
@@ -20,7 +20,10 @@
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.bugs.browser.bugtask import (
     BugTaskEditView,
     BugTasksAndNominationsView,
@@ -30,11 +33,15 @@
 from lp.services.propertycache import get_property_cache
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.testing import (
+    celebrity_logged_in,
     person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing._webservice import QueryCollector
-from lp.testing.matchers import HasQueryCount
+from lp.testing.matchers import (
+    BrowsesWithQueryLimit,
+    HasQueryCount,
+    )
 from lp.testing.sampledata import (
     ADMIN_EMAIL,
     NO_PRIVILEGE_EMAIL,
@@ -45,7 +52,7 @@
 
 class TestBugTaskView(TestCaseWithFactory):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def invalidate_caches(self, obj):
         store = Store.of(obj)
@@ -84,6 +91,24 @@
             LessThan(count_with_no_teams + 3),
             ))
 
+    def test_rendered_query_counts_constant_with_attachments(self):
+        with celebrity_logged_in('admin'):
+            browses_under_limit = BrowsesWithQueryLimit(
+                71, self.factory.makePerson())
+
+            # First test with a single attachment.
+            task = self.factory.makeBugTask()
+            self.factory.makeBugAttachment(bug=task.bug)
+        self.assertThat(task, browses_under_limit)
+
+        with celebrity_logged_in('admin'):
+            # And now with 10.
+            task = self.factory.makeBugTask()
+            self.factory.makeBugTask(bug=task.bug)
+            for i in range(10):
+                self.factory.makeBugAttachment(bug=task.bug)
+        self.assertThat(task, browses_under_limit)
+
     def test_interesting_activity(self):
         # The interesting_activity property returns a tuple of interesting
         # `BugActivityItem`s.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-04-01 00:47:03 +0000
+++ lib/lp/bugs/model/bug.py	2011-04-05 06:57:34 +0000
@@ -87,7 +87,10 @@
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
-from canonical.launchpad.database.librarian import LibraryFileAlias
+from canonical.launchpad.database.librarian import (
+    LibraryFileAlias,
+    LibraryFileContent,
+    )
 from canonical.launchpad.database.message import (
     Message,
     MessageChunk,
@@ -1935,10 +1938,11 @@
         # See bug 542274 for more details.
         store = Store.of(self)
         return store.find(
-            (BugAttachment, LibraryFileAlias),
+            (BugAttachment, LibraryFileAlias, LibraryFileContent),
             BugAttachment.bug == self,
-            BugAttachment.libraryfile == LibraryFileAlias.id,
-            LibraryFileAlias.content != None).order_by(BugAttachment.id)
+            BugAttachment.libraryfileID == LibraryFileAlias.id,
+            LibraryFileContent.id == LibraryFileAlias.contentID,
+            ).order_by(BugAttachment.id)
 
     @property
     def attachments(self):