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
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
