Support for MIME headers when using NewEmailFromReader

[tarmo-randma]

This PR resolves #139 - Missing MIME headers with NewEmailFromReader.

The issue once again:

In the latest master version (943e75fe5223047b345337ea74d5951d19d229ed) when e-mails are created with NewEmailFromReader and they contain attachments, the attachments will have empty MIME headers. This is due to commit b84218f6af87ca49ed7465d40545587bd94b379f on Dec 30, 2020 which resolved a different issue. The MIME headers were never covered by a Unit test, so this was not picked up.

The old removed implementation however was also buggy as it set the Content-Id header of the attachment always to the file name. This is incorrect behavior as Content-Id can have a different value and should be read directly from the MIME header.

This PR:

Resolves the issue by attaching the original MIME headers also to the Attachment object. The parsed values will thus be available separately as struct fields but the raw values will be available as well.

In order to maintain API compatibility a new variation of the Attach() function was also created named AttachWithHeaders() which takes an extra argument. It would have been possible to add the headers directly in the NewEmailFromReader() function and skip this new function but I thought it would be better to make it explicit in the API as well that the original function does not add headers. If this new function is too much clutter, I will be happy to amend the PR and add the headers directly in the NewEmailFromReader() function.

Tests have been created to cover the Content-Id header of attachments

[leucos]

Hello @tarmo-randma

Having support for Content-ID would be awesome :heart: However, I am not sure requiring attachment to have both inline AND filename set is correct (https://github.com/jordan-wright/email/pull/140/commits/a5c5e9b85911c0010cd7b3804beb50e4fc15ac9b).

Some emails just have Content-Disposition: inline + Content-Id. Here is an example found in the wild:

---------?=_25372-3420116718364
Content-Disposition: inline
Content-Id: <12562.2245409347447>
Content-Transfer-Encoding: base64
Content-Type: image/gif

R0lGODlhFAAUAIAAAAAAAAAAACH5BAEAAAAALAAAAAAUABQAAAIRhI+py+0Po5y02ouz3rz7rxUA
Ow==

What do you think ?

[bosim]

@leucos I agree, this could go both ways: the library could actually skip adding filename when at.HTMLRelated is set, just Content-ID should be sufficient.

[tarmo-randma]

I will try to take another pass at the PR. Have been busy lately. I am interested of handling the ContentID correctly for my own project as well.

[tarmo-randma]

The build is failing but I think this is due to the missing go.mod file not my changes? Could you please take look?

As for the changes I did in relation to content-id:

• In NewEmailFromReader inline attachments are now created if content-id is also defined. File name is no longer checked. The content-id check for inline attachments had to be kept due to the test TestMultipartNoContentType which wants to treat inline attachments as bodies. I read through some specs I thought applied but found no corresponding requirements. Still.. just to be sure this functionality will remain unaffected
• Tried to also follow @bosim advice and made sure that filename is not set in setDefaultHeaders when at.HTMLRelated is set.
• Updated TestAttachmentEmailFromReader and TestEmailWithHTMLAttachments to test these changes

[tarmo-randma]

I believe the build error is due to using Go 1.16 instead of 1.15? The change seems to have been really recent - this PR from just 2 days ago still uses 1.15: https://github.com/jordan-wright/email/pull/143/checks?check_run_id=2106134138

See Go 1.16 release notes: https://golang.org/doc/go1.16. I think the first part about modules applies:

Module-aware mode is enabled by default, regardless of whether a go.mod file is present in the current working directory or a parent directory. More precisely, the GO111MODULE environment variable now defaults to on. To switch to the previous behavior, set GO111MODULE to auto.

Might be something else but for me this looks like the prime suspect.

[leucos]

@tarmo-randma yes you're probably right. PR #143 passes but the author added a go.mod for this it seems:

https://github.com/jordan-wright/email/pull/143/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6

[tarmo-randma]

How would you like to handle this? Are you going to merge that go.mod? Proper module support would be great but I am not sure how adding it would impact compatibility for everyone. Maybe it should include a major version bump?

I am ofcourse OK with either solution - go-mod or a build flag to change the new default behavior

[tarmo-randma]

I think this works now. Should also allow accepting other PR-s