Ordered list access for BeanFactory.getBeanProvider(), superseding ObjectProvider<List> [SPR-17272]

[spring-projects-issues]

Sébastien Deleuze opened SPR-17272 and commented

Unlike DependencyObjectProvider which seems to support this use case, the ObjectProvider implementation returned by DefaultListableBeanFactory#getBeanProvider(ResolvableType) does not seems to support List<Foo> correctly.

This test is green:

@Test
public void registerBeanAndGetBeanProvider() {
     GenericApplicationContext context = new GenericApplicationContext();
     context.registerBean(ViewResolver.class, () -> new MustacheViewResolver());
     assertNotNull(context.getDefaultListableBeanFactory().getBeanProvider(ResolvableType.forClass(ViewResolver.class)).getIfAvailable());
}

This test is red:

@Test
public void registerBeanAndGetBeanProviderList() {
     GenericApplicationContext context = new GenericApplicationContext();
     context.registerBean(ViewResolver.class, () -> new MustacheViewResolver());
     assertNotNull(context.getDefaultListableBeanFactory().getBeanProvider(ResolvableType.forClassWithGenerics(List.class, ViewResolver.class)).getIfAvailable());
}

This difference of behavior prevents Kofu configuration and Dave Syer FuncApplication to instantiate WebFluxAutoConfiguration.WebFluxConfig in a functional way and get view resolution support.

---

Affects: 5.1 RC3

Issue Links:

• ObjectProvider iterable/stream access for "beans of type" resolution in @Bean methods [SPR-11419] #16046 ObjectProvider iterable/stream access for "beans of type" resolution in @Bean methods
• Autowiring inconsistency: @Qualifier works with HashMap but not with Map [SPR-17197] #21731 Autowiring inconsistency: @Qualifier works with HashMap but not with Map

Referenced from: commits 65c8fa4, 41d4cb5, 7562761

0 votes, 8 watchers

[spring-projects-issues]

Juergen Hoeller commented

As per an initial analysis, this is intentional since programmatic configuration means to avoid the semantic overload of List<X> or Map<String, X> indicating a single bean of that particular collection type versus several X beans aggregated into a List/Map. Preferably, ObjectProvider stream access should be used for multiple elements, declaring the affected provider as just ObjectProvider<ViewResolver> and then calling .stream() on it (or simply iterating over it since the provider is an Iterable as well). And if a local List is really needed, the recommended solution is .stream().collect(Collectors.toList()).

[spring-projects-issues]

Andy Wilkinson commented

It's taken me a little while to get my head around this, but I like that ObjectProvider<ViewResolver> can be used to request something that can provide ViewResolver beans and then ObjectProvider<List<ViewResolver>> potentially being specifically for providing List<ViewResolver> beans. Removing the ambiguity around what injecting List<ViewResolver> means (a list of ViewResolver beans or a List<ViewResolver> bean) is compelling.

Having said that, the current implementation and javadoc appear to be at odds with this goal. The javadoc of stream() makes no mention of providing multiple beans and suggests that you'll only ever get one or none. There's also no discussion of ordering which is important when handling multiple beans. This leaves code that wants to work with multiple beans to use ObjectProvider but rely upon actually being given a DependencyObjectProvider to get the behaviour that's required. This will have a detrimental effect on the readability of the code and require a fairly deep understanding of Framework's internals to not be mislead by the documentation on ObjectProvider.

[spring-projects-issues]

Dave Syer commented

I wonder if anyone ever actually uses the default stream() implementation? I would think it is misleading and wrong since it doesn't really stick to the implied contract. The fact that the contract is not very tight (because of the javadoc?) is probably the issue here, and since this method is new in 5.1, this is a good time to tighten it up (before 5.1.0). I suggest that we change the javadoc and remove the default implementation of stream().

[spring-projects-issues]

Juergen Hoeller commented

Point taken, I'll revise the javadoc as part of this ticket. It currently documents the default implementation in the ObjectProvider interface itself which isn't really that useful since DependencyObjectProvider as well as getBeanProvider results override it with actual multi-element behavior.

Also, good point about ordering... There is currently no sorting of elements (like for a Set<ViewResolver>) for the lazily resolved stream, so it's effectively bean registration order. Maybe we should add an explicit toList() method on ObjectProvider itself with the usual @Order support.

[spring-projects-issues]

Juergen Hoeller commented

The idea behind the default implementation of stream() was simply to provide backwards compatibility with custom ObjectProvider implementations. That said, I agree the semantics are really not useful. I'll rather change it to a default implementation that throws an UnsupportedOperationException.

