On Wed, 4 Nov 2020 14:51:07 GMT, Patrick Concannon <pconcan...@openjdk.org>
wrote:
> Hi,
>
> Could someone please review our code for JDK-8252304: 'Seed an
> HttpRequest.Builder from an existing HttpRequest'?
>
> This RFR proposes a new factory method for creating a new `HttpRequest`
> builder from an existing `HttpRequest`.
> This method can be used to build a new request equivalent to the given
> request, but with different attributes. For instance, it will allow the user
> to take an existing request and add or change a particular header, provide a
> new `URI`, etc.
>
>
> Kind regards,
> Patrick & Chris
Good work Patrick! Some comments...
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
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
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`
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
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.
-------------
Changes requested by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1059
