classification
Title: Raise a more helpful exception in email.message.Message.attach after set_payload("some string")
Type: enhancement Stage: resolved
Components: email, Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: barry, michael.henry, python-dev, r.david.murray, varun
Priority: normal Keywords: patch

Created on 2011-03-15 15:34 by michael.henry, last changed 2014-03-06 16:46 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
test_email_attach_to_string.py michael.henry, 2011-03-15 15:34 Demonstrates confusing exception returned from attach() after set_payload("some string")
string_payload_to_attach.patch varun, 2014-03-01 12:18 review
test_email_attach_to_string.patch varun, 2014-03-02 19:21 review
test_email_attach_to_string_11558.patch varun, 2014-03-02 22:06 review
test_email_attach_to_string_11558.patch varun, 2014-03-03 02:42 review
Messages (10)
msg130984 - (view) Author: Michael Henry (michael.henry) * Date: 2011-03-15 15:34
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.
msg212498 - (view) Author: Varun Sharma (varun) * Date: 2014-03-01 12:18
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.
msg212501 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-01 14:20
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.
msg212581 - (view) Author: Varun Sharma (varun) * Date: 2014-03-02 19:21
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.
msg212594 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-02 21:30
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 PEP8.
msg212596 - (view) Author: Varun Sharma (varun) * Date: 2014-03-02 22:06
I have fixed the pep8 voilations and shortened the regex as you suggested in the new patch.
msg212600 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-02 23:06
You missed a PEP8 line length fix.
msg212610 - (view) Author: Varun Sharma (varun) * Date: 2014-03-03 02:42
Sorry :)
msg212814 - (view) Author: Roundup Robot (python-dev) Date: 2014-03-06 16:44
New changeset 302c8fdb17e3 by R David Murray in branch 'default':
#11558: Better message if attach called on non-multipart.
http://hg.python.org/cpython/rev/302c8fdb17e3
msg212815 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-06 16:46
Committed, thanks Varun.

Note that I made two changes to your patch: added a missing space in the folded message (your second PEP8 change), and I renamed the test method to make it clearer what was being tested.
History
Date User Action Args
2014-03-06 16:46:52r.david.murraysetstatus: open -> closed
versions: + Python 3.4, Python 3.5
messages: + msg212815

type: enhancement
stage: resolved
2014-03-06 16:44:32python-devsetnosy: + python-dev
messages: + msg212814
2014-03-03 02:42:11varunsetfiles: + test_email_attach_to_string_11558.patch

messages: + msg212610
2014-03-02 23:06:10r.david.murraysetmessages: + msg212600
2014-03-02 22:06:51varunsetfiles: + test_email_attach_to_string_11558.patch

messages: + msg212596
2014-03-02 21:30:25r.david.murraysetmessages: + msg212594
2014-03-02 19:21:40varunsetfiles: + test_email_attach_to_string.patch

messages: + msg212581
2014-03-01 14:20:33r.david.murraysetnosy: + barry
messages: + msg212501
components: + email
2014-03-01 12:18:46varunsetfiles: + string_payload_to_attach.patch

nosy: + varun
messages: + msg212498

keywords: + patch
2011-03-15 15:34:15michael.henrycreate