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

Factory method is lost for AOT-processed beans that do not require autowiring #28748

Closed
artembilan opened this issue Jul 2, 2022 · 9 comments
Assignees
Labels
theme: aot An issue related to Ahead-of-time processing type: regression A bug that is also a regression
Milestone

Comments

@artembilan
Copy link
Member

The ListableBeanFactory has these methods:

String[] getBeanNamesForAnnotation(Class<? extends Annotation> annotationType);
Map<String, Object> getBeansWithAnnotation(Class<? extends Annotation> annotationType) throws BeansException;
<A extends Annotation> A findAnnotationOnBean(String beanName, Class<A> annotationType)

Which are useful in cases like this:

@Bean
@GlobalChannelInterceptor(patterns = "dateChannel")
WireTap loggingWireTap() {
	....
}

@Bean
@IntegrationConverter
Converter<Date, Integer> currentSeconds() {
    ...
}

After AOT has processed those beans and generated respective functional substitutions, there is no annotation info on bean definition anymore: an instance supplier just calls the target factory method via lambda:

  /**
   * Get the bean definition for 'currentSeconds'
   */
  public static BeanDefinition getCurrentSecondsBeanDefinition() {
    ResolvableType beanType = ResolvableType.forClassWithGenerics(Converter.class, Date.class, Integer.class);
    RootBeanDefinition beanDefinition = new RootBeanDefinition(beanType);
    beanDefinition.setInstanceSupplier(InstanceSupplier.of(IntegrationApplication__BeanDefinitions::getCurrentSecondsInstance));
    return beanDefinition;
  }

  /**
   * Get the bean instance for 'currentSeconds'.
   */
  private static Converter getCurrentSecondsInstance(RegisteredBean registeredBean) {
    return registeredBean.getBeanFactory().getBean(IntegrationApplication.class).currentSeconds();
  }

I workaround it with some Registration bean definition from the BeanDefinitionRegistryPostProcessor at the moment and that's probably will let me to avoid some potential reflection for those annotations at runtime, but I believe there might be some use-cases when the mentioned BeanFactory API is called at runtime.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 2, 2022
@snicoll
Copy link
Member

snicoll commented Jul 2, 2022

I believe that is a regression of how BeanDefinitions are registered.

Previously, we would use the resolved factory method as an input for the bean definition and I think this has been lost for beans that do not need autowiring as it seems to be autowiring specific now.

@snicoll snicoll added type: regression A bug that is also a regression theme: aot An issue related to Ahead-of-time processing and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 3, 2022
@snicoll snicoll added this to the 6.0.0-M5 milestone Jul 3, 2022
@snicoll
Copy link
Member

snicoll commented Jul 3, 2022

paging @philwebb

@snicoll snicoll changed the title Consider to preserve annotation info on beans after AOT generation phase Factory method is lost for AOT-processed beans that do not require autowiring Jul 11, 2022
@snicoll snicoll modified the milestones: 6.0.0-M5, 6.0.0-M6 Jul 13, 2022
@snicoll
Copy link
Member

snicoll commented Jul 13, 2022

So our plan is to change the signature of the method that provides a bean instance so that it has to provide the factoryMethod reference one way or the other. While brainstorming, we've also realized that AutowiredInstantiationArgumentsResolver might not be the ideal name for what that class does.

Rather than returning the bean instance and using InstanceSupplier.of we intend to return an instance of InstanceSupplier that takes care of the exacutable to use, something like:

/**
 * Create the bean instance supplier for 'restTemplateClientService'.
 */
private static BeanInstanceSupplier getRestTemplateClientServiceInstanceSupplier() {
    return BeanInstanceSupplier
            .forConstructor(RestTemplateBuilder.class)
            .withGenerator(args -> new RestTemplateClientService(args.get(0)));
}

snicoll added a commit to snicoll/spring-framework that referenced this issue Jul 18, 2022
philwebb pushed a commit to philwebb/spring-framework that referenced this issue Jul 18, 2022
@snicoll
Copy link
Member