[spring-projects-issues]

Andy Wilkinson commented

Thanks. Some discussion of @Primary would be good too. Could the contract specify what stream() and iterator() will produce when there are multiple candidates and one is primary? Is it all candidates including the primary one, or just the primary? If you just consider the current contract in isolation, I don't think it's clear which you should expect.

[spring-projects-issues]

Juergen Hoeller commented

It'll be all matching instances, including the primary one. getIfAvailable() will give you the primary in this case, as before.

[spring-projects-issues]

Juergen Hoeller commented

BTW thanks for all the timely feedback here! This is the most important core container addition in 5.1, so we'd better take these extra cycles to refine it and document it based on common use cases...

[spring-projects-issues]

Juergen Hoeller commented

I've added an ObjectProvider.toList() method now, providing the same ordering guarantees as a reflective injection point for an aggregated List, and fully resolving all object instances at the time of the call. Stream access and iteration over an ObjectProvider has consistent lazy resolution of target instances now, between both kinds of provider types, following bean registration order.

[spring-projects-issues]

Phil Webb commented

I know I'm coming late to the party on this one, but what if we had two distinct interfaces for "single" vs "multiple" use? It feels like getObject, getIfAvailable and getIfUnique are useful if your intent is that there is a single useful bean, where as iterator and stream are only useful if your injecting into something that supports multiple beans.

Put another way, the user of the ObjectProvider needs to make the decision about if they support a single or multiple means. Having two different interfaces would make that intent much clearer. Perhaps ObjectProvider vs ObjectsProvider?

[spring-projects-issues]

Juergen Hoeller commented

That's by design: It's a single handle that allows for any kind of reasoning here. For a multi-element match, getObject may identify the primary object, potentially next to some reasoning about all available instances. Also, for a single target element found, stream access may be useful to filter it according to some criteria and see whether something remains, or to be prepared to encounter several instances at some later point but filter according to certain criteria that the current single target matches. All in all, this can be seen as a more flexible variant of qualifier matching and primary/uniqueness identification, all of which potentially operate on multiple elements but narrow to a single element based on standard criteria.

If there is really only a single element to reason about, I wouldn't use ObjectProvider to begin with. A plain ObjectFactory declaration or an @Lazy declaration gets you a long way if all you need is lazy retrieval. The availability/uniqueness business is an advanced use case already.

[spring-projects-issues]

Phil Webb commented

Perhaps we just have some fairly unique requirements in Spring Boot. We've quite often used ObjectProvider when we support zero or one (alone or primary) instance. Unfortunately ObjectProvider doesn't support that.

...or to be prepared to encounter several instances at some later point but filter according to certain criteria that the current single target matches

The decision about supporting multiple beans has to be taken when the ObjectProvider is used. I think it would be sensible that the multi-object variant would still support a stream of a single element if there happens to be just one.

I guess I'm kind of seeing usage patterns in Boot that kind of mirror Mono vs Flux. We need to either support 0..1 beans or 0..N.

[spring-projects-issues]

Juergen Hoeller commented

Another way of seeing it: ObjectProvider is a pre-typed handle for flexible bean retrieval, along the lines of the retrieval methods on ListableBeanFactory but avoiding to re-specify the type for several invocations... and avoiding to deal with bean names.

And since it exists next to ObjectFactory and JSR-330's Provider interface, I'd rather keep it as a single unified type for less conceptual surface. This also allows for a simple pair of lookup methods on BeanFactory: getBeanProvider(Class) and getBeanProvider(ResolvableType).

[spring-projects-issues]

Sébastien Deleuze commented

Phil Webb Not sure if that's relevant, but we tend to differentiate Mono versus Flux mainly for return types, for incoming parameter we tend to use them as Publisher data provider regardless the cardinality, and the usage I have seen in Boot is more as input parameter.

[spring-projects-issues]

Phil Webb commented

Ignore me, I see that methods like getIfUnique are in the 5.0 version which already sends a strong signal that ObjectProvider may represent multiple items.

[spring-projects-issues]

Juergen Hoeller commented

Fair enough, ObjectProvider supports the zero-or-none case, even with special-purpose methods for it, but it doesn't semantically enforce it at its own type level. At injection time, we are not checking the availability of target beans in any case, so both an ObjectProvider and an ObjectsProvider (if both existed) would cleanly inject... and only fail later on when accessed, losing the type-level enforcement benefit. And indeed, ObjectProvider has multi-but-narrowed semantics in it already, so the 5.1 revision is only really extending the multi-case introspection facilities.

