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

RESTEasy Reactive doesn't apply ContainerRequestFilters to reactive methods #27979

Closed
kdubb opened this issue Sep 16, 2022 · 10 comments · Fixed by #28147
Closed

RESTEasy Reactive doesn't apply ContainerRequestFilters to reactive methods #27979

kdubb opened this issue Sep 16, 2022 · 10 comments · Fixed by #28147
Labels
Milestone

Comments

@kdubb
Copy link
Contributor

kdubb commented Sep 16, 2022

Describe the bug

When applying a name bound ContainerRequestFilter to a resource that has only reactive methods (in the discovery case they were Kotlin suspend methods) the filter is never called.

The filter is not applied even if the name binding annotation is explicitly applied to the method or if the annotation is applied to the class as a whole.

The filter did not do anything that made it blocking; not that it mattered since it was never called.

After changing the filter to implement ResteasyReactiveContainerRequestFilter the filter was called as expected.

Expected behavior

ContainerRequestFilters are applied to reactive methods.

Alternatively, some appropriate error should be generated to relay that ResteasyReactiveContainerRequestFilter must be implemented.

Actual behavior

Filters must implement ResteasyReactiveContainerRequestFilter for them to be called for reactive methods.

Also, interestingly they are also applied to server methods even if you accidentally implement the client version ResteasyReactiveClientRequestFilter; which I found out because that's what I initially did.

How to Reproduce?

Create NameBinding Filter

@NameBinding
@interface MyFilter {}

@MyFilter
class MyFilterImpl implements ContainerRequestFilter {...}

Apply Filter to a reactive resource

@MyFilter
class MyResource {
  Uni<Void> shouldBeIntercepted() {...}
}

Output of uname -a or ver

macOS 12.6

Output of java -version

OpenJDK 17.0.1

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.12.0.Final & 2.13.0.CR1

Build tool (ie. output of mvnw --version or gradlew --version)

Gradle 7.5.1

Additional information

No response

@kdubb kdubb added the kind/bug Something isn't working label Sep 16, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 16, 2022

@geoand
Copy link
Contributor

geoand commented Sep 16, 2022

Interesting case. Could you attach a sample application we could use to test fixes against?

@kdubb
Copy link
Contributor Author

kdubb commented Sep 16, 2022

Here's one for both Java/Mutiny and Kotlin coroutine. The reproducer was needed, apparently it requires the JAX-RS annotations to be on an API interface with the name binding filter on a resource implementation class.

code-with-quarkus-2.zip
code-with-quarkus.zip

@geoand
Copy link
Contributor

geoand commented Sep 16, 2022

Thanks. I'll have a look next week.

@geoand
Copy link
Contributor

geoand commented Sep 22, 2022

This happens because the name binding annotation is declared on the interface. I am not 100% sure we should support this

@kdubb
Copy link
Contributor Author

kdubb commented Sep 22, 2022

It's not on the interface, it's on the implementation. The JAX-RS annotations are on the interface.

This use case is pretty useful and I believe supported by the JAX-RS specification. We generate our JAX-RS interfaces and then need to apply certain annotation to the implementation.

@kdubb
Copy link
Contributor Author

kdubb commented Sep 22, 2022

Not to mention, it does work... just only for ResteasyReactiveContainerRequestFilter.

@geoand
Copy link
Contributor

geoand commented Sep 22, 2022

Then I must have lost track of the changes I need to see it working. Let me check again

@geoand
Copy link
Contributor

geoand commented Sep 22, 2022

Not to mention, it does work... just only for ResteasyReactiveContainerRequestFilter.

This does not appear to be true, at least not with https://github.com/quarkusio/quarkus/files/9581532/code-with-quarkus.zip

@geoand
Copy link
Contributor

geoand commented Sep 22, 2022

The fix does not seem to break any of the TCK tests, so let's go ahead and add it.

@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 22, 2022
geoand added a commit that referenced this issue Sep 22, 2022
Take @namebinding in the class hierarchy into account
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.1.Final Sep 30, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Oct 3, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants