Eberhard Wolff opened SPR-4632 and commented

Status Quo

The loadContext(\*\) methods in AbstractGenericContextLoader and AbstractGenericWebContextLoader invoke AnnotationConfigUtils.registerAnnotationConfigProcessors(context).

The result is that @Autowired, @Resource, @Inject, etc. all work by default, even if annotation-driven dependency injection is not explicitly configured in the test's ApplicationContext. Consequently, if a developer is not aware of this, then errors related to the lack of explicit annotation-driven DI support in production configuration will not be noticed until after testing which is typically undesirable.

Note, however, that this unexpected behavior only applies to XML-based configuration. With JavaConfig, an AnnotatedBeanDefinitionReader (used internally by AnnotationConfigContextLoader and AnnotationConfigWebContextLoader) automatically registers the annotation config processors.

Furthermore, BeanPostProcessors may inadvertently be applied to the test instance -- even though the test is not truly a bean in the ApplicationContext. This leads to potentially problematic behavior such as accidental proxying of the test instance, as described in #14113.

Deliverables

  1. [ ] As suggested in comments for this issue, create an ApplicationContext dedicated solely to the test instance (for the purpose of dependency injection and bean initialization) and set the parent of that context to the ApplicationContext loaded by the TCF.
  2. [ ] Stop invoking AnnotationConfigUtils.registerAnnotationConfigProcessors() in:
  3. [ ] AbstractGenericContextLoader
  4. [ ] AbstractGenericWebContextLoader
  5. [ ] Ensure that child contexts are properly closed with regard to context caching in the TCF.

Original Issue Summary

Do not enable annotation-driven DI for the entire ApplicationContext in the TestContext framework

Original Proposal

It would be better if the TestContext framework (TCF) only enabled annotation-driven dependency injection for test instances and not for the entire ApplicationContext.

Affects: 2.5.2

Attachments: - AnnotationConfigProcessorTestExecutionListener.java (1.42 kB)

Issue Links: - #13149 Spring Test should not modify the application context under test ("is duplicated by") - #14496 Support WebApplicationContext hierarchies in the TestContext Framework - #10284 Provide support for context hierarchies in the TestContext Framework - #18295 Support 'throw-away' context for a test class - #10719 Provide mechanism for disabling automatic annotation-driven autowiring in tests - #14113 Test instances should not be proxied in the TestContext framework

6 votes, 6 watchers

Comment From: spring-projects-issues

Juergen Hoeller commented

This has actually been debated before but was decided to be the better tradeoff overall. We also consider enabling annotation processing by default in forthcoming special environments, so that doesn't necessarily apply to test environments only.

Actually, integration test XML files are not necessarily supposed to be an exact match for production bean definition files in the first place. There are always going to be some configuration differences, typically isolated into a specific XML file that's part of a config set. Since it's sufficient to have a entry in one of the files in the config set, I don't think that the defaults in the test environment are bad there... Maybe not immediately expected but still not causing serious hassle either.

Juergen

Comment From: spring-projects-issues

Tuomas Kiviaho commented

Automatic post processor registration lead to intense debugging session for me since I wasn't aware of it before and I have a custom CommonAnnotationBeanPostProcessor which lead to double post processing causing latter to crash due to it's default configuration.

As workaround I just had to name my post processor identically to the default one ( (org.springframework.context.annotation.internalCommonAnnotationProcessor) preventing it from being registered.

One solution could be to use org.springframework.beans.factory.ListableBeanFactory#getBeanNamesForType to ensure that beanFactory doesn't already have post processors instead of name comparison.

Comment From: spring-projects-issues

Tuomas Kiviaho commented

I noticed that it is possible to register processors just for test class via extendingDependencyInjectionTestExecutionListener by using test container application context as parent of a test application context which sole purpose is to instantiate test.

By moving the AnnotationConfigUtils.registerAnnotationConfigProcessors from AbstractGenericContextLoader to DependencyInjectionTestExecutionListener is one way to solve this issue without API changes. By moving hooks as well from AbstractGenericContextLoader the extendability stays about the same.

Comment From: spring-projects-issues

Dave Syer commented

I agree with Tuomas: the test case should live in a child context, so that the behaviour of the parent context is the same as it would be in production. Can we get this into the 3.0.x roadmap?

Comment From: spring-projects-issues

Neale Upstone commented

Is there any hope of this making 3.2 now? It looks like there are enough related changes going in with spring-test-mvc that this should go too...

Comment From: spring-projects-issues

Sam Brannen commented

Hi Neal,

Pending completion of related issues #10284 and #14496, it may be possible to address this issue in the 3.2 time frame, but we'll just have to see how things play out.

Regards,

Sam

Comment From: spring-projects-issues

Sam Brannen commented

FYI: I have created a branch as a sort of playground for investigating options that do not include creating a child ApplicationContext. This code may well be thrown away at some point, but in case you're interested, you can check it out here for now: https://github.com/sbrannen/spring-framework/commits/SPR-4632

Regards,

Sam

Comment From: spring-projects-issues

Neale Upstone commented

Thanks. I've had a quick look, and do wonder if it's an approach that might lead to maintenance headaches in future. Are there drawbacks to the child context approach?

Comment From: spring-projects-issues

Sam Brannen commented

Hi Neale,

Thanks for taking a look and more importantly for the quick feedback!

The work in the branch is really just an exploration of one possible option, but you're right: the child context approach may well prove to be more pragmatic and elegant in the long run. In addition, introducing a child context may make the issue raised in #14113 a moot point, thus potentially killing two (or more) birds with one stone. So I'll continue investigating. ;)

