jakartaee / cdi

CDI specification
Apache License 2.0
205 stars 78 forks source link

Instance iterator and stream ordering #824

Open rmanibus opened 2 weeks ago

rmanibus commented 2 weeks ago

Is your feature request related to a problem? Please describe.

The current ordering of both iterator and stream of an Instance is undefined in the CDI specification.

Use case: an extension is gathering all existing instances to apply a side effect (for example to build a message transformation chain), and the ordering matter.

Describe the solution you'd like A clear and concise description of what you want to happen.

The specification could make sure that the Priority order is respected when iterating over instances

Ladicek commented 1 week ago

The first question is: what priority order should we use? The CDI specification generally orders things in the ascending manner (interceptors, decorators, observers), but there is at least one existing implementation (ahem... :-) ) that already provides an ordering guarantee for Instance, except it's descending.

manovotn commented 1 week ago

The first question is: what priority order should we use? The CDI specification generally orders things in the ascending manner (interceptors, decorators, observers), but there is at least one existing implementation (ahem... :-) ) that already provides an ordering guarantee for Instance, except it's descending.

I think Weld does that as well, so make it two :)

The specification could make sure that the Priority order is respected when iterating over instances

We would also need to define behavior for beans that have no priority which can be majority of them - so either say those are undefined or enforce some secondary level sorting.

rmanibus commented 1 week ago

is there a reason why they both choose the descending order ? If there is one, it might be good context to ground a decision here !

Also admitting it is decided to enforce a secondary level sorting based on let's say the bean name in alphabetical order, then people would just start naming their bean to get them in the right order and not use priority at all. My gut feeling is that if no priority is defined, the behavior should stay undefined.

manovotn commented 1 week ago

is there a reason why they both choose the descending order ? If there is one, it might be good context to ground a decision here !

I think it just follows the way you treat @Alternative in CDI - the highest priority is chosen. So in a similar way you order the instances to have the highest priority first.

My gut feeling is that if no priority is defined, the behavior should stay undefined.

FTR, the way Weld does that is to state that all beans without explicit priority have it set to 0.

Ladicek commented 1 week ago

I think Weld does that as well, so make it two :)

Ah, totally forgot. Thanks!

We would also need to define behavior for beans that have no priority which can be majority of them - so either say those are undefined or enforce some secondary level sorting.

Yeah, good point. Defaulting to 0 seems reasonable.

mkouba commented 1 week ago

FTR there was https://issues.redhat.com/browse/CDI-535.

We would also need to define behavior for beans that have no priority which can be majority of them - so either say those are undefined or enforce some secondary level sorting.

Yeah, good point. Defaulting to 0 seems reasonable.

Both ArC and Weld IMO use this default.

Also admitting it is decided to enforce a secondary level sorting based on let's say the bean name in alphabetical order, then people would just start naming their bean to get them in the right order and not use priority at all. My gut feeling is that if no priority is defined, the behavior should stay undefined.

@rmanibus I'm not quite sure I understand this paragraph. In any case, given the fact that the ordering is currently undefined the users should not rely on any ordering at all (unless using a specific implementation). In other words, it's not a breaking change.

Azquelt commented 1 week ago

That issue does raise a point that I was thinking about: in some circumstances @Priority might already have a meaning when applied to a class.

For example, it applies a priority to an alternative producer method defined in that class. (It also sets the priority of an interceptor, but I don't see that overlapping).

I guess @Priority having overlapping meanings when applied to a class isn't necessarily the worst thing, but we should probably at least consider what other Jakarta EE specs use it for.

I also had a think about how a user might accomplish the task of declaring an order on their beans without adding anything to the specification:

manovotn commented 1 week ago

For example, it applies a priority to an alternative producer method defined in that class. (It also sets the priority of an interceptor, but I don't see that overlapping).

In 4.1 we allowed @Priority declaration directly on producers, so this can be workarounded. I see your point though. Not much we can do about that.

I guess @Priority having overlapping meanings when applied to a class isn't necessarily the worst thing, but we should probably at least consider what other Jakarta EE specs use it for.

Fair enough, we should look around.

I also had a think about how a user might accomplish the task of declaring an order on their beans without adding anything to the specification:

Similarly to Weld, you could have a method in Instance that provides a priotity based comparator that users can apply to the stream. I'd rather have the ordering defined directly in the spec though. As Martin pointed out, there is no ordering defined at the moment so we wouldn't be breaking anything.

using Handle to get the bean metadata and ordering based on a qualifier or some other information from there

If you consume the stream, you can apply any custom filtering (obviously with metadata accessible from the Bean itself).

adding a method to the beans which returns a priority and sorting the instances using that

This sounds familiar; I think we discussed this in the past and decided against it. That'd imply that all beans have some non-null priority. I am not sure what the full scope of that discussion was but first thing coming to mind is basically the example you listed earlier - if all beans have some default priority, that means any alternative producers declared on them would have it too hence enabling them.

Azquelt commented 1 week ago

adding a method to the beans which returns a priority and sorting the instances using that

This sounds familiar; I think we discussed this in the past and decided against it. That'd imply that all beans have some non-null priority. I am not sure what the full scope of that discussion was but first thing coming to mind is basically the example you listed earlier - if all beans have some default priority, that means any alternative producers declared on them would have it too hence enabling them.

Sorry, I didn't word that suggestion very clearly.

The use case put forward was that the user is gathering all existing instances (presumably of a particular interface) and processing them in order. I'm suggesting the user adds a getPriority or getIndex method to the interface and uses that to order the instances before processing them.

mkouba commented 1 week ago

adding a method to the beans which returns a priority and sorting the instances using that

This sounds familiar; I think we discussed this in the past and decided against it. That'd imply that all beans have some non-null priority. I am not sure what the full scope of that discussion was but first thing coming to mind is basically the example you listed earlier - if all beans have some default priority, that means any alternative producers declared on them would have it too hence enabling them.

Sorry, I didn't word that suggestion very clearly.

The use case put forward was that the user is gathering all existing instances (presumably of a particular interface) and processing them in order. I'm suggesting the user adds a getPriority or getIndex method to the interface and uses that to order the instances before processing them.

Yes, but that's the downside of this approach - you need to instantiate all beans before the ordering happens which is not always practical.

manovotn commented 1 week ago

Right, I see. That would work too but like Martin says, it can be very inefficient and doesn't work for arbitrary beans. For that purpose, having a priority based comparator on the Instance interface would IMO work better.