quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

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

Open mkouba opened this issue 1 year ago • 2 comments

Description

  • https://quarkus.io/guides/cdi-integration#synthetic-injection-points
  • https://quarkus.io/guides/cdi-reference#injecting-multiple-bean-instances-intuitively

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

Implementation ideas

No response

mkouba avatar Aug 28 '24 12:08 mkouba

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] avatar Aug 28 '24 12:08 quarkus-bot[bot]

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

quarkus-bot[bot] avatar Aug 28 '24 12:08 quarkus-bot[bot]

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));

melloware avatar Aug 29 '24 13:08 melloware

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).

mkouba avatar Aug 29 '24 14:08 mkouba

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

melloware avatar Aug 29 '24 14:08 melloware

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 avatar Aug 29 '24 15:08 rmanibus

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

melloware avatar Aug 29 '24 15:08 melloware

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.

mkouba avatar Aug 30 '24 05:08 mkouba