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

Bug: batch-module gives error when there is a custom mailer #375

Closed
andrus opened this issue Jan 13, 2022 · 8 comments
Closed

Bug: batch-module gives error when there is a custom mailer #375

andrus opened this issue Jan 13, 2022 · 8 comments

Comments

@andrus
Copy link

andrus commented Jan 13, 2022

I am using SJM 6.x via Bootique integration. One feature that I need myself and is being requested by other Bootique users is the ability to override recipients of a message for testing purposes. Something like this:

transport.sendMessage(message, testRecipients);

This is ideal for QA, as it preserves the real To, Cc, Bcc, but delivers to a designated test mailbox. I tried implementing it as a CustomMailer in Bootique. Unfortunately it is not easy to do it, as the presence of CustomMailer prevents SJM stack from configuring itself properly. So a call like this results in an NPE:

@Override
public void sendMessage(
    OperationalConfig operationalConfig, 
    Session session, 
    Email email, 
    MimeMessage message) {
    try {
        TransportRunner.sendMessage(operationalConfig.getClusterKey(), session, message, overriddenRecipients);
    } catch (MessagingException e) {
        ...
    }
}
java.lang.NullPointerException: Connection pool used before it was initialized. This shouldn't be possible.
at java.base/java.util.Objects.requireNonNull(Objects.java:246)
at org.simplejavamail.internal.batchsupport.BatchSupport.acquireTransport(BatchSupport.java:110)
at org.simplejavamail.mailer.internal.util.TransportRunner.sendUsingConnectionPool(TransportRunner.java:84)
at org.simplejavamail.mailer.internal.util.TransportRunner.runOnSessionTransport(TransportRunner.java:72)
at org.simplejavamail.mailer.internal.util.TransportRunner.sendMessage(TransportRunner.java:48)

Wanted to ask for an expert opinion on a better way to do a custom dispatch via TransportRunner. Or maybe there's interest to make recipient override feature (which has been quite handy with a different mail stack) a part of SJM?

@bbottema
Copy link
Owner

bbottema commented Jan 13, 2022

So I understand what you're trying to do, but that's not how the CustomMailer was designed. The point was to be able to replace the sending engine with your own, not tweak mails coming through and then still use the initial (internal) sending engine (using TransportRunner).

So what's happening here is that you have both a CustomMailer, but then also the batch-module on the classpath. Currently, Simple Java Mail skips the cluster when a custom mailer is present. Your method would work find if you removed the batch-module. The reasoning here is that the batch module prevents the JVM from shutting down (because you need to stop the clusters/connection pool manually) and that doesn't make sense if you provide your own custom mailer implementation.

One thing I am considering is that if both CustomMailer and batch-module are available, Simple Java Mail will throw a more descriptive error rather than some vague statement about a connection pool.

I'm not sure otherwise how to elegantly define an API on the builder that flags the system to still use batch-module even though there is a custom mailer.

@andrus
Copy link
Author

andrus commented Jan 14, 2022

Thanks for the reply. It confirms what I inferred about CustomMailer's intent.

The reasoning here is that the batch module prevents the JVM from shutting down (because you need to stop the clusters/connection pool manually) and that doesn't make sense if you provide your own custom mailer implementation.

If a user has a CustomMailer that does not rely on the batch-module, they can simply not include batch-module on the classpath. So if batch-module is on classpath, we can assume this was a conscious decision, and hence it needs to be initialized whether there is a CustomMailer or not.

Does this logic make sense?

@bbottema
Copy link
Owner

bbottema commented Jan 14, 2022

It does and I will give this some more thought...

@andrus
Copy link
Author

andrus commented Jan 21, 2022

@bbottema . So what do you think? I may even try and create a PR for this task if that's the direction you'd like to proceed.

bbottema added a commit that referenced this issue Jan 22, 2022
…iler (necessity implied by inclusion of batch-module)
@bbottema bbottema changed the title Recipient override for testing Bug: batch-module gives error when there is a custom mailer Jan 22, 2022
@bbottema bbottema added this to the 7.0.1 milestone Jan 22, 2022
@bbottema
Copy link
Owner

I've released a fix for this in 7.0.1. Enjoy!

@andrus
Copy link
Author

andrus commented Jan 22, 2022

This was fast :) Thanks!

What do you think about backporting it to 6.x? In Bootique I would like to support both javax and jakarta APIs for some time

bbottema added a commit that referenced this issue Jan 22, 2022
…iler (necessity implied by inclusion of batch-module)
@bbottema
Copy link
Owner

Done. It's in 6.7.6

@andrus
Copy link
Author

andrus commented Jan 22, 2022

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