Merge lp:~lifeless/launchpad/malone into lp:launchpad

[Robert Collins]

Remove listification from the internals of the bugs/message API.

Remove listification from the internals of the bugs/message API.

[Edwin Grubbs (edwin-grubbs)]

Hi Robert,

This is a nice improvement. I have a few comments below.

-Edwin

>=== modified file 'lib/canonical/launchpad/database/message.py'
>--- lib/canonical/launchpad/database/message.py 2010-08-29 21:24:47 +0000
>+++ lib/canonical/launchpad/database/message.py 2010-09-15 19:20:47 +0000
>@@ -158,10 +159,14 @@
> """See IMessage."""
> return self.owner
>
>- @property
>+ @cachedproperty
> def text_contents(self):
> """See IMessage."""
>- bits = [unicode(chunk) for chunk in self if chunk.content]
>+ return Message.chunks_text(self.chunks)
>+
>+ @classmethod
>+ def chunks_text(klass, chunks):

PEP8 wants you to use cls instead of klass.

>+ bits = [unicode(chunk) for chunk in chunks if chunk.content]
> return '\n\n'.join(bits)
>
> # XXX flacoste 2006-09-08: Bogus attribute only present so that
>
>=== modified file 'lib/lp/bugs/model/bug.py'
>--- lib/lp/bugs/model/bug.py 2010-09-09 17:02:33 +0000
>+++ lib/lp/bugs/model/bug.py 2010-09-15 19:20:47 +0000
>@@ -58,6 +58,7 @@
> SQLRaw,
> Union,
> )
>+from storm.info import ClassAlias
> from storm.store import (
> EmptyResultSet,
> Store,
>@@ -426,21 +427,79 @@
> @property
> def indexed_messages(self):
> """See `IMessageTarget`."""
>+ return self._indexed_messages(content=True)
>+
>+ def _indexed_messages(self, content=False):
>+ """Get the bugs messages, indexed.
>+
>+ :param content: If True retrieve the content for the msssages too.

s/msssages/messages/
The "content" parameter sounds more like it should contain content
as opposed to being a boolean. How about "include_content" instead?

>+ """
>+ # Make all messages be 'in' the main bugtask.
> inside = self.default_bugtask
>- messages = list(self.messages)
>- message_set = set(messages)
>-
>- indexed_messages = []
>- for index, message in enumerate(messages):
>- if message.parent not in message_set:
>- parent = None
>- else:
>- parent = message.parent
>-
>- indexed_message = IndexedMessage(message, inside, index, parent)
>- indexed_messages.append(indexed_message)
>-
>- return indexed_messages
>+ store = Store.of(self)
>+ message_by_id = {}
>+ def eager_load_owners(rows):
>+ # Because we may have multiple owners, we spend less time in storm
>+ # with very large bugs by not joining and instead querying a second
>+ # time. If this starts to show high db time, we can left outer join

Long lines.

>+ # instead.
>+ owner_ids = set(row[0].ownerID for row in rows)
>+ owner_ids.discard(None)
>+ if not owner_ids:
>+ return
>+ list(store.find(Person, Person.id.is_in(owner_ids)))
>+ def eager_load_content(rows):
>+ # To avoid the complexity of having multiple rows per
>+ # message, or joining in the database (though perhaps in
>+ # future we should do that), we do a single separate query
>+ # for the message content.
...

review: Approve (code)

=== modified file 'lib/canonical/launchpad/database/message.py'
--- lib/canonical/launchpad/database/message.py	2010-08-29 21:24:47 +0000
+++ lib/canonical/launchpad/database/message.py	2010-09-15 23:43:54 +0000
@@ -82,6 +82,7 @@
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.job.model.job import Job
+from lp.services.propertycache import cachedproperty
 
 # this is a hard limit on the size of email we will be willing to store in
 # the database.
@@ -158,10 +159,14 @@
         """See IMessage."""
         return self.owner
 
-    @property
+    @cachedproperty
     def text_contents(self):
         """See IMessage."""
-        bits = [unicode(chunk) for chunk in self if chunk.content]
+        return Message.chunks_text(self.chunks)
+
+    @classmethod
+    def chunks_text(cls, chunks):
+        bits = [unicode(chunk) for chunk in chunks if chunk.content]
         return '\n\n'.join(bits)
 
     # XXX flacoste 2006-09-08: Bogus attribute only present so that

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-09-09 17:02:33 +0000
+++ lib/lp/bugs/model/bug.py	2010-09-15 23:43:54 +0000
@@ -58,6 +58,7 @@
     SQLRaw,
     Union,
     )
