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

ApplicationContextRunner has inconsistent behaviour with duplicate auto-configuration class names #17963

Open
michael-simons opened this issue Aug 26, 2019 · 5 comments
Labels
type: bug A general bug
Milestone

Comments

@michael-simons
Copy link
Contributor

michael-simons commented Aug 26, 2019

Given the following configurations:

package com.example.demo.a;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import com.example.outside_app.SomeService;

@Configuration
public class SomeConfig {

	@Bean
	public SomeService someServiceFromA() {
		return new SomeService();
	}

}

and in another package, under the same, simple name:

package com.example.demo.b;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

import com.example.outside_app.SomeService;

@Configuration
public class SomeConfig {

	@Bean
	public SomeService someServiceFromB() {
		return new SomeService();
	}

}

Spring Framework refuses to work with two configuration classes having the same bean name (someConfig). This is fine, the error is usable.

However, the ApplicationContextRunnerdoes not do this and takes in the following configuration

@Test
public void f2() {
	new ApplicationContextRunner()
		.withUserConfiguration(com.example.demo.a.SomeConfig.class, com.example.demo.b.SomeConfig.class)
	.run(ctx -> {
		assertThat(ctx).hasBean("someServiceFromB");
		assertThat(ctx).hasBean("someServiceFromA");
	});
}

And fails the test by silently ignoring or overwriting the first configuration given.

While it would have been noticed in this case, you don't notice it when you test for the absence of one bean and the presence of another. As one config has maybe silently dropped, conditions are not tested.

This happens also with overlapping autoconfigurations having the same class name and this is where I actually found the issue:

Spring Boot and or Spring Framework do apparently support autoconfigurations with the same simple class name.

Given this bean

package com.example.outside_app;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

@Configuration
public class JpaRepositoriesAutoConfiguration {

	@Bean
	public SomeService someServiceFromAutoConfig() {
		return new SomeService();
	}

}

and Spring Factories containing

org.springframework.boot.autoconfigure.EnableAutoConfiguration = com.example.outside_app.JpaRepositoriesAutoConfiguration

I can test my application:

@RunWith(SpringRunner.class)
@SpringBootTest
public class DemoApplicationTests {

	@Autowired
	private Map<String, SomeService> someServices;

	@Test
	public void contextLoads() {
		assertThat(someServices).containsKeys("someServiceFromAutoConfig");
	}

}

However, the ApplicationContextRunner disagrees and

@Test
	public void onlyAutoConfig() {

		new ApplicationContextRunner()
			.withConfiguration(
				AutoConfigurations.of(
					org.springframework.boot.autoconfigure.data.jpa.JpaRepositoriesAutoConfiguration.class,
					com.example.outside_app.JpaRepositoriesAutoConfiguration.class
				))
			.run(ctx -> assertThat(ctx).hasBean("someServiceFromAutoConfig"));
	}

fails.

While the duplicate names need good reasons and are of course a thing to debate, the application context runner should agree during tests with the framework to prevent errors in custom starters or applications, depending on what is actually tested.

I have attached the demo project.

autoconfigissue.zip