snicoll commented Jul 20, 2022

Unfortunately, the fix breaks other use cases as the supplier is composed in the caller and we now return a InstanceSupplier<Object> whereas previously it was an actual typed instance. Example code:

InstanceSupplier<WebMvcAutoConfiguration.EnableWebMvcConfiguration> instanceSupplier = WebMvcAutoConfiguration_EnableWebMvcConfiguration__BeanDefinitions.getEnableWebMvcConfigurationInstanceSupplier();
instanceSupplier = instanceSupplier.andThen(WebMvcAutoConfiguration_EnableWebMvcConfiguration__Autowiring::apply);

Thanks @OlgaMaciaszek for the report!

@snicoll snicoll reopened this Jul 20, 2022
@snicoll
Copy link
Member

snicoll commented Jul 20, 2022

This sample showcases the problem:

package org.springframework;

import org.springframework.beans.factory.aot.BeanInstanceSupplier;
import org.springframework.beans.factory.config.BeanDefinition;
import org.springframework.beans.factory.support.InstanceSupplier;
import org.springframework.beans.factory.support.RegisteredBean;
import org.springframework.beans.factory.support.RootBeanDefinition;

public class Sample {

	/**
	 * Get the bean definition for 'sampleConfiguration'
	 */
	public static BeanDefinition getSampleConfigurationBeanDefinition() {
		Class<?> beanType = SampleConfiguration.class;
		RootBeanDefinition beanDefinition = new RootBeanDefinition(beanType);
		beanDefinition.setAttribute("org.springframework.context.annotation.ConfigurationClassPostProcessor.configurationClass", "lite");
		InstanceSupplier<SampleConfiguration> instanceSupplier = getSampleConfigurationInstanceSupplier();
		instanceSupplier = instanceSupplier.andThen(SampleConfiguration__Autowiring::apply);
		beanDefinition.setInstanceSupplier(instanceSupplier);
		return beanDefinition;
	}

	private static BeanInstanceSupplier getSampleConfigurationInstanceSupplier() {
		return BeanInstanceSupplier.forConstructor().withGenerator(SampleConfiguration::new);
	}

	static class SampleConfiguration {

	}

	static class SampleConfiguration__Autowiring {

		 static SampleConfiguration apply(RegisteredBean registeredBean, SampleConfiguration sampleConfiguration) {
			// autowiring stuff
			 return sampleConfiguration;
		}

	}

}

@snicoll
Copy link
Member

snicoll commented Jul 20, 2022

DefaultBeanRegistrationCodeFragments is creating an InstanceSupplier with the bean class. If the bean exposes generic information, these are lost which could be a problem for the thing post-processing the bean (+ uncheck warning). That probably used to apply to the method that created the instance previously.

philwebb added a commit that referenced this issue Jul 20, 2022
Update `InstanceSupplier.andThen` to propagate the result of
`getFactoryMethod()`.

See gh-28748
@philwebb
Copy link
Member

I've updated InstanceSupplier.andThen() so that the factory method isn't lost. We still need to fix the generics.

philwebb added a commit that referenced this issue Jul 21, 2022
Update `BeanInstanceSupplier` to support a generic type.

See gh-28748
@philwebb
Copy link
Member

I've added a generic to BeanInstanceSupplier which I hope will fix this. I'd like to leave this one open to see if we can improve the generated code even more.

olegz added a commit to spring-cloud/spring-cloud-stream that referenced this issue Jul 22, 2022
This commit also includes partial work to ensure framework works in full native/AOT mode.

Also, PollableBean remains and will simply not work in AOT mode until spring-projects/spring-framework#28748 is resolved. That said, i will be deprecating t and it will be removed in the next release given that we already have configurable alternative

Resolves #2456
@snicoll
Copy link
Member

snicoll commented Jul 26, 2022

Phil and I discussed this and agreed to close this issue as the reported problem is fixed. We've brainstormed on how we could make it better, see #28875.

@snicoll snicoll closed this as completed Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: aot An issue related to Ahead-of-time processing type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants