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

ContainerResponseContext has null default MediaType when using RESTEasy reactive #34839

Open
gnieser opened this issue Jul 19, 2023 · 15 comments · May be fixed by #44166
Open

ContainerResponseContext has null default MediaType when using RESTEasy reactive #34839

gnieser opened this issue Jul 19, 2023 · 15 comments · May be fixed by #44166
Labels
area/rest kind/bug Something isn't working triage/consider-closing Bugs that are considered to be closed because too old. Using the label to do a mark and sweep proces

Comments

@gnieser
Copy link
Contributor

gnieser commented Jul 19, 2023

Describe the bug

Hello,

I noticed a difference between RESTEasy reactive and RESTEasy classic when using ContainerResponseFilter.
A simple endpoint returning a Response and that does explicitly its type will result a in a null MediaType in the ContainerResponseContext of a ContainerResponseFilter.

Notes:

  • The same code has a MediaType set correctly to the default application/json when using RESTEasy classic
  • The same filter works correctly is the type is set explicitly, hence I think the issue is only with the default MediaType
  • The contentType is nonetheless correct in the HTTP Response header

Considering a simple endpoint:

    @GET
    @Produces(MediaType.APPLICATION_JSON)
    public Response hello() {
        Greeting greeting = new Greeting();
        greeting.message = "Hello RESTEasy";
        return Response.ok(greeting).build();
    }

The following filter will throw its exception in reactive mode, but not in classic.

@Priority(5000)
@Provider
public class MediaTypeResponseFilter implements ContainerResponseFilter {

    @Override
    public void filter(ContainerRequestContext requestContext, ContainerResponseContext responseContext) throws IOException {
        if (responseContext.getMediaType() == null) {
            throw new IOException("Oh! MediaType is null");
        }
    }
}

Expected behavior

It would help is the MediaType was available to the ContainerResponseFilter.

Actual behavior

Unfortunately, the MediaType is null.

A workaround is to force on the response:

return Response.ok(greeting).type(MediaType.APPLICATION_JSON_TYPE).build();

It is however a bit tedious to add to all the Response, and defeats the purpose of a default MediaType.

How to Reproduce?

The following project is a reproducer https://github.com/gnieser/quarkus-mediatype

Output of uname -a or ver

Windows 10

Output of java -version

mandrel-java17-22.3.1.0-Final

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.16.8.Final

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

OpenJDK Runtime Environment Temurin-17.0.6+10

Additional information

No response

@gnieser gnieser added the kind/bug Something isn't working label Jul 19, 2023
@quarkus-bot quarkus-bot bot added area/mandrel area/rest env/windows Impacts Windows machines labels Jul 19, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 19, 2023

/cc @FroMage (resteasy-reactive), @Karm (mandrel), @Sgitario (resteasy-reactive), @galderz (mandrel), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive), @zakkak (mandrel)

@geoand geoand removed env/windows Impacts Windows machines area/mandrel labels Jul 19, 2023
@geoand
Copy link
Contributor

geoand commented Jul 19, 2023

I am pretty sure there is no TCK test for this so the behavior is pretty much unspecified.

I am inclined to close this as won't fix as I personally view RESTEasy Reactive's behavior more reasonable here.

@geoand geoand added the triage/consider-closing Bugs that are considered to be closed because too old. Using the label to do a mark and sweep proces label Jul 19, 2023
@gnieser
Copy link
Contributor Author

gnieser commented Jul 19, 2023

Thahnks for answering!
I tend to also think is more reasonable... However, I have to concede it is much less practical for my scenario at least ;)

I can check for nullity in the filter, but I would still need to guess what the default value will be. It could lead to duplication, in the ContainerResponseFilter, of the logic quarkus-resteasy-reactive already uses for the smart defaults. I am not even sure how it is doable from within the ContainerResponseFilter since the return type of the endpoint method are used.

Anyway, for my real world use case, I can treat in the same way a null value resulting a contentType text/plain, text/html, application/json. I'm more worried about binary content...

PS: you may find worth a mention in the "Migrating to RESTEasy Reactive" guide.

@geoand
Copy link
Contributor

geoand commented Jul 19, 2023

Anyway, for my real world use case, I can treat in the same way a null value resulting a contentType text/plain, text/html, application/json. I'm more worried about binary content...

I am pretty sure you could handle your use case with a jakarta.ws.rs.ext.WriterInterceptor

@gnieser
Copy link
Contributor Author

gnieser commented Jul 20, 2023

I am pretty sure you could handle your use case with a jakarta.ws.rs.ext.WriterInterceptor

To be fair, only partially I'm afraid. The WriterInterceptor doesn't have the request and response contexts, only its own context.
As far as I know, one cannot easily get the HTTP method of the request. The ContainerResponseFilter makes it trivial using requestContext.getMethod(). I would be happy to be proven wrong though!

@geoand
Copy link
Contributor

geoand commented Jul 20, 2023