@snicoll snicoll changed the title Bug: ApplicationContextRunner has inconsistent behaviour with duplicate configuration bean names. ApplicationContextRunner has inconsistent behaviour with duplicate configuration bean names Aug 26, 2019
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 26, 2019
michael-simons added a commit to neo4j/neo4j-java-driver-spring-boot-starter that referenced this issue Aug 26, 2019
- Don’t try manually adding the health indicator for OGM, but run after Neo4jHealthIndicatorAutoConfiguration.class.
- Also rename the configuration (See spring-projects/spring-boot#17963).
@philwebb
Copy link
Member

I wonder if this might also be related to SpringApplication.allowBeanDefinitionOverriding which we added in 2.1.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 26, 2019
@philwebb philwebb added this to the 2.2.x milestone Aug 26, 2019
@michael-simons
Copy link
Contributor Author

At least I ran into the problem of not being allowed to overwrite beans because my Test with faulty. Error was in my config, but I didn’t spot it. Happens easily with the behavior of the test runner.

@philwebb philwebb added type: bug A general bug and removed type: enhancement A general enhancement labels Aug 30, 2019
@philwebb philwebb modified the milestones: 2.2.x, 2.1.x Aug 30, 2019
@snicoll snicoll self-assigned this Aug 30, 2019
@snicoll snicoll changed the title ApplicationContextRunner has inconsistent behaviour with duplicate configuration bean names ApplicationContextRunner has inconsistent behaviour with duplicate configuration class names Sep 2, 2019
@snicoll snicoll changed the title ApplicationContextRunner has inconsistent behaviour with duplicate configuration class names ApplicationContextRunner has inconsistent behaviour with duplicate auto-configuration class names Sep 2, 2019
@snicoll
Copy link
Member

snicoll commented Sep 2, 2019

The problem is that Configurations (and its AutoConfigurations implementation) do not register classes the same way AutoConfigurationImportSelector does. The former calls a simple register on the context (with a sensible order) while the latter is an ImportSelector that returns the name of the classes to load.

The net result is that the framework does something different. The register is at the same level as a bean so the simple name of the class is used (just like any other @Component really), while the import selector uses the FQN.

@snicoll
Copy link
Member

snicoll commented Sep 2, 2019

I've tried a few things and I am not sure how to tackle this problem. The context is created manually with a list of configuration classes that I know at runtime so I can't really use an ImportSelector here.

@jhoeller do you have an idea for us? I've tried to use a BeanFactoryPostProcessor but it doesn't process a custom bean definition as a Configuration class, probably because ConfigurationClassPostProcessor has already ran.

@snicoll snicoll modified the milestones: 2.1.x, 2.2.x Sep 3, 2019
snicoll added a commit to snicoll/spring-boot that referenced this issue Sep 3, 2019
@snicoll
Copy link
Member

snicoll commented Sep 3, 2019

I've been looking at fixing the issue (see snicoll@035cdda). The idea is to mimic what ImportSelector (and DeferredImportSelector) does. It turns out that having a FQN as a bean name is a fallback behaviour of the core container when the bean name is null.

There is a timing issue that's a bit odd but I might have nailed it. When the auto-configuration backs off because a condition with REGISTER_PHASE doesn't match, it looks like the configuration class is processed anyway and blows up.

You can reproduce by running SpringDataWebAutoConfigurationTests#autoConfigurationBacksOffInNonWebApplicationContexts

With the change it fails with

org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'org.springframework.boot.autoconfigure.data.web.SpringDataWebAutoConfiguration': Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.boot.autoconfigure.data.web.SpringDataWebProperties' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}
 	at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:787)
 	at org.springframework.beans.factory.support.ConstructorResolver.autowireConstructor(ConstructorResolver.java:226)
 	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.autowireConstructor(AbstractAutowireCapableBeanFactory.java:1359)
 	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1205)
 	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:557)
 	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:517)
 	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:323)

Everything works as expected as far as I can see if we replace the BeanDefinitionRegistryPostProcessor by regular calls to a GenericApplicationContext (see commented code in the branch). Unfortunately, we can't use that situation as a runner with a traditional deployment has a context that doesn't inherit from that hierarchy.

@philwebb philwebb modified the milestones: 2.2.x, 2.3.x Dec 16, 2020
@wilkinsona wilkinsona modified the milestones: 2.3.x, 2.4.x Jun 10, 2021
@snicoll snicoll removed their assignment Jul 8, 2021
@wilkinsona wilkinsona modified the milestones: 2.4.x, 2.5.x Nov 15, 2021
@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.6.x May 19, 2022
@wilkinsona wilkinsona modified the milestones: 2.6.x, 2.7.x Nov 24, 2022
@philwebb philwebb modified the milestones: 2.7.x, 3.1.x Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants