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

[Daniel Fuchs]

On Fri, 8 Apr 2022 10:19:08 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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.
>
>> 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.
> 
> Please disregard this part of the comment. As you and Daniel rightly noted in a private conversation, the `HttpRequestBuilderImpl` overrides the `HEAD()` method to set `null` to the body publisher.

@jaikiran @c-cleary 


// 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");
    }
}


Sounds good to me.

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

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