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

Feature: be able to set defaults and overrides on the Mailer level, rather than email or global level #383

Closed
jllanes3 opened this issue Feb 15, 2022 · 9 comments

Comments

@jllanes3
Copy link

Currently, we have to set the the FROM (i.e. the sender address) at the request level. This is problematic in clustered mode when multiple instances will retry the same request since they all end up using the same FROM address.

        final EmailPopulatingBuilder emailBuilder = EmailBuilder.startingBlank()
                .from(emailRequest.getFromAddress()) // <-- we have to set it here today
                .to(emailRequest.getToAddress())
                .withSubject(emailData.getSubject())
                .fixingMessageId(emailMessageId);

We want to be able to set the FROM address to be the same for each mailer instance if they call different endpoints since we want the FROM address for each of those endpoints to be different.

MailerBuilder
.withFromAddress("mailer.level.from.address")

If there is a workaround that will help as well.

Thanks,

@bbottema
Copy link
Owner

bbottema commented Feb 15, 2022

This is problematic in clustered mode when multiple instances will retry the same request since they all end up using the same FROM address.

I don't understand what you are saying here. Retrying why, because of an exception or something? And why shouldn't the FROM stay the same? That is an email level property, I don't see why that would be tied to a specific mailer which is nothing more than the transport facility.

Can you please elaborate on the use case here?

@jllanes3
Copy link
Author

The use case is that all mail outgoing to a particular smtp endpoint MUST use the same FROM address, and the same applies to every endpoint. Basically, fore every smtp endpoint there will be a FROM address that must be used to send through it for a variety of reasons. This is more of a business requirement and not a protocol one, of course.

Since in clustered mode, the same request which contains the FROM set at that level goes to any of the instances (which the caller does not have access to, that is the point), then this constraint is violated. Hence why I ask if the FROM can be set at a level that applies for all outgoing email to a particular endpoint (i.e. Mailer), instead of at the message level, as it is today.

@morki
Copy link

morki commented Jan 9, 2023

This would be awesome for us too. Every application with user configurable SMTP settings contains FROM in those settings, because there are limitations for FROM (like - the same as session user, from the same domain, etc.)

@bbottema
Copy link
Owner

bbottema commented Jan 9, 2023

The problem of course, is that the email, including FROM address, is produced before handing it over to a sending mechanism, be it direct Transport or the Batch Module. To enable this kind of override/default based on the SMTP Transport currently in use, I would have to defer the creation of mime messages to an encapsulated wrapper that can be invoked at a later point in time.

Furthermore, this changes the performance signature of the connection pool, which is now based on connection sharing and maximizing server utilization. I'm not sure how including producing mime message would impact this. For example do we need to change default pool size? Hot / cold threads? Increase claim timeouts?

A more problematic complicating factor is the fact that setting a default FROM on a Mailer is not enough, since multiple mailers clustered together means emails sent through mailer A, doesn't guarantee that the email is sent through the server configured through A; since it is a cluster, the load is balanced over all SMTP servers configured by all mailers participating in the pool (and the pooled generated Transport connections don't have a back-reference to the mailer instance that created them). So how do you define defaults/overrides on Mailer level, when the actual server being used is not tied to a specific Mailer in runtime.

I have to think about it :P - the idea of having email defaults (any default, not just FROM) on Mailer level does seem interesting, but requires a bit of an overhaul.

@bbottema bbottema changed the title Add support to set the FROM at the mailer level Feature: be able to set defaults and overrides on the Mailer level, rather than email or global level Jan 16, 2023
bbottema added a commit that referenced this issue Jan 16, 2023
commit 623a175
Author: bbottema <benny@bennybottema.com>
Date:   Mon Jan 16 23:22:35 2023 +0100

    When not using the internal Transport system (Batch Module or plain), still convert to MimeMessage for logging purposes or to hand it over to a custom mailer if configured.

commit 648d908
Author: bbottema <benny@bennybottema.com>
Date:   Mon Jan 16 22:46:04 2023 +0100

    Updated smtp-connection-pool to released version which exposes Transport's parent Session

commit dce2a26
Author: bbottema <benny@bennybottema.com>
Date:   Sat Jan 14 00:19:26 2023 +0100

    clean up imports

commit f4c133a
Author: bbottema <benny@bennybottema.com>
Date:   Fri Jan 13 23:57:28 2023 +0100

    Added email defaults and overrides on Mailer level (rather than global level through properties). This way you can have defaults/overrides per SMTP server. For this a lot had to change: 1. Session related to pooled Transport had to be exposed and 2. conversion to MimeMessage had to be encapsulated and deferred to a later time. All this so we can find our wat back to the parent Mailer instance and produce and send en MimeMessage using properties from the defaults/overrides
@bbottema bbottema added this to the 7.7.0 milestone Jan 16, 2023
@bbottema
Copy link
Owner

Released in 7.7.0! Check usage documentation over at simplejavamail.org/configuration.

image

@morki
Copy link

morki commented Jan 17, 2023

Thank you very much @bbottema, you are a hero! :)

@rodonal
Copy link

rodonal commented Jan 18, 2023

@bbottema I presume that the change from 7.6 to 7.7 should not introduce breaking changes. But the changes you have done to the CustomMailer class creates problems in reusing the TransportRunner class in conjuction with a Custom Mailer implementation. This happens because of the conversion between Email and MimeMessage parameters. To be more concrete one cannot override the sendMessage method of the CustomMailer and in this override use the sendMessage method of TransportRunner class. In our codebase we have such an implementation and the yesterday's release caused compilation problems. As a roundabout way of solving the problem we had to implement a 'CustomTransportRunner' (inheriting from the TransportRunner class you provide us) to get rid of the issue of the parameters missing. My question would be whether this issue was foreseen or the change did not take under consideration such issue ?
Thanks,
Redion

@bbottema
Copy link
Owner

bbottema commented Jan 18, 2023

I thought I had reverted any change here, obviously I didn't do it properly. I will release a patch version ASAP that reinstates the missing parameter. Thank you for bringing it to my intention. I will release it today probably, but in the meantime, you can stay on 7.6.x.

Reminder: on principle, since this was a SEMVER minor release and not a patch release, some breaking changes are to be expected (in this case this was not intentional of course and I will correct it asap).

@bbottema
Copy link
Owner

bbottema commented Jan 18, 2023

Fix released in 7.7.1. Might take a few minutes before Maven Central has finished synchronizing.

And good to know the Custom Mailer feature is in use (I would be interested to know your use case).

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

4 participants