The mail sending API is structured around the two interfaces MailSender and JavaMailSender, together with the class JavaMailSenderImpl which implements the two aforementioned interfaces. The numerous overloads of send(..)-methods in the interfaces makes it a bit cumbersome and errorprone to extend these interfaces directly.
Default methods introduced with Java 8 can to a large degree resolve this issue by moving from JavaMailSenderImpl to default methods in the interfaces. For methods which is "piping" to another method of the public API, this is a trivial refactoring (a820753). But JavaMailSenderImpl.send(SimpleMailMessage...) can not be refactored in such a trivial manner, as this method delegates to the internal doSend-method of JavaMailSenderImpl. To rectify, this pull-request also contains a suggested change to allow passing SimpleMailMessages contained in SmartMimeMessages further down the method stack, and allow the doSend to resolve the original message instances in case of errors, so existing behaviour is preserved. (f95659b168d479db3711d64ff02f533f437d0418)
The interfaces are now simpler to extend directly, and this also allows for more clean composable decorators, as demonstrated with the new JavaMailSenderDecorator class in 39724cdb0150b3894b23276bb1f287bdccc35af8. It "seals" the default methods as final, allowing decorating an arbitrary JavaMailSender with only overriding one method, which is part of the public API. It is simple to wrap several layers of JavaMailSenderDecorators, if one should need to, and eventually end in a JavaMailSenderImpl.
I have used this structure for a client to separately implement a whitelisting JavaMailSenderDecorator, and a performance monitoring "aspect" JavaMailSenderDecorator, which are composed together, and ultimately wrapping an instance of JavaMailSenderImpl.
This also may yield some advantages when testing, as one would only have to intercept the one send(..)-method which is not provided by the interfaces, and tests does not need to know which overload is used by the main code. JavaMailSenderImpl instances can be separated strictly away from testing. For instance, if using mock libraries as Mockito, mocking a JavaMailSenderDecorator, it will ensure that all real final methods are invoked, and one may verify invocations of only one method regardless of which overloaded method is invoked by the main code.
I hope this can be a relevant contribution, and I am very open for any feedback and discussion on this design!
Thank you :)
Comment From: runeflobakk
The pull-request was inspired by a talk I saw with @jhoeller on last year's JavaZone where he encouraged contributions which took advantage of the new language features of Java 8.
Is it something I can do to enable some progress on this? Thank you for any feedback!
Comment From: snicoll
@runeflobakk thanks for the PR and sorry it took so long to review. I stopped at the bearer thing as it looks quite unrelated to me. If you want to pursue this, we'd need a bit more detail (feel free to create a separate issue).
Comment From: runeflobakk
Thanks for accepting the contribution, @snicoll! I understand that JavaMailSender may not have been a top priority, perhaps especially with the recent large effort to migrate to the jakarta namespace 😅
IMHO, the "bearer thing" is not actually unrelated, but a facility to preserve the information conveyed in the exception handling inside the JavaMailSenderImpl.doSend(MimeMessage[] mimeMessages, Object[] originalMessages) method.
JavaMailSenderImpl.send(SimpleMailMessage ... message), which is now pulled up as a default method to JavaMailSender, does no longer invoke doSend(MimeMessage[] mimeMessages, Object[] originalMessages) (because the method is not available). It instead calls send(MimeMessage... mimeMessages), which implementation in JavaMailSenderImpl invokes doSend(mimeMessages, null). So the original SimpleMailMessages are lost when they are translated to MimeMessage. The existing exception handling in JavaMailSenderImpl.doSend(..) includes the original message objects if they are available, and the change which keeps a reference to the original message object in the SmartMimeMessage is to preserve the semantics that you will get your actual passed argument back in an exception. Now the originalMessages parameter of doSend will always be null, and this is will be a subtle change in behavior from the old implementation. If you call any of the methods with accepts SimpleMailMessage, and a MailSendException is thrown in doSend(..), failedMessages will contain MimeMessages instead of the SimpleMailMessages which was originally passed.
This is maybe not a big issue in practise, and I am fine with leaving it as this. But I included the change to ensure that the existing behavior that you get the same message objects in an exception that you passed to .send(..). I can raise it as a separate issue if you think this behavior should be preserved.
It may be less of a surprise to get the actual message objects you passed in a caught exception, and if you rely on them being the same for correlating with what you tried to send, then this will break if not preserving this behavior.
The decorator is a value-add, and not strictly necessary, though I think it demonstrates some of the value of this restructuring/refactoring. You can implement this yourself as a user of Spring, though it would be nice to have such a facility provided by Spring. But again, may as well be a separate issue/PR.
Comment From: snicoll
@runeflobakk It is unrelated in the sense that it does not need to be done for your original proposal to be complete (including with the decorator). As I've said before, we're happy to review this is if you create a separate issue with a small sample that demonstrates the problem. We won't implement it the way you've done it though.
Comment From: runeflobakk
Absolutely. My intention was only to point out that https://github.com/spring-projects/spring-framework/commit/c3d1a886def14348aaf8eb607e004ad1813dbf10, as it is now, introduces a minor behavioral change, and I wanted to make sure this was understood and intentional on your part.
Comment From: snicoll
JavaMailSenderImpl.send(SimpleMailMessage ... message) is part of MailSender and has not be pulled there. Can you clarify what you mean?
Comment From: runeflobakk
Apologize, I missed the detail that send(SimpleMailMessage... simpleMessages) is still present in JavaMailSenderImpl, and not pulled up as a default method in JavaMailSender.
So you still need to implement two methods if extending JavaMailSender, which the original intent of this PR was to avoid. (to allow for more ergonomics when decorating and /or mocking).
Totally fine with this, of course.