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
1=== modified file 'lib/canonical/launchpad/interfaces/message.py'
2--- lib/canonical/launchpad/interfaces/message.py 2009-06-25 05:30:52 +0000
3+++ lib/canonical/launchpad/interfaces/message.py 2009-12-01 12:38:11 +0000
4@@ -189,15 +189,21 @@
5 description=_("The index of this message in the list "
6 "of messages in its context."))
7
8+
9 class IndexedMessage:
10 """Adds the `inside` and `index` attributes to an IMessage."""
11 delegates(IMessage)
12 implements(IIndexedMessage)
13
14- def __init__(self, context, inside, index):
15+ def __init__(self, context, inside, index, parent=None):
16 self.context = context
17 self.inside = inside
18 self.index = index
19+ self._parent = parent
20+
21+ @property
22+ def parent(self):
23+ return self._parent
24
25
26 class IMessageChunk(Interface):
27
28=== modified file 'lib/lp/bugs/model/bug.py'
29--- lib/lp/bugs/model/bug.py 2009-11-25 13:22:01 +0000
30+++ lib/lp/bugs/model/bug.py 2009-12-01 12:38:11 +0000
31@@ -279,9 +279,19 @@
32 def indexed_messages(self):
33 """See `IMessageTarget`."""
34 inside = self.default_bugtask
35- return [
36- IndexedMessage(message, inside, index)
37- for index, message in enumerate(self.messages)]
38+ message_set = set(self.messages)
39+
40+ indexed_messages = []
41+ for index, message in enumerate(message_set):
42+ if message.parent not in message_set:
43+ parent = None
44+ else:
45+ parent = message.parent
46+
47+ indexed_message = IndexedMessage(message, inside, index, parent)
48+ indexed_messages.append(indexed_message)
49+
50+ return indexed_messages
51
52 @property
53 def displayname(self):
54
55=== added file 'lib/lp/bugs/tests/test_bug_messages.py'
56--- lib/lp/bugs/tests/test_bug_messages.py 1970-01-01 00:00:00 +0000
57+++ lib/lp/bugs/tests/test_bug_messages.py 2009-12-01 12:38:11 +0000
58@@ -0,0 +1,44 @@
59+
60+# Copyright 2009 Canonical Ltd. This software is licensed under the
61+# GNU Affero General Public License version 3 (see the file LICENSE).
62+
63+"""Webservice unit tests related to Launchpad Bugs."""
64+
65+__metaclass__ = type
66+
67+import unittest
68+
69+from canonical.launchpad.ftests import login
70+from canonical.testing import DatabaseFunctionalLayer
71+
72+from lp.testing import TestCaseWithFactory
73+
74+
75+class TestBugIndexedMessages(TestCaseWithFactory):
76+ """Test the workings of IBug.indexed_messages."""
77+
78+ layer = DatabaseFunctionalLayer
79+
80+ def setUp(self):
81+ super(TestBugIndexedMessages, self).setUp()
82+ login('foo.bar@canonical.com')
83+
84+ bug_1 = self.factory.makeBug()
85+ self.bug_2 = self.factory.makeBug()
86+
87+ message_1 = self.factory.makeMessage()
88+ message_2 = self.factory.makeMessage()
89+ message_2.parent = message_1
90+
91+ bug_1.linkMessage(message_1)
92+ self.bug_2.linkMessage(message_2)
93+
94+ def test_indexed_message_null_parents(self):
95+ # Accessing the parent of an IIndexedMessage will return None if
96+ # the parent isn't linked to the same bug as the
97+ # IIndexedMessage.
98+ for indexed_message in self.bug_2.indexed_messages:
99+ self.failUnlessEqual(None, indexed_message.parent)
100+
101+def test_suite():
102+ return unittest.TestLoader().loadTestsFromName(__name__)
103
104=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
105--- lib/lp/bugs/tests/test_bugs_webservice.py 2009-08-20 04:46:48 +0000
106+++ lib/lp/bugs/tests/test_bugs_webservice.py 2009-12-01 12:38:11 +0000
107@@ -134,5 +134,33 @@
108 rendered_comment, self.expected_comment_html)
109
110
111+class TestBugMessages(TestCaseWithFactory):
112+
113+ layer = DatabaseFunctionalLayer
114+
115+ def setUp(self):
116+ super(TestBugMessages, self).setUp('test@canonical.com')
117+ self.bug = self.factory.makeBug()
118+ self.message1 = self.factory.makeMessage()
119+ self.message2 = self.factory.makeMessage(parent=self.message1)
120+ # Only link message2 to the bug.
121+ self.bug.linkMessage(self.message2)
122+ self.webservice = LaunchpadWebServiceCaller(
123+ 'launchpad-library', 'salgado-change-anything')
124+
125+ def test_messages(self):
126+ # When one of the messages on a bug is linked to a parent that
127+ # isn't linked to the bug, the webservice should still return
128+ # the correct collection link for the bug's messages.
129+ response = self.webservice.get('/bugs/%d/messages' % self.bug.id)
130+ self.failUnlessEqual(response.status, 200)
131+ # The parent_link for the latest message should be None
132+ # because the parent is not a member of this bug's messages
133+ # collection itself.
134+ latest_message = response.jsonBody()['entries'][-1]
135+ self.failUnlessEqual(self.message2.subject, latest_message['subject'])
136+ self.failUnlessEqual(None, latest_message['parent_link'])
137+
138+
139 def test_suite():
140 return unittest.TestLoader().loadTestsFromName(__name__)