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@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-dev
More help : https://help.launchpad.net/ListHelp
