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

@Priority(LOWEST_PRECEDENCE) still higher than no priority #31544

Closed
FyiurAmron opened this issue Nov 2, 2023 · 11 comments
Closed

@Priority(LOWEST_PRECEDENCE) still higher than no priority #31544

FyiurAmron opened this issue Nov 2, 2023 · 11 comments
Labels
status: invalid An issue that we don't feel is valid

Comments

@FyiurAmron
Copy link

FyiurAmron commented Nov 2, 2023

Affects: all Spring Framework versions (spring-beans module)


@Priority(LOWEST_PRECEDENCE) is still higher than no priority - which makes it impossible to lower the priority of selected @Component via this annotation.

(see #26241 for a similar issue, although this one proposes a simpler solution for a particular subcase)

Let's assume this:

@Component
@Priority(HIGHEST_PRECEDENCE)
class ComponentHighestPrecedence extends SomeComponent {}

@Component
@Priority(LOWEST_PRECEDENCE)
class ComponentLowestPrecedence extends SomeComponent {}

@Component
class ComponentNoExplicitPrecedence extends SomeComponent {}

In this case, ComponentHighestPrecedence will be always selected, as expected. If we remove it, however, ComponentLowestPrecedence will be always selected, due to how https://github.com/spring-projects/spring-framework/blob/main/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java#L1787 is currently implemented.

Although that's probably by design, there is a much bigger issue here: there is no way to say that a component should have priority lower than or equal to "no explicit priority". LOWEST_PRECEDENCE is thus a misnomer in this case, it's "lower than any explicit but higher than all implicit".

The use case when this is problematic is very simple: let's say we create a library that provides a bean that needs to be used only if the library consumer doesn't provide his own implementation. The workaround is to force the consumer to use @Primary, but that forces maintenance on the consumer. OTOH, if we would have a default reasonable priority value for components with no @Priority, we could just declare @Priority(DEFAULT_PRECEDENCE - 1) on the library component, and be done with it.

Changing this is obviously a breaking change, but maybe there is some reasonable way to provide this as a configuration option somewhere, that would make e.g. getPriority return 0 (or even better, some DEFAULT_PRECEDENCE value) instead of null for the beans that have no priority given explicitly?

@snicoll
Copy link
Member

snicoll commented Nov 2, 2023

Although that's probably by design

Yes.

LOWEST_PRECEDENCE is thus a misnomer in this case, it's "lower than any explicit but higher than all implicit".

I disagree. Not having a priority (or an @Order) at all means that you don't express any preference. The Javadoc for OrderComparator states:

Any object that does not provide its own order value is implicitly assigned a value of Ordered.LOWEST_PRECEDENCE, thus ending up at the end of a sorted collection in arbitrary order with respect to other objects with the same order value.

If you implement Ordered then you must provide a default value. You can even do so with a default method whose value is 0 if you feel like it's a good default. Unfortunately, we can't change the default behavior of a component that doesn't express a priority.

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2023
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: declined A suggestion or change that we don't feel we should currently apply labels Nov 2, 2023
@FyiurAmron
Copy link
Author

FyiurAmron commented Nov 2, 2023

@snicoll

Any object that does not provide its own order value is implicitly assigned a value of Ordered.LOWEST_PRECEDENCE, thus ending up at the end of a sorted collection in arbitrary order with respect to other objects with the same order value.

but this is not the case here! - if that was the case, there would be an exception thrown here due to two beans having Ordered.LOWEST_PRECEDENCE (one explicit, one implicit) - instead, the component has precedence lower than LOWEST_PRECEDENCE, which is contrary to what you wrote and what's in the doc you linked to (putting aside it's a misnomer). If having no @Priority would be equal to LOWEST_PRECEDENCE, that would be understandable here - but, as I wrote, this is not the case, it's actually lower due to current implementation.

@snicoll
Copy link
Member

snicoll commented Nov 2, 2023

Please review the doc once again.

thus ending up at the end of a sorted collection in arbitrary order with respect to other objects with the same order value.

You can have as many component with the same priority value as you want. If they have the same value, the order is arbitrary. There's no after or before as you claim.

@FyiurAmron
Copy link
Author

FyiurAmron commented Nov 2, 2023

You can have as many component with the same priority value as you want.

@snicoll with the same @Order, yes. Not with the same @Priority:

https://github.com/spring-projects/spring-framework/blob/main/spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java#L1791

						if (candidatePriority.equals(highestPriority)) {
							throw new NoUniqueBeanDefinitionException(requiredType, candidates.size(),
									"Multiple beans found with the same priority ('" + highestPriority +
									"') among candidates: " + candidates.keySet());
						}

I'm under the impression you're mistaking @Order-handling code with @Priority-handling code.

@FyiurAmron
Copy link
Author

FyiurAmron commented Nov 2, 2023

@snicoll even the docs say that it's not what you say:

	 * @return the priority assigned to that bean or {@code null} if none is set
	 */
	@Nullable
	protected Integer getPriority(Object beanInstance) {

Point here, it's not that "If you implement Ordered then you must provide a default value." - there is no Ordered implementation here, it's about how the default/implicit priority values are handled - i.e. null is returned from getPriority(), component is ignored in comparison and thus always ends at below LOWEST_PRECEDENCE.

@snicoll
Copy link
Member

snicoll commented Nov 2, 2023

That's just an internal detail of how this is implemented and we're mixing the two indeed but I am trying to explain why this is the expected behavior. Your third component does not have a priority so it passes the check above. At the end of the day, for the ordering purpose, a component that does not have a priority (order) comes "at the end". From an ordering perspective, it means assigning it LOWEST_PRECEDENCE and then the order of the components that have the same order value is arbitrary in the sense that you can't expect one component with the same order to be invoked before another one.

If having no @priority would be equal to LOWEST_PRECEDENCE, that would be understandable

Behind the scenes, that's exactly what's happening.

Looking at the spec not much is said about what is happening if a component does not have a value but it's totally logical to me that it as a lower priority than the lowest priority that is defined explicitly.

@FyiurAmron
Copy link
Author

FyiurAmron commented Nov 2, 2023

@snicoll TBH, for me it would be logical if the term "lowest priority" would imply that a thing would have lowest priority of all, and not of just a subset of cases, but I'm not at liberty to argue about logics here. Thank you for your prompt response; if it's by design, then it's by design.

@snicoll
Copy link
Member

snicoll commented Nov 2, 2023

LOWEST_PRECEDENCE is a concept of @Order and has nothing to do with JakartaEE's priority. It just happens that both annotations accept an integer as the value of the order or the priority, respectively. I don't really understand why you're arguing about the semantic of a separate concept.

I believe the current design is consistent. If you don't have an order, then you're invoked after every component that have one. Same for priority.

@FyiurAmron
Copy link
Author

FyiurAmron commented Nov 2, 2023

@snicoll I could just provide Integer.MIN_VALUE as a reference, the problem would be exactly the same - no way to de-prioritize by the use of the annotation. That's exactly why the call for @Secondary etc. appeared in the first place IMVHO.

I don't really understand why you're arguing about the semantic of a separate concept.

It's not really that separate, especially taking into account things like https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/core/PriorityOrdered.html etc. , and taking into account that the implementation of priority-related methods repeatedly talk about ordering, and use methods called as such.

E.g.

	protected Integer getPriority(Object beanInstance) {
		Comparator<Object> comparator = getDependencyComparator();
		if (comparator instanceof OrderComparator orderComparator) {
			return orderComparator.getPriority(beanInstance);
		}
		return null;
	}

and from OrderComparator:

Any object that does not provide its own order value is implicitly assigned a value of Ordered.LOWEST_PRECEDENCE, thus ending up at the end of a sorted collection in arbitrary order with respect to other objects with the same order value.

So yes, I'd say both the docs and the implmentations explicitly link the concepts.

@FyiurAmron
Copy link
Author

FyiurAmron commented Nov 2, 2023

@snicoll this is also from the docs themselves (AnnotationAwareOrderComparator):

This implementation retrieves an @javax.annotation.Priority value, allowing for additional semantics over the regular @Order annotation: typically, selecting one object over another in case of multiple matches but only one object to be returned.

I'm thus, saying "@Order (...) has nothing to do with javax/JakartaEE's priority" is either a blatant lie or wishful thinking; if something is described as "allowing for additional semantics" over something, then it has everything to do with it.

It just happens that both annotations accept an integer as the value of the order or the priority, respectively.

See above. It's mentioned multiple times over the docs that it's not "a conicidence" they both use integer values; they were intended to be semantically related. I think I provided enough source reference for that.

I don't really understand why you're arguing about the semantic of a separate concept.

If that's a "separate concept", you should rewrite all the docs and code you have in Spring - because, as currently stands, it's not separate by any means. They are mentioned in tandem virtually everywhere, the code concerning both overlaps, and the use cases as well - resulting in long-lived confusion; search the internet for how many people believe that @Order affects autowiring priority, if you're willing to argue about being "logically unrelated" further.

@FyiurAmron
Copy link
Author

@jhoeller & @sbrannen are the people who changed the docs to reflect that, are you guys still around and can you chime in on whether @Order and @Priority are really "completely unrelated", or not?

see e.g. 1ff3af6#diff-ec91a2069056e7887259843f7f76c280b13d5737e25f9e019bde45e927394d2eR67 , c6d29f1#diff-7647ef528e5ffcfdb8ce8b88d189a906a1e7b46778323bc92cb9783a4991d5abR1225 etc.

@spring-projects spring-projects locked as too heated and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

3 participants