On Fri, 13 Nov 2020 17:21:02 GMT, Daniel Fuchs <dfu...@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
