silent error in email.message.Message.get_payload

[rndblnch]

BPO	1672568
Nosy	@warsaw, @devdanzin, @bitdancer
Files	unsilent_get_payload_decoding_errors.patch: rough patch noisy_get_payload.diff: Add a test case for get_payload with silent=False base64_payload_defects.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/bitdancer'
closed_at = <Date 2012-05-28.01:28:30.336>
created_at = <Date 2007-03-02.17:04:08.000>
labels = ['type-feature', 'library', 'expert-email']
title = 'silent error in email.message.Message.get_payload'
updated_at = <Date 2012-05-28.01:28:30.336>
user = 'https://bugs.python.org/rndblnch'

bugs.python.org fields:

activity = <Date 2012-05-28.01:28:30.336>
actor = 'r.david.murray'
assignee = 'r.david.murray'
closed = True
closed_date = <Date 2012-05-28.01:28:30.336>
closer = 'r.david.murray'
components = ['Library (Lib)', 'email']
creation = <Date 2007-03-02.17:04:08.000>
creator = 'rndblnch'
dependencies = []
files = ['13631', '13967', '25738']
hgrepos = []
issue_num = 1672568
keywords = ['patch']
message_count = 19.0
messages = ['31413', '84660', '85337', '85605', '85632', '86270', '87606', '87684', '110522', '110526', '110566', '111494', '123138', '161722', '161726', '161728', '161745', '161746', '161747']
nosy_count = 6.0
nosy_names = ['barry', 'rndblnch', 'ajaksu2', 'r.david.murray', 'BreamoreBoy', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1672568'
versions = ['Python 3.3']

[rndblnch]

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

[devdanzin]

Renaud: providing a test script would help us here.

[rndblnch]

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?

[devdanzin]

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?

[rndblnch]

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.