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: add native support for overriding envelope-level receiver(s) #483

Closed
andrus opened this issue Dec 9, 2023 · 20 comments
Closed

Comments

@andrus
Copy link

andrus commented Dec 9, 2023

I am trying to upgrade Bootique integration of SJM from 7.4.0 to 8.x, but the custom mailer can no longer obtain the preconfigured SJM Mailer. Somewhere along the way MailerRegularBuilderImpl.buildServerConfig() started returning null when there is a custom mailer present. So that code that worked previously stopped working.

An earlier issue #375 expalins the motivation for this particular custom mailer. Its goal is not to have a fully custom transport, but rather to override recipients at the transport level without changing to/cc/bcc of the original message. So I would very much want to reuse SJM transport. Having said that, I do feel like using a custom mailer for my purpose is too fragile, and I'd rather there was an option in the EmailBuilder to provide "recipient override" address for testing.

@bbottema
Copy link
Owner

bbottema commented Dec 10, 2023

Thanks for reporting @andrus. Let's have a look.

I am trying to upgrade Bootique integration of SJM from 7.4.0 to 8.x, but the custom mailer can no longer obtain the preconfigured SJM Mailer. Somewhere along the way MailerRegularBuilderImpl.buildServerConfig() started returning null when there is a custom mailer present. So that code that worked previously stopped working.

Well, since Simple Java Mail is not the one sending the mail anymore, there is no relevant server config. In fact, that method used to validate the data like mandatory host, which makes no sense if you use your own implementation (aside from that, this method is documented as for internal use).

However, I understand you have a use case here, so let's look at that.

An earlier issue #375 expalins the motivation for this particular custom mailer. Its goal is not to have a fully custom transport, but rather to override recipients at the transport level without changing to/cc/bcc of the original message. So I would very much want to reuse SJM transport. Having said that, I do feel like using a custom mailer for my purpose is too fragile, and I'd rather there was an option in the EmailBuilder to provide "recipient override" address for testing.

So really, your custom implementation wants to send it with Simple Java Mail still, so why not just use a separate Mailer and move the server config there?

@andrus
Copy link
Author

andrus commented Dec 10, 2023

Thanks for the quick reply!

So really, your custom implementation wants to send it with Simple Java Mail still, so why not just use a separate Mailer and move the server config there?

This may actually work! Instead of fighting with CustomMailer and related lifecycle, I might decorate / subclass a Mailer, overriding its sendMail. Let me try it.

@bbottema
Copy link
Owner

bbottema commented Dec 10, 2023

Ok, that's not what I intended :D - I thought you might just configure two mailers separately. One properly configured for everything but sending, but with custom mailer allowing you to do any overrides. And then a dumb sender that you relay the modified Email to (possibly configured as a cluster).

But still, let me know how your experiment turns out :)

@andrus
Copy link
Author

andrus commented Dec 10, 2023

And then a dumb sender that you relay the modified Email to (possibly configured as a cluster).

Unfortunately, Email object does not have a "recipient override" property. With Email, I can wipe out "to/cc/bcc", and replace them with the "override" address, but that does not allow the recipient of the "overridden" message (e.g. a QA team member) to inspect how the original email would have looked like without the override. That's why I need to mess with the transport directly.

@bbottema
Copy link
Owner

Can you share the code you used before this? I'm not sure I follow the semantics of your override use-case.

@andrus
Copy link
Author

andrus commented Dec 10, 2023

Essentially, I am taking advantage of the difference between the SMTP "envelope" destination addresses and the addresses in the message itself. Overriding the former, and preserving the latter allows to create a nice test experience.

@bbottema
Copy link
Owner

Hhm, the current version of TransportRunner doesn't even support recipients separately, so this won't even fly. I'd have to think deeply about allowing some way of interception or overriding behaviour. The Custom Mailer concept wasn't designed or meant for affecting existing internal behaviour; it was meant to replace it with your own.

@bbottema
Copy link
Owner

Thinking out loud, the problem with this is that 'overriding' behaviour can apply to every step of the mailing process. So do I devise some kind of step-filter concept, where each step a filter can be inserted. Or do I approach this from very specific use cases (such as your) and devise a solution for that.

@andrus
Copy link
Author

andrus commented Dec 11, 2023

I have only a superficial familiarity with all the mail RFCs, but it seems like providing a model that describes all possible properties (e.g. explicit properties of the SMTP "envelope" on the Email object) is more beneficial than extra override hooks. Though one approach does not preclude the other.

@bbottema
Copy link
Owner

bbottema commented Dec 11, 2023

