Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jodd-mail: SendMailSession.createAttachmentBodyPart(): Content-Disposition and Content-ID should be independent from each other #404

Closed
JensPiegsa opened this issue Apr 11, 2017 · 11 comments

Comments

@JensPiegsa
Copy link

Use case:

  • Sending emails with embedded images that do not show up as attachments

The problem:

  • In general it is up to the mail client whether / how to present inline attachments
  • For decorative images, it is not intended to make them visible as attachments
  • To support the intended behaviour, it seems to be a common practice to explicitly not set the Content-Disposition but only the Content-ID in the image mail part header

Suggestions:

  • adjust SendMailSession.createAttachmentBodyPart() to test for attachment.isInline() and attachment.getContentId() != null independently
  • introduce EmailAttachmentBuilder.setContentId(String contentId) which sets contentId but does not touch inline
@igr igr added the jodd-mail label Apr 11, 2017
@igr igr self-assigned this Apr 11, 2017
@igr
Copy link
Member

igr commented Apr 17, 2017

Hey @JensPiegsa! If I understood right, your suggestion is to separate setting inline and contentId flag. Few questions, just to be sure:

  • setting inline without setting contentId does not have much sense, right? I mean, embedding attachments without contentId make no sense, so I wonder if I can join these methods into one:
     setContentId(String contentId, boolean inline)

(Of course, overloaded 1-argument version would be available).

  • The following two attachments would be different:
	.embed(attachment().bytes(new File(PNG)).setContentId("jodd1.png"))
	.embed(attachment().bytes(new File(PNG)).setContentId("jodd2.png").setInline(true))

I don't see any difference in Gmail web client, both inline and non-inline shows as 'embedded', i.e. attachment is not shown. But again, as you said, its just email client. Do you have a public one, just to test?

  • I prepared changes, but this time it might be smart if someone else check them. Would you be so kind to review the changes if I push them to a separate branch?

Thank you, I think this is all for now :)

@JensPiegsa
Copy link
Author

Hi @igr !

I completely agree with your ideas - that's how it should be.

The different behaviour was observed with Group Office and Outlook.

I'd be happy to review / test this change.

Jens

@igr
Copy link
Member

igr commented Apr 19, 2017

Hey @JensPiegsa

If you can find some time to review: https://github.com/oblac/jodd/tree/email-404

It would be super cool! 🥇

@igr igr added this to the v3.8.6 milestone Apr 19, 2017
@JensPiegsa
Copy link
Author

Hey @igr

I build and tested the branch and want to give you a short update about this issue.

I'm still puzzled here. Eliminating the spotted difference between the working and the non-working email did not lead to the expected solution. There seems to be something else causing the wanted behaviour. Maybe it's about the kind of nesting of multi-parts.

Given that this is a rather minor issue, we should propably leave it well alone, until there is a clear way to solve it with plain java-mail.

Thanks again for the effort!

@JensPiegsa
Copy link
Author

PS: some unit test are not compatible with Windows:

jodd.io.FileNameUtilTest > testGetRelativePaths FAILED
org.junit.ComparisonFailure at FileNameUtilTest.java:157

org.junit.ComparisonFailure: expected:<..[/../b/]c> but was:<..[..\b]c>

jodd.io.FileNameUtilTest > testResolveHome FAILED
org.junit.ComparisonFailure at FileNameUtilTest.java:151

org.junit.ComparisonFailure: expected:<[C:\Users\piegsaj]/> but was:<[~]/>

jodd.util.PathUtilTest > testResolve FAILED
org.junit.ComparisonFailure at PathUtilTest.java:43

org.junit.ComparisonFailure: expected:<[/aaa/bbb/]ccc> but was:<[\aaa\bbb]ccc>

@igr
Copy link
Member

igr commented Apr 24, 2017

@JensPiegsa if you find such email you can download it to EML format (plaintext) and send it to me, so I can check the headers...

Thanx for testing!

@JensPiegsa
Copy link
Author

.. I xed out the addresses.

@igr
Copy link
Member

igr commented Apr 24, 2017

Thanx @JensPiegsa, this one has just content-ID set, no inlines.

More I think I believe that the branch change is good overall. In my tests, both inline and content-id behaves the same, in Gmail web client and in few local clients.

@igr
Copy link
Member

igr commented Apr 25, 2017

After reading some RFCs and discussions I can conclude the following:

  • inline and content-ID can be set separately.
  • embedded images should be set using Content-Disposition. Of course, setting content-ID is required if you want to use them
  • I could not find any text about having just inline and not content-ID.

So I guess, the embed() method should set both content-ID and inline for user.

@igr
Copy link
Member

igr commented Jan 19, 2018

Added to the mail FAQ.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants