Make sure the default content type is available in ContainerResponseFilters and WriterInterceptors when returning Response #44166

gsmet · 2024-10-29T14:16:40Z

Fixes #34839

gsmet · 2024-10-29T14:16:59Z

This will need a test but I would like for @geoand to make sure I'm not hallucinating :).

geoand · 2024-10-30T06:08:33Z

Thanks!

Seems reasonable to me.

gsmet · 2024-10-31T09:32:28Z

@geoand I tried to fix the other issue with ContainerResponseFilter. Let me know what you think.

I also added tests for the first fix.

That being said, I just ran the TCK locally and... the second commit introduces a test failure in JAXRSClient0016:

    @Test
    public void defaultResponseErrorTest() throws Fault {
        setProperty(Property.REQUEST, buildRequest(Request.POST, "error"));
        setProperty(Property.CONTENT, "anything");
        setProperty(Property.REQUEST_HEADERS, "Accept: text/*");
        setProperty(Property.STATUS_CODE, getStatusCode(Status.NOT_ACCEPTABLE));
        invoke();
    }

The resource is this one:

    @POST
    @Produces("text/*")
    public Response response(String msg) {
        return Response.ok(msg).build();
    }

So it basically expects an error when the Response type is not defined even if we have a @Produces around.

It seems a bit odd to me and in contradiction with what I understand from https://jakarta.ee/specifications/restful-ws/4.0/jakarta-restful-ws-spec-4.0#determine_response_type .
When I read the first line, I'm understanding that if Response doesn't define any content type, you will consider the ones from @Produces.

The only thing that makes sense here would be if Response enforces some default content type but I couldn't find the info.

/cc @FroMage

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

geoand · 2024-10-31T09:44:34Z

Yeah, what you describe does seem reasonable, but I am stopped trying to make sense of the TCK when it comes to media types...

FroMage

LGTM.

FroMage · 2024-10-31T10:54:31Z

