appending tags to bug.tags is not supported properly on lp_save()

[Markus Korn]

Other then documented in the reference (http://people.ubuntu.com/~flacoste/launchpad-api-doc.html#bug) bug.tags is not a whitespace separated string but a list. It would be nice if launchpadlib could support the related list operations properly, for example:

In [38]: b = launchpad.bugs[253799]

In [39]: b.tags
Out[39]: [u'bar', u'foo']

In [40]: b.tags.append("far")

n [41]: b._dirty_attributes
Out[41]: {}

This only works for changing the complete object:
In [42]: b.tags = ["bood"]

In [43]: b._dirty_attributes
Out[43]: {'tags': ['bood']}

Markus

---

Workaround: rather than appending to the list, set the attribute to a new list that has the value you want:

Not: b.tags.append('foo')

but rather: b.tags = b.tags + ['foo']

[Leonard Richardson (leonardr)]

It's not really practical to monitor a list for changes; we'd have to wrap it in a proxy object that knew about the entire Python list interface. We could prevent the problem by deserializing tags into a tuple rather than a list. Then you'd know you had to replace the tuple with another one.

[Markus Korn (thekorn)]

Hi Leonard,
thanks for your reply. I don not really care about monitoring changes to an object, I just choose this example to show why the following workflow is not working:

In [38]: b = launchpad.bugs[253799]

In [39]: b.tags
Out[39]: [u'bar', u'foo']

In [40]: b.tags.append("far")

n [41]: b.lp_save()

This fails silently, because launchpadlib does not know about changes of mutable attributes of an object.
IMHO, changing the type of the tags attribute to a tuple or a string is not a good solution, the workflow described above is the most natural one.

Without knowing all the internals of launchpadlib I agree that wrapping it in a proxy object is not a good solution here. But what about storing mutable objects in a cache right after fetching the data, and when calling lp_save() compare the current value with the value in the cache. This way you know if a mutable object changed. - just my suggestion.

Markus

[Barry Warsaw (barry)]

You could potentially inherit bug.tags from built-in list and do the overriding (i.e. proxying), but it's kind of a tricky situation. It reminds me of the workarounds that something like ZODB has to do.

The easiest thing to do would be to expose a method that allowed you to mark an attribute as dirty so that you could explicitly say, e.g. to save tags when you call lp_save().

I'm not sure the idea of caching the mutable objects is workable. They could be very large, and you'd have to keep a unique copy to do the comparison. You could just always save any mutable attributes, but I'm not sure that the wadl gives you that information.

So I think the simplest solution is also the most explicit:

>>> b.tags.append('foo')
>>> b.lp_mark('tags')
>>> b.lp_save() # patches tags

[Markus Korn (thekorn)]

I attached two branches to implement this workflow:

>>> b.tags.append("foo")
>>> b.lp_save()

This is done in wadllib by implementing a subclass of list with an additional property 'changed' called ChangedList.
This class is used for the 'tags' attribute of the '#bug' namespace

Markus

[Rick Spencer (rick-rickspencer3)]

I was pointed to a workaround:

  #first create a fresh bug
  bug = self.launchpad.bugs[bug.id]
  #then reassign the tags list
  bug.tags = bug.tags + ['needs-bug-squad']
  #then you can save it
  bug.lp_save()

HTH until it is fixed

[Facundo Batista (facundo)]

Maybe the easiest way to handle this, is not to make the tags attribute a list, but a tuple!

Nobody will try to modify the tuple, :)

Also, the docs should be fixed (https://edge.launchpad.net/+apidoc/#bug), because they say that tags are "Separated by whitespace."

Thanks!

[Matt Zimmerman (mdz)]

Is there something wrong with the branches attached to this bug, which seem to fix the problem?

[Robert Collins (lifeless)]

Neither of the branches have been proposed for merging, and Markus hasn't (AFAICT) signed the canonical contributors agreement.

Markus, we'd love to incorporate these fixes - what you need to do is sign the canonical contributor agreement - http://www.canonical.com/contributors - add a CC to francis dot lacoste at canonical dot com as the project lead for Launchpad. Then you need to propose both of your branches for merging. To do that just click on 'propose for merge' and the default location it suggests will be correct.

With both of those things done our code reviewers will be able to look at your branch and assess whether the fix is appropriate and ready to merge, or whether it needs further work done (which they may do, or may ask you to do).

Cheers,
Rob

[Markus Korn (thekorn)]

I think I signed the agreement sometime back in 2009, I will check tomorrow, and see if these two branches are still mergeable. Since I haven't look at them for a long time.

[Bryce Harrington (bryce)]

Hi Markus, can you confirm about the agreement?

[Colin Watson (cjwatson)]

Markus, you don't seem to be a member of ~contributor-agreement-canonical, so either you never signed it or it got lost ...