On Thu, 7 Apr 2022 13:53:35 GMT, Conor Cleary <ccle...@openjdk.org> 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
