homepage

I rencently had trouble finding a bug in an email processing script because of an error that pass silently in the email module.
The get_payload() method in the email.message module always return something when the decode argument is set to True.
This behaviour is well documented, but is their a reason to catch the decoding errors ?
Why not let them pop-up to the calling code ?
renaud

Renaud: providing a test script would help us here.

Daniel: i can't remember the exact scenario (i filled this bug 2 years
ago !)
after having a look back at email.message.Message.get_payload, i
remember the problem: the decoding errors are silented by the method and
you have no way to know if the decoding has been successful or not.
to find my malformed email i had to patch the module like that
(basically just removing the try/except blocks):
<patch>
--- message.py.orig	2009年04月03日 23:46:47.000000000 +0200
+++ message.py	2009年04月03日 23:48:27.000000000 +0200
@@ -192,19 +192,11 @@
 if cte == 'quoted-printable':
 return utils._qdecode(payload)
 elif cte == 'base64':
- try:
- return utils._bdecode(payload)
- except binascii.Error:
- # Incorrect padding
- return payload
+ return utils._bdecode(payload)
 elif cte in ('x-uuencode', 'uuencode', 'uue', 'x-uue'):
 sfp = StringIO()
- try:
- uu.decode(StringIO(payload+'\n'), sfp, quiet=True)
- payload = sfp.getvalue()
- except uu.Error:
- # Some decoding problem
- return payload
+ uu.decode(StringIO(payload+'\n'), sfp, quiet=True)
+ return sfp.getvalue()
 # Everything else, including encodings with 8bit or 7bit are
returned
 # unchanged.
 return payload
</patch>
once again, the behaviour is documented, so it's not really a bug.
but it caused me a lot of trouble (and it does not conforms very well to
"Errors should never pass silently.":)
but i guess applying such a patch could potentially break number of
client code so... is it worth the change?

Hmm, ISTM that a change that breaks existing code that relies on
documented behavior has a negligible chance of being accepted.
However, I agree that the current behavior isn't developer-friendly. I
think adding an alternative behavior to get_payload (add a new
parameter?), allowing callers to log errors and/or displaying warnings
could have a better chance of being accepted, while still providing the
control you want.
Barry?

good idea, why not something like sketched in the attached patch?
it does not break any existing code, while providing a way for new users
to have a chance to get the decoding errors.
of course, the doc should be updated accordingly, and tests should be added.

Looks good to me, adding tests and docs could be a nice Bug Day task.

Renaud,
Here's your patch with a test case against trunk.

looks very good to me.
thanks daniel for your work
renaud

To me a bug is a bug is a bug, but I'm sure there are different opinions so feel free to swap this back to feature request if you think that fits. I'll try and have a crack at this tomorrow.

No, it's not a bug if it is working as documented. (Well, it might be a design bug, but fixes to those are called "feature requests" :)
We're planning on addressing this is email6, by the way.
I'm OK with trying to get this into 3.2, but I'll be giving higher priority to bugs.

I've patched py3k test_email.py without patching message.py and the test passes on Windows Vista. Either the patch is wrong, I've had finger problems or I'm misunderstanding the intent, can someone please advise.

I've double checked this and still all tests pass before patching message.py. Could somebody else please pick this one up, thanks.

I've taken another look at this, and the email module is pretty consistent about just passing through data if it can't interpret it according to standards. I think it would lead to a cluttered API if we add support for being strict and raising errors piecemeal. So I'm deferring this to 3.3 and the email6 work, where we plan to have a comprehensive 'strict' mode.

OK, here is patch based on the new policy support in 3.3. I have some concern that the behavior change it introduces might cause some issues, but since it seems like a reasonable change and is happening at a feature release boundary, I think it should be OK.
The behavior change is that now by default get_payload will always produce *some* kind of decoding of a base64 part, and will register defects if it finds padding errors or characters outside the base64 character set.

Oh, and to be clear on how this addresses the bug report: if you set 'raise_on_defect' to true when you call the parser, then you'll get an exception when you call msg.get_payload(decode=True).

Hmm. Thinking about it, though, that might not work if there are other errors in the message, many of which are more benign. Probably the raise_on_defect control is a little too coarse.
I've been thinking that we need a way to set the policy on an already existing message, which would make this work more usefully. But that's a different issue.

New changeset 17341b51af4f by R David Murray in branch 'default':
#1672568: email now registers defects for base64 payload format errors.
http://hg.python.org/cpython/rev/17341b51af4f 

New changeset d68e30be755e by R David Murray in branch 'default':
News item for #1672568.
http://hg.python.org/cpython/rev/d68e30be755e 

Fix committed.