[4.x]: Fix static content sending 304 with entity.

http/http/src/main/java/io/helidon/http/HttpException.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ public class HttpException extends RuntimeException {
 
     private final Status status;
     private final boolean keepAlive;
+    private final ServerResponseHeaders headers = ServerResponseHeaders.create();
 
     /**
      * Creates {@link HttpException} associated with {@link Status#INTERNAL_SERVER_ERROR_500}.
@@ -113,4 +114,24 @@ public final Status status() {
     public boolean keepAlive() {
         return keepAlive;
     }
+
+    /**
+     * Set a response header that should be used if this exception is not handled.
+     *
+     * @param header header to set
+     * @return updated instance
+     */
+    public HttpException header(Header header) {
+        headers.set(header);
+        return this;
+    }
+
+    /**
+     * Headers as currently configured in this exception.
+     *
+     * @return headers configured for this exception
+     */
+    public Headers headers() {
+        return headers;
+    }
 }

webclient/http1/src/main/java/io/helidon/webclient/http1/RedirectionProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2023, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -30,7 +30,10 @@ private RedirectionProcessor() {
     }
 
     static boolean redirectionStatusCode(Status status) {
-        return status.family() == Status.Family.REDIRECTION;
+        // 304 is not an actual redirect - it is telling the client to use a cached value, which is outside of scope
+        // of Helidon WebClient, the user must understand such a response, as they had to send an ETag
+        return status.code() != Status.NOT_MODIFIED_304.code()
+            && status.family() == Status.Family.REDIRECTION;
     }
 
     static Http1ClientResponseImpl invokeWithFollowRedirects(Http1ClientRequestImpl request, byte[] entity) {

webserver/static-content/src/main/java/io/helidon/webserver/staticcontent/StaticContentHandler.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2017, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -81,16 +81,20 @@ static void processEtag(String etag, ServerRequestHeaders requestHeaders, Server
             return;
         }
         etag = unquoteETag(etag);
+
+        Header newEtag = HeaderValues.create(HeaderNames.ETAG, true, false, '"' + etag + '"');
         // Put ETag into the response
-        responseHeaders.set(HeaderNames.ETAG, '"' + etag + '"');
+        responseHeaders.set(newEtag);
+
         // Process If-None-Match header
         if (requestHeaders.contains(HeaderNames.IF_NONE_MATCH)) {
             List<String> ifNoneMatches = requestHeaders.get(HeaderNames.IF_NONE_MATCH).allValues();
             for (String ifNoneMatch : ifNoneMatches) {
                 ifNoneMatch = unquoteETag(ifNoneMatch);
                 if ("*".equals(ifNoneMatch) || ifNoneMatch.equals(etag)) {
                     // using exception to handle normal flow (same as in reactive static content)
-                    throw new HttpException("Accepted by If-None-Match header", Status.NOT_MODIFIED_304, true);
+                    throw new HttpException("Accepted by If-None-Match header", Status.NOT_MODIFIED_304, true)
+                            .header(newEtag);
                 }
             }
         }
@@ -108,7 +112,8 @@ static void processEtag(String etag, ServerRequestHeaders requestHeaders, Server
                     }
                 }
                 if (!ifMatchChecked) {
-                    throw new HttpException("Not accepted by If-Match header", Status.PRECONDITION_FAILED_412, true);
+                    throw new HttpException("Not accepted by If-Match header", Status.PRECONDITION_FAILED_412, true)
+                            .header(newEtag);
                 }
             }
         }

webserver/static-content/src/test/java/io/helidon/webserver/staticcontent/StaticContentHandlerTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -68,7 +68,7 @@ void etag_InNoneMatch_NotAccept() {
         when(req.get(IF_NONE_MATCH)).thenReturn(HeaderValues.create(IF_NONE_MATCH, "\"ccc\"", "\"ddd\""));
         ServerResponseHeaders res = mock(ServerResponseHeaders.class);
         StaticContentHandler.processEtag("aaa", req, res);
-        verify(res).set(ETAG, ETAG_VALUE);
+        verify(res).set(HeaderValues.create(ETAG, true, false, ETAG_VALUE));
     }
 
     @Test
@@ -79,7 +79,7 @@ void etag_InNoneMatch_Accept() {
         when(req.get(IF_NONE_MATCH)).thenReturn(HeaderValues.create(IF_NONE_MATCH, "\"ccc\"", "W/\"aaa\""));
         ServerResponseHeaders res = mock(ServerResponseHeaders.class);
         assertHttpException(() -> StaticContentHandler.processEtag("aaa", req, res), Status.NOT_MODIFIED_304);
-        verify(res).set(ETAG, ETAG_VALUE);
+        verify(res).set(HeaderValues.create(ETAG, true, false, ETAG_VALUE));
     }
 
     @Test
@@ -90,7 +90,7 @@ void etag_InMatch_NotAccept() {
         when(req.get(IF_MATCH)).thenReturn(HeaderValues.create(IF_MATCH, "\"ccc\"", "\"ddd\""));
         ServerResponseHeaders res = mock(ServerResponseHeaders.class);
         assertHttpException(() -> StaticContentHandler.processEtag("aaa", req, res), Status.PRECONDITION_FAILED_412);
-        verify(res).set(ETAG, ETAG_VALUE);
+        verify(res).set(HeaderValues.create(ETAG, true, false, ETAG_VALUE));
     }
 
     @Test
