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

Support programmatic lazy-int exclusion #16615

Closed
wants to merge 1 commit into from
Closed

Support programmatic lazy-int exclusion #16615

wants to merge 1 commit into from

Conversation

tkvangorder
Copy link

No description provided.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 20, 2019
@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Apr 24, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Apr 24, 2019
@wilkinsona
Copy link
Member

Thanks for the PR, @tkvangorder. We might explore the possibility of the callback interface extending Predicate<Class<?>> as part of merging this.

@tkvangorder
Copy link
Author

@wilkinsona A predicate is a good idea and gives downstream projects a little more control. I made some changes to support a predicate, is this what you had in mind?

for (String beanName : beanFactory.getBeanDefinitionNames()) {
BeanDefinition beanDefinition = beanFactory.getBeanDefinition(beanName);
if (eagerPredicateList.stream().anyMatch(
(predicate) -> predicate.test(beanFactory.getType(beanName)))) {
Copy link
Member

@wilkinsona wilkinsona Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this may lead to factory beans being instantiated so we need to avoid it. We probably need something that's somewhat similar to BeanTypeRegistry. Making that class public doesn't feel like a good idea, but it's also complicated enough that copying it doesn't feel like a good idea either. In short, a change is needed here but I don't know what it should be. I wonder if Framework could provide an API for getting a bean's type that gives up and returns null rather than instantiating a factory bean that lacks a generic type.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can revert back to using a customizer that passes in a list of classes that should be ignored? This basically inverts the problem and allows the use of beanFactory.isTypeMatch(). I really like the idea of a Predicate (as you can build much smarter ignore logic) but I am wondering if there are any cases where a list of classes wouldn't suffice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we can't safely use isTypeMatch() either. If a bean is a FactoryBean, Framework may instantiate it to determine the type of the object that it produces.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that could be done (at least for collecting the Predicates) would be to use SpringFactoriesLoader rather than scanning the list of bean definitions. Of course, once we have the list of predicates we are still back to the need of passing each bean definition's type to those predicates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We considered spring.factories (it has some notable benefits over beans being created very early) but preferred the @Bean-based approach that allows the predicate and the auto-configuration to which it relates to be defined together in the same class.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Apr 29, 2019
@wilkinsona
Copy link
Member

Flagged for team attention so we can discuss the best way to go from a bean definition to the type of the defined bean.

@philwebb
Copy link
Member

I've raised spring-projects/spring-framework#23056 to see what (if any) of our BeanTypeRegistry can be pulled up into Spring Framework.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label May 29, 2019
@philwebb philwebb removed their assignment Jun 1, 2019
@philwebb philwebb added the status: blocked An issue that's blocked on an external project change label Jun 1, 2019
@philwebb
Copy link
Member

philwebb commented Aug 1, 2019

Although the BeanTypeRegistry code has now been pulled up, we're still blocked on spring-projects/spring-framework#23374

@tkvangorder
Copy link
Author

@wilkinsona @philwebb

Since we now have a method to extract all beans of a given type from the factory (without initializing the factory beans):

How would you feel about not using a predicate, but rather reverting back to a "customizer" that returns a list of classes that should be excluded from the lazy-loading processor? I can then just iterate over the classes from the customizers and use getBeansOfType(Class<?>, false, false); without requiring any additional changes to the BeanFactory.

@philwebb
Copy link
Member

@tkvangorder I'm still hopeful that we can get a fix for spring-projects/spring-framework#23374 into Framework before GA

@tkvangorder
Copy link
Author

@philwebb I have updated this PR to take advantage of the new BeanFactory.getType(name, boolean) method.

@philwebb
Copy link
Member

philwebb commented Sep 4, 2019

@tkvangorder Wow that was quick!! It only landed in Framework a few hours ago 👍

@tkvangorder
Copy link
Author

@philwebb @wilkinsona I know you are both crazy busy, but I think this PR is ready to go. Do you want me to do anything else with it?

@philwebb
Copy link
Member

@tkvangorder I'm afraid we haven't got to it yet but this one is really high on the list and we need to merge it before RC1. Rest assured, we've not forgotten :)

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Sep 16, 2019
@philwebb philwebb changed the title Adding ability to contribute customizers to lazy init post processor Support programmatic lazy-int exclusion Sep 27, 2019
philwebb pushed a commit that referenced this pull request Sep 27, 2019
Allow the `LazyInitializationBeanFactoryPostProcessor` to skip setting
lazy-init based on a programmatic callback. This feature allows
downstream projects to deal with edge-cases in which it is not easy to
support lazy-loading (such as in DSLs that dynamically create additional
beans).

See gh-16615
philwebb added a commit that referenced this pull request Sep 27, 2019
@philwebb philwebb closed this in 8db598b Sep 27, 2019
@philwebb
Copy link
Member

Thanks very much @tkvangorder, this has now been merged to master.

I've refined things a little in commit 3ffc5f2. I decided to rename EagerLoadingBeanDefinitionPredicate to LazyInitializationExcludeFilter to match some of the other callback interface names that we already have. I also extended the signature to pass in the beanName and beanDefinition as well as the beanType. Since that will complicate one of the more obvious implementations, I also added a static factory method so you can do LazyInitializationExcludeFilter.forBeanTypes(IntegrationFlow.class) to quickly create a type based filter.

Thanks again for all your work on this, and please let me know if you find any issues with my changes!

@philwebb philwebb modified the milestones: 2.2.x, 2.2.0.RC1 Sep 27, 2019
@tkvangorder
Copy link
Author

I like the changes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants