Skip to content

Use ObjectProvider<T> with ordered list access instead of ObjectProvider<List<T>> #14467

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

Closed
sdeleuze opened this issue Sep 13, 2018 · 9 comments
Assignees
Labels
type: task A general task
Milestone

Comments

@sdeleuze
Copy link
Contributor

As discussed in SPR-17272 (which is now fixed on Spring Framework master), ObjectProvider now provides ordered list access, superseding ObjectProvider<List>.

In addition to follow best practices, updating Boot auto-configuration classes to use ObjectProvider<T> parameters with ordered list access instead of ObjectProvider<List<T>> would make it usable with BeanFactory.getBeanProvider(), required with functional bean registration like in Kofu configuration or @dsyer work in spring-boot-micro-apps.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 13, 2018
@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 14, 2018
@philwebb philwebb added this to the 2.1.x milestone Sep 14, 2018
@wilkinsona wilkinsona self-assigned this Sep 14, 2018
wilkinsona added a commit to wilkinsona/spring-boot that referenced this issue Sep 19, 2018
@wilkinsona
Copy link
Member

I've got an attempt at this in this branch. Unfortunately, CustomHibernateJpaAutoConfigurationTests.hibernatePropertiesCustomizersAreAppliedInOrder() is currently failing due to the customisers being applied in the wrong order. The customizers are being retrieved using the following code:

hibernatePropertiesCustomizers.orderedStream().collect(Collectors.toList())

hibernatePropertiesCustomizers is an ObjectProvider<HibernatePropertiesCustomizer>. Injecting ObjectProvider<List<HibernatePropertiesCustomizer>> into the same place and calling getIfAvailable() produces a list with the same two customisers but in the reverse order. This looks like a Framework bug but I've yet to reproduce it in a standalone test. /cc @jhoeller for awareness just now. I'll dig some more tomorrow.

@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Sep 19, 2018
@jhoeller
Copy link

Let's try to sort this out tomorrow morning if we can. This is important enough for 5.1 GA still.

@wilkinsona
Copy link
Member

The problem's occurring because the ordering information isn't available. It isn't available as the order source provider doesn't have the right information. In resolveMultipleBeans, the matching beans returned by findAutowireCandidates(String, Class<?>, DependencyDescriptor) is a Map<String, Class> with the following contents:

{sampleCustomizer=interface org.springframework.boot.autoconfigure.orm.jpa.HibernatePropertiesCustomizer, anotherCustomizer=interface org.springframework.boot.autoconfigure.orm.jpa.HibernatePropertiesCustomizer}

In createFactoryAwareOrderSourceProvider(Map<String, ?>) the instancesToBeanNames map ends up with a single entry as the keys clash. When the order is being looked up, the key that's used is the lambda that implements HibernatePropertiesCustomizer so nothing's found and LOWEST_PRECEDENCE is used in each case.

Despite figuring this much out, I'm not yet able to reproduce it in a standalone test. In my attempt at doing so, the map returned from findAutowireCandidates contains actual instances rather than just the Class and everything works as expected from there.

@jhoeller
Copy link

Ah, Class references... We put those in for beans which aren't initialized yet at the time of retrieval, resolving them on the fly as their instances are being accessed... But of course that doesn't work once the order is being derived with those as keys. I'll reopen SPR-17272 accordingly.

@jhoeller
Copy link

This is sorted out on master now. Please give it a try!

@wilkinsona wilkinsona modified the milestones: 2.1.x, 2.1.0.M4 Sep 20, 2018
@wilkinsona
Copy link
Member

@sdeleuze has identified some cases with Spring Fu where a failure occurs due to a Stream being used more than use. It's probably too brittle to assume single-use, even in places where the annotation-based configuration model guarantees that'll be the case.

@wilkinsona wilkinsona reopened this Sep 20, 2018
@jhoeller
Copy link

As far as I see, a Java 8 Stream isn't meant to be reusable: see e.g. https://stackoverflow.com/questions/36255007/is-there-any-way-to-reuse-a-stream. From my perspective, ObjectProvider is a factory for single-use Stream instances and should simply be used as a starting point for each individual stream.

@wilkinsona
Copy link
Member

Indeed. I wasn't intending a Stream to be reused, but Spring Fu triggers some code paths where a single-use assumption doesn't hold true. Holding onto the ObjectProvider rather than the Stream is probably what I'll do.

@jhoeller
Copy link

Definitely hold on to the ObjectProvider in general. This reuse problem is also a reason why we don't support the injection of Stream as a direct autowiring argument but rather force people to go through ObjectProvider as a factory for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

No branches or pull requests

5 participants