launchpad-dev team mailing list archive

[James Westby <jw+debian@xxxxxxxxxxxxxxx>]

Hi Brad,

Thanks for writing this up. It reminded of another bug to do with
validators and dict ordering:

  https://bugs.launchpad.net/lazr.restful/+bug/561521

I'm not sure if there are changes that could be made to avoid these
sorts of problems in future, but in that bug Michael makes a statement
that someone may be able to confirm or deny, which could eliminate these
problems (at the risk of introducing them elsewhere of course.)

Thanks,

James

On Fri, 30 Sep 2011 16:41:00 -0400, Brad Crittenden <brad.crittenden@xxxxxxxxxxxxx> wrote:
> Hi,
> 
> I just spent a long time trying to figure out a very frustrating bug so I thought I'd share, even though I'm a bit dubious of any general lessons.
> 
> Some rather innocuous changes resulted in test failures dealing with conjoined bugtasks.  Poring over diffs of the changes didn't reveal anything obvious that could be causing the problem.
> 
> It turns out that BugTask attributes use Storm validators extensively.  (Really these are "transformers" as they don't set errors, return True or False, or throw exceptions. Instead they return transformed values.)  Further these validators were implicitly depending on the order of attributes are set when a new BugTask is constructed.
> 
> One section of 'validate_conjoined_attribute' has this snippet:
> 
>     # If this bugtask has no bug yet, then we are probably being
>     # instantiated.
>     if self.bug is None:
>         return value
> 
> BugTask has never been updated to a Storm class, and is instantiated in BugTaskSet 'createTask' with:
> 
>    bugtask = BugTask(**create_params)
> 
> Those kwargs eventually get passed down to the Storm SQLObjectBase class and are used like:
> 
>     def set(self, **kwargs):
>         for attr, value in kwargs.iteritems():
>             setattr(self, attr, value)
> 
> The problem is the order the attributes are set depends on the ordering returned by iteritems(), which for a dict is arbitrary and should never be relied upon.  However the code depended upon self.bug being None, i.e. not yet set, as a check for whether a new object was being constructed or a mature object was having an attribute changed.
> 
> The code I was working on simply changed the name of one attribute from 'status' to '_status' and this upset the order of the attributes returned by .iteritems().  Before my change 'bug' was consistently being set as the last attribute.  After my change it was set before '_status' and the behavior of the validator changed.
> 
> The minimal fix is easy: replace the test for 'self.bug is None' with 'self._SO_creating' an attribute SQLObjectBase sets during object construction. 
> 
> In retrospect the comment
> 
>     # If this bugtask has no bug yet, then we are probably being
>     # instantiated.
> 
> should have been a red flag to me trying to debug my code, to the reviewer who originally looked at the branch where it was introduced, and to the developer who wrote it.  "Probably" is a weasel-word that shouldn't be used to describe code behavior.
> 
> Thanks to Danilo helping me to figure out the change in behavior.
> 
> --bac
> _______________________________________________
> Mailing list: https://launchpad.net/~launchpad-dev
> Post to     : launchpad-dev@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~launchpad-dev
> More help   : https://help.launchpad.net/ListHelp
>