Launchpad itself

Merge lp:~gmb/launchpad/indexed-message-parents into lp:launchpad

  1. indexed-message-parents
  2. Merge into devel
Proposed by Graham Binns
Status: Merged
Merged at revision: not available
Proposed branch: lp:~gmb/launchpad/indexed-message-parents
Merge into: lp:launchpad
Diff against target: 140 lines (+92/-4)
4 files modified
lib/canonical/launchpad/interfaces/message.py (+7/-1)
lib/lp/bugs/model/bug.py (+13/-3)
lib/lp/bugs/tests/test_bug_messages.py (+44/-0)
lib/lp/bugs/tests/test_bugs_webservice.py (+28/-0)
To merge this branch: bzr merge lp:~gmb/launchpad/indexed-message-parents
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) release-critical Approve
Abel Deuring (community) Approve
Review via email: mp+15486@code.launchpad.net

Commit message

The webservice machinery should no longer blow up when encountering messages whose parents aren't linked to the same bug as their children.

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote :

This branch fixes bug 394097.

The bug was caused by messages in the offending bug's set of message having parent messages that weren't linked to the bug. This caused the webservice machinery to blow up.

To counter this, we've made it so that bug.indexed_messages ensures that IIndexedMessage.parent returns None if the parent of a message isn't linked to the same bug as the child message.

Gavin has added a regression test to cover the bug and I've added tests to cover the changes to Bug.indexed_messages.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/interfaces/message.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/tests/test_bug_messages.py
  lib/lp/bugs/tests/test_bugs_webservice.py

== Pylint notices ==

lib/canonical/launchpad/interfaces/message.py
    35: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    36: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    37: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)

lib/lp/bugs/model/bug.py
    25: [F0401] Unable to import 'email.Utils' (No module named Utils)
    38: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)
    40: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)

Revision history for this message
Abel Deuring (adeuring) :
review: Approve
Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (3.5 KiB)

On Tue, 2009-12-01 at 12:33 +0000, Graham Binns wrote:
> From:
> Graham Binns <email address hidden>
> Reply-to:
> <email address hidden>
> To:
> Guilherme Salgado
> <email address hidden>
> Subject:
> Request to review proposed merge of
> lp:~gmb/launchpad/indexed-message-parents into lp:launchpad/devel
> Date:
> 12/01/2009 10:33:48 AM (Tue, 01 Dec
> 2009 12:33:48 -0000)
>
>
> You have been requested to review the proposed merge of
> lp:~gmb/launchpad/indexed-message-parents into lp:launchpad/devel.
>
> This branch fixes bug 394097.
>
> The bug was caused by messages in the offending bug's set of message
> having parent messages that weren't linked to the bug. This caused the
> webservice machinery to blow up.

When can a message have a parent which is linked to a different bug?
Might be worth noting that in a comment in the code.

>
> To counter this, we've made it so that bug.indexed_messages ensures
> that IIndexedMessage.parent returns None if the parent of a message
> isn't linked to the same bug as the child message.
>
> Gavin has added a regression test to cover the bug and I've added
> tests to cover the changes to Bug.indexed_messages.
>

> === modified file 'lib/canonical/launchpad/interfaces/message.py'
> --- lib/canonical/launchpad/interfaces/message.py 2009-06-25
> 05:30:52 +0000
> +++ lib/canonical/launchpad/interfaces/message.py 2009-12-01
> 12:30:30 +0000
> @@ -189,15 +189,21 @@
> description=_("The index of this message in the list
> "
> "of messages in its context."))
>
> +
> class IndexedMessage:
> """Adds the `inside` and `index` attributes to an IMessage."""

Do you think it's worth extending this docstring to mention that the
original message's .parent is overwritten here with whatever is given in
__init__?

> delegates(IMessage)
> implements(IIndexedMessage)
>
> - def __init__(self, context, inside, index):
> + def __init__(self, context, inside, index, parent=None):
> self.context = context
> self.inside = inside
> self.index = index
> + self._parent = parent
> +
> + @property
> + def parent(self):
> + return self._parent
>
>
> class IMessageChunk(Interface):
>
> === modified file 'lib/lp/bugs/model/bug.py'
> --- lib/lp/bugs/model/bug.py 2009-11-25 13:22:01 +0000
> +++ lib/lp/bugs/model/bug.py 2009-12-01 12:30:30 +0000
> @@ -279,9 +279,19 @@
> def indexed_messages(self):
> """See `IMessageTarget`."""
> inside = self.default_bugtask
> - return [
> - IndexedMessage(message, inside, index)
> - for index, message in enumerate(self.messages)]
> + message_set = set(self.messages)
> +
> + indexed_messages = []
> + for index, message in enumerate(message_set):
> + if message.parent not in message_set:
> + parent = None
> + else:
> + parent = message.parent

I think the if/else block above should have a com...

Read more...

Revision history for this message
Guilherme Salgado (salgado) wrote :

Looks good; needs just a few minor tweaks

review: Approve (release-critical)
Revision history for this message
Graham Binns (gmb) wrote :
Download full text (5.2 KiB)

> On Tue, 2009-12-01 at 12:33 +0000, Graham Binns wrote:
> > From:
> > Graham Binns <email address hidden>
> > Reply-to:
> > <email address hidden>
> > To:
> > Guilherme Salgado
> > <email address hidden>
> > Subject:
> > Request to review proposed merge of
> > lp:~gmb/launchpad/indexed-message-parents into lp:launchpad/devel
> > Date:
> > 12/01/2009 10:33:48 AM (Tue, 01 Dec
> > 2009 12:33:48 -0000)
> >
> >
> > You have been requested to review the proposed merge of
> > lp:~gmb/launchpad/indexed-message-parents into lp:launchpad/devel.
> >
> > This branch fixes bug 394097.
> >
> > The bug was caused by messages in the offending bug's set of message
> > having parent messages that weren't linked to the bug. This caused the
> > webservice machinery to blow up.
>
> When can a message have a parent which is linked to a different bug?
> Might be worth noting that in a comment in the code.

We don't actually know yet what's caused that data to end up like that.
We're filing a bug to add an OOPS the next time a situation like this is
created, but we haven't tracked the cause down so commenting on it would
be a bit fruitless.

> >
> > To counter this, we've made it so that bug.indexed_messages ensures
> > that IIndexedMessage.parent returns None if the parent of a message
> > isn't linked to the same bug as the child message.
> >
> > Gavin has added a regression test to cover the bug and I've added
> > tests to cover the changes to Bug.indexed_messages.
> >
>
> > === modified file 'lib/canonical/launchpad/interfaces/message.py'
> > --- lib/canonical/launchpad/interfaces/message.py 2009-06-25
> > 05:30:52 +0000
> > +++ lib/canonical/launchpad/interfaces/message.py 2009-12-01
> > 12:30:30 +0000
> > @@ -189,15 +189,21 @@
> > description=_("The index of this message in the list
> > "
> > "of messages in its context."))
> >
> > +
> > class IndexedMessage:
> > """Adds the `inside` and `index` attributes to an IMessage."""
>
> Do you think it's worth extending this docstring to mention that the
> original message's .parent is overwritten here with whatever is given in
> __init__?

Well, it isn't overwritten, just masked in cases where the data are
obviously wrong. I'll add a docstring for the parent property though.

> > delegates(IMessage)
> > implements(IIndexedMessage)
> >
> > - def __init__(self, context, inside, index):
> > + def __init__(self, context, inside, index, parent=None):
> > self.context = context
> > self.inside = inside
> > self.index = index
> > + self._parent = parent
> > +
> > + @property
> > + def parent(self):
> > + return self._parent
> >
> >
> > class IMessageChunk(Interface):
> >
> > === modified file 'lib/lp/bugs/model/bug.py'
> > --- lib/lp/bugs/model/bug.py 2009-11-25 13:22:01 +0000
> > +++ lib/lp/bugs/model/bug.py 2009-12-01 12:30:30 +0000
> > @@ -279,9 +279,19 @@
> > def indexed_messages(self):
> > """See `IMessageTarget`."""
> > ...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/interfaces/message.py'
--- lib/canonical/launchpad/interfaces/message.py 2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/interfaces/message.py 2009-12-01 12:38:11 +0000
@@ -189,15 +189,21 @@
189 description=_("The index of this message in the list "189 description=_("The index of this message in the list "
190 "of messages in its context."))190 "of messages in its context."))
191191
192
192class IndexedMessage:193class IndexedMessage:
193 """Adds the `inside` and `index` attributes to an IMessage."""194 """Adds the `inside` and `index` attributes to an IMessage."""
194 delegates(IMessage)195 delegates(IMessage)
195 implements(IIndexedMessage)196 implements(IIndexedMessage)
196197
197 def __init__(self, context, inside, index):198 def __init__(self, context, inside, index, parent=None):
198 self.context = context199 self.context = context
199 self.inside = inside200 self.inside = inside
200 self.index = index201 self.index = index
202 self._parent = parent
203
204 @property
205 def parent(self):
206 return self._parent
201207
202208
203class IMessageChunk(Interface):209class IMessageChunk(Interface):
204210
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2009-11-25 13:22:01 +0000
+++ lib/lp/bugs/model/bug.py 2009-12-01 12:38:11 +0000
@@ -279,9 +279,19 @@
279 def indexed_messages(self):279 def indexed_messages(self):
280 """See `IMessageTarget`."""280 """See `IMessageTarget`."""
281 inside = self.default_bugtask281 inside = self.default_bugtask
282 return [282 message_set = set(self.messages)
283 IndexedMessage(message, inside, index)283
284 for index, message in enumerate(self.messages)]284 indexed_messages = []
285 for index, message in enumerate(message_set):
286 if message.parent not in message_set:
287 parent = None
288 else:
289 parent = message.parent
290
291 indexed_message = IndexedMessage(message, inside, index, parent)
292 indexed_messages.append(indexed_message)
293
294 return indexed_messages
285295
286 @property296 @property
287 def displayname(self):297 def displayname(self):
288298
=== added file 'lib/lp/bugs/tests/test_bug_messages.py'
--- lib/lp/bugs/tests/test_bug_messages.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bug_messages.py 2009-12-01 12:38:11 +0000
@@ -0,0 +1,44 @@
1
2# Copyright 2009 Canonical Ltd. This software is licensed under the
3# GNU Affero General Public License version 3 (see the file LICENSE).
4
5"""Webservice unit tests related to Launchpad Bugs."""
6
7__metaclass__ = type
8
9import unittest
10
11from canonical.launchpad.ftests import login
12from canonical.testing import DatabaseFunctionalLayer
13
14from lp.testing import TestCaseWithFactory
15
16
17class TestBugIndexedMessages(TestCaseWithFactory):
18 """Test the workings of IBug.indexed_messages."""
19
20 layer = DatabaseFunctionalLayer
21
22 def setUp(self):
23 super(TestBugIndexedMessages, self).setUp()
24 login('foo.bar@canonical.com')
25
26 bug_1 = self.factory.makeBug()
27 self.bug_2 = self.factory.makeBug()
28
29 message_1 = self.factory.makeMessage()
30 message_2 = self.factory.makeMessage()
31 message_2.parent = message_1
32
33 bug_1.linkMessage(message_1)
34 self.bug_2.linkMessage(message_2)
35
36 def test_indexed_message_null_parents(self):
37 # Accessing the parent of an IIndexedMessage will return None if
38 # the parent isn't linked to the same bug as the
39 # IIndexedMessage.
40 for indexed_message in self.bug_2.indexed_messages:
41 self.failUnlessEqual(None, indexed_message.parent)
42
43def test_suite():
44 return unittest.TestLoader().loadTestsFromName(__name__)
045
=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py 2009-08-20 04:46:48 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py 2009-12-01 12:38:11 +0000
@@ -134,5 +134,33 @@
134 rendered_comment, self.expected_comment_html)134 rendered_comment, self.expected_comment_html)
135135
136136
137class TestBugMessages(TestCaseWithFactory):
138
139 layer = DatabaseFunctionalLayer
140
141 def setUp(self):
142 super(TestBugMessages, self).setUp('test@canonical.com')
143 self.bug = self.factory.makeBug()
144 self.message1 = self.factory.makeMessage()
145 self.message2 = self.factory.makeMessage(parent=self.message1)
146 # Only link message2 to the bug.
147 self.bug.linkMessage(self.message2)
148 self.webservice = LaunchpadWebServiceCaller(
149 'launchpad-library', 'salgado-change-anything')
150
151 def test_messages(self):
152 # When one of the messages on a bug is linked to a parent that
153 # isn't linked to the bug, the webservice should still return
154 # the correct collection link for the bug's messages.
155 response = self.webservice.get('/bugs/%d/messages' % self.bug.id)
156 self.failUnlessEqual(response.status, 200)
157 # The parent_link for the latest message should be None
158 # because the parent is not a member of this bug's messages
159 # collection itself.
160 latest_message = response.jsonBody()['entries'][-1]
161 self.failUnlessEqual(self.message2.subject, latest_message['subject'])
162 self.failUnlessEqual(None, latest_message['parent_link'])
163
164
137def test_suite():165def test_suite():
138 return unittest.TestLoader().loadTestsFromName(__name__)166 return unittest.TestLoader().loadTestsFromName(__name__)