Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Fri, 8 Apr 2022 10:19:08 GMT, Jaikiran Pai 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
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Tue, 19 Apr 2022 16:44:35 GMT, Daniel Fuchs wrote:
>> test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 202:
>>
>>> 200: } else {
>>> 201: String responseBody = exchange.getRequestMethod() + "
>>> request contained an unexpected " +
>>> 202: "Content-length header.";
>>
>> Maybe the message could include the value of `Content-Length` that was
>> received.
>
> Also it's always better to drain the request input stream even if there is no
> bytes.
You could possibly do that in `handleResponse()`
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Thu, 7 Apr 2022 13:53:35 GMT, Conor Cleary 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
test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 202:
> 200: } else {
> 201: String responseBody = exchange.getRequestMethod() + "
> request contained an unexpected " +
> 202: "Content-length header.";
Maybe the message could include the value of `Content-Length` that was received.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Tue, 19 Apr 2022 16:43:12 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8283544: Updated URI creation
>
> test/jdk/java/net/httpclient/ContentLengthHeaderTest.java line 202:
>
>> 200: } else {
>> 201: String responseBody = exchange.getRequestMethod() + "
>> request contained an unexpected " +
>> 202: "Content-length header.";
>
> Maybe the message could include the value of `Content-Length` that was
> received.
Also it's always better to drain the request input stream even if there is no
bytes.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Fri, 8 Apr 2022 09:35:31 GMT, Jaikiran Pai wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Thu, 7 Apr 2022 13:53:35 GMT, Conor Cleary 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
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
On Thu, 7 Apr 2022 13:53:35 GMT, Conor Cleary 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 @Michael-Mc-Mahon @djelinski @jaikiran the src/ and test/ changes have been updated with reference to your comments. Seeking additional feedback if there is any? Also thanks for the previous reviews, all been very helpful! - PR: https://git.openjdk.java.net/jdk/pull/8017
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v5]
> **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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8017/files - new: https://git.openjdk.java.net/jdk/pull/8017/files/92d5309a..c1ef7d29 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8017=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8017=03-04 Stats: 18 lines in 1 file changed: 7 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8017.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8017/head:pull/8017 PR: https://git.openjdk.java.net/jdk/pull/8017
