On Fri, 8 Apr 2022 10:19:08 GMT, Jaikiran Pai <j...@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

Reply via email to