spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.31k stars 38k forks source link

`ContextCustomizer` does not respect lifecycle best practices #31527

Open snicoll opened 11 months ago

snicoll commented 11 months ago

org.springframework.test.context.ContextCustomizer has been introduced to allow implementations to customize the context programmatically. Looking at implementations of the interface, it is primarily used in Spring Boot.

The unique method states the following:

Customize the supplied ConfigurableApplicationContext after bean definitions have been loaded into the context but before the context has been refreshed.

There are two main use cases I can see:

Because the context is not refreshed, the second bit should really register a bean definition and let the container creates it like any regular bean. ConfigurableApplicationContext does not expose a way to register a bean definition and it does this on purpose to separate the layer of responsability. As a result, we can see a number of implementations using a cast to get access to the registry.

The other problem is that by providing ConfigurableApplicationContext, the implementation has access to a lot more than it should, in particular on a context that has not been refreshed yet. This leads to issues like https://github.com/spring-projects/spring-kafka/issues/2870.

I don't know if the problem is big enough to consider a refactoring but I think the casting does not look right.

sbrannen commented 10 months ago

You raise some good points.

Because the context is not refreshed, the second bit should really register a bean definition and let the container creates it like any regular bean. ConfigurableApplicationContext does not expose a way to register a bean definition and it does this on purpose to separate the layer of responsability. As a result, we can see a number of implementations using a cast to get access to the registry.

I agree that the need to cast is unfortunate.

For a bit of background, this mechanism/approach has been in place for a while, long before the ContextCustomizer API was introduced.

ContextCustomizer and org.springframework.test.context.support.AbstractContextLoader.customizeContext(ConfigurableApplicationContext, MergedContextConfiguration) have existed since 4.3, and those contracts were based on org.springframework.web.context.ContextLoader.customizeContext(ServletContext, ConfigurableWebApplicationContext) which has existed since at least 15 years (probably longer, but our Git history doesn't go back any further).

Refactoring either of those APIs at this point would likely constitute a breaking change, unless we figure out some way to support the old and the new way of doing things simultaneously -- perhaps something analogous to the ContextLoader/SmartContextLoader dichotomy in the TCF.

@jhoeller, thoughts?

jhoeller commented 8 months ago

Based on the analogy with the web ContextLoader and its template methods, the present arrangement seems to be fair enough. However, in terms of guidance for implementers of the hook, there is certainly room for improvement. Let's schedule this in 6.x Backlog for a potential revisit in 6.2, either revising the contract or documenting the nuances of the existing contract.

snicoll commented 2 months ago

I would like to follow-up with a related issue around the same contract. AOT support is really quite hard to get right with ContextCustomizer as this use of beanDefinitionRegistry & beanFactory means that implementations may have a mixed use of registering singletons and bean definitions. While the former isn't managed by AOT, the latter is.

A recent example of such problem is #33272