@gsmet: the failure is legit according to the algo described in the spec. You have Accept = text/* and Produces = text/* so you have M = text/*, get all the way to step 8 and have no concrete type, so 9 does not apply and 10 kicks in.

You haven't been able to find a proper content type, because neither Accept nor Produces told you what type it is really, and text/* is not a valid content type for a response. Also we can't infer it from the method type since it's Response.

Now, we could deduct it from the value being passed to the Response but that's another change…

gsmet · 2024-10-31T11:55:22Z

@FroMage interesting. I'll dig a bit more then.

gsmet · 2024-10-31T14:19:59Z

@FroMage thanks for putting me in the right direction, it was actually an ugly bug in my newly written code. From what I can see the TCK is passing correctly now.

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

quarkus-bot · 2024-10-31T17:57:34Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 27381e3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/panache/hibernate-reactive-rest-data-panache/deployment

✖ io.quarkus.hibernate.reactive.rest.data.panache.deployment.repository.PanacheRepositoryResourcePutMethodTest.shouldUpdateComplexObject - History

1 expectation failed. JSON path name doesn't match. Expected: is "updated collection" Actual: empty collection - java.lang.AssertionError

java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.
Expected: is "updated collection"
  Actual: empty collection

	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingTestCase.testContinuousTestingScenario2 - History

io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor\#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/vectorized/redpanda:v24.1.2 at io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor.startKafkaDevService(DevServicesKafkaProcessor.java:105) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException

java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/vectorized/redpanda:v24.1.2
	at io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor.startKafkaDevService(DevServicesKafkaProcessor.java:105)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)

FroMage

Not sure about this change, there's a bit of code I don't understand yet 😅

FroMage · 2024-11-04T16:42:52Z

.../resteasy/reactive/server/handlers/VariableProducesSetDefaultContentTypeResponseHandler.java

+            }
+        }
+        if (res == null) { // fallback to ensure that MessageBodyWriter is passed the proper media type
+            res = mediaTypeList.negotiateProduces(requestContext.serverRequest().getRequestHeader(HttpHeaders.ACCEPT))


Oh, I see, I suppose that requestContext.getHttpHeaders() might have been overridden by a filter, while requestContext.serverRequest() is immutable?
Otherwise I don't understand why these would be different. Besides the fact that I suspect that here, getRequestHeader will not properly split the header…
I'm a bit lost here :-/

I haven't invented this code, it comes straight from VariableProducesHandler. I just copied the exact same behavior as I supposed that's what we wanted.

And I agree I don't really understand what is going on here either.

Code comes from this PR: #40022 .

/cc @geoand

Yeah, lovely stuff... Anything media type related is horrible - both in the spec and the impl

In any case, I think being consistent in how we handle things for Response/not Response is a good thing.

Definitely agreed

Maybe I should add a comment in both classes though so that they are kept in sync if updates are made.

I will do that once we agree with the big picture.

A comment would be nice because if we don't understand why it's like this now, we will also not understand it later if an issue is raised and we need to know that there are two places where the same behaviour occurs. Or you refactor the two into a single method, also with a comment saying this is fishy and point at this PR for this discussion, for later archeology.

gsmet mentioned this pull request Oct 29, 2024

ContainerResponseContext has null default MediaType when using RESTEasy reactive #34839

Open

gsmet force-pushed the quarkus-rest-media-type-writer-interceptor branch from 43ab439 to 8584ea8 Compare October 31, 2024 09:06

quarkus-bot bot added the area/rest label Oct 31, 2024

gsmet changed the title ~~Use proper media type when executing writer interceptors~~ Make sure the default content type is available in ContainerResponseFilters and WriterInterceptors when returning Response Oct 31, 2024

Use proper media type when executing writer interceptors

a60aca2

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 force-pushed the quarkus-rest-media-type-writer-interceptor branch from 8584ea8 to 6497776 Compare October 31, 2024 09:44

FroMage approved these changes Oct 31, 2024

View reviewed changes

gsmet force-pushed the quarkus-rest-media-type-writer-interceptor branch from 6497776 to be55617 Compare October 31, 2024 14:13

gsmet marked this pull request as ready for review October 31, 2024 14:20

Populate the default content type even when using Response

27381e3

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 force-pushed the quarkus-rest-media-type-writer-interceptor branch from be55617 to 27381e3 Compare October 31, 2024 14:39

quarkus-bot bot added the triage/flaky-test label Oct 31, 2024

FroMage requested changes Nov 4, 2024

View reviewed changes

geoand requested a review from FroMage November 6, 2024 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure the default content type is available in ContainerResponseFilters and WriterInterceptors when returning Response #44166

Make sure the default content type is available in ContainerResponseFilters and WriterInterceptors when returning Response #44166

gsmet commented Oct 29, 2024 •

edited

Loading

gsmet commented Oct 29, 2024

geoand commented Oct 30, 2024

gsmet commented Oct 31, 2024 •

edited

Loading

geoand commented Oct 31, 2024

FroMage left a comment

FroMage commented Oct 31, 2024

gsmet commented Oct 31, 2024

gsmet commented Oct 31, 2024

quarkus-bot bot commented Oct 31, 2024 •

edited by github-actions bot

Loading

FroMage left a comment

FroMage Nov 4, 2024

gsmet Nov 5, 2024

gsmet Nov 5, 2024

geoand Nov 5, 2024

gsmet Nov 5, 2024

geoand Nov 5, 2024

gsmet Nov 5, 2024

FroMage Nov 6, 2024

Make sure the default content type is available in ContainerResponseFilters and WriterInterceptors when returning Response #44166

Are you sure you want to change the base?

Make sure the default content type is available in ContainerResponseFilters and WriterInterceptors when returning Response #44166

Conversation

gsmet commented Oct 29, 2024 • edited Loading

gsmet commented Oct 29, 2024

geoand commented Oct 30, 2024

gsmet commented Oct 31, 2024 • edited Loading

geoand commented Oct 31, 2024

FroMage left a comment

Choose a reason for hiding this comment

FroMage commented Oct 31, 2024

gsmet commented Oct 31, 2024

gsmet commented Oct 31, 2024

quarkus-bot bot commented Oct 31, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/panache/hibernate-reactive-rest-data-panache/deployment

📦 extensions/smallrye-reactive-messaging-kafka/deployment

FroMage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Oct 29, 2024 •

edited

Loading

gsmet commented Oct 31, 2024 •

edited

Loading

quarkus-bot bot commented Oct 31, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`