On Mon, 16 Nov 2020 15:54:18 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 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
