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

AOT generated code should consider visibility of FactoryBean target type #28809

Closed
christophstrobl opened this issue Jul 13, 2022 · 12 comments
Closed
Assignees
Labels
theme: aot An issue related to Ahead-of-time processing type: bug A general bug
Milestone

Comments

@christophstrobl
Copy link
Member

christophstrobl commented Jul 13, 2022

A FactoryBean<T> using a package private target type T leads to compile errors in the generated code when the FactoryBean implementation is not in the same package.

Given a package private repository CustomerRepository is located in com.example.data.mongo, and a factory BeanDefinition using MongoRepositoryFactoryBean<CustomerRepository, Object, Object> as target results in the following code being generated in the org.springframework.data.mongodb.repository.support package, where the MongoRepositoryFactoryBean is located.

package org.springframework.data.mongodb.repository.support;

/**
 * Bean definitions for {@link MongoRepositoryFactoryBean}
 */
public class MongoRepositoryFactoryBean__BeanDefinitions {
  /**
   * Get the bean definition for 'customerRepository'
   */
  public static BeanDefinition getCustomerRepositoryBeanDefinition() {
    ResolvableType beanType = ResolvableType.forClassWithGenerics(MongoRepositoryFactoryBean.class, CustomerRepository.class, Object.class, Object.class);
    RootBeanDefinition beanDefinition = new RootBeanDefinition(beanType);
    beanDefinition.setLazyInit(false);
    beanDefinition.getConstructorArgumentValues().addIndexedArgumentValue(0, "com.example.data.mongo.CustomerRepository");
    beanDefinition.getPropertyValues().addPropertyValue("queryLookupStrategyKey", QueryLookupStrategy.Key.CREATE_IF_NOT_FOUND);
    beanDefinition.getPropertyValues().addPropertyValue("lazyInit", false);
    beanDefinition.getPropertyValues().addPropertyValue("namedQueries", new RuntimeBeanReference("mongo.named-queries#0"));
    beanDefinition.getPropertyValues().addPropertyValue("repositoryFragments", new RuntimeBeanReference("mongodb.CustomerRepository.fragments#0"));
    beanDefinition.getPropertyValues().addPropertyValue("mongoOperations", new RuntimeBeanReference("mongoTemplate"));
    beanDefinition.getPropertyValues().addPropertyValue("createIndexesForQueryMethods", false);
    beanDefinition.setAttribute("factoryBeanObjectType", "com.example.data.mongo.CustomerRepository");
    beanDefinition.setInstanceSupplier(InstanceSupplier.of(MongoRepositoryFactoryBean__BeanDefinitions::getCustomerRepositoryInstance));
    return beanDefinition;
  }

Visibility checks should consider the generic arguments and potentially guard/load those via reflection.

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

snicoll commented Jul 13, 2022

I think rather the code that identifies where the code should be generated should consider the target type for a FactoryBean. MongoRepositoryFactoryBean__BeanDefinitions is a bit odd.

Can you expand on the ... please? I wonder if the code has been generated there because there some sort of package private thing on the factory bean we need to call.

@snicoll snicoll added type: bug A general bug 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 13, 2022
@snicoll snicoll added this to the 6.0.0-M6 milestone Jul 13, 2022
@snicoll snicoll changed the title AOT generated code should consider visibility of FactoryBean target type. AOT generated code should consider visibility of FactoryBean target type Jul 13, 2022
@odrotbohm
Copy link
Member

If you need a sample to play with, here is an example that explicitly makes package protected repositories public to cause them to generate compiling AOT code.

The workaround is at the top of the branch. You should see the build failing if you check out HEAD^ of that.

@snicoll
Copy link
Member

snicoll commented Jul 19, 2022

My fix means that a FactoryBean that isn't public will generate code in the target package whereas previously it was in the FactoryBean package. This breaks Spring Boot's SharedMetadataReaderFactoryBean.

@snicoll
Copy link
Member

snicoll commented Jul 19, 2022

@odrotbohm I think your use case is already fixed by #28812 as we're now using the target type of the FactoryBean so it should generate the code in the repository's package. You won't be able to give that a try until Spring Boot itself switches to framework's snapshots.

@snicoll snicoll self-assigned this Jul 19, 2022
@odrotbohm
Copy link
Member

I just upgraded to the latest snapshots of Boot and Framework and still see the bean definitions for package protected repositories be generated into the org.springframework.… space. I see the constructor argument in the generated code use a String with the fully qualified classname, but the beanType a couple of lines still uses a reference to the class literal that invisible from that location.

Wouldn't it make sense to rather generate the entire bean definition into the package of the actual target type (if that is package protected)?

@odrotbohm odrotbohm reopened this Jul 22, 2022
@snicoll
Copy link
Member

snicoll commented Jul 22, 2022

Wouldn't it make sense to rather generate the entire bean definition into the package of the actual target type (if that is package protected)?

It does that, unless the FactoryBean is not publicly accessible. I'll give that a try again

@snicoll
Copy link
Member

snicoll commented Jul 22, 2022

So the problem is that the code doesn't handle a FactoryBean that itself has a generic. We'll have to tune how the discovery logic works.

@odrotbohm
Copy link
Member

Thanks, Stéphane! Just for reference: you should be able to do the following for Restbucks to see the failure or verify a fix:

$ git co d08baf431a7cec2e1e08612f9bbf792fdecb37f1
$ mvn -Paot

@snicoll
Copy link
Member

snicoll commented Jul 22, 2022

I've polished things up and the sample above does work now. I have a feeling it will require more rounds of update as it is a bit tricky right now.

@odrotbohm going forward, please do not reopen the issue as the team may decide to fix this either as part of a polish of the existing fix, or a separate issue.

@odrotbohm
Copy link
Member

Sure thing, sorry about the hassle. I'll give the fix a try as soon as it's made it into some binaries.

@odrotbohm
Copy link
Member

Got the binaries, works fine now. Thanks! 🙇

@cyw320712
Copy link

@christophstrobl
I really want to say thank you from the bottom of my heart. I was at a loss for a long time in the process of adding MongoOperations in the process of dynamically registering the MongoRepository bean, and it was even more difficult because I could not find any source.(Even any piece of examples!! using MongoRepositoryFactoryBean) Thanks to the example you left in this Issue, I was able to solve the problem. Again, I want to say thank you very much.

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: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants