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

[Daniel Fuchs]

On Fri, 13 Nov 2020 17:21:02 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> 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/10a14c48...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...

Good work on the API documentation and with adding the `BiPredicate` filter. More testing needed.

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

Or if you don't want to trust `HttpHeaders.of` then you should probably use a `record` and `mapMulti` to build a stream of `(name, value)` pairs and then reassemble the map in the end (possibly using `groupingBy`).

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

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