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

[Daniel Fuchs]

On Fri, 13 Nov 2020 16:45:31 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 12 additional commits since the last revision:
> 
>  - 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)
>  - 8252304: assertBodyPublisherEqual added to test; added comment to newBuilder
>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>  - Merge remote-tracking branch 'origin/master' into JDK-8252304
>  - ... and 2 more: https://git.openjdk.java.net/jdk/compare/74f1b7e7...fd6cc9e7

I would like to see more tests that tests all possible usages of the `filter`. In particular, all usages documented in the `@apiNote` should be tested to prove that they work as expected. And then a non trivial usage that remove two non-contiguous values in a multi-valued header that has e.g. 5 or 7 values. Possibly also testing removing the first, removing the last, removing neither the first nor the last, etc...

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

> 169:         while (iter1.hasNext() && iter2.hasNext())
> 170:             assertEquals(iter1.next(), iter2.next());
> 171:     }

This code doesn't seem to be correct. It won't work if the `BiPredicate` vetoes only 1 value for a multi-valued request. This lets me think that this scenario is not tested, or if it is tested, then it's not tested with this method.
A simpler implementation would be to rely on map equality. That should work as both map are case insensitive.
That is something like:
(r1.headers().map(), HttpHeaders.of(r2.headers().map(), filter));```
assuming r1 is the request in which headers have been removed according to `filter`.

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

Changes requested by dfuchs (Reviewer).

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