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