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

Taking into account the @Priority annotation when registering context providers #32942

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented Apr 27, 2023

At the moment, users can register multiple context providers for handing the same class by using:

Client client = QuarkusRestClientBuilder.newBuilder()
                    .baseUri(URI.create(uri))
                    .register(MyLowPriorityContextProvider.class)
                    .register(MyHighPriorityContextProvider.class)
                    .build(Client.class);

When doing this, REST Client Reactive will always return the first registered context provider for the matching type:

@Priority(2)
    public static class MyLowPriorityContextProvider implements ContextResolver<Person> {

        @Override
        public Person getContext(Class<?> aClass) {
// ...
        }
    }

    @Priority(1)
    public static class MyHighPriorityContextProvider implements ContextResolver<Person> {

        @Override
        public Person getContext(Class<?> aClass) {
// ...
        }
    }

With these changes, the @Priority annotation is taking into account to return the correct context provider regardless to when it has been registered.

@Sgitario Sgitario requested a review from geoand April 27, 2023 08:55
@Sgitario Sgitario changed the title Taking into account the @Priority annotation in context providers Taking into account the @Priority annotation when registering context providers Apr 27, 2023
@geoand
Copy link
Contributor

geoand commented Apr 27, 2023

Are you sure that the a larger number means higher priority? In JAX-RS, it's usually the other way around (unless we are talking about @Provider classes on the write path).

Perhaps we should make a check with quarkus-rest-client and see how it behaves?

@Sgitario
Copy link
Contributor Author

Are you sure that the a larger number means higher priority? In JAX-RS, it's usually the other way around (unless we are talking about @Provider classes on the write path).

Perhaps we should make a check with quarkus-rest-client and see how it behaves?

You're right! Thanks for spotting.

@geoand
Copy link
Contributor

geoand commented Apr 27, 2023

🙏🏼

@Sgitario Sgitario force-pushed the context_provider_priority branch from 4da3de9 to b73870b Compare April 27, 2023 09:15
@Sgitario
Copy link
Contributor Author

PR updated

@geoand geoand added triage/backport? triage/waiting-for-ci Ready to merge when CI successfully finishes labels Apr 27, 2023
@quarkus-bot

This comment has been minimized.

At the moment, users can register multiple context providers for handing the same class by using: 

```java
Client client = QuarkusRestClientBuilder.newBuilder()
                    .baseUri(URI.create(uri))
                    .register(MyLowPriorityContextProvider.class)
                    .register(MyHighPriorityContextProvider.class)
                    .build(Client.class);
```

When doing this, REST Client Reactive will always return the first registered context provider for the matching type:

```java
@priority(2)
    public static class MyLowPriorityContextProvider implements ContextResolver<Person> {

        @OverRide
        public Person getContext(Class<?> aClass) {
// ...
        }
    }

    @priority(1)
    public static class MyHighPriorityContextProvider implements ContextResolver<Person> {

        @OverRide
        public Person getContext(Class<?> aClass) {
// ...
        }
    }
```

With these changes, the `@Priority` annotation is taking into account to return the correct context provider regardless to when it has been registered.
@Sgitario Sgitario force-pushed the context_provider_priority branch from b73870b to a96d221 Compare April 27, 2023 17:30
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 27, 2023

Failing Jobs - Building a96d221

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/mongodb-panache 

📦 integration-tests/mongodb-panache

io.quarkus.it.mongodb.panache.reactive.ReactiveMongodbPanacheResourceTest.testMoreRepositoryFunctionalities line 351 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

@Sgitario
Copy link
Contributor Author

The failures are unrelated to these changes.

@Sgitario Sgitario merged commit 7f65037 into quarkusio:main Apr 28, 2023
@Sgitario Sgitario deleted the context_provider_priority branch April 28, 2023 03:34
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 28, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone Apr 28, 2023
@gsmet gsmet modified the milestones: 3.1 - main, 3.0.2.Final May 2, 2023
@gsmet gsmet modified the milestones: 3.0.2.Final, 3.1 - main May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants