Configuration should be changeable in Code

[schmitch]

Hello, currently the configuration of the Mailer is only changeable inside the application.conf,
however that is aweful since sometimes you wan't to make it User Configurable.

[ggrossetie]

Hi @schmitch, currently the configuration is extract from the Play configuration object but I think we need to rework that part. See #40 (comment)

I would extract the configuration into its own class, independent of CommonsMailer, then you can test that. This has the advantage of being able to programatically configure CommonsMailer, instead of requiring a configuration object.
// jroper

Feel free to open a pull request.

[schmitch]

@Mogztter yeah it's true that this needs to be reworked, however due to the fact that CommonsMailer is a injected class there is a way to work around that.

Also it sucks that send give's back a string and not a Future[String] if the config get's pulled from slick3 we would need to block the database call, if this would be a Future it would be better, as a way to deprecate it would be to have a blocking send and a future send.

[ggrossetie]

Also it sucks that send give's back a string and not a Future[String] if the config get's pulled from slick3 we would need to block the database call, if this would be a Future it would be better, as a way to deprecate it would be to have a blocking send and a future send.

Yes, there's an open issue about that: #13

[genachka]

+1

[schmitch]

+1
Since it will work even when introducing something like sendAsync. As specified in #13

[pedrolopix]

+1

[ggrossetie]

I've created a pull request but I want feedback/review before merging. Can you take a look and tell me what you think ?

[genachka]

@Mogztter
Any chance this could be back ported to the 2.3.x support?

[ggrossetie]

Why not... you are not ready to take the big step of Play 2.4 ? ;)
Le 8 juil. 2015 7:24 PM, "genachka" [email protected] a écrit :

@Mogztter https://github.com/Mogztter
Any chance this could be back ported to the 2.3.x support?

—
Reply to this email directly or view it on GitHub
#51 (comment)
.

[schmitch]

@Mogztter could you make a new version after you've merged this (especially for sbt/mvn)?

[jroper]

I can cut a new version, however, I think binary compatibility may have been broken in #54. @Mogztter would you be able to add mima to the build?

[ggrossetie]

I'm on vacation but I will see what I can do

[ggrossetie]

@jroper Here is the result (comparison with version 3.0.1):

> mima-report-binary-issues
[info] Resolving javax.activation#activation;1.1.1 ...
[info] play-mailer: found 4 potential binary incompatibilities
[error]  * abstract method configure(play.Configuration)play.libs.mailer.MailerClient in interface play.libs.mailer.MailerClient does not have a correspondent in old version
[error]    filter with: ProblemFilters.exclude[MissingMethodProblem]("play.libs.mailer.MailerClient.configure")
[error]  * method configure(play.Configuration)play.libs.mailer.MailerClient in trait play.api.libs.mailer.MailerClient does not have a correspondent in old version
[error]    filter with: ProblemFilters.exclude[MissingMethodProblem]("play.api.libs.mailer.MailerClient.configure")
[error]  * method configure(play.api.Configuration)play.api.libs.mailer.MailerClient in trait play.api.libs.mailer.MailerClient does not have a correspondent in old version
[error]    filter with: ProblemFilters.exclude[MissingMethodProblem]("play.api.libs.mailer.MailerClient.configure")
[error]  * method this(java.lang.String,Int,Boolean,Boolean,scala.Option,scala.Option,Boolean,scala.Option,scala.Option)Unit in class play.api.libs.mailer.SMTPMailer does not have a correspondent in new version
[error]    filter with: ProblemFilters.exclude[MissingMethodProblem]("play.api.libs.mailer.SMTPMailer.this")
[trace] Stack trace suppressed: run last *:mimaReportBinaryIssues for the full output.
[error] (*:mimaReportBinaryIssues) play-mailer: Binary compatibility check failed!
[error] Total time: 0 s, completed 16 juil. 2015 01:16:42

[jroper]

So, generally, that means we should cut a new major version.

But, maybe a different approach to configuration would be a better idea - at the moment, the client is mutable, when it gets reconfigured, it gets reconfigured globally. I think a better way to do this would be to simply remove the configure method, and let users create their own instance of CommonsMailer when they need it. Binary compatibility can be maintained by overriding the constructor to accept Configuration, as well as the new SMTPConfiguration case class for the primary constructor.

[schmitch]

@jroper do you think that is needed at the first place, i mean most people would want to store their mailer configuration inside a database, not having many mailers. A lot of frameworks have this kind of usage.
I know It's bad If you start to have two Mailer since that could lead to serious issues. Especially when you have some Background and Frontend work which will use a Mailer with different Configuration.

