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

Instance iterator and stream ordering #824

Open
rmanibus opened this issue Aug 30, 2024 · 11 comments
Open

Instance iterator and stream ordering #824

rmanibus opened this issue Aug 30, 2024 · 11 comments

Comments

@rmanibus
Copy link

rmanibus commented Aug 30, 2024

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
Copy link
Contributor

Ladicek commented Sep 2, 2024

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
Copy link
Contributor

manovotn commented Sep 2, 2024

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
Copy link
Author

rmanibus commented Sep 2, 2024

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
Copy link
Contributor

manovotn commented Sep 2, 2024

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
Copy link
Contributor

Ladicek commented Sep 2, 2024

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
Copy link
Contributor

mkouba commented Sep 2, 2024

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
Copy link
Contributor

Azquelt commented Sep 2, 2024

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:

  • using Handle to get the bean metadata and ordering based on a qualifier or some other information from there
  • adding a method to the beans which returns a priority and sorting the instances using that

@manovotn
Copy link
Contributor

manovotn commented Sep 2, 2024

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
Copy link
Contributor

Azquelt commented Sep 2, 2024

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
Copy link
Contributor

mkouba commented Sep 3, 2024

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
Copy link
Contributor

manovotn commented Sep 3, 2024

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.

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

5 participants