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

Use @EnableConfigurationProperties to define the MessageSourceProperties bean #42181

Closed
mmoayyed opened this issue Sep 8, 2024 · 15 comments
Closed
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@mmoayyed
Copy link
Contributor

mmoayyed commented Sep 8, 2024

Spring Boot 3.4 M2.

It appears that when one defines their own messageSource bean, it's not then possible to inject MessageSourceProperties into the overriding bean for an alternative implementation to reuse certain bits of the configuration schema. That is, if I have my own:

@Bean
public MessageSource messageSource(MessageSourceProperties properties) {
    // my own implementation...
}

This will fail with:

[org.springframework.boot.diagnostics.LoggingFailureAnalysisReporter] - <Application failed to start due to an exception>
org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 
'org.springframework.boot.autoconfigure.context.MessageSourceProperties' available: expected at 
least 1 bean which qualifies as autowire candidate. Dependency annotations: {}

I propose that alternative implementations of this bean should still be able to use MessageSourceProperties.

This is of course because the MessageSourceAutoConfiguration defines the following:

@ConditionalOnMissingBean(
    name = {"messageSource"},
    search = SearchStrategy.CURRENT
)

...and in the same class, MessageSourceProperties is declared as a bean:

    @Bean
    @ConfigurationProperties(
        prefix = "spring.messages"
    )
    public MessageSourceProperties messageSourceProperties() {
        return new MessageSourceProperties();
    }

So when a competing bean is found, this prevents MessageSourceProperties to be created.

I find this somewhat unusual that MessageSourceProperties is created explicitly as a Bean, and the class itself unlike many others is not tagged with @ConfigurationProperties(prefix ="spring.messages"). I suspect that if this were the case, the problem would disappear. The binding would be recognized by the runtime and yet the bean implementation will be skipped in favor of the alternative but there may be a deeper rationale behind the change here.

Thank you for your time!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 8, 2024
@philwebb
Copy link
Member

philwebb commented Sep 9, 2024

I'm not sure why MessageSourceProperties is declared as a @Bean method, that was done a long time ago as part of #9666. I'm pretty sure we can change it. Flagging for team attention in case anyone remembers more about the history of those properties.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Sep 9, 2024
@snicoll
Copy link
Member

snicoll commented Sep 9, 2024

I am not 100% sure but this might be an oversight of #13824 where we could have moved this to @EnableConfigurationProperties.

We have a lot of auto-configurations that rely on the absence of a bean, and then only create the configuration. So if we changed this, the user code should still have @EnableConfigurationProperties(MessageSourceProperties.class).

@mmoayyed in the meantime, have you tried to create the bean yourself? Does it work?

@Bean
@ConfigurationProperties(prefix = "spring.messages")
public MessageSourceProperties messageSourceProperties() {
	return new MessageSourceProperties();
}

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Sep 9, 2024
@mmoayyed
Copy link
Contributor Author

mmoayyed commented Sep 9, 2024

Unfortunately, no. Defining a bean directly as you note produces the same problem.

Other factors that might be of interest here:

  1. This app is using JDK dynamic proxies. No CGLib.
  2. This app is using Spring Cloud and heavily relies on RefreshScope annotations. (None of such beans are annotated with that though)
  3. The issue appears to only manifest itself when spring.messages properties are defined (which likely makes sense since that would kick the binding ops into effect).

@wilkinsona
Copy link
Member

Thanks for the additional info. Unfortunately, it's not clear why that wouldn't work. If you have defined your own MessageSourceProperties bean then such a bean should be available for injection.

If you would like us to spend some more time investigating, please spend some time providing a complete yet minimal sample that reproduces the problem. You can share it with us by pushing it to a separate repository on GitHub or by zipping it up and attaching it to this issue.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 9, 2024
@mmoayyed
Copy link
Contributor Author

mmoayyed commented Sep 9, 2024

My apologies, I misspoke. My previous test ran a stale copy of the application without that bean defined. To be accurate, yes defining a MessageSourceProperties bean myself DOES indeed remove the issue. Having said that, I am happy to work on a reproducer if you need to see the original problem in action.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 9, 2024
@wilkinsona
Copy link
Member

Great, thanks. No need for a reproducer as I think we're all on the same page now. I think we should be able to change things such that MessageSourceProperties is annotated with @ConfigurationProperties("spring.messages") and enabled through @EnableConfigurationProperties(MessageSourceProperties.class) on MessageSourceAutoConfiguration.

Note that, as @snicoll mentions above, once this change is in place, if you've defined your own messageSource bean and want to inject MessageSourceProperties, you will have to use @EnableConfigruationProperties(MessageSourceProperties.class). This will be necessary because the presence of the messageSource bean will cause MessageSourceAutoConfiguration to back off so it won't be enabling the properties any more.

@wilkinsona wilkinsona changed the title MessageSourceAutoConfiguration should not create a MessageSourceProperties bean Use @EnableConfigurationProperties to define the MessageSourceProperties bean Sep 9, 2024
@wilkinsona wilkinsona added type: task A general task and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Sep 9, 2024
@wilkinsona wilkinsona added this to the 3.4.x milestone Sep 9, 2024
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed type: task A general task labels Sep 9, 2024
@wilkinsona wilkinsona self-assigned this Sep 9, 2024
@wilkinsona wilkinsona modified the milestones: 3.4.x, 3.4.0-M3 Sep 9, 2024
@mmoayyed
Copy link
Contributor Author

mmoayyed commented Sep 9, 2024

Understood, thank you very much everyone for the fix and the notes.

@quaff
Copy link
Contributor

quaff commented Sep 10, 2024

Does MessageSourceProperties backoff if custom messageSource bean present due to the @ConditionalOnMissingBean on class level ?

@wilkinsona
Copy link
Member

Yes.

@quaff
Copy link
Contributor

quaff commented Sep 10, 2024

Yes.

It's not consistent with other AutoConfiguration's which will not backoff ConfigurationProperties, @EnableConfigurationProperties(MessageSourceProperties.class) is required by application if they want build its own MessageSource.

@bclozel
Copy link
Member

bclozel commented Sep 10, 2024

@quaff Stéphane already shared what developers should to to get properties even if the auto-configuration backs off. Can you explain your use case and how you're defining your custom MessageSource?

@wilkinsona
Copy link
Member

Also, why don't you think it's consistent? Any class annotated with @EnableConfigurationProperties and one or more conditions will not enable the configuration properties if one of the conditions does not match.

@quaff
Copy link
Contributor

quaff commented Sep 10, 2024

My point is @ConditionalOnMissingBean should be annotated on method messageSource() not MessageSourceAutoConfiguration like other AutoConfigurations, then application could customize like this:

@Configuration
@EnableConfigurationProperties(MessageSourceProperties.class) // it should not be required
public class MyMessageSourceConfiguration {
	@Bean
	public MessageSource messageSource(MessageSourceProperties properties) {
		...
	}
}

@bclozel
Copy link
Member

bclozel commented Sep 10, 2024

If we do what you're suggesting in our codebase, then Spring Boot apps would get dozens of *Properties beans even if they're not used at all as the application already provided its opinion. This is consistently applied in our codebase as soon as the presence of a bean implies that developers are taking full control over the configuration. This is the case for the MVC auto-configuration, for example.

@quaff
Copy link
Contributor

quaff commented Sep 10, 2024

I'm not against that, just point it out that developers may be confused, since most of *Properties doesn't backoff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants