Raise a more helpful exception in email.message.Message.attach after set_payload("some string")

[michaelhenry]

BPO	11558
Nosy	@warsaw, @bitdancer
Files	test_email_attach_to_string.py: Demonstrates confusing exception returned from attach() after set_payload("some string") string_payload_to_attach.patch test_email_attach_to_string.patch test_email_attach_to_string_11558.patch test_email_attach_to_string_11558.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 = None
closed_at = <Date 2014-03-06.16:46:52.768>
created_at = <Date 2011-03-15.15:34:15.414>
labels = ['type-feature', 'library', 'expert-email']
title = 'Raise a more helpful exception in email.message.Message.attach after set_payload("some string")'
updated_at = <Date 2014-03-06.16:46:52.766>
user = 'https://bugs.python.org/michaelhenry'

bugs.python.org fields:

activity = <Date 2014-03-06.16:46:52.766>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2014-03-06.16:46:52.768>
closer = 'r.david.murray'
components = ['Library (Lib)', 'email']
creation = <Date 2011-03-15.15:34:15.414>
creator = 'michael.henry'
dependencies = []
files = ['21214', '34258', '34269', '34270', '34273']
hgrepos = []
issue_num = 11558
keywords = ['patch']
message_count = 10.0
messages = ['130984', '212498', '212501', '212581', '212594', '212596', '212600', '212610', '212814', '212815']
nosy_count = 5.0
nosy_names = ['barry', 'r.david.murray', 'python-dev', 'michael.henry', 'varun']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue11558'
versions = ['Python 3.4', 'Python 3.5']

[michaelhenry]

The attached test program, test_email_attach_to_string.py, demonstrates the
desire for email.message.Message.attach to raise a more helpful exception when
the user incorrectly invokes attach() after setting the payload to a string.

[varun]

I have made a patch which raises TypeError whenever a string type payload is attached to message using email.Message.attach() method.I have also added a unit test for the same. Need review.

[bitdancer]

Thanks, Varun. Your patch addresses an issue with the current API, but it doesn't address the problem raised in this issue. The problem in this issue is what happens when the *payload* is a string, and you call attach (to attach a message object) on that message.

Your fix addresses what happens if you pass a string to attach itself. This also results in an invalid message (it ends up with a payload that contains a list consisting of a single string). Thinking about changing this raises some interesting questions, though. For one, the problem isn't that the argument to attach was a string, but that it was not an object that generator knows how to handle (that is, it wasn't a Message object). For another, is it possible someone is using the email package in a weird context where attaching non-message objects is actually useful? If so, and we disallow it, then we break someones code.

Since no one has reported calling attach with a non-message object as an actual bug, I'm inclined not to make this change. Python is a "consenting adults" language, so unless there is a specific reason to disallow something, we generally allow it.

To work on a fix for the reported issue, you should start by turning the code in test_email_attach_to_string into a unit test.

[varun]

Afer getting a lot of help from david on irc, i finally improved the patch. Now it catches AttributeError and raises TypeError for non-object types in attach function.

[bitdancer]

Yes, this is the right idea.

A couple of comments: I'm a bit concerned that the AttributeError might catch something other than the specific error generated by this case, but since that is unlikely and in any case would be revealed by the chained traceback, I think this is OK, and not worth the effort it
would take to narrow it.

The test, while correct, is IMO overbroad. I prefer to only look for the essential parts of the message, which in this case would be mention of 'attach' and 'non-multipart'. Thus the regex I suggested on irc for use in the test: 'attach.*non-multipart'.

Also, you should wrap all lines to less than 79 characters, per PEP-8.

[varun]

I have fixed the PEP-8 voilations and shortened the regex as you suggested in the new patch.

[bitdancer]

You missed a PEP-8 line length fix.

[varun]

Sorry :)

[python-dev]

New changeset 302c8fdb17e3 by R David Murray in branch 'default':
bpo-11558: Better message if attach called on non-multipart.
http://hg.python.org/cpython/rev/302c8fdb17e3

[bitdancer]

Committed, thanks Varun.

Note that I made two changes to your patch: added a missing space in the folded message (your second PEP-8 change), and I renamed the test method to make it clearer what was being tested.