@@ -101,7 +101,7 @@ void etag_InMatch_Accept() {
         when(req.get(IF_MATCH)).thenReturn(HeaderValues.create(IF_MATCH, "\"ccc\"", "\"aaa\""));
         ServerResponseHeaders res = mock(ServerResponseHeaders.class);
         StaticContentHandler.processEtag("aaa", req, res);
-        verify(res).set(ETAG, ETAG_VALUE);
+        verify(res).set(HeaderValues.create(ETAG, true, false, ETAG_VALUE));
     }
 
     @Test

webserver/tests/static-content/src/test/java/io/helidon/webserver/tests/staticcontent/StaticContentTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2022, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,7 +16,10 @@
 
 package io.helidon.webserver.tests.staticcontent;
 
+import io.helidon.http.Header;
+import io.helidon.http.HeaderNames;
 import io.helidon.http.Status;
+import io.helidon.webclient.api.ClientResponseTyped;
 import io.helidon.webclient.http1.Http1Client;
 import io.helidon.webclient.http1.Http1ClientResponse;
 import io.helidon.webserver.http.HttpRouting;
@@ -26,6 +29,7 @@
 
 import org.junit.jupiter.api.Test;
 
+import static io.helidon.common.testing.http.junit5.HttpHeaderMatcher.hasHeader;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 
@@ -81,4 +85,39 @@ void testOutOfDirectory() {
             assertThat(response.status(), is(Status.NOT_FOUND_404));
         }
     }
+
+    @Test
+    void testIfNoneMatch() {
+        ClientResponseTyped<String> response = client.get("/files/static-content.txt")
+                .request(String.class);
+
+        assertThat(response.status(), is(Status.OK_200));
+        assertThat(response.headers(), hasHeader(HeaderNames.ETAG));
+
+        Header header = response.headers()
+                .get(HeaderNames.ETAG);
+
+        response = client.get("/files/static-content.txt")
+                .header(HeaderNames.IF_NONE_MATCH, header.get())
+                .request(String.class);
+
+        assertThat(response.status(), is(Status.NOT_MODIFIED_304));
+        assertThat(response.headers(), hasHeader(HeaderNames.ETAG));
+    }
+
+    @Test
+    void testIfMatch() {
+        ClientResponseTyped<String> response = client.get("/files/static-content.txt")
+                .request(String.class);
+
+        assertThat(response.status(), is(Status.OK_200));
+        assertThat(response.headers(), hasHeader(HeaderNames.ETAG));
+
+        response = client.get("/files/static-content.txt")
+                .header(HeaderNames.IF_MATCH, "\"wrong\"")
+                .request(String.class);
+
+        assertThat(response.status(), is(Status.PRECONDITION_FAILED_412));
+        assertThat(response.headers(), hasHeader(HeaderNames.ETAG));
+    }
 }

webserver/webserver/src/main/java/io/helidon/webserver/http/DirectHandlers.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2022, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -84,9 +84,8 @@ public void handle(RequestException httpException, ServerResponse res, boolean k
                 httpException.responseHeaders(),
                 httpException);
 
-        Status usedStatus;
-
-        res.status(response.status());
+        Status status = response.status();
+        res.status(status);
         response.headers()
                 .forEach(res::header);
         if (!keepAlive) {
@@ -99,7 +98,18 @@ public void handle(RequestException httpException, ServerResponse res, boolean k
         }
 
         try {
-            response.entity().ifPresentOrElse(res::send, res::send);
+            if (status.code() == Status.NO_CONTENT_204.code()
+                    || status.code() == Status.RESET_CONTENT_205.code()
+                    || status.code() == Status.NOT_MODIFIED_304.code()) {
+                // https://www.rfc-editor.org/rfc/rfc9110#status.204
+                // A 204 response is terminated by the end of the header section; it cannot contain content or trailers
+                // ditto for 205, and 304
+                res.header(HeaderValues.CONTENT_LENGTH_ZERO)
+                        .send();
+            } else {
+                // otherwise send the entity if present
+                response.entity().ifPresentOrElse(res::send, res::send);
+            }
         } catch (IllegalStateException ex) {
             // failed to send - probably output stream was already obtained and used, so status is written
             // we can only close the connection now
@@ -109,9 +119,7 @@ public void handle(RequestException httpException, ServerResponse res, boolean k
                     ex);
         }
 
-        usedStatus = response.status();
-
-        if (usedStatus == Status.INTERNAL_SERVER_ERROR_500) {
+        if (response.status() == Status.INTERNAL_SERVER_ERROR_500) {
             LOGGER.log(WARNING, "Internal server error", httpException);
         }
     }

webserver/webserver/src/main/java/io/helidon/webserver/http/ErrorHandlers.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2022, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -191,6 +191,7 @@ private void unhandledError(ConnectionContext ctx, ServerRequest request, Routin
                     .status(httpException.status())
                     .setKeepAlive(httpException.keepAlive())
                     .request(DirectTransportRequest.create(request.prologue(), request.headers()))
+                    .update(it -> httpException.headers().forEach(it::header))
                     .build());
         } else {
             // to be handled by error handler