RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest

[Daniel Fuchs]

On Wed, 4 Nov 2020 14:51:07 GMT, Patrick Concannon <pconcannon at 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