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

[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 opened this issue Apr 30, 2024 · 3 comments

Comments

@johannesg-px
Copy link

johannesg-px commented Apr 30, 2024

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 added a commit that referenced this issue Apr 30, 2024
…sion.getInstance` instead of `Session.getDefaultInstance` to avoid tainting global state. Also fixed a bug `emlToEmailBuilder` used `emlToMimeMessage`
@bbottema bbottema changed the title Creating a new Session in EmailConverter.java [enhancement+bug] Make EmailConverter API more consistent regarding Session parameter, don't use Session.getDefaultInstance anymore and fix bug where emlToEmailBuilder used emlToMimeMessage Apr 30, 2024
@bbottema bbottema added this to the 8.10.0 milestone Apr 30, 2024
@bbottema
Copy link
Owner

bbottema commented Apr 30, 2024

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
Copy link
Owner

v8.10.0 was published to Maven Central. Enjoy!

@johannesg-px
Copy link
Author

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

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

No branches or pull requests

2 participants