Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]

2022-05-16 Thread Jaikiran Pai
On Mon, 16 May 2022 08:59:43 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: Added in missing case

Thank you for the changes, Conor. The latest state looks fine to me.

-

Marked as reviewed by jpai (Committer).

PR: https://git.openjdk.java.net/jdk/pull/8017


Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]

2022-05-16 Thread Daniel Fuchs
On Mon, 16 May 2022 08:59:43 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: Added in missing case

LGTM. If the test is stable you can integrate.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8017


Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v8]

2022-05-16 Thread Conor Cleary
> **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: Added in missing case

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8017/files
  - new: https://git.openjdk.java.net/jdk/pull/8017/files/f6efc915..28edba8c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8017=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8017=06-07

  Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 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