On Fri, 13 Nov 2020 16:45:31 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 > > 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
