bbottema / simple-java-mail

Simple API, Complex Emails (Jakarta Mail smtp wrapper)
http://www.simplejavamail.org
Apache License 2.0
1.2k stars 261 forks source link

[enhancement+bug] Make EmailConverter API more consistent regarding Session parameter, don't use `Session.getDefaultInstance` anymore and fix bug where `emlToEmailBuilder` used `emlToMimeMessage` #508

Closed johannesg-px closed 2 months ago

johannesg-px commented 2 months ago

Hi,

thanks for this lib, its really helpful.

However we have run into an issue:

In org.simplejavamail.converter.EmailConverter, some places need an instance of jakarta.mail.Session, so there is the method createDummySession(), which returns Session.getDefaultInstance(new Properties()). In our project setup, we have a mail session from another context, which cannot be used in this context. Session.getDefaultInstance(new Properties()) will always get the existing session, which leads to a race condition: if createDummySession() is called first, the session is created here, but then it wont work in the other context; if the session is created by our logic first, it is returned in createDummySession(), which leads to errors.

The most obvious fix would be to override createDummySession() to return Session.getInstance(new Properties()) instead, since this always creates a new instance. However, we cannot do this, because EmailConverter is final.

Another thing we noticed, is that in some places of the API, we can provide our own session. However this is not very consistent, and the API we are using does not allow handing over our own session (and internally calls createDummySession() nested after some delegations). Specifically we are using emlToEmail(@NotNull InputStream emlInputStream, @Nullable Pkcs12Config pkcs12Config) and outlookMsgToEmail(@NotNull InputStream msgInputStream, @Nullable Pkcs12Config pkcs12Config).

As a fix Would it be possible to make EmailConverter not final so we can override the dummy session? Alternatively, could createDummySession() return a fresh session with Session.getInstance(new Properties()), since (to our understanding) it's only used as a dummy? Thanks.

bbottema commented 2 months ago

Hi @johannesg-px, I couldn't agree more. The missing Session parameters have been a pending issue for a long time, mostly because I actually think a fluent builder pattern might be in order here, with all the various combinations of method parameters. However, I think the list is pretty much exhaustive now, so I suppose it's manageable.

I also switch to Session.getInstance and I've fixed a bug where one of the emlToEmail(...) methods actually incorrectly delegated to outlookMsgToEmail(...).

bbottema commented 2 months ago

v8.10.0 was published to Maven Central. Enjoy!

johannesg-px commented 2 months ago

That was quick, thanks a lot for the fix! Much appreciated!