classification
Title: EmailMessage.add_attachment() creates parts with spurious MIME-Version header.
Type: behavior Stage: commit review
Components: email Versions: Python 3.6, Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: barry, groner, r.david.murray
Priority: normal Keywords: patch

Created on 2015-09-25 19:47 by groner, last changed 2016-09-24 21:50 by barry.

Files
File name Uploaded Description Edit
test_add_attachment_does_not_add_MIME_Version_in_attachment.patch groner, 2015-09-25 19:47 failing test case review
test_add_attachment_does_not_add_MIME_Version_in_attachment.patch groner, 2015-09-25 20:05 revised failing tests review
test_MIME_Version.patch groner, 2015-09-25 20:07 Actually revised patch. review
fix_mime_version_headers.patch r.david.murray, 2016-09-10 03:11 review
fix_mime_version_headers-2.patch r.david.murray, 2016-09-14 00:34 review
Messages (10)
msg251600 - (view) Author: Kai Groner (groner) * Date: 2015-09-25 19:47
Because MIMEPart.add_attachment() creates parts using type(self), EmailMessage.add_attachment() creates parts of type EmailMessage.  This results in a MIME-Version header being added to parts where it isn't needed.

https://tools.ietf.org/html/rfc2045#section-4
msg251601 - (view) Author: Kai Groner (groner) * Date: 2015-09-25 20:05
Relatedly EmailMessage.make_mixed(), etc don't set MIME-Version on the multipart (it is only set on the part).  Additional tests attached.
msg275490 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-09-09 23:24
It turns out that solving this header problem via MIMEPart isn't really a good idea, given the rest of the API.  I don't want to not use 'type', or rather I do want to use the new policy.message_factory, and I don't really want to have to have two message_factory attributes.  I'm considering the possibility of only adding a MIME header in the Generator at the top level if and only if there are no parser derived headers.  (This will do the right think only if you stick to the new API, since the old API adds the header indiscriminately in several places).
msg275552 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-09-10 03:10
Attached is my proposal (absent the doc changes).  I'm not happy with it, though, because it means that you don't have the MIME-Version header until you do bytes(msg) (or otherwise generate it), *and* you get a MIME-Version header if you do str(part) for a subpart.  That makes debugging significantly confusing, but only with respect to whether or not you have a MIME-Version header.

I'm inclined to commit it, since this whole thing applies *only* to fully constructed messages, rather than parsed-and-manipulated messages.  I don't like it, but I haven't come up with a better solution so far.
msg275836 - (view) Author: Kai Groner (groner) * Date: 2016-09-11 19:27
From https://tools.ietf.org/html/rfc2045#section-4
>   Note that the MIME-Version header field is required at the top level
>   of a message.  It is not required for each body part of a multipart
>   entity.  It is required for the embedded headers of a body of type
>   "message/rfc822" or "message/partial" if and only if the embedded
>   message is itself claimed to be MIME-conformant.

This patch looks like it avoids adding a MIME-Version header to parts with no mime metadata, but it can actually be omitted from most parts.
msg275840 - (view) Author: Kai Groner (groner) * Date: 2016-09-11 19:41
I agree that there isn't a way to know if a header should be added implicitly, until Generator is called.

A possible change to the Generator behavior is to generate the header in the serialized output, without modifying the original.  I don't know that it makes debugging easier, but it would keep the Generator from having this side effect.

Maybe being explicit is better, a utility factory could be used to keep the common case simple.  I don't know if such a change is reasonable at this point.
msg275963 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-09-12 05:08
Hmm.  Only doing it in the output is an interesting idea.  It's a bit ugly to implement, but doable.
msg276365 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-09-14 00:34
Here's patch that only puts the MIME-Version in the output stream and doesn't modify the message (permanently, at least).  I like this better.  This patch is against 3.5 (the previous one was against 3.6).  Since this stuff is provisional in 3.5, I'm thinking I'll make the full change there, including deleting MIMEMessage from the docs.
msg277320 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-09-24 17:05
Barry, would you care to render an opinion on this proposed fix?
msg277337 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2016-09-24 21:50
On Sep 24, 2016, at 05:06 PM, R. David Murray wrote:

>Barry, would you care to render an opinion on this proposed fix?

I think the general approach is probably the best you can do.  I noticed a
couple of things though with RDM's v.2 patch.

First, I get test failures when applying to the 3.5 branch, specifically
test_mime_version_added_to_mime_message() fails.  I won't attach the failure
I'm seeing unless you can't reproduce it.

Second, if I'm reading RFC 2045#section-4 correctly, I think the embedded
rfc822 attachment should have a MIME-Version header, in this code:

-----snip snip-----
from email.message import EmailMessage

m = EmailMessage()
m.set_content('This is a body')

o = EmailMessage()
o.add_attachment(m)

print(o)

print(m['mime-version'])
-----snip snip-----

But instead I get:

Content-Type: multipart/mixed; boundary="===============4744209610526815348=="
MIME-Version: 1.0

--===============4744209610526815348==
Content-Type: message/rfc822
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment

MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

This is a body

--===============4744209610526815348==--

1.0
History
Date User Action Args
2016-09-24 21:50:35barrysetmessages: + msg277337
2016-09-24 17:06:00r.david.murraysetmessages: + msg277320
stage: patch review -> commit review
2016-09-14 00:34:51r.david.murraysetfiles: + fix_mime_version_headers-2.patch

messages: + msg276365
stage: patch review
2016-09-12 05:08:18r.david.murraysetmessages: + msg275963
2016-09-11 19:41:26gronersetmessages: + msg275840
2016-09-11 19:27:13gronersetmessages: + msg275836
2016-09-10 03:11:19r.david.murraysetfiles: + fix_mime_version_headers.patch
2016-09-10 03:10:52r.david.murraysetmessages: + msg275552
2016-09-09 23:24:59r.david.murraysetmessages: + msg275490
versions: + Python 3.6
2015-09-25 20:07:39gronersetfiles: + test_MIME_Version.patch
2015-09-25 20:05:04gronersetfiles: + test_add_attachment_does_not_add_MIME_Version_in_attachment.patch

messages: + msg251601
2015-09-25 19:47:28gronercreate