I think I have a clear understanding now and I'm looking at some solutions, but I'm still a little confused as to why you need MailerRegularBuilderImpl.buildServerConfig(). If I understand correctly, you provide your own CustomMailer which goes to TransportRunner.sendMessage() with your own recipients. Where does the server config come into play?

@andrus
Copy link
Author

andrus commented Dec 11, 2023

Good question. It didn't fail before. I just pushed a branch with my attempt to upgrade the existing code base to 8.x, and if you run mvn clean verify on it, there will be a failure.

https://github.com/bootique/bootique-simplejavamail/tree/8

@bbottema
Copy link
Owner

bbottema commented Dec 12, 2023

I'm not able to build it with Maven 3.8.6. What version are you on? Actually, just a stacktrace will do for me.

@andrus
Copy link
Author

andrus commented Dec 12, 2023

It builds for me on Mac with either 3.8.x or 3.9.x versions of Maven, Java 11, 17, 21. Master should build successfully, branch 8 should result in a unit test failure.

@bbottema
Copy link
Owner

If you could provide a stack trace, then that's enough I think.

@andrus
Copy link
Author

andrus commented Dec 12, 2023

What happens is that instead of the preconfigured Mailer, in the presence of CustomMailer shown on that branch, SJM implicitly creates a new stack with default settings, and then the exception is caused by the attempt to connect to localhost:25 instead od the right port.

org.simplejavamail.mailer.internal.MailerException: Failed to send email [ID: '<2027227708.1.1702386582319@[192.168.16.2]>'], reason: Unknown error
	at org.simplejavamail.mailer.internal.SendMailClosure.handleException(SendMailClosure.java:85)
	at org.simplejavamail.mailer.internal.SendMailClosure.executeClosure(SendMailClosure.java:76)
	at org.simplejavamail.mailer.internal.AbstractProxyServerSyncingClosure.run(AbstractProxyServerSyncingClosure.java:56)
	at org.simplejavamail.mailer.internal.MailerImpl.sendMail(MailerImpl.java:364)
	at io.bootique.simplejavamail.SimpleJavaMailDeliveryIT.sendSyncNoRetrieve(SimpleJavaMailDeliveryIT.java:241)
	at io.bootique.simplejavamail.SimpleJavaMailDeliveryIT.sendMail_RecipientOverride(SimpleJavaMailDeliveryIT.java:206)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
...
Caused by: org.simplejavamail.smtpconnectionpool.TransportHandlingException: Error when trying to open connection to the server, session:
	{mail.transport.protocol=smtp, mail.smtp.writetimeout=60000, mail.smtp.ssl.trust=*, mail.smtp.starttls.required=false, mail.smtp.starttls.enable=true, mail.smtp.timeout=60000, SESSION_BASED_EMAIL_TO_MIME_MESSAGE_CONVERTER_KEY=SessionBasedEmailToMimeMessageConverter(session=jakarta.mail.Session@67fe380b, operationalConfig=OperationalConfigImpl(async=false, properties={}, sessionTimeout=60000, threadPoolSize=2, threadPoolKeepAliveTime=10, clusterKey=d7252774-04fa-47f9-907b-f46b990e75d9, connectionPoolCoreSize=0, connectionPoolMaxSize=4, connectionPoolClaimTimeoutMillis=2147483647, connectionPoolExpireAfterMillis=5000, connectionPoolLoadBalancingStrategy=ROUND_ROBIN, transportModeLoggingOnly=false, debugLogging=false, disableAllClientValidation=false, sslHostsToTrust=[], trustAllSSLHost=true, verifyingServerIdentity=true, executorService=org.simplejavamail.internal.batchsupport.concurrent.NonJvmBlockingThreadPoolExecutor@4a325eb9[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0], executorServiceIsUserProvided=false, customMailer=io.bootique.simplejavamail.MailerWithOverriddenRecipients@3dedb4a6), emailGovernance=EmailGovernanceImpl(emailValidator=EmailValidator[validationRuleCount=3], emailDefaults=Email{
	id=null
	sentDate=null
	fromRecipient=null,
	replyToRecipients=[],
	bounceToRecipient=null,
	text='null',
	textHTML='null',
	textCalendar='null (method: null)',
	contentTransferEncoding='quoted-printable',
	subject='null',
	recipients=[]
}, emailOverrides=Email{
	id=null
	sentDate=null
	fromRecipient=null,
	replyToRecipients=[],
	bounceToRecipient=null,
	text='null',
	textHTML='null',
	textCalendar='null (method: null)',
	contentTransferEncoding='quoted-printable',
	subject='null',
	recipients=[]
}, maximumEmailSize=null)), simplejavamail.transportstrategy=SMTP, mail.smtp.connectiontimeout=60000}
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.connectTransport(TransportAllocator.java:73)
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.allocate(TransportAllocator.java:45)
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.allocate(TransportAllocator.java:28)
	at org.bbottema.genericobjectpool.GenericObjectPool.claimOrCreateNewObjectIfSpaceLeft(GenericObjectPool.java:164)
	at org.bbottema.genericobjectpool.GenericObjectPool.claimOrCreateOrWaitUntilAvailable(GenericObjectPool.java:149)
	at org.bbottema.genericobjectpool.GenericObjectPool.claim(GenericObjectPool.java:93)
	at org.bbottema.clusteredobjectpool.core.ResourcePool.claim(ResourcePool.java:44)
	at org.bbottema.clusteredobjectpool.core.ResourceClusters.claimResourceFromCluster(ResourceClusters.java:124)
	at org.simplejavamail.internal.batchsupport.BatchSupport.getSessionTransportPoolableObject(BatchSupport.java:128)
	at org.simplejavamail.internal.batchsupport.BatchSupport.acquireTransport(BatchSupport.java:118)
	at io.bootique.simplejavamail.MailerWithOverriddenRecipients.sendUsingConnectionPool(MailerWithOverriddenRecipients.java:102)
	at io.bootique.simplejavamail.MailerWithOverriddenRecipients.sendMessage(MailerWithOverriddenRecipients.java:83)
	at io.bootique.simplejavamail.MailerWithOverriddenRecipients.sendMessage(MailerWithOverriddenRecipients.java:67)
	at org.simplejavamail.mailer.internal.SendMailClosure.executeClosure(SendMailClosure.java:67)
	... 75 more
