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

Improve docs for AnnotatedBeanDefinitionReader, @Configuration, and @ContextConfiguration regarding "annotated classes" #23638

Closed
10 tasks done
ttddyy opened this issue Sep 13, 2019 · 5 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: test Issues in the test module type: documentation A documentation task
Milestone

Comments

@ttddyy
Copy link
Contributor

ttddyy commented Sep 13, 2019

Original Description

I find this test interesting:

@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = RestTemplate.class)
class MyTest {

    @Autowired
    RestTemplate restTemplate;

    @Test
    void check() {
        assertThat(restTemplate).isNotNull();
    }

}

This works because while loading a context, AnnotationConfigContextLoader registers @ContextConfiguration provided classes(RestTemplate in this case) to bean definitions, so that they are available for injections.

So, this is also possible:

@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = { MyTest2.Foo.class, MyTest2.Bar.class })
class MyTest2 {

    static class Foo {
        public Foo(Bar bar) {  // dependency to Bar
        }
    }

    static class Bar {
        public Bar() {
        }
    }

    @Autowired
    Foo foo;

    @Autowired
    Bar bar;

    @Test
    void check() {
        assertThat(this.foo).isNotNull();
        assertThat(this.bar).isNotNull();
    }

}

I think this is not an intended usage of @ContextConfiguration#classes.

Probably, by default, filter-out or validate those classes to be @Configuration classes.
For the case of allowing non @Configuration classes (for example, lite-mode), probably provide an explicit option(new attribute) on @ContextConfiguration. e.g.: @ContextConfiguration(classes=MyBean.class, liteMode=true)


This is what I found in real world code base:

@ExtendWith({MockitoExtension.class, SpringExtension.class})
@ContextConfiguration(classes = {ResourceBundleMessageSource.class, MyService.class,
        MyExceptionHandler.class, MyPropertyConfiguration.class, RestTemplate.class})
public class MyExceptionHandlerTest {
    // ...
}

So, would be nice not seeing such test class :)


Deliverables

Improve documentation for the following regarding "annotated classes".

  • AnnotationConfigRegistry
  • AnnotationConfigApplicationContext
  • AnnotationConfigWebApplicationContext
  • AnnotatedBeanDefinitionReader
  • @Configuration
  • @Import
  • AnnotationConfigContextLoader
  • AnnotationConfigContextLoaderUtils
  • @ContextConfiguration
  • Reference Manual: testing.adoc
@sbrannen
Copy link
Member

The fact that annotated classes (other than @Configuration classes) are supported is a documented feature.

Annotated Classes

The term annotated class can refer to any of the following.

  • A class annotated with @Configuration
  • A component (i.e., a class annotated with @Component, @Service, @Repository, etc.)
  • A JSR-330 compliant class that is annotated with javax.inject annotations
  • Any other class that contains @Bean-methods

So we cannot change that, since it would be a breaking change.

In addition, it can be quite useful to test a very limited combination of components.

Having said that, however, the fact that a non-annotated class is automatically registered as a Spring bean is not documented. So I'll investigate that further.

@sbrannen sbrannen self-assigned this Sep 14, 2019
@sbrannen sbrannen changed the title Make "@ContextConfiguration#classes" to take only "@Configuration" by default Make @ContextConfiguration(classes) accept only @Configuration classes by default Sep 14, 2019
@sbrannen sbrannen added status: pending-design-work Needs design work before any code can be developed and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 14, 2019
@sbrannen sbrannen added this to the 5.2 GA milestone Sep 14, 2019
@sbrannen sbrannen added type: documentation A documentation task and removed status: pending-design-work Needs design work before any code can be developed labels Sep 14, 2019
@sbrannen
Copy link
Member

sbrannen commented Sep 14, 2019

Having said that, however, the fact that a non-annotated class is automatically registered as a Spring bean is not documented. So I'll investigate that further.

A valid use case for this behavior of AnnotatedBeanDefinitionReader.register() would be component scanning based on custom annotations or naming conventions and relying on convention based autowiring of non-annotated constructors. @Import is another valid use case.

So, in light of that, I'm changing the focus of this issue to improve the corresponding documentation.

@sbrannen sbrannen modified the milestones: 5.2 GA, 5.1.10 Sep 14, 2019
@sbrannen sbrannen changed the title Make @ContextConfiguration(classes) accept only @Configuration classes by default Improve docs for AnnotatedBeanDefinitionReader and @ContextConfiguration(classes) regarding "annotated classes" Sep 14, 2019
@sbrannen sbrannen changed the title Improve docs for AnnotatedBeanDefinitionReader and @ContextConfiguration(classes) regarding "annotated classes" Improve docs for AnnotatedBeanDefinitionReader, @Configuration, and @ContextConfiguration regarding "annotated classes" Sep 14, 2019
@ttddyy
Copy link
Contributor Author

ttddyy commented Sep 15, 2019

Hi @sbrannen

The current @ContextConfiguration javadoc states:

@ContextConfiguration can be used to declare either path-based resource locations (via the locations() or value() attribute) or annotated classes (via the classes() attribute).

However, in fact, ContextConfiguration#classes can take any classes. For non-annotated classes, they either need a default constructor or the arguments for constructor injection need to be resolvable.

Also, undocumented behavior is that classes that have specified in ContextConfiguration#classes can be injected.

I still think restricting ContextConfiguration#classes to take only annotated classes would be ideal behavior.

In addition, it can be quite useful to test a very limited combination of components.

I agree allowing non-annotated classes to be injectable makes certain tests useful but very limited, and @Configuration has much more flexibility.

It would be beneficial to mention in documentation such that using non-annotated classes have limitation and favoring @Configuration instead.

So, extreme case, this is possible:

@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = {
        MyConfigTest.MyConfig.class,  // annotated class
        RuntimeException.class,  // non annotated class
        String.class,  // non annotated class
        MyConfigTest.Foo.class  // since its constructor is resolvable, can be listed here
//        MyConfigTest.Bar.class  // constructor injection fails, so cannot be here
})
class MyConfigTest {

    @Autowired
    MyConfig myConfig; // annotaed class. This might be useful for testing @Configuration itself

    @Autowired
    RuntimeException runtimeException; // non annotated class

    @Autowired
    String myString; // non annotated class

    @Autowired
    Foo foo; // non annotated class with String constructor

    @Test
    void check() {
        assertThat(myConfig).isNotNull();
        assertThat(myString).isNotNull();
        assertThat(runtimeException).isNotNull();
        assertThat(foo).isNotNull();
    }

    @Configuration
    static class MyConfig {
    }

    static class Foo {
        public Foo(String s) {
        }
    }
    static class Bar {
        public Bar(Integer i) {
        }
    }

}

@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Sep 16, 2019
@sbrannen sbrannen added the in: test Issues in the test module label Sep 16, 2019
@sbrannen
Copy link
Member

Updated deliverables are now being tracked in this issue's description.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 26, 2019
@sbrannen
Copy link
Member

Resolved for 5.1.10 in 7d126d3 and for 5.2.0 in f05b462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: test Issues in the test module type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

4 participants