RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]

[Jaikiran Pai]

On Thu, 7 Apr 2022 13:53:35 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> **Issue**
>> When using the `HttpClient.send()` to send a GET request created using the `HttpRequest.newBuilder()`, a `Content-length: 0` header is set. This behaviour causes issues with many services as a body related header is usually not expected to be included with a GET request. 
>> 
>> **Solution**
>> `Http1Request.java` was modified so that when the request method is a GET, a `Content-length` header is not added to the request. However, if a developer chooses to include a body in a GET request (though it is generally considered bad practice), a `Content-length` header with the appropriate value will be added.
>
> Conor Cleary has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8283544: Updated URI creation

Hello Conor,

I had a look at this latest update to the `Http1Request`.  The github diff isn't easy to understand/explain in this case, so I'll paste here the latest code contained in this PR, from that method. It looks like:


if (requestPublisher == null) {
    // Not a user request, or maybe a method, e.g. GET, with no body.
    contentLength = 0;
} else {
    contentLength = requestPublisher.contentLength();
}

// GET, HEAD and DELETE with no request body should not set the Content-Length header
if (requestPublisher != null) {
    if (contentLength == 0) {
        systemHeadersBuilder.setHeader("Content-Length", "0");
    } else if (contentLength > 0) {
        systemHeadersBuilder.setHeader("Content-Length", Long.toString(contentLength));
        streaming = false;
    } else {
        streaming = true;
        systemHeadersBuilder.setHeader("Transfer-encoding", "chunked");
    }
}

I think we don't need the additional/new ` if (requestPublisher != null)` block and can instead move the contents of this `if` block into the immediately preceding `else` block. Thinking a bit more, I think this entire above code can be reduced to just:


// absence of a requestPublisher indicates a request with no body, in which 
// case we don't explicitly set any Content-Length header
if (requestPublisher != null) {
    var contentLength = requestPublisher.contentLength();
    if (contentLength == 0) {
        systemHeadersBuilder.setHeader("Content-Length", "0");
    } else if (contentLength > 0) {
        systemHeadersBuilder.setHeader("Content-Length", Long.toString(contentLength));
        streaming = false;
    } else {
        streaming = true;
        systemHeadersBuilder.setHeader("Transfer-encoding", "chunked");
    }
}

and the previous if/else block completely deleted. The absence of a `requestPublisher` would mean a request with no body. 

Additionally, I noticed that the `HttpRequest.Builder` does this for `HEAD` method:


/**
 * Sets the request method of this builder to HEAD.
 *
 * @implSpec The default implementation is expected to have the same behaviour as:
 * {@code return method("HEAD", BodyPublishers.noBody());}
 *
 * @return this builder
 * @since 18
 */
default Builder HEAD() {
    return method("HEAD", BodyPublishers.noBody());
}

This is unlike other methods, for example `DELETE()` where the body publisher itself is `null`. In the case of `HEAD` the body publisher is present but it still represents that there's no body to that request. Should we perhaps detect even this specific case (i.e. `instanceof RequestPublishers.EmptyPublisher`) and skip setting the `Content-Length` header. If we don't add this additional check, from what I see with this updated code now, we will still end up explicitly setting `Content-Length` to `0` when a `HEAD` request is generated using the `HttpRequest.Builder.HEAD()` API, since the `EmptyPublisher` will return `0` from its `contentLength()` implementation.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8017