Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v7]
> 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 13 additional commits since the last revision: - 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) - 8252304: assertBodyPublisherEqual added to test; added comment to newBuilder - Merge remote-tracking branch 'origin/master' into JDK-8252304 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/edc65dc9...0de09975 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1059/files - new: https://git.openjdk.java.net/jdk/pull/1059/files/fd6cc9e7..0de09975 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=05-06 Stats: 223 lines in 5 files changed: 140 ins; 12 del; 71 mod Patch: https://git.openjdk.java.net/jdk/pull/1059.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1059/head:pull/1059 PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8255758: JEP 380 spec clarifications [v2]
> Minor spec changes from spec approved in initial CSR Michael McMahon 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 five additional commits since the last revision: - spec update after Alan's comments - Merge branch 'master' into 8255758-spec-change - net-properties.html change only. src apidoc change to come - update to net-properties.html - 8255758: JEP 380 spec clarifications - Changes: - all: https://git.openjdk.java.net/jdk/pull/1021/files - new: https://git.openjdk.java.net/jdk/pull/1021/files/e17e78b4..52c12762 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1021&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1021&range=00-01 Stats: 118240 lines in 1590 files changed: 64315 ins; 42007 del; 11918 mod Patch: https://git.openjdk.java.net/jdk/pull/1021.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1021/head:pull/1021 PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications [v2]
On Sun, 15 Nov 2020 08:32:28 GMT, Alan Bateman wrote: > The clarification to the SecurityException looks good. > > The API docs specify that bind(null) will "bind to an automatically assigned > socket address". Would it be better to lead with that phrase in the > properties doc rather than switching to "automatically bound". That might > also avoid it getting confused with an implicitly bound sockets. > > A minor comment on the test is that "peer" might be a better than "acc1". I've just updated the spec per these comments and will update the CSR accordingly. Thx. - PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications [v2]
On Mon, 16 Nov 2020 13:29:26 GMT, Michael McMahon wrote:
>> Minor spec changes from spec approved in initial CSR
>
> Michael McMahon 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 five additional
> commits since the last revision:
>
> - spec update after Alan's comments
> - Merge branch 'master' into 8255758-spec-change
> - net-properties.html change only. src apidoc change to come
> - update to net-properties.html
> - 8255758: JEP 380 spec clarifications
Marked as reviewed by dfuchs (Reviewer).
src/java.base/share/classes/java/net/doc-files/net-properties.html line 276:
> 274: {@code conf/net.properties} configuration file.
> 275:
> 276: Implicit binding of {@link java.nio.channels.SocketChannel
> SocketChannel}s
Nit: could this be changed to:
Implicit binding of a {@link java.nio.channels.SocketChannel
SocketChannel}
-
PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8255758: JEP 380 spec clarifications [v2]
On Mon, 16 Nov 2020 14:42:02 GMT, Daniel Fuchs wrote:
>> Michael McMahon 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 five additional
>> commits since the last revision:
>>
>> - spec update after Alan's comments
>> - Merge branch 'master' into 8255758-spec-change
>> - net-properties.html change only. src apidoc change to come
>> - update to net-properties.html
>> - 8255758: JEP 380 spec clarifications
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 276:
>
>> 274: {@code conf/net.properties} configuration file.
>> 275:
>> 276: Implicit binding of {@link java.nio.channels.SocketChannel
>> SocketChannel}s
>
> Nit: could this be changed to:
> Implicit binding of a {@link java.nio.channels.SocketChannel
> SocketChannel}
Done. Would you mind adding yourself as reviewer of the CSR?
-
PR: https://git.openjdk.java.net/jdk/pull/1021
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v8]
> 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/5f4e0a40...03b13e7a - Changes: - all: https://git.openjdk.java.net/jdk/pull/1059/files - new: https://git.openjdk.java.net/jdk/pull/1059/files/0de09975..03b13e7a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1059&range=06-07 Stats: 196 lines in 10 files changed: 101 ins; 48 del; 47 mod Patch: https://git.openjdk.java.net/jdk/pull/1059.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1059/head:pull/1059 PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v6]
On Fri, 13 Nov 2020 17:16:21 GMT, Daniel Fuchs wrote: >> 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`). I removed the stream check and replaced it with an assert based on map equality. I've also added more test cases to check the scenarios suggested above. See https://github.com/openjdk/jdk/pull/1059/commits/03b13e7aa9ff7bc222bd0c466b223ec522ff2973 - PR: https://git.openjdk.java.net/jdk/pull/1059
Re: RFR: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest [v8]
On Mon, 16 Nov 2020 15:54:18 GMT, Patrick Concannon
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 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 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 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
Re: RFR: 8256427: Test com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on AIX
On Mon, 16 Nov 2020 22:28:08 GMT, Christoph Langer wrote: > The test com/sun/jndi/dns/ConfigTests/PortUnreachable.java is not working on > AIX. > > It tests that when a DNS server is unreachable it fails quickly with a > PortUnreachableException due to ICMP Destination Unreachable packets received > and not having to wait for the full timeout interval. > Unfortunately, on AIX such ICMP packets are not received, so the only > exception cause will be the timeout. Hence, this test can't work on AIX, so > it should not be executed there. > > At SAP, we had this test excluded for a long time already in our private > exclude list for AIX. I suggest this test update to be the final solution. test/jdk/com/sun/jndi/dns/ConfigTests/PortUnreachable.java line 2: > 1: /* > 2: * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights > reserved. The copyright year had been updated in https://github.com/openjdk/jdk/pull/1116 Thanks. - PR: https://git.openjdk.java.net/jdk/pull/1241
Re: RFR: 8256427: Test com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on AIX [v2]
> The test com/sun/jndi/dns/ConfigTests/PortUnreachable.java is not working on > AIX. > > It tests that when a DNS server is unreachable it fails quickly with a > PortUnreachableException due to ICMP Destination Unreachable packets received > and not having to wait for the full timeout interval. > Unfortunately, on AIX such ICMP packets are not received, so the only > exception cause will be the timeout. Hence, this test can't work on AIX, so > it should not be executed there. > > At SAP, we had this test excluded for a long time already in our private > exclude list for AIX. I suggest this test update to be the final solution. Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: Test jdk/com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on AIX - Changes: https://git.openjdk.java.net/jdk/pull/1241/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1241&range=01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1241.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1241/head:pull/1241 PR: https://git.openjdk.java.net/jdk/pull/1241
Re: RFR: 8256427: Test com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on AIX [v2]
On Tue, 17 Nov 2020 03:33:58 GMT, Jie Fu wrote: >> Christoph Langer has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains one commit: >> >> Test jdk/com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work >> on AIX > > test/jdk/com/sun/jndi/dns/ConfigTests/PortUnreachable.java line 2: > >> 1: /* >> 2: * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights >> reserved. > > The copyright year had been updated in > https://github.com/openjdk/jdk/pull/1116 > Thanks. Thanks, I've updated my change with a force-push since there were no reviews yet. - PR: https://git.openjdk.java.net/jdk/pull/1241
Re: RFR: 8256427: Test com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on AIX [v2]
On Tue, 17 Nov 2020 05:02:07 GMT, Christoph Langer wrote: >> The test com/sun/jndi/dns/ConfigTests/PortUnreachable.java is not working on >> AIX. >> >> It tests that when a DNS server is unreachable it fails quickly with a >> PortUnreachableException due to ICMP Destination Unreachable packets >> received and not having to wait for the full timeout interval. >> Unfortunately, on AIX such ICMP packets are not received, so the only >> exception cause will be the timeout. Hence, this test can't work on AIX, so >> it should not be executed there. >> >> At SAP, we had this test excluded for a long time already in our private >> exclude list for AIX. I suggest this test update to be the final solution. > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > Test jdk/com/sun/jndi/dns/ConfigTests/PortUnreachable.java does not work on > AIX LGTM - Marked as reviewed by jiefu (Committer). PR: https://git.openjdk.java.net/jdk/pull/1241
RFR: 8255675: Typo in java.net.HttpURLConnection
8255675: Typo in java.net.HttpURLConnection - Commit messages: - Merge pull request #1 from anupamdev20/JDK-8255675 - 8255675: Typo in java.net.HttpURLConnection Changes: https://git.openjdk.java.net/jdk/pull/1250/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1250&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255675 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1250/head:pull/1250 PR: https://git.openjdk.java.net/jdk/pull/1250
