Skip to content

@ConfigurationPropertiesBinding should be @Role(BeanDefinition.ROLE_INFRASTRUCTURE) #45622

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

Closed
wilkinsona opened this issue May 20, 2025 · 4 comments
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: declined A suggestion or change that we don't feel we should currently apply

Comments

@wilkinsona
Copy link
Member

wilkinsona commented May 20, 2025

Any bean that is @ConfigurationPropertiesBinding will be created during bean post-processing for configuration property binding. As such, it may not be eligible for full post-processing which will result in a warning being logged. The warning will be something like this:

2025-05-20T15:19:49.363+01:00  WARN 26216 --- [gh-45618] [           main] trationDelegate$BeanPostProcessorChecker : Bean 'stringOrNumberMigrationVersionConverter' of type [org.springframework.boot.autoconfigure.flyway.FlywayAutoConfiguration$StringOrNumberToMigrationVersionConverter] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). Is this bean getting eagerly injected/applied to a currently created BeanPostProcessor [methodValidationPostProcessor]? Check the corresponding BeanPostProcessor declaration and its dependencies/advisors. If this bean does not have to be post-processed, declare it with ROLE_INFRASTRUCTURE.

As the warning suggests, it can be avoided by declaring the bean in the infrastructure role (as well as declaring the bean method static (#45621)). I think we should meta-annotate @ConfigurationPropertiesBinding with @Role(BeanDefinition.ROLE_INFRASTRUCTURE) but I'd like to discuss this with the team to make sure I haven't overlooked anything and to agree upon the branches that will have the fix applied.

@wilkinsona wilkinsona added type: bug A general bug status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels May 20, 2025
@wilkinsona
Copy link
Member Author

We discussed this today and decided that the recommendation to use static is sufficient.

@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed type: bug A general bug status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels May 21, 2025
@nosan
Copy link
Contributor

nosan commented May 21, 2025

I am not sure that the static modifier alone is sufficient, at least the following test shows otherwise:

@ExtendWith(OutputCaptureExtension.class)
class ConfigurationPropertiesBindingTests {

	@Test
	void infracheck(CapturedOutput capturedOutput) {
		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
		context.register(InfraBeanPostProcessorConfiguration.class, InfraConfig.class);
		context.refresh();
		context.close();
		assertThat(capturedOutput.getAll()).doesNotContain("is not eligible for getting processed");
	}

	@ConfigurationProperties("infra")
	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
	static class InfraProperties {

	}

	@Configuration(proxyBeanMethods = false)
	@EnableConfigurationProperties(InfraProperties.class)
	static class InfraConfig {

		@Bean
		@ConfigurationPropertiesBinding
		static Converter<Object, Object> infraConverter() {
			return (source) -> source;
		}

	}

	@Configuration(proxyBeanMethods = false)
	static class InfraBeanPostProcessorConfiguration {

		@Bean
		static InfraBeanPostProcessor infraBeanPostProcessor(InfraProperties infraProperties) {
			return new InfraBeanPostProcessor();
		}

	}

	static class InfraBeanPostProcessor implements BeanPostProcessor {

	}

}

When @Role(BeanDefinition.ROLE_INFRASTRUCTURE) is used together with @ConfigurationPropertiesBinding on the converter's @Bean method, the warning is no longer logged.

@wilkinsona
Copy link
Member Author

Yes, that's right. However, our opinion is that the better fix is not to inject dependencies into a method that defines a BeanPostProcessor. The static just prevents InfraBeanPostProcessorConfiguration from also being affected.

@nosan
Copy link
Contributor

nosan commented May 21, 2025

I understand this, but since @Role(BeanDefinition.ROLE_INFRASTRUCTURE) can be used on @ConfigurationProperties, users might expect
that they can inject it into a BeanPostProcessor (which is a valid use case). However, they cannot, due to @ConfigurationPropertiesBinding.
I think this is exactly what happened in #45618. Everyone expected it to work, but it did not.

If, for some reason, someone tries to do the same in the future, they will probably face the same issue because of a similar cause, and users can't change FlywayAutoConfiguration or something else in the Spring Boot codebase to avoid this issue.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale May 22, 2025
@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-meeting An issue we'd like to discuss as a team to make progress status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

2 participants