+from storm.info import ClassAlias
 from storm.store import (
     EmptyResultSet,
     Store,
@@ -426,21 +427,101 @@
     @property
     def indexed_messages(self):
         """See `IMessageTarget`."""
+        return self._indexed_messages(include_content=True)
+
+    def _indexed_messages(self, include_content=False, include_parents=True):
+        """Get the bugs messages, indexed.
+
+        :param include_content: If True retrieve the content for the messages
+            too.
+        :param include_parents: If True retrieve the object for parent messages
+            too. If False the parent attribute will be *forced* to None to
+            reduce database lookups.
+        """
+        # Make all messages be 'in' the main bugtask.
         inside = self.default_bugtask
-        messages = list(self.messages)
-        message_set = set(messages)
-
-        indexed_messages = []
-        for index, message in enumerate(messages):
-            if message.parent not in message_set:
-                parent = None
+        store = Store.of(self)
+        message_by_id = {}
+        if include_parents:
+            def to_messages(rows):
+                return [row[0] for row in rows]
+        else:
+            def to_messages(rows):
+                return rows
+        def eager_load_owners(messages):
+            # Because we may have multiple owners, we spend less time in storm
+            # with very large bugs by not joining and instead querying a second
+            # time. If this starts to show high db time, we can left outer join
+            # instead.
+            owner_ids = set(message.ownerID for message in messages)
+            owner_ids.discard(None)
+            if not owner_ids:
+                return
+            list(store.find(Person, Person.id.is_in(owner_ids)))
+        def eager_load_content(messages):
+            # To avoid the complexity of having multiple rows per
+            # message, or joining in the database (though perhaps in
+            # future we should do that), we do a single separate query
+            # for the message content.
+            message_ids = set(message.id for message in messages)
+            chunks = store.find(
+                MessageChunk, MessageChunk.messageID.is_in(message_ids))
+            chunks.order_by(MessageChunk.id)
+            chunk_map = {}
+            for chunk in chunks:
+                message_chunks = chunk_map.setdefault(chunk.messageID, [])
+                message_chunks.append(chunk)
+            for message in messages:
+                if message.id not in chunk_map:
+                    continue
+                cache = IPropertyCache(message)
+                cache.text_contents = Message.chunks_text(
+                    chunk_map[message.id])
+        def eager_load(rows, slice_info):
+            messages = to_messages(rows)
+            eager_load_owners(messages)
+            if include_content:
+                eager_load_content(messages)
+        def index_message(row, index):
+            # convert row to an IndexedMessage
+            if include_parents:
+                message, parent = row
+                if parent is not None:
+                    # If there is an IndexedMessage available as parent, use
+                    # that to reduce on-demand parent lookups.
+                    parent = message_by_id.get(parent.id, parent)
             else:
-                parent = message.parent
-
-            indexed_message = IndexedMessage(message, inside, index, parent)
-            indexed_messages.append(indexed_message)
-
-        return indexed_messages
+                message = row
+                parent = None # parent attribute is not going to be accessed.
+            result = IndexedMessage(message, inside, index, parent)
+            # This message may be the parent for another: stash it to permit
+            # use.
+            message_by_id[message.id] = result
+            return result
+        # There is possibly some nicer way to do this in storm, but
+        # this is a lot easier to figure out.
+        if include_parents:
+            ParentMessage = ClassAlias(Message, name="parent_message")
+            tables = SQL("""
+Message left outer join
+message as parent_message on (
+    message.parent=parent_message.id and
+    parent_message.id in (
+        select bugmessage.message from bugmessage where bugmessage.bug=%s)),
+BugMessage""" % sqlvalues(self.id))
+            results = store.using(tables).find(
+                (Message, ParentMessage),
+                BugMessage.bugID == self.id,
+                BugMessage.messageID == Message.id,
+                )
+        else:
+            results = store.find(Message,
+                BugMessage.bugID == self.id,
+                BugMessage.messageID == Message.id,
+                )
+        results.order_by(Message.datecreated, Message.id)
+        return DecoratedResultSet(results, index_message,
+            pre_iter_hook=eager_load, slice_info=True)
 
     @property
     def displayname(self):
@@ -1785,7 +1866,7 @@
         when attachments is evaluated, not when the resultset is processed).
         """
         message_to_indexed = {}
-        for message in self.indexed_messages:
+        for message in self._indexed_messages(include_parents=False):
             message_to_indexed[message.id] = message
         def set_indexed_message(row):
             attachment = row[0]

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2010-09-02 19:30:03 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2010-09-15 23:43:54 +0000
@@ -143,7 +143,7 @@
             rendered_comment, self.expected_comment_html)
 
 
-class TestBugAttachments(TestCaseWithFactory):
+class TestBugScaling(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -165,12 +165,12 @@
         collector = QueryCollector()
         collector.register()
         self.addCleanup(collector.unregister)
-        url = '/bugs/%d/attachments' % self.bug.id
+        url = '/bugs/%d/attachments?ws.size=75' % self.bug.id
         #First request
         store.flush()
         store.reset()
         response = webservice.get(url)
-        self.assertThat(collector, HasQueryCount(LessThan(24)))
+        self.assertThat(collector, HasQueryCount(LessThan(21)))
         with_2_count = collector.count
         self.failUnlessEqual(response.status, 200)
         login(USER_EMAIL)
@@ -181,7 +181,44 @@
         store.flush()
         store.reset()
         response = webservice.get(url)
-        self.assertThat(collector, HasQueryCount(Equals(with_2_count+1)))
+        self.assertThat(collector, HasQueryCount(Equals(with_2_count)))
+
+    def test_messages_query_counts_constant(self):
+        # XXX Robert Collins 2010-09-15 bug=619017
+        # This test may be thrown off by the reference bug. To get around the
+        # problem, flush and reset are called on the bug storm cache before
+        # each call to the webservice. When lp's storm is updated to release
+        # the committed fix for this bug, please see about updating this test.
+        login(USER_EMAIL)
+        bug = self.factory.makeBug()
+        store = Store.of(bug)
+        self.factory.makeBugComment(bug)
+        self.factory.makeBugComment(bug)
+        self.factory.makeBugComment(bug)
+        person = self.factory.makePerson()
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        url = '/bugs/%d/messages?ws.size=75' % bug.id
+        #First request
+        store.flush()
+        store.reset()
+        response = webservice.get(url)
+        self.assertThat(collector, HasQueryCount(LessThan(21)))
+        with_2_count = collector.count
+        self.failUnlessEqual(response.status, 200)
+        login(USER_EMAIL)
+        for i in range(50):
+            self.factory.makeBugComment(bug)
+        self.factory.makeBugAttachment(bug)
+        logout()
+        #Second request
+        store.flush()
+        store.reset()
+        response = webservice.get(url)
+        self.assertThat(collector, HasQueryCount(Equals(with_2_count)))
 
 
 class TestBugMessages(TestCaseWithFactory):

=== modified file 'lib/lp/bugs/tests/test_searchtasks_webservice.py'
--- lib/lp/bugs/tests/test_searchtasks_webservice.py	2010-08-20 21:48:35 +0000
+++ lib/lp/bugs/tests/test_searchtasks_webservice.py	2010-09-15 23:43:54 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-from unittest import TestLoader
-
 from canonical.launchpad.ftests import login
 from lp.testing import TestCaseWithFactory
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
@@ -37,7 +35,3 @@
         response = self.webservice.named_get('/mebuntu/inkanyamba',
             'searchTasks', api_version='devel').jsonBody()
         self.assertEqual(response['total_size'], 1)
-
-
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)