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

@Bean overriding does not pick up metadata from most specific method [SPR-10992] #15620

Closed
spring-issuemaster opened this issue Oct 16, 2013 · 9 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Oct 16, 2013

Benoit Lacelle opened SPR-10992 and commented

While overrding a Javaconfig Bean by extending the original @Configuration class, I would like to add a @DependsOn for the new Bean definition.

However, this depends-on seems not to be taken in account. here is a TestCase reproducing my issues:

public class SpringTest {

	@Test
	public void testDependsOnTakenInAccount() {
		AnnotationConfigApplicationContext ctx2 = new AnnotationConfigApplicationContext(AConfig.class, CConfig.class);
		Assert.assertEquals("overriden", ctx2.getBean("bean"));
	}

	@Configuration
	public static class AConfig {

		@Bean
		public Object bean() {
			return "not overriden";
		}

	}

	@Configuration
	public static class CConfig extends AConfig {

		protected boolean isInitialized = false;

		@Bean
		public Void doInit() {
			isInitialized = true;

			return null;
		}

		@Bean
		@DependsOn("doInit")
		public Object bean() {
			if (!isInitialized) {
				throw new RuntimeException("Not initialized");
			}

			return "overriden";
		}

	}

}

Is this an expected behavior? If yes, how can I add dependency while overriding a bean?


Affects: 3.2.3

Issue Links:

  • #15370 Cannot override singleton with scoped proxy
  • #15653 ConfigurationClass.validate() should allow for overloading in general or not at all

0 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 18, 2013

Jose Luis Martin commented

When overriding a @Bean factory method in a Configuration class, the parent BeanDefinition wins and get registered on the BeanFactory overriding the child one.

I guess that problem start in ConfigurationClassParser

// recursively process the configuration class and its superclass hierarchy
  do {
      metadata = doProcessConfigurationClass(configClass, metadata);
  }
  while (metadata != null);

That result on overriden method added to CongurationClass.beanMethods.

I guess that it could be fixed checking if the beanMethod was already added from a superclass in ConfigurationClass.addBeanMethod()

public void addBeanMethod(BeanMethod method) {
		String newMethodName = method.getMetadata().getMethodName();
		String newDeclaringClassName = method.getMetadata().getDeclaringClassName();
		
		// Check if it's already added from superclass. 
	    for (BeanMethod beanMethod : beanMethods) {
	    	if (newMethodName.equals(beanMethod.getMetadata().getMethodName()) &&
	    			!newDeclaringClassName.equals(beanMethod.getMetadata().getDeclaringClassName()))

	    		// already added, skip.
	            return;
	        }

	    this.beanMethods.add(method);

If makes sense to completely ignore overriden bean methods in a ConfigurationClass.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 19, 2013

Jose Luis Martin commented

I have done a pull request to fix the issue.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 20, 2013

Jose Luis Martin commented

After reading #15616, I don't see now a very good idea to hide bean methods to ConfigurationClass.validate() so I update the proposed fix to filter bean method elegible for bean definitions in a separate method.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2013

Phil Webb commented

If the beans that you are dealing with are all created by @Configuration classes you can call methods directly to create the same effect:

// ...

@Bean
public Object bean() {
	doInit(); // usually would be a method that returns a bean
	// ....
	return "overriden";
}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2013

Benoit Lacelle commented

Thanks Phil,

I see hoy do do this in the dependeny is in the same @Configuration class. Could this be achieved as easily if the dependency is in a different @Configuration class?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2013

Phil Webb commented

You can @Autowire another @Configuration class to call methods on it.

BTW: I think your original bug report is still valid, this just might let you work around it.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 23, 2013

Benoit Lacelle commented

I see. Nice!

I've workarounded it by extending the bean with a more classic override, and put the @DependsOn on this additional bean:


@DependsOn("doInit")
@Bean
public Void notOverridingBean() {
	return null;
}

@Bean
public Object bean(Object notOverridingBean) {
	return "overriden";
}

I would prefer your solution but this issue is in the same context as #15616: the doInit method requires an @Autowire list, which would imply 'bean' to override a parent bean after adding an @Autowire dependency: fail :(

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 4, 2013

Juergen Hoeller commented

I've revised the @Bean registration algorithm to pick the most concrete version of a method for the bean definition metadata that is being registered with the container. A side effect of this is that @Bean method overriding now also works with the container's allowBeanDefinitionOverriding flag set to false: After all, that flag isn't meant to apply to Java overriding of an @Bean method in a subclass but rather just to bean definitions of the same name from a different source (e.g. from a different configuration class).

This works for regular overriding as well as for overloading in subclasses, where the metadata is now always being taken from the @Bean annotation in the subclass - even if the factory method resolution algorithm subsequently picks a superclass variant of that bean's factory method. At bean registration time, we cannot reliably say which variant of the factory method is being picked at instance creation time (which is a dynamic choice by design), so the best we can do is to pick the metadata from the most concrete subclass.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 4, 2013

Benoit Lacelle commented

Thanks a lot Juergen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.