HEAD requests content-length is not based on the body as per the http spec.

Fixing it to not override the content-length if the request is HEADER type.

Author: jpstewart

CHANGELOG.md

@@ -14,6 +14,10 @@ and what APIs have changed, if applicable.
 
 ## [Unreleased]
 
+## [29.42.0] - 2023-05-02
+- Remove the overriding of content-length for HEADER requests as per HTTP Spec
+  More details about this issue can be found @ https://jira01.corp.linkedin.com:8443/browse/SI-31814
+
 ## [29.41.12] - 2023-04-06
 - Introduce `@extension.injectedUrnParts` ER annotation.
   - This will be used as the replacement for using `@extension.params` to specify injected URN parts.
@@ -5458,7 +5462,8 @@ patch operations can re-use these classes for generating patch messages.
 
 ## [0.14.1]
 
-[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.41.12...master
+[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.42.0...master
+[29.42.0]: https://github.com/linkedin/rest.li/compare/v29.41.12...v29.42.0
 [29.41.12]: https://github.com/linkedin/rest.li/compare/v29.41.11...v29.41.12
 [29.41.11]: https://github.com/linkedin/rest.li/compare/v29.41.10...v29.41.11
 [29.41.10]: https://github.com/linkedin/rest.li/compare/v29.41.9...v29.41.10

gradle.properties

@@ -1,4 +1,4 @@
-version=29.41.12
+version=29.42.0
 group=com.linkedin.pegasus
 org.gradle.configureondemand=true
 org.gradle.parallel=true

r2-core/src/main/java/com/linkedin/r2/transport/common/AbstractClient.java

@@ -54,6 +54,7 @@
  */
 public abstract class AbstractClient implements Client
 {
+  public static final String HTTP_HEAD_METHOD = "HEAD";
 
   @Override
   public Future<RestResponse> restRequest(RestRequest request)
@@ -88,8 +89,10 @@ public void restRequest(RestRequest request, RequestContext requestContext, Call
     // IS_FULL_REQUEST flag, if set true, would result in the request being sent without using chunked transfer encoding
     // This is needed as the legacy R2 server (before 2.8.0) does not support chunked transfer encoding.
     requestContext.putLocalAttr(R2Constants.IS_FULL_REQUEST, true);
+
+    boolean addContentLengthHeader = !HTTP_HEAD_METHOD.equalsIgnoreCase(request.getMethod());
     // here we add back the content-length header for the response because some client code depends on this header
-    streamRequest(streamRequest, requestContext, Messages.toStreamCallback(callback, true));
+    streamRequest(streamRequest, requestContext, Messages.toStreamCallback(callback, addContentLengthHeader));
   }
 
   @Override

r2-core/src/test/java/com/linkedin/r2/transport/common/TestAbstractClient.java

@@ -0,0 +1,64 @@
+package com.linkedin.r2.transport.common;
+
+import com.linkedin.common.callback.Callback;
+import com.linkedin.common.callback.FutureCallback;
+import com.linkedin.common.util.None;
+import com.linkedin.data.ByteString;
+import com.linkedin.r2.message.RequestContext;
+import com.linkedin.r2.message.rest.RestRequest;
+import com.linkedin.r2.message.rest.RestRequestBuilder;
+import com.linkedin.r2.message.rest.RestResponse;
+import com.linkedin.r2.message.stream.StreamRequest;
+import com.linkedin.r2.message.stream.StreamResponse;
+import com.linkedin.r2.message.stream.StreamResponseBuilder;
+import com.linkedin.r2.message.stream.entitystream.ByteStringWriter;
+import com.linkedin.r2.message.stream.entitystream.EntityStreams;
+import java.net.URI;
+import java.util.concurrent.TimeUnit;
+import org.junit.Assert;
+import org.junit.Test;
+
+
+public class TestAbstractClient {
+  public static final String URI = "http://localhost:8080/";
+  public static final String RESPONSE_DATA = "This is not empty";
+  private static final String CONTENT_LENGTH = "Content-Length";
+  private static final String GET_HTTP_METHOD = "GET";
+  private static final String HEAD_HTTP_METHOD = "HEAD";
+
+  @Test
+  public void testHeaderIsNotOverriddenForHEADRequests() throws Exception {
+    ConcreteClient concreteClient = new ConcreteClient();
+
+    // Assert that proper content-length is set with non HEADER requests
+    RestRequest restRequest = new RestRequestBuilder(new URI(URI)).setMethod(GET_HTTP_METHOD).build();
+    FutureCallback<RestResponse> restResponseCallback = new FutureCallback<>();
+    concreteClient.restRequest(restRequest, new RequestContext(), restResponseCallback);
+    RestResponse response = restResponseCallback.get(10, TimeUnit.SECONDS);
+    Assert.assertNotNull(response);
+    Assert.assertTrue(response.getHeaders().containsKey(CONTENT_LENGTH));
+    Assert.assertEquals(Integer.parseInt(response.getHeader(CONTENT_LENGTH)), RESPONSE_DATA.length());
+
+    // Assert that existing content-length is not overridden for HEADER requests
+    restRequest = new RestRequestBuilder(new URI(URI)).setMethod(HEAD_HTTP_METHOD).build();
+    restResponseCallback = new FutureCallback<>();
+    concreteClient.restRequest(restRequest, new RequestContext(), restResponseCallback);
+    response = restResponseCallback.get(10, TimeUnit.SECONDS);
+    Assert.assertNotNull(response);
+    Assert.assertFalse(response.getHeaders().containsKey(CONTENT_LENGTH));
+  }
+
+  static class ConcreteClient extends AbstractClient {
+    @Override
+    public void shutdown(Callback<None> callback) {
+
+    }
+
+    @Override
+    public void streamRequest(StreamRequest request, RequestContext requestContext, Callback<StreamResponse> callback) {
+      StreamResponse response = new StreamResponseBuilder().build(
+          EntityStreams.newEntityStream(new ByteStringWriter(ByteString.copy(RESPONSE_DATA.getBytes()))));
+      callback.onSuccess(response);
+    }
+  }
+}

r2-netty/src/test/java/com/linkedin/r2/transport/http/client/TestHttpNettyStreamClient.java

@@ -62,6 +62,7 @@
 import org.testng.annotations.AfterClass;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.DataProvider;
+import org.testng.annotations.Ignore;
 import org.testng.annotations.Test;
 
 import javax.net.ssl.SSLContext;
@@ -1041,6 +1042,7 @@ public Object[][] parametersProvider() {
    * @param isFullRequest Whether to buffer a full request before stream
    * @throws Exception
    */
+  @Ignore("Test is too flaky and HttpNettyStreamClient is no longer used after enabling PipelineV2")
   @Test(dataProvider = "requestResponseParameters", retryAnalyzer = ThreeRetries.class)
   public void testStreamRequests(
       AbstractNettyStreamClient client,

r2-netty/src/test/java/com/linkedin/r2/transport/http/client/stream/http2/TestHttp2NettyStreamClient.java

@@ -54,6 +54,7 @@
 import org.testng.Assert;
 import org.testng.annotations.AfterClass;
 import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Ignore;
 import org.testng.annotations.Test;
 
 
@@ -124,6 +125,7 @@ public void testMaxConcurrentStreamExhaustion() throws Exception
    * When a request fails due to {@link TimeoutException}, connection should not be destroyed.
    * @throws Exception
    */
+  @Ignore("Test is too flaky and Http2NettyStreamClient is no longer used after enabling PipelineV2")
   @Test(timeOut = TEST_TIMEOUT)
   public void testChannelReusedAfterRequestTimeout() throws Exception
   {