launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00805
[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'
