ArC - register synthetic beans for List injection points

[mkouba]

• unless a matching bean exists

[mkouba]

Here's an idea - javax.enterprise.inject.Instance is not very intuitive when it comes to injection of multiple beans which is perfectly legal although not very common in the CDI world. In this PR, we register a synthetic bean for any List<> injection point for which a regular bean does not exist. As a result, a user can do e.g. @Inject List<Service> even if there's no such bean that would match this type (per type-safe resolution rules). There's a test that demonstrates the usage.

Improvement ideas:

• support List<InstanceHandle<Service>> as well
• the list could be sorted, e.g. the community discussed the possibility to use the @Priority

[mkouba]

CC @ebullient @manovotn @Ladicek @stuartwdouglas

[Ladicek]

I saw the discussion on Zulip and thought about it for a while. I think it's full of corner cases. The most obvious one: what happens when a bean of type List<Service> suddenly appears, when previously there was none? (Rhetorical question. I know what happens. I don't think it's good.) Note that our CDI container is flat, so adding a dependency often means adding new beans. Also note that there are libraries that actually do provide beans of collection types (I can't immediately think of anything actually providing beans of type java.util.List, but I recall the Quarkus Infinispan extension provides beans of type Cache, which transitively extends java.util.Map, so this happens easier than you might think).

I ended up thinking that this would be best done either using a dedicated type (BeanCollection? InstanceCollection?), or a dedicated annotation (@InjectAll?), otherwise it's too brittle.

[mkouba]

The most obvious one: what happens when a bean of type List suddenly appears, when previously there was none? (Rhetorical question. I know what happens. I don't think it's good.)

But this can happen even now. You add a dependency and get a new bean for type Foo and boom...

Note that our CDI container is flat, so adding a dependency often means adding new beans. Also note that there are libraries that actually do provide beans of collection types...

Well, the libraries should not provide beans of type List<SomeApplicationClass> which is the main use case.

I ended up thinking that this would be best done either using a dedicated type (BeanCollection? InstanceCollection?), or a dedicated annotation (@InjectAll?), otherwise it's too brittle.

I have a problem with those names ^. Yes, initially I was thinking about a dedicated type but that's not very convenient. I was also thinking about a special qualifier (e.g. @InjecAll List<Service> or even @Inject @All List<Service>) which would be the least evil but still not the most intuitive.

[mkouba]

Hm, I actually quite like the @Inject @All List<Service>...

[Ladicek]

The most obvious one: what happens when a bean of type List suddenly appears, when previously there was none? (Rhetorical question. I know what happens. I don't think it's good.)

But this can happen even now. You add a dependency and get a new bean for type Foo and boom...

Boom is good (especially in our case, when it fails the build). Silently changing what is injected, not so much.

Note that our CDI container is flat, so adding a dependency often means adding new beans. Also note that there are libraries that actually do provide beans of collection types...

Well, the libraries should not provide beans of type List<SomeApplicationClass> which is the main use case.

Fair point... until you consider parametric bean types :-) Here's the Infinispan example I mentioned above: https://github.com/quarkusio/quarkus/blob/2.4.1.Final/extensions/infinispan-client/runtime/src/main/java/io/quarkus/infinispan/client/runtime/InfinispanClientProducer.java#L283

Haha. Yes. It's an advanced case. Sorry, I somehow got into a mode where this kind of crap pop up into my brain almost instantly 😆

[mkouba]

The most obvious one: what happens when a bean of type List suddenly appears, when previously there was none? (Rhetorical question. I know what happens. I don't think it's good.)

But this can happen even now. You add a dependency and get a new bean for type Foo and boom...

Boom is good (especially in our case, when it fails the build). Silently changing what is injected, not so much.

It does not always fail the build. Think about javax.enterprise.inject.Instance.

Note that our CDI container is flat, so adding a dependency often means adding new beans. Also note that there are libraries that actually do provide beans of collection types...

Well, the libraries should not provide beans of type List<SomeApplicationClass> which is the main use case.

Fair point... until you consider parametric bean types :-) Here's the Infinispan example I mentioned above: https://github.com/quarkusio/quarkus/blob/2.4.1.Final/extensions/infinispan-client/runtime/src/main/java/io/quarkus/infinispan/client/runtime/InfinispanClientProducer.java#L283

Parameterized bean types are OK. What's not OK is to define a bean with bean type List<T> or Map<K,V>. I'm not sure if it's the case of the infinispan producer. Maybe they should even use @Typed(RemoteCache.class)...

