RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v3]

[Patrick Concannon]

On Wed, 4 Nov 2020 15:11:50 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Patrick Concannon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
>> 
>>  - 8252304: Removed catch block from newBuilder(HttpRequest)
>>  - 8252304: assertBodyPublisherEqual added to test; added comment to newBuilder
>>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>>  - 8252304: Seed an HttpRequest.Builder from an existing HttpRequest
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 317:
> 
>> 315:      *         the given request (for instance, if the request contains illegal
>> 316:      *         parameters)
>> 317:      * @since TBD
> 
> `@since 16`
> 
> we can change it again if it doesn't make the cut

Release number added in commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b

> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 339:
> 
>> 337:                         }
>> 338:                     }
>> 339:             );
> 
> I know what this piece of code does, and why it is necessary, but it would be good to add some `//` comments above to explain what is going on for the benefice of future maintainers

Comment added to clarify ifPresentOrElese statment. See commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b

> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 272:
> 
>> 270:        assertThrows(IAE, () -> HttpRequest.newBuilder(r).build());
>> 271:    }
>> 272: }
> 
> It is customary to have a new line at the end of files.

New line added at end of file. See commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b

> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 63:
> 
>> 61:            new NamedAssertion("headers",        (r1,r2) -> assertEquals(r1.headers(), r2.headers())),
>> 62:            new NamedAssertion("expectContinue", (r1,r2) -> assertEquals(r1.expectContinue(), r2.expectContinue()))
>> 63:    );
> 
> There should be an assertion for request bodies as well. You could check the following:
> 
> 1. if r1 has a body, r2 must have a body
> 2. if r1 has no body, then r2 must not have a body
> 3. if both r1 and r2 have a body, then the body type & content length should be identical
> 4. you could subscribe to each body publisher and verify that the set of bytes you eventually get are identical

Additional assertion added to check all 4 points raised see L109-142 in commit https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b

> test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 47:
> 
>> 45: * @test
>> 46: * @bug 8252304
>> 47: * @summary HttpRequest.NewBuilder(HttpRequest) API and behaviour checks
> 
> Nit: `HttpRequest.newBuilder`

Typo fixed. See https://github.com/openjdk/jdk/pull/1059/commits/7928083ae954c44b5357064b86af0e1a3f53588b

-------------

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