Fix BeanPostProcessorChecker warnings

[hpoettker]

This PR is a suggestion how #4519 could be addressed. It might also offer a solution to #4489, but I haven't really checked the case of XML configuration.

The severety of the logged warnings can be debated. On the one hand, they can probably be ignored by many users, so silencing them in the logging configuration of their applications is often viable. On the other hand, there may well be users who are negatively affected by the early bean initializations, and the more than 10 lines of warnings that are currently produced by a plain Spring Boot app fresh from the Spring Initializr are definitely detrimental to the Getting Started experience.

I think it would be great to have a fix with release 5.1.1 but I don't see a fix that is completely backwards compatible. The PR contains the most backwards compatible version that I could come up with. Unless a better solution is found, I think it should be considered for release 5.1.1 for the sake of the Getting Started experience.

The change is backwards compatible for users that use the respective methods on DefaultBatchConfiguration to only access the JobRegistry or the JobRegistryBeanPostProcessor, or don't use these methods at all. It is a breaking change for users that have overridden these methods to change the beans. These users would need to adapt and expose their replacements as beans with name jobRegistry or jobRegistryBeanPostProcessor respectively.

Another point to note is that declaring the JobRegistry as an infrastructure bean is a bit of a gray area. Many end-users may not be aware of it but there are most likely some who directly use it.

The result of the change is that only the MapJobRegistry is initialized early as it is required by the JobRegistryBeanPostProcessor, and no more warnings are logged by a plain Spring Boot app (or any other similar app that uses DefaultBatchConfiguration).

Please let me know if you are interested in the PR. I'll mark it as a draft until then. It's mostly fine, but I think some additional tests, a polish of the Javadoc, and an edit of the reference documentation would be nice.

Please feel free to close the PR, in case you want to pursue a different direction.

[hpoettker]

An alternative would be to add a JobRegistrySmartInitializingSingleton that does the same thing as JobRegistryBeanPostProcessor but implements SmartInitializingSingleton instead of BeanPostProcessor.

The benefit of the JobRegistrySmartInitializingSingleton is that it is initialized like any other bean, but that SmartInitializingSingleton::afterSingletonsInstantiated is only called after all job beans have been initialized. This way, the JobRegistry can be injected without declaring it as an infrastructure bean or preventing it from being post-processed, and no early initialization of any beans that the DefaultBatchConfiguration depends on are triggered.

The JobRegistrySmartInitializingSingleton could then be exposed as a bean much more safely than the JobRegistryBeanPostProcessor. Such a change would also only break things added in 5.1.0., i.e. the automatic exposure of JobRegistryBeanPostProcessor.

This seems to work nicely. Let me know if you're interested. I'll then polish the code a bit and push it.

[fmbenhassine]

Thank you for the PRs, @hpoettker ! It is unfortunate that the log level change in SF was silently unnoticed and that this issue was only caught after the release of Batch 5.1.. Anyway, those warnings definitely impact the getting started experience and I agree we should try to address them as early as 5.1.1 if possible.

Looking at your suggestions to solve the problem with a BeanDefinitionRegistryPostProcessor or JobRegistrySmartInitializingSingleton, those are fine for a minor release but due to the breaking changes they introduce, we typically try to avoid such changes in patch releases. Have you tried to move the job registry and the BPP to a separate configuration class (similar to ScopeConfiguration)?. This should also fix the problem but I am not sure it will be backward compatible for a patch release. I will try on my side as well and share my feedback here.

[hpoettker]

As far as I can tell, there is no backwards compatible way to address the issue, which is unfortunate with 5.2.0 only scheduled for the end of the year. But it should definitely be fixed, not only because of the warnings but also because the early bean initializations can cause difficult to track issues for users.

Moving the MapJobRegistry and the post processor to a different configuration class wouldn't really help, I think. You'd still get the warning for the JobRegistry or the new configuration class (depending on the implementation). Declaring the bean factory methods for the JobRegistry and the post processor as static would be the same thing.

The best way forward in my opinion would be to revert the unconditional exposure of the BeanDefinitionRegistryPostProcessor and offer the JobRegistrySmartInitializingSingleton (or a similar class) as an alternative. This should affect the smallest number of users and provide a solution for those that have been using the BeanDefinitionRegistryPostProcessor before 5.1.0.

[fmbenhassine]

I am trying to test this from a user's point of view (which is also what is mentioned in the docs) against this PR but it fails (while it passes on the current main). Here is a patch that slightly modifies the canonical example I used for tests:

test_pr4520.patch

[fmbenhassine]

My initial thinking about moving the configuration of the registry and the BPP to a different class is to have more freedom on the configuration (ie making the registry static and avoid passing it as a dependency to the BPP, making the bean definition methods static, etc) without impacting the end result of having both of them defined as beans in the application context and without introducing API breaking changes in DefaultBatchConfiguration. I attached a patch to illustrate the idea, which fixes the warnings without bean lifecycle issues. Note that tests in DefaultBatchConfigurationTests pass when run individually.

gh4489.patch

That said, such approach is equally broken (does not seem to support a custom registry) and breaking (requires a custom BPP to be declared as static, similar to https://github.com/spring-projects/spring-batch/pull/4520/files#diff-181f3335924657fef9aabc7aeb06fc5c802b551f79d1f0da98d52f1be6431136R163). I don't recall that was ever a requirement, so there should be a way to populate the registry with a non static BPP.

I will continue to investigate and share any findings here.

[fmbenhassine]

After digging into this, it seems that having dependencies in BPP is not recommended and might cause issues in SF. Here is the excerpt from the SF docs I am referring to:

If you have beans wired into your BeanPostProcessor by using autowiring or @Resource (which may
fall back to autowiring), Spring might access unexpected beans when searching for type-matching
dependency candidates and, therefore, make them ineligible for auto-proxying or other kinds of
bean post-processing

So I think the mechanism of using a BPP to populate the registry has always been problematic in Spring Batch, and only made visible now after the recent log level change in SF. BTW, the same issue happens with XML config, as well as @EnableBatchProcessing. Therefore, I believe it is a good time to review this mechanism and replace it with a smart initializing singleton like in #4521. I am closing this PR in favor of #4521. Thank you for the suggestion!