[jroper]

But at the moment there's a race condition, what if the app sends an email before you load it from the database? There's nothing stopping it from doing that. If you want to load the configuration from a database, you can do that easily, simply implement a new JSR330 provider for SMTPConfiguration that, instead of looking it up out of the Play configuration, looks it up out of the database.

[ggrossetie]

I agree on the principle but at the moment CommonsMailer class is almost an internal class.
Plus we now provide mailer instances with dependency injection, so I think users should not create their own instances.

simply implement a new JSR330 provider for SMTPConfiguration

Why not but SMTPConfiguration is related to one provider. If we implement more providers, we will have to create one factory for each provider configuration. I don't know if this is a good or bad thing ?

[ggrossetie]

Binary compatibility can be maintained by overriding the constructor to accept Configuration, as well as the new SMTPConfiguration case class for the primary constructor.

So if SMTPConfiguration is provided we use this configuration else we fall back on the Play (default) configuration ?

[ggrossetie]

@jroper With this approach how a user can reconfigure the injected MailerClient at runtime (or inject another MailerClient with a different configuration) ?
If someone declare the following code:

  @Provides
  def provideSMTPConfiguration() = {
    new SMTPConfiguration("typesafe.org", 1234)
  }
}

Then all the injected MailerClient instances will have this configuration... or maybe I'm missing something ?

[ggrossetie]

Binary compatibility can be maintained by overriding the constructor to accept Configuration, as well as the new SMTPConfiguration case class for the primary constructor.

That not exactly what you described but I think that the following code maintains binary compatibility:

class CommonsMailer @Inject() (configuration: Configuration) extends MailerClient {

  def this(smtpConfiguration: SMTPConfiguration) = {
    this(Configuration.from(smtpConfiguration) /* the method doesn't exist, but you get the idea */)
  }

  // ...
}

That allow users to programmatically configure the mailer client by instantiating CommonsMailer:

CommonsMailer(SMTPConfiguration("typesafe.org", 1234))

Maybe a better (but breaking compatibility) solution would be to:

• rename CommonsMailer to SMTPMailer (not absolutely necessary but makes things more consistent)
• have a default constructor annotated with @Inject that only accept SMTPConfiguration
• provide a default instance of SMTPConfiguration based on the Configuration object
• allow user to override SMTPConfiguration

I don't know how to do the last part but that way we don't force users to do manual instantiation because they can implement a JSR330 provider for SMTPConfiguration and use @Inject (unless they want to have multiple instances with different configurations).

@jroper Wdyt ?

[schmitch]

Any Updates on this?

[ggrossetie]

I've opened a new pull request to fix the global reconfiguration (mutable state) but I do need feedback because I can't decide if this is a better approach or not.

I would love to hear from a Scala / Guice expert 😄

[schmitch]

@jroper will there soon be a new version since #63 is merged now?

[jroper]

@Mogztter, what version should be released? Should there be any new forks for the repo to maintain old versions?

[ggrossetie]

@jroper I think we should create the branch 2.4.x (or maybe 3.x since we now have 2 versions compatible with Play 2.4.x) from the 3.0.1 tag and release a 4.0.0-M1 from master.

Plugin version	Play version
2.x (latest 2.4.1)	2.3.x
3.x (latest 3.0.1)	2.4.x
4.x (latest 4.0.0-M1)	2.4.x
next branch	latest snapshot (2.5.x)

[schmitch]

Wouldn't it be better that the Plugin version matches the Play Version? so creating a play2-mailer - 2.4.2 would be best?

[ggrossetie]

Wouldn't it be better that the Plugin version matches the Play Version?

Yes and no 😄
It may look simple at first but it's not. I think it's better to follow semantic versioning because play-mailer has its own release cycle.
Also other popular plugins are doing the same (see https://github.com/playframework/play-slick)

[jroper]

Matching the version numbers would essentially force play-mailer to wait for a new Play major release before it can release any breaking changes.

[jroper]

@Mogztter sounds good to me. If you can setup the branches, I'll cut the release once you're done.

[ggrossetie]

I've created branch 3.x and renamed branch 2.3.x to 2.x for consistency.

[ggrossetie]

@jroper do you need anything else to cut the release ?

[jroper]

4.0.0-M1 is released, pending maven central sync to CDN nodes.

[ggrossetie]

Thanks @jroper, I think we can now close this issue 🎉