Haha. Yes. It's an advanced case. Sorry, I somehow got into a mode where this kind of crap pop up into my brain almost instantly laughing

No problem ;-). I will modify the PR and introduce the @All qualifier...

[Ladicek]

The most obvious one: what happens when a bean of type List suddenly appears, when previously there was none? (Rhetorical question. I know what happens. I don't think it's good.)

But this can happen even now. You add a dependency and get a new bean for type Foo and boom...

Boom is good (especially in our case, when it fails the build). Silently changing what is injected, not so much.

It does not always fail the build. Think about javax.enterprise.inject.Instance.

Fair enough. But it would still fail at runtime with ambiguous dependency (where the Instance used to resolve to 1 bean, it would now "resolve" to 2). Or not, if the user doesn't call get but instead iterate over what the Instance has. At which point, presumably the author expects that there may be multiple beans of the type, so hard to tell.

Note that our CDI container is flat, so adding a dependency often means adding new beans. Also note that there are libraries that actually do provide beans of collection types...

Well, the libraries should not provide beans of type List<SomeApplicationClass> which is the main use case.

Fair point... until you consider parametric bean types :-) Here's the Infinispan example I mentioned above: https://github.com/quarkusio/quarkus/blob/2.4.1.Final/extensions/infinispan-client/runtime/src/main/java/io/quarkus/infinispan/client/runtime/InfinispanClientProducer.java#L283

Parameterized bean types are OK.

Parameterized (List<String>) yes. Parametric (List<T>) no. I don't have better terminology unfortunately. (JLS uses "generic" instead of "parametric", but I'm not sure if that would be an improvement.)

What's not OK is to define a bean with bean type List<T> or Map<K,V>. I'm not sure if it's the case of the infinispan producer. Maybe they should even use @Typed(RemoteCache.class)...

Agree. My point is that it is rather easy to create a bean of type List<T> without even knowing.

No problem ;-). I will modify the PR and introduce the @All qualifier...

I don't want to force you to do that, maybe other people will convince you that my objections are crap and should be ignored :-)

[stuartwdouglas]

IMHO it would be very unusual for there to be a conflict like we are talking about.

Personally I am +1 to this change, as it makes things a lot more intuitive for users.

[geoand]

Definitely +1 from me. The use of the @All qualifier makes sense to me to disambiguate things.

[mkouba]

I've just pushed the @Inject @All solution. The implementation is a bit more complex but the UX is quite nice and we're more safe. In addition, the presence of @All is good for clarity - the javadoc defines the contract.

[ebullient]

This is great! But I don't know that it will change much for me as I'm using a recorder (to fetch/iterate instances programmatically) rather than using injection. Unless @geoand can prove me wrong, he's been fixing all of my early mistakes. ;)

[geoand]

This is great! But I don't know that it will change much for me as I'm using a recorder (to fetch/iterate instances programmatically) rather than using injection. Unless @geoand can prove me wrong, he's been fixing all of my early mistakes. ;)

Which pieces do you have in mind? I'd be glad to take a look

[manovotn]

+1, this idea sounds good and adding @All is definitely a good call to distinguish it.

[ebullient]

This is great! But I don't know that it will change much for me as I'm using a recorder (to fetch/iterate instances programmatically) rather than using injection. Unless @geoand can prove me wrong, he's been fixing all of my early mistakes. ;)

Which pieces do you have in mind? I'd be glad to take a look

Brain dumping to poor @geoand in DMs. Have pity on his 🧠

[geoand]

Brain dumping to poor @geoand in DMs. Have pity on his brain

It's all good! :)

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 6e02cca

Status	Name	Step	Failures	Logs	Raw logs
✖	Native Tests - Windows - hibernate-validator	Build	Failures	Logs	Raw logs

Failures

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

[mkouba]

I think that this PR is ready for review now ;-)

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 9269735

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 17	Build	Failures	Logs	Raw logs
✖	Native Tests - Windows - hibernate-validator	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/kafka-devservices 

📦 integration-tests/kafka-devservices

✖ io.quarkus.it.kafka.KafkaAdminTest.health - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

---

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

[Ladicek]

Can't say I really understand it, but it seems fine :-)

[gastaldi]

Good idea!

[gsmet]

This is too big to be backported post CR1.

[mkouba]

This is too big to be backported post CR1.

No problem.