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

ContainerResponseFilter not invoked for not found urls #40331

Closed
vkn opened this issue Apr 29, 2024 · 11 comments
Closed

ContainerResponseFilter not invoked for not found urls #40331

vkn opened this issue Apr 29, 2024 · 11 comments
Assignees
Labels
area/resteasy-reactive kind/bug Something isn't working

Comments

@vkn
Copy link
Contributor

vkn commented Apr 29, 2024

Describe the bug

Hello,
after migrating from 3.8.x to 3.9.x response filter annotated with @Provider is not called anymore if the response is 404
This was working in 3.8.x and before with resteasy reactive extension.

@Provider
public class ResponseFilter implements ContainerResponseFilter {
    private static final Logger LOGGER = LoggerFactory.getLogger(ResponseFilter.class);

    private final HttpServerRequest request;
    private final LoggingBean loggingBean;

    @Inject
    public ResponseFilter(@Context HttpServerRequest request, LoggingBean loggingBean) {
        this.request = request;
        this.loggingBean = loggingBean;
    }

    @Override
    public void filter(ContainerRequestContext requestCtx, ContainerResponseContext responseCtx) {
        String uri = request.uri();
        LOGGER.info("ResponseFilter url {}", uri);
        loggingBean.log(uri);
    }
}

Expected behavior

ContainerResponseFilter is not invoked for 404 responses

Actual behavior

ContainerResponseFilter should also work for 404 responses

How to Reproduce?

code-with-quarkus.tar.gz
Run GreetingResourceTest

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

@vkn vkn added the kind/bug Something isn't working label Apr 29, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 29, 2024

/cc @FroMage (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

@geoand
Copy link
Contributor

geoand commented Apr 29, 2024

@phillip-kruger this seems to have broke as part of #39897

@phillip-kruger
Copy link
Member

I'll have a look

@phillip-kruger phillip-kruger self-assigned this Apr 29, 2024
@geoand
Copy link
Contributor

geoand commented Apr 29, 2024

🙏🏼

@phillip-kruger
Copy link
Member

Ok, so what we have here is what was described in the PR: #37680

From that PR description:

Breaking change (with a workaround if needed)

In the current version, if you have a JAX-RS Filter, that filter will run, even in the case that the requested resource was not a REST resource (in my opinion, that is wrong). This will not happen anymore. Filters for JAX-RS will now only run on JAX-RS resources. In the case that you want your Filters to also run on non JAX-RS resource, you need to add your own ExceptionMapper:

package io.quarkus.resteasy.reactive.server.test.customproviders;

import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.ext.ExceptionMapper;
import jakarta.ws.rs.ext.Provider;

@Provider
public class NotFoundExeptionMapper implements ExceptionMapper<NotFoundException> {
    @Override
    public Response toResponse(NotFoundException exception) {
        return Response.status(404).build();
    }
}

Now, JAX-RS becomes responsible for the Not Found resources, and the Filter will run. In the current version where this always happens, it makes it impossible for other extensions to handle Not Found.

@phillip-kruger
Copy link
Member

So /hello/non-existing will work, but /non-existing not, except if you add your own handler.

@geoand
Copy link
Contributor

geoand commented Apr 30, 2024

In the current version, if you have a JAX-RS Filter, that filter will run, even in the case that the requested resource was not a REST resource (in my opinion, that is wrong). This will not happen anymore

I agree that the previous behavior was wrong

Filters for JAX-RS will now only run on JAX-RS resources

+1

@geoand
Copy link
Contributor

geoand commented Apr 30, 2024

@phillip-kruger should we add something to the migration guide and close this?

@vkn
Copy link
Contributor Author

vkn commented Apr 30, 2024

Filters for JAX-RS will now only run on JAX-RS resources. In the case that you want your Filters to also run on non JAX-RS resource,

Makes sense. Thanks for looking into it

@geoand geoand closed this as not planned Won't fix, can't repro, duplicate, stale Apr 30, 2024
@geoand
Copy link
Contributor

geoand commented Apr 30, 2024

Thanks @phillip-kruger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resteasy-reactive kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants