Skip to content
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

CDI instance injection ordering #478

Closed
starksm64 opened this issue May 18, 2021 · 17 comments
Closed

CDI instance injection ordering #478

starksm64 opened this issue May 18, 2021 · 17 comments

Comments

@starksm64
Copy link
Contributor

Linking to this previous

https://issues.redhat.com/browse/CDI-535

Description
We should allow ordering of bean instance injection using the Instance when an instance injection is used.

@manovotn
Copy link
Contributor

@starksm64 since you brought it up again, do you have any specific use case for this? Just wondering.

BTW I think Weld has something similar implemented as part of WeldInstance.
See https://github.com/weld/api/blob/master/weld/src/main/java/org/jboss/weld/inject/WeldInstance.java#L65-L108

@starksm64
Copy link
Contributor Author

It came up on the Quarkus developer list regarding a problem with ordering of a security provider instance.

@manovotn
Copy link
Contributor

Is this issue still relevant even after introduction of the Handle interface via #521?
If so, what functionality is missing?

@Ladicek
Copy link
Contributor

Ladicek commented Oct 12, 2021

It seems to me that indeed the Handle changes are sufficient to satisfy all use cases for this. Having a priority comparator would be nice, but the Weld one is somewhat weird (in the treatment of beans without priority) and in general, the less implementation code in the API, the better.

So I'd be +1 to closing this.

@manovotn
Copy link
Contributor

I agree. However I wanted to hear from @starksm64 since he created the issue and maybe we're missing something :)

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2022

I stumbled upon this again in the context of smallrye/smallrye-reactive-messaging#1856, where they are adding their own notion of [message] interceptors and want them ordered according to priority (ideally in the same way CDI orders interceptors according to priority :-) ).

I mentioned above that an Instance.Handle should allow everyone to implement their own comparator, but I think I was actually wrong about that -- Bean doesn't expose the priority value. Checking if the Bean class implements Prioritized seems rather pointless, because that may be true only for custom Bean implementations. It's never true for managed beans (or producers, assuming we had #556). Even Bean instances for synthetic beans registered via BeanConfigurator don't implement Prioritized (or at least don't have to, and don't in practice in Weld).

Am I missing something?

@manovotn
Copy link
Contributor

manovotn commented Sep 1, 2022

Yes, due to various ways in which you can define beans getting priority is no easy feat.
Plus you can even still define an alternative without any @Priority and not implementing Prioritized and still enable it via beans.xml. The latter makes it very difficult (impossible?) to tell if a bean is even enabled by just looking at the metatada.

@mkouba
Copy link
Contributor

mkouba commented Sep 1, 2022

Having a priority comparator would be nice, but the Weld one is somewhat weird (in the treatment of beans without priority) and in general, the less implementation code in the API, the better.

WeldInstance.getPriorityComparator() was the only option to make it possible and usable (to some extent).

FTR ArC has the io.quarkus.arc.InjectableBean#getPriority() method and honors the priority during programmatic lookup (@Inject Instance<Foo>) and when injecting a list of beans (e.g. @Inject @All List<Foo>).

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2022

So I'm mostly thinking about ordering, which was the original purpose of this issue. Enablement is a different kind of thing and not enabled beans don't even make it to Instance<T>, do they?

Iteration order of Instance is unspecified at the moment, so we could do either of these:

  1. specify that iteration order is from lowest priority value to highest priority value;
  2. add an additional method Instance.inPriorityOrder() that would return "the same" Instance, just with a well defined iteration order;
  3. add some means of obtaining a priority-based comparator for Bean<?> or Instance.Handle<?> (Weld provides one on WeldInstance, which doesn't seem like the best place, but I can't think of a better one?);
  4. anything else?

In all cases, an additional question is how to order beans that have no priority value. (The comparator in Weld currently treats them as priority value of 0. ArC's InjectableBean.getPriority() also returns 0 in such case. I'm personally not sure.)

@mkouba
Copy link
Contributor

mkouba commented Sep 1, 2022

In all cases, an additional question is how to order beans that have no priority value. (The comparator in Weld currently treats them as priority value of 0. ArC's InjectableBean.getPriority() also returns 0 in such case. I'm personally not sure.)

Zero is fine if higher priority goes first which is IMO more intuitive (and both WeldInstance.getHandlePriorityComparator() and ArC follow this rule).

@manovotn
Copy link
Contributor

manovotn commented Sep 1, 2022

Enablement is a different kind of thing and not enabled beans don't even make it to Instance, do they?

I only pointed it out because you can effectively get enabled alternative without any priority attached to it and it is still valid.

specify that iteration order is from lowest priority value to highest priority value;

Why not highest to lowest? Then you can keep zero for enabled beans without numeric value on priority.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2022

If CDI has to process multiple things in priority order (interceptors, decorators, is there anything else?), lower priority values always come before higher priority values.

Indeed Weld and ArC sort from higher to lower, which is IMHO unfortunate.

@mkouba
Copy link
Contributor

mkouba commented Sep 1, 2022

If CDI has to process multiple things in priority order (interceptors, decorators, is there anything else?), lower priority values always come before higher priority values.

For alternatives and ambiguous names the beans with the highest priority values are taken. Interceptors with lower priority values are called first which effectively means that interceptors with higher priority values are "closer" to the the target method invocation and thus can affect the "context", e.g. change the call arguments and override the arguments set by interceptors with lower priority values. So the semantics is IMO a bit disputable.

Indeed Weld and ArC sort from higher to lower, which is IMHO unfortunate.

I disagree.

@manovotn
Copy link
Contributor

manovotn commented Sep 1, 2022

If CDI has to process multiple things in priority order (interceptors, decorators, is there anything else?), lower priority values always come before higher priority values.

Indeed Weld and ArC sort from higher to lower, which is IMHO unfortunate.

What's unfortunate about that?
With alternatives, CDI spec dictates that that higher priority wins. Therefore, it IMO makes sense to do the sorting higher to lower or, in other words, starting by what would CDI pick as a winner during typesafe resolution.

For alternatives and ambiguous names the beans with the highest priority values are taken. Interceptors with lower priority values are called first which effectively means that interceptors with higher priority values are "closer" to the the target method invocation and thus can affect the "context", e.g. change the call arguments and override the arguments set by interceptors with lower priority values. So the semantics is IMO a bit disputable.

+1

@Ladicek
Copy link
Contributor

Ladicek commented Sep 1, 2022

Right. There are basically 2 contexts where priority matters:

  1. There's multiple things and you need to pick one. In that case, CDI picks the one with highest priority value.
  2. There's multiple things and you need to order them. In that case, CDI orders from lowest priority value to highest priority value. In case there's something being "wrapped", this indeed means that highest priority value means closest to the "wrapped" thing.

I take it you argue that iterating over alternatives in priority order is different from applying interceptors/decorators because there's nothing being "wrapped", but I just see an unnecessary inconsistency and confusion.

@mkouba
Copy link
Contributor

mkouba commented Sep 1, 2022

In case there's something being "wrapped", this indeed means that highest priority value means closest to the "wrapped" thing.

But you don't need to wrap anything in case of injecting multiple beans. It's a different use case.

@manovotn
Copy link
Contributor

manovotn commented Sep 1, 2022

There's multiple things and you need to order them. In that case, CDI orders from lowest priority value to highest priority value.

Not really, you actually do both based on whether you are looking at the ordering prior to "wrapped" method invocation or post the invocation.
For instance, with interceptors, you basically do:
lowest-to-highest -> methodInvocation -> highest-to-lowest

I take it you argue that iterating over alternatives in priority order is different from applying interceptors/decorators

Actually the opposite, I think it is very similar in that the highest priority has the most "power" in terms of controlling/affecting the actual "wrapped" method based on being invoked immediately before/after the method itself.

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

No branches or pull requests

4 participants