[spring-projects-issues]

Phil Webb commented

I hate to ask for another round on this, but can we reconsider the name of the toList method? I'm finding the new API a little confusing since at first glance it appears that toList is just a convenience method to get a List from the Stream. The different order aspect is mentioned if you read the Javadoc, but I think it's easy to miss that because:

1. The name of the method exactly matches the toList method in the stream Collectors class
2. The default implementation simply calls stream().collect which implies that ordering is not important

I wonder if changing it to something like getOrdered and making the default implementation throw UnsupportedOperationException("Ordered mulit-element access not supported"); might help make it clearer that:

1. By default the provider is unordered
2. The getOrdered method is not just a convenience method around the Stream

Usage would then look like this:

public MyClass(ObjectProvider<Customer> customers) {
  customers.getOrdered().forEach(...);
}

[spring-projects-issues]

Andy Wilkinson commented

Looking at the lastest code in master, this doesn't feel quite right to me yet and I share Phil's feeling about the ordering.

Why, if I want Order and Ordered to be honoured, is a client forced to use toList()? There are many places in Boot where an ordered Stream would meet our needs nicely.

Rather than renaming toList(), I wonder if it would be better if the contract was more Stream focussed? To that end, both iterator() and toList() could be dropped in favour of a few different methods that return different types of Stream. Perhaps two would suffice. stream() that returns a Stream in an undefined order (registration order typically) and orderedStream() that returns a Stream that's ordered (honouring Order and Ordered).

One last point. The current distinction between lazily resolved and fully resolved leaves me a bit confused. Looking at the implementation, beans appear to be fully resolved in both cases. I may well have misunderstood the code, though. Assuming for a moment that I haven't, if stream() did offer lazy resolution, then that would be a further downside to forcing the use of toList() to get something that honours Order and Ordered as lazy resolution would be lost.

[spring-projects-issues]

Phil Webb commented

Looking at the implementation, beans appear to be fully resolved in both cases

 

I was confused by that as well, but there are actually two different implementations. The DependencyObjectProvider seems to always be eager, where as the inner class in the getBeanProvider has lazy resolution. I guess it's lazy when possible (although that's perhaps an implementation details that doesn't need to be in the Javadoc)

[spring-projects-issues]

Andy Wilkinson commented

Ah, I see. I had only been looking at DependencyObjectProvider. If it is indeed always eager then it doesn't honour the contract of ObjectProvider as it's currently written. Similar to ordering, if doing things lazily is important from a client's perspective, I think it would be better if that was reflected in the method names (or in something that's passed in that describes a mode of behaviour) rather than being a side-effect of choosing between a stream, a list, or an iterator.

[spring-projects-issues]

Juergen Hoeller commented

As for lazy resolution, this was referring to on-the-fly resolution of target bean instances, in case they haven't been created yet. The instances are being created during iteration, and further bean creation stops if iteration terminates. Once Ordered / @Order comes in, the instances have to be fully resolved upfront in order to compare them properly, and subsequent iteration effectively happens over a fully pre-resolved list of instances. This is the same for both ObjectProvider implementations in the meantime. Anyway, since this is indeed an implementation detail, I've removed the corresponding comments from the javadoc.

iterator() is there for the Iterable implementation, enabling the Java 5 style for-each loop. We do that in a few other places as well, offering Iterable as well as stream() along the lines of a Collection, so it's a quite consistent pattern.

So regarding toList(), good points indeed. I went with orderedStream() now, applying the order comparator (effectively pre-resolving the stream that way... as an implementation detail) to it, which still allows for Collectors.toList() resolution if needed.

One more detail: orderedStream() is always ordered according to OrderComparator at least, even if the configured dependencyComparator on DefaultListableBeanFactory is different or none. In that sense, it enforces a certain kind of order... whereas list/array ordering for regular injection points follows dependencyComparator and might be ordered differently or just in registration order in custom factory setups.

[spring-projects-issues]

Juergen Hoeller commented

Reopened due to a lazy resolution ordering conflict raised in spring-projects/spring-boot#14467...

[spring-projects-issues]

Juergen Hoeller commented

Resolved through explicitly triggering instance resolution for ordered streams now, delivering a consistent experience against early/lazy initialized beans and also against both kinds of ObjectProvider (since only the @Autowired injected variant was affected by this).

[spring-projects-issues]

Andy Wilkinson commented

Looks good now. Thank you.