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

[Chris Hegarty]

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

Mostly looks good.

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

Please add the specific release number, in this case `16`.

src/java.net.http/share/classes/java/net/http/HttpRequest.java line 307:

> 305: 
> 306:     /**
> 307:      * Creates a {@code Builder} seeded from a {@code HttpRequest}.

We use *an* HttpXXX consistently elsewhere. Please do the same here.

test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 48:

> 46: * @bug 8252304
> 47: * @summary HttpRequest.NewBuilder(HttpRequest) API and behaviour checks
> 48: * @compile --enable-preview -source ${jdk.version} HttpRequestNewBuilderTest.java

records are now final so these command line args can be removed.

src/java.net.http/share/classes/java/net/http/HttpRequest.java line 344:

> 342:             throw ex;
> 343:         } catch (RuntimeException r) {
> 344:             throw new IllegalArgumentException("Illegal request parameters", r);

I'm a little concerned about this. It seems unnecessary, and adds complexity to an otherwise straightforward piece of code. Any accessor of the given request that throws should probably just be allowed to flow out. If needed, we could even mention that in the specification.

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

Changes requested by chegar (Reviewer).

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