quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.56k stars 2.63k forks source link

ArC: synthetic injection points should support `@Inject @All List<>` #42834

Closed mkouba closed 5 days ago

mkouba commented 2 weeks ago

Description

Zulip discussion: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/.E2.9C.94.20Synthetic.20vs.20CDI.20Beans.20in.20Extension

Implementation ideas

No response

quarkus-bot[bot] commented 2 weeks ago

You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip

This message is automatically generated by a bot.

quarkus-bot[bot] commented 2 weeks ago

/cc @Ladicek (arc), @manovotn (arc)

melloware commented 2 weeks ago

Not sure if this is related but I opened this ticket in Temporal:

https://github.com/quarkiverse/quarkus-temporal/issues/88

We want to make sure @Priority is respected and this unit test is failing showing @Priority is not respected.

https://github.com/quarkiverse/quarkus-temporal/pull/90

However it could be because we are doing our Instance like this.

.addInjectionPoint(ParameterizedType.create(Instance.class, ClassType.create(WorkflowClientInterceptor.class)),
                                AnnotationInstance.builder(Any.class).build())

When we get that in our Recorder with the following they are definitely not in priority order.

// discover interceptors
        Instance<WorkflowClientInterceptor> interceptorInstance = context.getInjectedReference(new TypeLiteral<>() {
        }, Any.Literal.INSTANCE);

        List<WorkflowClientInterceptor> interceptors = interceptorInstance.stream()
                .collect(Collectors.toCollection(ArrayList::new));
mkouba commented 2 weeks ago

We want to make sure @Priority is respected and this unit test is failing showing @Priority is not respected.

The priority should be respected (although it's a non-standard feature). Higher priority goes first. So if I understand your unit test correctly the LowerPriorityWorkflowClientInterceptor should be first because it has @Priority(99) and TestWorkflowClientInterceptor should be second because it's annotated with @Priority(1).

melloware commented 2 weeks ago

Oh let me switch it I thought lower was higher priority I will get back to you.

rmanibus commented 2 weeks ago

yeah I just double checked the CDI spec does not say anything about stream() and iterator() ordering. If quarkus is ordering beans returned by those methods respecting the priority, I think this should be clarified in the doc. For now the doc in this section only mention ordering of the @Inject @All List<>

melloware commented 2 weeks ago

OK i flipped them and verified the test is working. My bad!

mkouba commented 2 weeks ago

yeah I just double checked the CDI spec does not say anything about stream() and iterator() ordering. If quarkus is ordering beans returned by those methods respecting the priority, I think this should be clarified in the doc. For now the doc in this section only mention ordering of the @Inject @All List<>

@rmanibus Yeah, it's mentioned in the javadoc of io.quarkus.arc.InjectableBean.getPriority(). But you're right that it would not hurt to document this behavior for Instance<> as well. Feel free to send a PR/create a new issue ;-)

OK i flipped them and verified the test is working. My bad!

@melloware No problem at all.