Code review comment for lp:~gmb/launchpad/indexed-message-parents

> 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`."""
> > 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 comment explaining why
> it's needed; it's clear to me as I know the bug it's fixing, but won't
> be clear to others, I'm pretty sure.
>

Agreed. Done.

> > +
> > + indexed_message = IndexedMessage(message, inside, index,
> > parent)
> > + indexed_messages.append(indexed_message)
> > +
> > + return indexed_messages
> >
> > @property
> > def displayname(self):
> >

Incremental diff of changes:

=== modified file 'lib/canonical/launchpad/interfaces/message.py'
--- lib/canonical/launchpad/interfaces/message.py 2009-12-01 11:53:59 +0000
+++ lib/canonical/launchpad/interfaces/message.py 2009-12-01 14:12:31 +0000
@@ -203,6 +203,12 @@

     @property
     def parent(self):
+ """Return the parent of the current message.
+
+ Note that the parent property is taken from whatever is passed
+ in the parent parameter of __init__() rather than deferring to
+ the current context.
+ """
         return self._parent

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2009-12-01 11:53:59 +0000
+++ lib/lp/bugs/model/bug.py 2009-12-01 14:11:00 +0000
@@ -284,6 +284,10 @@
         indexed_messages = []
         for index, message in enumerate(message_set):
             if message.parent not in message_set:
+ # If the message's parent isn't also linked to the
+ # current bug we tell the IndexedMessage to treat it's
+ # parent as None. This is to avoid a recurrance of bug
+ # 394097.
                 parent = None
             else:
                 parent = message.parent