addAttachment() crashes with UnicodeDecodeError:

Reproducer (works on staging):

b=lp.bugs[89040]
b.addAttachment(comment='', data=open('thread.txt').read(), description='test353805', filename='test353805', is_patch=False)

This apparently happens because something in launchpadlib assumes that data is UTF-8 encoded. While stack traces are by and large text, they can have binary stuff in it, such as this one.

This even happens if I supply an explicit content type:

>>> b.addAttachment(comment='', data=open('/home/martin/thread.txt').read(), description='test353805', filename='test353805', is_patch=False, content_type='application/octet-stream')

with application/octet-stream it shouldn't make any assumptions about the content at least.

Also, in the Launchpad web UI I could attach thread.txt just fine, and it's even treated as text/plain (which is good for stack traces).

Is there a quick workaround for this?

Easier reproducer without external file:

>>> b=lp.bugs[89040]
>>> b.addAttachment(comment='', data='"]\xb6"\n', description='test353805', filename='test353805', is_patch=False)

I work around this with

  data=report['Stacktrace'].decode('UTF-8', 'replace')

for now, but I'd like to revert this soon, since it is destructive.

Obviously launchpadlib should not be running binary data through simplejson. The question is how launchpadlib is supposed to know that binary data is appropriate for that particular argument, 'data'.

The 'data' argument is published as an IBytes field in Launchpad. If the WADL for that field specified its type as 'binary', launchpadlib and wadllib could be programmed to handle it correctly. We'd probably have to change lazr.restful, wadllib, and lazr.restfulclient, but all of the changes would be fairly small. I'd prefer to wait on this until we have the lazr.restful and lazr.restfulclient separations complete.

I got whacked by this bug, so I'm going to start fixing it tomorrow.

Before we close out this bug, we should add a launchpadlib branch containing an integration test. All the tests written so far are unit tests.

Cross-posted from launchpad-users in case anyone interested is watching this bug.

The fix for this bug means that anyone who's currently happy sending *non-binary data* into addAttachment, will have to upgrade to the new version of launchpadlib. I'd like to know if anyone is sending plain text diffs or crash reports to addAttachment. If so, how difficult would it be for you to upgrade? This way I can decide whether to put in backwards compatibility code.

Leonard Richardson [2009-05-11 20:03 -0000]:
> The fix for this bug means that anyone who's currently happy sending
> *non-binary data* into addAttachment, will have to upgrade to the new
> version of launchpadlib. I'd like to know if anyone is sending plain
> text diffs or crash reports to addAttachment. If so, how difficult would
> it be for you to upgrade? This way I can decide whether to put in
> backwards compatibility code.

For me and apport this would be okay, just takes some work to backport
the packages to hardy/intrepid/jaunty.

Hi,

I'm interested in a fix for this bug. I would like to add binary attachments to launchpad, but this bug is preventing me from doing it.

Please correct me if I'm saying something *VERY* stupid.
For backwards compatibility I would suggest to leave the current addAttachment as it is and add 'addAttachmentBinary', or something like that, where we don't pass binary data to simplejson.

Rgds,

Fer

The backwards compatibility problem is not that launchpadlib needs to do different things for binary and non-binary data. The problem is that when we change the web service, existing installations of launchpadlib will stop working. As people upgrade to the new version the problem will go away. The question is whether it's acceptable to tell anyone who has problems to upgrade to the latest version.

The same problem can be seen in Bug #408605, with project_release.addFile(). Most release tarballs are actually binary :-).

Maybe an additional data_base64/file_content_base64 parameter?

This should be fixed in launchpadlib 1.5.1. In fact there's a script in samples/ for creating a release file.