grails / grails-mail

The Grails Mail Plugin
https://grails.github.io/grails-mail/
Apache License 2.0
14 stars 25 forks source link

Refactoring config access for Grails 5 #48

Closed davidkron closed 1 year ago

davidkron commented 2 years ago

As the access of the Grails configuration through dot notation is deprecated (warning Accessing config through dot notation is deprecated), I tried refactoring the configuration access.

puneetbehl commented 2 years ago

I think we should do something as following:

@Configuration
@ConfigurationProperties(prefix = "grails.mail")
public class MailPluginConfiguration {

    private String hostName;
    private int port;
    private String from;

}
davidkron commented 2 years ago

@puneetbehl I can try to implement it with @ConfigurationProperties. Is it preferred to use the Micronaut or the Spring annotation?

Another thing that took my attention is the onConfigChange implementation in the plugin: https://github.com/grails3-plugins/mail/blob/7b5499a9df45c531bb938b8c48afdb7df78195c4/src/main/groovy/grails/plugins/mail/MailGrailsPlugin.groovy#L95-L115

Would this even work with the new reloading mechanism since Grails 4 using @ConfigurationProperties? Maybe this should be removed?

puneetbehl commented 2 years ago

Using the Micronaut @ConfigurationProperties would allow you to use it in Micronaut ApplicationContext. It basically depends on how you would want it.

I am not so sure about onConfigChange. I believe it would not work the same after change but we would still need to re-configure MailSender. I would suggest to identify how it affects the application.

davidkron commented 1 year ago

I finally had some spare time to look at this again and did some rewrites to use @ConfigurationProperties. I did decide to go with the Spring variant for maximum compatibility to not break existing applications. For example the Micronaut context can't access the Spring context, so if an existing application does override beans from the mail-plugin in resources.groovy (in some of our applications we actually do this), Micronaut would not find them.

About the onConfigChange: It seems the config change currently is only working for the application.groovy file (see https://github.com/grails/grails-core/blob/v5.2.2/grails-core/src/main/groovy/org/grails/plugins/AbstractGrailsPluginManager.java#L78 and https://github.com/grails/grails-core/blob/v5.2.2/grails-core/src/main/groovy/org/grails/plugins/AbstractGrailsPluginManager.java#L505). So it wouldn't work for application.properties, application.yml or even application-development.groovy. And when I do change the application.groovy file, a reload via spring-devtools is triggered and therefore a new context with the new configuration applied is generated anyway. So maybe it might be even worth deprecating the onConfigChange in grails-core?

darxriggs commented 1 year ago

Is there already a schedule when to publish a new release including this change?

puneetbehl commented 1 year ago

Let me try to spend some time this week to see if I can push out a release for Mail plugin.