As far as I know, one cannot easily get the HTTP method of the request

You can always add a @Context jakarta.ws.rs.core.Request request field to your class and obtain the HTTP method from that.

@gnieser
Copy link
Contributor Author

gnieser commented Jul 20, 2023

Thanks for the reminder!

I gave a try to the WriterInterceptor, and unfortunately the MediaType is null on the WirterInterceptorContext

PS: I couldn't find the HTTP status code

@FroMage
Copy link
Member

FroMage commented Aug 14, 2023

I do find it weird that MediaType is not set in the filter, because we know it, and it's set in the HTTP response. Why do you view this as more reasonable?

@gnieser
Copy link
Contributor Author

gnieser commented Aug 16, 2023

I was feeling so because it's not set explicitly. But I admit (and faced it), it's not practical at all.

@geoand
Copy link
Contributor

geoand commented Aug 21, 2023

We should probably take a closer look at this

@crisgonzalezallaria
Copy link

Hi team, I'm having the same problem. Is there any chance this can be fixed? Or what would be an appropriate solution? Of all the frameworks I've used, I've always used ContainerResponseFilter because it facilitates the logic for obtaining the HTTP/REST context. Is there any reason why if the Controller defines the MediaType, the ContainerResponseFilter cannot access it?

Thanks

gsmet added a commit to gsmet/quarkus that referenced this issue Oct 29, 2024
The one from the response can be null if we return a partial response
but the type is defined by @produces.
We need to consider the one from the context.

Fixes quarkusio#34839
@gsmet
Copy link
Member

gsmet commented Oct 29, 2024

From what I can see, there are two different issues, one is a bug, the other I'm not entirely sure.

ContainerResponseFilter

In the case of a method returning a Response, the *ProducesHandler are not installed and thus the @Produces is not injected early in the ResteasyReactiveRequestContext which would be necessary for the ContainerResponseFilter to work.

My personal opinion is that we should probably install a handler in this case - even if it does less things than the current *ProducesHandler and just set the content type. But my knowledge in this area are limited so I might be incorrect.

At least when the content type is static, it would make sense to inject it early IMO. But this decision will require more knowledge of Quarkus REST than I currently have.

WriterInterceptor

This one is a different beast, it's executed late and the content type is actually injected in the ResteasyReactiveRequestContext at this stage (it has been injected by the ServerSerialisers).
Unfortunately, we are not injecting the correct media type when executing the writer interceptors. We are getting the one from the Response without defaulting to the one that has been resolved for the context if null.

Which is what #44166 tries to fix.

@FroMage
Copy link
Member

FroMage commented Oct 30, 2024

My opinion is that @Produces, implicit media types and setting the media type directly in Response should all lead to the same observable behaviour in filters and interceptors.

gsmet added a commit to gsmet/quarkus that referenced this issue Oct 31, 2024
The one from the response can be null if we return a partial response
but the type is defined by @produces.
We need to consider the one from the context.

Fixes quarkusio#34839
gsmet added a commit to gsmet/quarkus that referenced this issue Oct 31, 2024
Even when we use Response, it's interesting to push the default content
type to the context as it might get used by ContainerResponseFilters.

If the Response actually overrides it, it's all taken care of.

Fixes quarkusio#34839
@gsmet
Copy link
Member

gsmet commented Oct 31, 2024

See the discussion here: #44166 (comment) .

It seems that when using Response, we are supposed to ignore the @Produces. At least, that's what the TCK is testing.

So we can fix the WriterInterceptor case but it seems the behavior for ContainerResponseFilter is expected.

gsmet added a commit to gsmet/quarkus that referenced this issue Oct 31, 2024
The one from the response can be null if we return a partial response
but the type is defined by @produces.
We need to consider the one from the context.

Fixes quarkusio#34839
gsmet added a commit to gsmet/quarkus that referenced this issue Oct 31, 2024
Even when we use Response, it's interesting to push the default content
type to the context as it might get used by ContainerResponseFilters.

If the Response actually overrides it, it's all taken care of.

Fixes quarkusio#34839
gsmet added a commit to gsmet/quarkus that referenced this issue Oct 31, 2024
Even when we use Response, it's interesting to push the default content
type to the context as it might get used by ContainerResponseFilters.

If the Response actually overrides it, it's all taken care of.

Fixes quarkusio#34839
@gsmet
Copy link
Member

gsmet commented Oct 31, 2024

AFAICS, it was just an ugly bug in my code, things should be fine provided full CI is happy.

gsmet added a commit to gsmet/quarkus that referenced this issue Oct 31, 2024
Even when we use Response, it's interesting to push the default content
type to the context as it might get used by ContainerResponseFilters.

If the Response actually overrides it, it's all taken care of.

Fixes quarkusio#34839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working triage/consider-closing Bugs that are considered to be closed because too old. Using the label to do a mark and sweep proces
Projects
None yet
5 participants