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

"Ordinal" versus "Priority" is confusing #553

Open
dmlloyd opened this issue Apr 7, 2020 · 9 comments
Open

"Ordinal" versus "Priority" is confusing #553

dmlloyd opened this issue Apr 7, 2020 · 9 comments
Labels
bug 🪲 An error in the implementation code or documentation

Comments

@dmlloyd
Copy link
Contributor

dmlloyd commented Apr 7, 2020

Describe the bug

For ConfigSource we talk about "ordinal", which is a word that means "relating to position in a series", for example (first, second, third, etc.). For other things we use "priority" (and the @Priority annotation). This is very confusing because when we talk about ordinals in language, the thing that is first is first, but we're now treating ConfigSource ordinality the reverse way: higher (or later) ordinals take effect over lower (or earlier) ordinals.

We should change all specification language referring to "ordinal" to say "priority" instead. We should also look into using @Priority for ConfigSource by default (possibly as a follow-up issue).

@dmlloyd dmlloyd added the bug 🪲 An error in the implementation code or documentation label Apr 7, 2020
@radcortez
Copy link
Contributor

The pain point here will be the ConfigSource#getOrdinal method.

@OndroMih
Copy link
Contributor

OndroMih commented May 3, 2020

We could add a new default method ConfigSource#getPriority which will call ConfigSource#getOrdinal. getOrdinal is already implemented as default and thus optional.

Old config sources would work and new config sources can just define getPriority.

@tomas-langer
Copy link

I am not sure I follow - Priority works (at least in most meanings) as "the lower number wins" - hence the term "Number one priority" (see https://javaee.github.io/javaee-spec/javadocs/javax/interceptor/Interceptor.Priority.html as an example).
So how would replacing the ordinal with priority fix this issue?
The specification is inverse to usual understanding of priority and ordinal. So unless this is changed (which would be a big backward incompatible change), it does not really matter if you call it ordinal or priority.

@radcortez
Copy link
Contributor

One of the issues is that we use ordinal for ConfigSource but priority for Converter, which is confusing in my opinion. The proposal was also to make the spec consistent.

@ljnelson
Copy link
Contributor

But see also: "highest priority". Either interpretation works in English, which is of course the main problem: which interpretation is more primal? Sadly, either, depending on usage.

"Ordinal", in its mathematical sense, is unambiguous: smallest number wins ("first" trumps "second"). Sadly, we've apparently redefined it in this specification. Regardless of that, the real question is: is the audience of MicroProfile Config users and MicroProfile Config-compatible configuration document authors aware of its unambiguous meaning? I'm guessing no, i.e. no matter its definition it is sufficiently technical that "ordinary people" would have to look it up. After all, MicroProfile Config itself has redefined it! So maybe it's a poor choice.

To sum up: if we want something truly unambiguous that means "largest number wins" we need a new unambiguous term IMHO.

So that I'm saying something constructive: other terms I can think of in this general ballpark that I've seen in other projects include "index", "rank" (HK2), "position"…but sadly these all mean "smallest number wins"!

The only one I can think of right now that unambiguously means "largest number wins" is "weight". I'm sure there are others; maybe sleep on it and see if another one comes up. My two cents.

@radcortez
Copy link
Contributor

Yes that is another source of confusion, because ordinal means a different thing than what we use in the specification.

There are 2 issues with this:

  • The literal meaning of ordinal redefined by MP Config
  • Inconsistency between Ordinal and Priority in ConfigSource and Converter

We can define the ordinal in a ConfigSource by overriding getOrdinal or setting a config_ordinal property, but we cannot use an annotation to do it. There is no @Ordinal annotation. On Converter you are able to use @Priority to set the priority and that is it.

For me this is a poor developer experience, we only have 3 interfaces that a consumer can implement. In two of these interfaces, we want to provide a way to order the implementations, and we use different terms / API's to provide the ordering rules.

It would be interesting if we could discuss to make these more consistent.

@tomas-langer
Copy link

tomas-langer commented Jun 30, 2020

Agreed, the usage of @Priority to define ordinal is unusual, as is the combination of approaches.
Would it be possible to just switch to priority as the term and also modify the property used (with some backward compatibility...)?

Such as:

  • @Priority used on any service loader service defines its relative priority to other services (can be MP wide)
  • config_priority key defines a config source priority

The only thing that must be decided (and consistently across whole of MP) is whether Priority is "higher wins" or "lower wins".

@radcortez
Copy link
Contributor

Usually, with priority, the lower number wins.

From the Jakarta Interceptors Spec:

Priorities that define the order in which interceptors are invoked. These values are intended to be used with the Priority annotation for interceptors that are defined by means of interceptor binding.

Interceptors with smaller priority values are called first. If more than one interceptor has the same priority, the relative order of those interceptors is undefined.

From the CDI spec:

getAlternatives() returns the ordered list of enabled alternatives for the application, sorted by priority in ascending order. Alternatives enabled for a bean archive are not included in the list.

getInterceptors() returns the ordered list of enabled interceptors for the application, sorted by priority in ascending order. Interceptors enabled for a bean archive are not included in the list.

getDecorators() returns the ordered list of enabled decorators for the application, sorted by priority in ascending order. Decorators enabled for a bean archive are not included in the list.

From JAX-RS spec:

The order in which filters and interceptors are executed as part of their corresponding chains is controlled by the @priority annotation defined in [12]. Priorities are represented by integer numbers. Execution chains for extension points ContainerRequest, PreMatchContainerRequest, ClientRequest, ReadFrom and WriteTo are sorted in ascending order; the lower the number the higher the priority. Execution chains for extension points ContainerResponse and ClientResponse are sorted in descending order; the higher the number the higher the priority. These rules ensure that response filters are executed in reversed order of request filters.

From our Config Converters spec:

A custom Converter can define a priority with the @javax.annotation.Priority annotation.
If a Priority annotation isn't applied, a default priority of 100 is assumed.
The Config will use the Converter with the highest Priority for each target type.

Which is not very clear about the behaviour. Looking into the TCK:

@Test
public void testDonaldConversionWithMultipleLambdaConverters() {
// defines 2 config with the lambda converters defined in different orders.
// Order must not matter, the lambda with the upper case must always be used as it has the highest priority
Config config1 = ConfigProviderResolver.instance().getBuilder().addDefaultSources()
.withConverter(Donald.class, 101, (s) -> Donald.iLikeDonald(s.toUpperCase()))
.withConverter(Donald.class, 100, (s) -> Donald.iLikeDonald(s))
.build();
Config config2 = ConfigProviderResolver.instance().getBuilder().addDefaultSources()
.withConverter(Donald.class, 100, (s) -> Donald.iLikeDonald(s))
.withConverter(Donald.class, 101, (s) -> Donald.iLikeDonald(s.toUpperCase()))
.build();
Donald donald = config1.getConverter(Donald.class).get().convert("Duck");
Assert.assertNotNull(donald);
Assert.assertEquals(donald.getName(), "DUCK",
"The converter with the highest priority (using upper case) must be used.");
donald = config2.getConverter(Donald.class).get().convert("Duck");
Assert.assertNotNull(donald);
Assert.assertEquals(donald.getName(), "DUCK",
"The converter with the highest priority (using upper case) must be used.");
}

Now we see that the priority for Converter is sorted by descending order.

Generally, Jakarta specs follow the rule smaller priority values is called first. Doing that change for MP Config will be a huge backwards incompatible change, so I'm not sure if we should do it, but maybe we need to consider it.

Considering that we already redefine how priority behaves with Converter (higher priority value is called first), which is the same behaviour we have with ConfigSource and ordinal, I think we could just rename ordinal to priority and at least keep the naming consistent.

@Emily-Jiang
Copy link
Member

The above statement is only about invocation order not about which wins. In CDI spec:
https://jakarta.ee/specifications/cdi/3.0/jakarta-cdi-spec-3.0.html#unsatisfied_and_ambig_dependencies

All the beans left are alternatives with a priority, or producer methods or fields of beans that are alternatives with a priority, then the container will determine the highest priority value, and eliminate all beans, except for alternatives with the highest priority and producer methods and fields of alternatives with the highest priority value. If there is exactly one bean remaining, the container will select this bean, and the ambiguous dependency is called resolvable.

It clearly specifies the highest priority wins, which is the behaviour we are having here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 An error in the implementation code or documentation
Projects
None yet
Development

No branches or pull requests

6 participants