Caused by: com.sun.mail.util.MailConnectException: Couldn't connect to host, port: localhost, 25; timeout 60000;
  nested exception is:
	java.net.ConnectException: Connection refused
	at com.sun.mail.smtp.SMTPTransport.openServer(SMTPTransport.java:2210)
	at com.sun.mail.smtp.SMTPTransport.protocolConnect(SMTPTransport.java:722)
	at jakarta.mail.Service.connect(Service.java:342)
	at jakarta.mail.Service.connect(Service.java:222)
	at jakarta.mail.Service.connect(Service.java:171)
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.connectTransport(TransportAllocator.java:70)
	... 88 more
Caused by: java.net.ConnectException: Connection refused
	at java.base/sun.nio.ch.Net.pollConnect(Native Method)
	at java.base/sun.nio.ch.Net.pollConnectNow(Net.java:672)
	at java.base/sun.nio.ch.NioSocketImpl.timedFinishConnect(NioSocketImpl.java:547)
	at java.base/sun.nio.ch.NioSocketImpl.connect(NioSocketImpl.java:602)
	at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327)
	at java.base/java.net.Socket.connect(Socket.java:633)
	at com.sun.mail.util.WriteTimeoutSocket.connect(WriteTimeoutSocket.java:91)
	at com.sun.mail.util.SocketFetcher.createSocket(SocketFetcher.java:333)
	at com.sun.mail.util.SocketFetcher.getSocket(SocketFetcher.java:214)
	at com.sun.mail.smtp.SMTPTransport.openServer(SMTPTransport.java:2160)
	... 93 more

@bbottema bbottema changed the title CustomMailer can no longer reuse preconfigured mailer transport Enhancement: add native support for overriding envelope-level receiver(s) Dec 12, 2023
@bbottema
Copy link
Owner

Ok well, I'll leave that for what it is for the moment 😅

I've released 8.4.0, which includes the override receivers feature, see documentation here.

@andrus
Copy link
Author

andrus commented Dec 12, 2023

That did it. I was able to implement a Mailer decorator that does not require transport hacking. Way cleaner than before. Thanks!!

@jkorri
Copy link

jkorri commented Dec 13, 2023

This looks great! And it seems this is not just for testing. Amazon SES strongly suggests you to send messages with one api call per recipient (https://aws.amazon.com//blogs/messaging-and-targeting/how-to-send-messages-to-multiple-recipients-with-amazon-simple-email-service-ses/). Am I correct in assuming this will make it possible to adhere to that guideline and have the e.g. the to-field contain all the recipients?

@bbottema
Copy link
Owner

I think the SMTP protocol makes sure each envelope recipient is handled separately. It may be a single SMTP session, but multiple send actions on the protocol level.

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

3 participants