imp: don't send Content-length header for GET requests (#10218)

http-client-jdk/src/main/java/io/micronaut/http/client/jdk/HttpRequestFactory.java

@@ -21,6 +21,7 @@
 import io.micronaut.core.async.publisher.Publishers;
 import io.micronaut.core.type.Argument;
 import io.micronaut.http.HttpHeaders;
+import io.micronaut.http.HttpMethod;
 import io.micronaut.http.MediaType;
 import io.micronaut.http.client.HttpClientConfiguration;
 import io.micronaut.http.codec.MediaTypeCodec;
@@ -56,8 +57,14 @@ public static <I> HttpRequest.Builder builder(
     ) {
         final HttpRequest.Builder builder = HttpRequest.newBuilder().uri(uri);
         configuration.getReadTimeout().ifPresent(builder::timeout);
-        HttpRequest.BodyPublisher bodyPublisher = publisherForRequest(request, bodyType, mediaTypeCodecRegistry);
-        builder.method(request.getMethod().toString(), bodyPublisher);
+        if (request.getMethod() == HttpMethod.GET) {
+            builder.GET();
+        } else if (request.getMethod() == HttpMethod.DELETE) {
+            builder.DELETE();
+        } else {
+            HttpRequest.BodyPublisher bodyPublisher = publisherForRequest(request, bodyType, mediaTypeCodecRegistry);
+            builder.method(request.getMethod().toString(), bodyPublisher);
+        }
         request.getHeaders().forEach((name, values) -> values.forEach(value -> builder.header(name, value)));
         if (request.getContentType().isEmpty()) {
             builder.header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON);

http-client-tck/src/main/java/io/micronaut/http/client/tck/tests/ClientDisabledCondition.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright 2017-2023 original authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.micronaut.http.client.tck.tests;
+
+import org.junit.jupiter.api.extension.ConditionEvaluationResult;
+import org.junit.jupiter.api.extension.ExecutionCondition;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+import static org.junit.jupiter.api.extension.ConditionEvaluationResult.disabled;
+import static org.junit.jupiter.api.extension.ConditionEvaluationResult.enabled;
+import static org.junit.platform.commons.util.AnnotationUtils.findAnnotation;
+
+/**
+ * A condition that enables or disables a test based on the presence of the {@link ClientDisabled} annotation.
+ * <p>
+ * The condition is enabled by default, unless the annotation is present and the {@link ClientDisabled#jdk()} or
+ * {@link ClientDisabled#httpClient()} parameters match the current JDK version and the HTTP client configuration
+ * parameter.
+ */
+public class ClientDisabledCondition implements ExecutionCondition {
+
+    public static final String JDK = "jdk";
+    public static final String NETTY = "netty";
+    public static final String HTTP_CLIENT_CONFIGURATION = "httpClient";
+
+    private static final ConditionEvaluationResult ENABLED_BY_DEFAULT = enabled("@ClientDisabled is not present");
+    private static final ConditionEvaluationResult ENABLED = enabled("Enabled");
+    private static final ConditionEvaluationResult DISABLED = disabled("Disabled");
+
+    private static boolean jdkMajorVersionMatches(ClientDisabled d) {
+        return Integer.toString(Runtime.version().feature()).equals(d.jdk());
+    }
+
+    private static boolean clientParameterMatches(ExtensionContext context, ClientDisabled d) {
+        return context.getConfigurationParameter(HTTP_CLIENT_CONFIGURATION).orElse("").equalsIgnoreCase(d.httpClient());
+    }
+
+    @Override
+    public ConditionEvaluationResult evaluateExecutionCondition(ExtensionContext context) {
+        return findAnnotation(context.getElement(), ClientDisabled.class)
+            .map(d -> clientParameterMatches(context, d) && jdkMajorVersionMatches(d)
+                ? DISABLED
+                : ENABLED)
+            .orElse(ENABLED_BY_DEFAULT);
+    }
+
+    /**
+     * Annotation that can be used to disable a test based on the JDK version and the HTTP client configuration.
+     */
+    @Documented
+    @Target({ElementType.TYPE, ElementType.METHOD})
+    @Retention(RetentionPolicy.RUNTIME)
+    @ExtendWith(ClientDisabledCondition.class)
+    public @interface ClientDisabled {
+
+        String httpClient() default "";
+
+        String jdk() default "";
+    }
+}

http-client-tck/src/main/java/io/micronaut/http/client/tck/tests/ContentLengthHeaderTest.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright 2017-2023 original authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.micronaut.http.client.tck.tests;
+
+import com.sun.net.httpserver.Headers;
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpHandler;
+import com.sun.net.httpserver.HttpServer;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.core.io.socket.SocketUtils;
+import io.micronaut.http.HttpRequest;
+import io.micronaut.http.MutableHttpRequest;
+import io.micronaut.http.client.HttpClient;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import reactor.core.publisher.Flux;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.URL;
+
+import static io.micronaut.http.HttpHeaders.CONTENT_LENGTH;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+class ContentLengthHeaderTest {
+
+    private static final String PATH = "/content-length";
+
+    private HttpServer server;
+
+    private ApplicationContext applicationContext;
+    private HttpClient httpClient;
+
+    @BeforeEach
+    void setUp() throws IOException {
+        server = HttpServer.create(new InetSocketAddress(SocketUtils.findAvailableTcpPort()), 0);
+        server.setExecutor(java.util.concurrent.Executors.newCachedThreadPool());
+        server.createContext(PATH, new MyHandler());
+        server.start();
+
+        applicationContext = ApplicationContext.run();
+        httpClient = applicationContext.createBean(HttpClient.class, new URL("http://localhost:" + server.getAddress().getPort()));
+    }
+
+    @AfterEach
+    void tearDown() {
+        if (server != null) {
+            server.stop(0);
+        }
+        applicationContext.stop();
+    }
+
+    @ParameterizedTest(name = "blocking={0}")
+    @ValueSource(booleans = {true, false})
+    void postContainsHeader(boolean blocking) {
+        MutableHttpRequest<String> post = HttpRequest.POST(PATH, "tim");
+        String retrieve = blocking
+            ? httpClient.toBlocking().retrieve(post)
+            : Flux.from(httpClient.retrieve(post)).blockFirst();
+        assertEquals("POST:3", retrieve);
+    }
+
+    @ParameterizedTest(name = "blocking={0}")
+    @ValueSource(booleans = {true, false})
+    @ClientDisabledCondition.ClientDisabled(httpClient = ClientDisabledCondition.JDK, jdk = "17") // The bug is fixed in the JDK HttpClient 21, so we disable the test for prior versions
+    void getContainsHeader(boolean blocking) {
+        MutableHttpRequest<String> get = HttpRequest.GET(PATH);
+        String retrieve = blocking
+            ? httpClient.toBlocking().retrieve(get)
+            : Flux.from(httpClient.retrieve(get)).blockFirst();
+        assertEquals("GET:", retrieve);
+    }
+
+    static class MyHandler implements HttpHandler {
+
+        private String header(Headers requestHeaders, String name) {
+            if (requestHeaders.containsKey(name)) {
+                return String.join(", ", requestHeaders.get(name));
+            } else {
+                return "";
+            }
+        }
+
+        @Override
+        public void handle(HttpExchange exchange) throws IOException {
+            String method = exchange.getRequestMethod();
+            String contentLength = header(exchange.getRequestHeaders(), CONTENT_LENGTH);
+            exchange.sendResponseHeaders(200, 0);
+            try (var os = exchange.getResponseBody()) {
+                os.write("%s:%s".formatted(method, contentLength).getBytes());
+            }
+            exchange.close();
+        }
+    }
+}

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

@@ -1376,7 +1376,9 @@ private static NettyRequestWriter postToWriter(String newUri, HttpPostRequestEnc
     private static FullHttpRequest withBytes(HttpRequest request, ByteBuf bytes) {
         HttpHeaders headers = request.headers();
         headers.remove(HttpHeaderNames.TRANSFER_ENCODING);
-        headers.set(HttpHeaderNames.CONTENT_LENGTH, bytes.readableBytes());
+        if (permitsRequestBody(request.method())) {
+            headers.set(HttpHeaderNames.CONTENT_LENGTH, bytes.readableBytes());
+        }
         return new DefaultFullHttpRequest(
             request.protocolVersion(),
             request.method(),
@@ -1387,6 +1389,17 @@ private static FullHttpRequest withBytes(HttpRequest request, ByteBuf bytes) {
         );
     }
 
+    private static boolean requiresRequestBody(io.netty.handler.codec.http.HttpMethod method) {
+        return method != null && (method.equals(io.netty.handler.codec.http.HttpMethod.POST) || method.equals(io.netty.handler.codec.http.HttpMethod.PUT) || method.equals(io.netty.handler.codec.http.HttpMethod.PATCH));
+    }
+
+    private static boolean permitsRequestBody(io.netty.handler.codec.http.HttpMethod method) {
+        return method != null && (requiresRequestBody(method)
+            || method.equals(io.netty.handler.codec.http.HttpMethod.OPTIONS)
+            || method.equals(io.netty.handler.codec.http.HttpMethod.DELETE)
+        );
+    }
+
     private Flux<MutableHttpResponse<?>> readBodyOnError(@Nullable Argument<?> errorType, @NonNull Flux<MutableHttpResponse<?>> publisher) {
         if (errorType != null && errorType != HttpClient.DEFAULT_ERROR_TYPE) {
             return publisher.onErrorResume(clientException -> {

test-suite-http-client-tck-jdk/src/test/java/io/micronaut/http/client/tck/jdk/tests/JdkHttpMethodTests.java

@@ -1,5 +1,7 @@
 package io.micronaut.http.client.tck.jdk.tests;
 
+import io.micronaut.http.client.tck.tests.ClientDisabledCondition;
+import org.junit.platform.suite.api.ConfigurationParameter;
 import org.junit.platform.suite.api.ExcludeClassNamePatterns;
 import org.junit.platform.suite.api.SelectPackages;
 import org.junit.platform.suite.api.Suite;
@@ -11,6 +13,7 @@
 @Suite
 @SelectPackages("io.micronaut.http.client.tck.tests")
 @SuiteDisplayName("HTTP Client TCK for the HTTP Client Implementation based on Java HTTP Client")
+@ConfigurationParameter(key = ClientDisabledCondition.HTTP_CLIENT_CONFIGURATION, value = ClientDisabledCondition.JDK)
 @SuppressWarnings("java:S2187") // This runs a suite of tests, but has no tests of its own
 @ExcludeClassNamePatterns({
     "io.micronaut.http.client.tck.tests.ContinueTest", // Unsupported body type errors

test-suite-http-client-tck-netty/src/test/java/io/micronaut/http/client/tck/netty/tests/NettyHttpMethodTests.java

@@ -1,12 +1,15 @@
 package io.micronaut.http.client.tck.netty.tests;
 
+import io.micronaut.http.client.tck.tests.ClientDisabledCondition;
+import org.junit.platform.suite.api.ConfigurationParameter;
 import org.junit.platform.suite.api.SelectPackages;
 import org.junit.platform.suite.api.Suite;
 import org.junit.platform.suite.api.SuiteDisplayName;
 
 @Suite
 @SelectPackages("io.micronaut.http.client.tck.tests")
 @SuiteDisplayName("HTTP Client TCK for the HTTP Client Implementation based on Netty")
+@ConfigurationParameter(key = ClientDisabledCondition.HTTP_CLIENT_CONFIGURATION, value = ClientDisabledCondition.NETTY)
 @SuppressWarnings("java:S2187") // This runs a suite of tests, but has no tests of its own
 public class NettyHttpMethodTests {
 }