On Fri, 8 Apr 2022 10:19:08 GMT, Jaikiran Pai <[email protected]> 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