Cheers,

Sam

Comment From: spring-projects-issues

Marten Deinum commented

As this would be a fix for issue #14113 is there any change of seeing this included in Spring 5.1? #14113 is an issue and with the ease of testing with @MockBean from Spring Boot not GC'ing not used contexts will become an issue (as the @MockBean is part of the key for caching the context!). 

Next to that I believe that enabling additional BeanFactoryPostProcessor and BeanPostProcessor instances on an existing context might lead to unexpected results (as Eberhard Wolff also pointed out that auto wiring magically works). 

Comment From: spring-projects-issues

Marten Deinum commented

Sam Brannen / Juergen Hoeller any progress/information on this? As we heavily use @MockBean this is biting us quite often. We already have the "fix" from #14113 in-place but that is quite easy to circumvent (people forget to extend our base class for instance).

Comment From: spring-projects-issues

Marten Deinum commented

Although creating a fresh ApplicationContext to use to inject dependencies into a test class might seem like a good idea, the proposed solution will also result in some issues. I did some tests with an AnnotationConfigApplicationContext!

If a test class wants access to the ApplicationContext or uses @WebAppConfiguration this would be troublesome. The first would result in injection of the temporary ApplicationContext instead of the actually used one, the use of @WebAppConfiguration would require a WebApplicationContext (which should then also leak into the DependencyInjectionTestExecutionListener). 

After some further digging, the main issue lies within the line beanFactory.initializeBean(bean, testContext.getTestClass().getName());. This results in all BeanPostProcessors being applied to the test-case. Whereas, we probably only want the init/destroy callbacks and aware callbacks to be applied. 

Sam Brannen is that along the lines of your proposed solution in your specific branch? 

Comment From: spring-projects-issues

Sam Brannen commented

Marten Deinum, you raise some interesting questions.

I hadn't really pondered the scenario where the ApplicationContext (or WebApplicationContext (WAC)) is injected into the test instance. In theory, we could create the child context as a WAC if the parent is a WAC, but we wouldn't necessarily be able to create a child context that is the exact same type as the parent since third parties can create their own ApplicationContext implementations (e.g., Spring Boot).

I think we would have to investigate the possibilities here in greater depth with actual prototype code.

Sam Brannen is that along the lines of your proposed solution in your specific branch?

Yes, that was the goal of my proof of concept. I would actually prefer to be able to avoid the introduction of a child context while simultaneously limiting what is applied to the test instance.

The main challenge with the latter is that the core DI support in Spring is unaware of the testing framework. So there is currently no mechanism for instructing the core DI support not to do certain things (other than having the test class implement Advisor, as mentioned elsewhere as a workaround).

Juergen Hoeller, what do you think about introducing some sort of mechanism in the core DI support that would allow the TestContext framework to specify that only certain BPPs should be applied to the test instance?

Comment From: spring-projects-issues

Sam Brannen commented

FYI: Juergen Hoeller graciously and unwittingly resolved #14113 while addressing #21674.

Thus, unnecessary proxying of test instances is no longer an issue as of Spring Framework 5.1 RC3.

Comment From: sbrannen

Since #14113 was addressed in 5.1 and due to other enhancements in the framework in the interim, we have not heard from the community about a concrete need for loading a child context in the TCF in modern Spring applications. In addition, as stated in the above discussions, introducing a child context also introduces issues of its own.

In light of that, I am closing this issue.