Prevent duplicate HTTP headers when WriterInterceptor is used

[geoand]

Fixes: #27325

[Sgitario]

After digging into the code, I see that what this change is doing is avoiding calling ServerSerializers.encodeResponseHeaders (https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java#L71).

This consumer is being invoked because it has been previously registered in https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java#L194.

Therefore, I have two questions/comments:
a. Instead of setting the listener to null, why not avoid setting it in here: https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java#L71
b. Or why not improving the logic of ServerSerializers.encodeResponseHeaders to remove the response header before adding it: vertxResponse.removeResponseHeader(header).

Note that I tried to do the option b. without removing the listener and it worked fine.

[geoand]

Very good questions indeed!

Below are my answers on why we can't do one or the other:

a. Instead of setting the listener to null, why not avoid setting it in here:

This code path is needed for all other cases other than the writer interceptor handling

b. Or why not improving the logic of ServerSerializers.encodeResponseHeaders to remove the response header before adding it

We don't want to do this because it's possible for Vert.x handlers that run before RESTEasy Reactive to add headers that are then written out when RESTEasy Reactive completes the processing

[Sgitario]

Very good questions indeed!

Below are my answers on why we can't do one or the other:

a. Instead of setting the listener to null, why not avoid setting it in here:

This code path is needed for all other cases other than the writer interceptor handling

b. Or why not improving the logic of ServerSerializers.encodeResponseHeaders to remove the response header before adding it

We don't want to do this because it's possible for Vert.x handles that run before RESTEasy Reactive to add headers that are then written out

I don't fully understand the scenario of your last statement, but if you have already thought about these two solutions and decided to implement the current change, I trust your solution is the best approach :) Approving.

[geoand]

I don't fully understand the scenario of your last statement

The most common use case is this feature.

[Sgitario]

I don't fully understand the scenario of your last statement

The most common use case is this feature.

I've added a test in quarkus-resteasy-reactive/deployment to try to reproduce the scenario you described (using the property quarkus.http.header...). The test covers the same thing as the one you added in the independent project plus using the header property too.

The thing is that the test is behaving exactly the same either if you use the changes of this pull request or if you remove the header before setting it right before this line

quarkus/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java

Line 497 in effe3f9

String header = entry.getKey();

.

This is the implementation:

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

import static io.restassured.RestAssured.when;
import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.Matchers.is;

import java.io.IOException;
import java.util.Arrays;
import java.util.Date;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.container.ContainerResponseContext;
import javax.ws.rs.container.ContainerResponseFilter;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.ext.Provider;
import javax.ws.rs.ext.WriterInterceptor;
import javax.ws.rs.ext.WriterInterceptorContext;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.http.Headers;

public class CustomHeadersAndWriterInterceptorTest {

    @RegisterExtension
    static QuarkusUnitTest runner = new QuarkusUnitTest()
            .withApplicationRoot((jar) -> jar
                    .addClasses(DummyWriterInterceptor.class, TestResource.class))
            .overrideConfigKey("quarkus.http.header.\"propertyHeader\".value", "foo")
            .overrideConfigKey("quarkus.http.header.\"aggregateHeader\".value", "1");

    @Test
    void testResponseHeaders() {
        Headers headers = when()
                .get("/test")
                .then()
                .statusCode(200)
                .header("etag", is("0"))
                .extract().headers();
        assertThat(headers.getList("etag")).hasSize(1);
        assertThat(headers.getList("Last-Modified")).hasSize(1);
        assertThat(headers.getList("customHeader")).hasSize(1);
        assertThat(headers.getValue("customHeader")).isEqualTo("[a]");
        assertThat(headers.getValue("propertyHeader")).isEqualTo("foo");
        assertThat(headers.getList("aggregateHeader")).hasSize(2);
        assertThat(headers.getValue("aggregateHeader")).isEqualTo("[2]");
    }

    @Path("/test")
    public static class TestResource {

        @GET
        @Produces(MediaType.TEXT_PLAIN)
        public Response hello() {
            return Response.ok("123").lastModified(new Date()).header("etag", 0).build();
        }
    }

    @Provider
    public static class MyCustomHeaderFilter implements ContainerResponseFilter {

        @Override
        public void filter(ContainerRequestContext requestContext, ContainerResponseContext responseContext)
                throws IOException {
            responseContext.getHeaders().add("customHeader", Arrays.asList("a"));
            responseContext.getHeaders().add("aggregateHeader", Arrays.asList("2"));
        }
    }

    @Provider
    public static class DummyWriterInterceptor implements WriterInterceptor {

        @Override
        public void aroundWriteTo(WriterInterceptorContext context) throws IOException, WebApplicationException {
            context.proceed();
        }
    }

}

[geoand]

I might be misremembering when that Vert.x handler runs, but you should be able to reproduce the scenario I mentioned above if you create a custom Vert.x Web handler that runs before RESTEasy Reactive anb simply sets an HTTP header.