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

[Daniel Fuchs]

On Mon, 16 Nov 2020 15:54:18 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
>
> 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 15 additional commits since the last revision:
> 
>  - 8252304: Added more test cases to check header filter
>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>  - 8252304: BiPredicate parameter added 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
>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>  - 8252304: Removed catch block from newBuilder(HttpRequest)
>  - ... and 5 more: https://git.openjdk.java.net/jdk/compare/bb344550...03b13e7a

Changes requested by dfuchs (Reviewer).

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

> 321: 
> 322:         var r = HttpRequest.newBuilder(request, filter).build();
> 323:         assertFalse(r.headers().map().containsKey("testName1"));

You could also additionally:

assertTrue(r.headers().firstValue("testName1").isEmpty());

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

> 318:     @Test(dataProvider = "testRequests")
> 319:     public void testRemoveHeader(HttpRequest request) {
> 320:         BiPredicate<String, String> filter = (n, v) -> !n.equalsIgnoreCase("testName1");

This assumes that there is at least one `request` such that `request.headers().firstValue("testName1").isPresent()` otherwise the test will trivially pass.
I wonder if we need a specific dataProvider here, so that we can assert:

assertTrue(request.headers().firstValue("testName1").isPresent());

first thing in the test? That - or have an AtomicInteger that counts how many time the original request has the conf we want to remove, and then assert that the counter > 0 in an `@AfterClass` method?

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

> 328:     public void testRemoveSingleHeaderValue(HttpRequest request) {
> 329:         BiPredicate<String, String> filter = (n, v) ->
> 330:                 n.equalsIgnoreCase("testName1") && !v.equalsIgnoreCase("testValue1");

Well same comment here - we need some kind of guarantee that at least one request has that config.

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

> 339:         BiPredicate<String, String> filter = (n, v) ->
> 340:                 !(((n.equalsIgnoreCase("testName1") && v.equalsIgnoreCase("testValue1")))
> 341:                 || ((n.equalsIgnoreCase("testName2") && v.equalsIgnoreCase("testValue2"))));

It would probably be much clearer if you used `BiPredicate.negate` and `BiPredicate.or` here to combine the two BiPredicates.

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

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