Add support for setting response status and response headers support via annotations #20242

steel-judoka · 2021-09-17T15:27:37Z

If you approve of this solution, then I will add the necessary documentation

Closes #20100

/cc @geoand, @FroMage, @stuartwdouglas

...reactive/common-runtime/src/main/java/io/quarkus/resteasy/reactive/server/common/Header.java

...rc/test/java/io/quarkus/resteasy/reactive/server/test/responseheader/ResponseHeaderTest.java

...rc/test/java/io/quarkus/resteasy/reactive/server/test/responsestatus/ResponseStatusTest.java

geoand

Thanks a lot for this!

I am personally in favor of this idea, but let's see what Stuart and Stephane say

steel-judoka · 2021-09-17T19:23:42Z

@geoand I changed the logic according to your comments. For now, everything will work for Uni and Multi(I assume it's right if refer to the requirements), but I can add logic that was implemented the first time, and annotations will work for Response as well

geoand · 2021-09-17T19:39:01Z

Thanks for the update!

I would argue that the annotations should not work of the return type is Response or RestResponse, but we need more feedback on this

...rc/test/java/io/quarkus/resteasy/reactive/server/test/responseheader/ResponseHeaderTest.java

geoand · 2021-09-17T19:44:29Z

...me/src/main/java/org/jboss/resteasy/reactive/server/core/ResteasyReactiveRequestContext.java

@@ -132,6 +133,9 @@
     */
    private List<UriMatch> matchedURIs;

+    private Map<String, String> responseHeaders;
+    private Integer responseStatus;


I am not to fond of adding new fields here to be honest...

Since we only have to change headers and statuses if everything is ok, I don't see a more elegant solution than passing them to the ResteasyReactiveRequestContext, and change them if successful

I understand the rationale here, but I don't want to set the precedent of adding new fields for little gain.
Furthermore, by looking at the class, one would not really know when these fields are set, when it's safe to use them and when they actually affect the final result.

In theory, I can add annotation checking to AsyncReturnTypeScanner and pass values to the handler, but this will spawn a chain of new fields though the life cycle of annotation will be more clear

Given the fact that I completely certain that when I look at this code down the line I will not know when these fields are used, it's going to be a hard no to adding these. We should not have to add custom fields each time we add peripheral functionality.
Please consider alternatives.

steel-judoka · 2021-09-18T08:46:46Z

@geoand can you take a look? I changed implementation a bit, removed additional fields from Context as you suggested, now it supports all return types, I also added additional tests for CompletionStage and plain return types

…al tests added;

geoand · 2021-09-20T08:58:22Z

Thanks.

I'll have a look later on today or tomorrow.

geoand · 2021-09-21T06:25:28Z

...rc/main/java/org/jboss/resteasy/reactive/server/handlers/CompletionStageResponseHandler.java

@@ -15,6 +15,7 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti

            result.handle((v, t) -> {
                if (t != null) {
+                    requestContext.serverResponse().clearResponseHeaders();


Can you explain why this (and similar calls in other places) is needed please?

Asking because it is not apparent why it's needed and it seems that it was added just to make something specific work.

Furthermore, this could have performance implications

It was added to clear custom headers in case of exceptions, but this doesn't clear standard headers like content-type and content-length. I think this will be useful even if there will be other implementations of custom headers in the future

In the same spirit as one of my previous comments, I don't like the fact that we need to make changes to unrelated areas just to support this feature.

I would have set the status and headers after the resource method was invoked thus eliminating the need to clear headers when an exception occurs.

Satisfying all the requirements I've set in the various comment might require more "infrastructure" than we currently have. To be honest, I didn't expect this to be picked up by a contributor :)

I don't think it's an unrelated area) But I got you, will think about another implementation, but for now I'll close this PR

geoand · 2021-09-21T09:15:38Z

I can take over this issue if you like and add you as a coauthor.

steel-judoka · 2021-09-21T09:24:59Z

I can take over this issue if you like and add you as a coauthor.

You don't need to add me as a co-author, I'll take on another issue

geoand · 2021-09-21T10:40:10Z

I will add you, because you didn't very nice work on this. Moreover, for the tests I'll likely copy what you did verbatim 😎

steel-judoka · 2021-09-21T10:48:36Z

I will add you, because you didn't very nice work on this. Moreover, for the tests I'll likely copy what you did verbatim 😎

I guess you meant "did" instead of "didn't"😁 But anyway, I appreciate that

geoand · 2021-09-21T11:09:57Z

I indeed meant did :)

Response status and Response headers support added

94afd1b

This comment has been minimized.

Sign in to view

quarkus-bot bot added the area/rest label Sep 17, 2021

steel-judoka changed the title ~~Response status and Response headers support added #20100~~ Response status and Response headers support added Sep 17, 2021