On Tue, 11 Oct 2022 12:11:57 GMT, Jaikiran Pai <[email protected]> wrote:
>> Darragh Clarke has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fixed test
>
> test/jdk/java/net/httpclient/http2/UserInfoTest.java line 62:
>
>> 60: is.readAllBytes();
>> 61: is.close();
>> 62: if (e.getRequestURI().getAuthority().contains("user@")) {
>
> I think we should be checking the value of the `:authority` header here. I
> haven't yet checked the test server code to see how we construct the URI
> returned by `Http2TestExchange.getRequestURI`, but even if we used the
> `:authority` header to create that URI, I still think we should be checking
> the request headers to verify that the right request header value is received.
Yes - good catch! I missed that. The Http2TestServerConnection does that:
String us = scheme + "://" + authority + path;
URI uri = new URI(us);
but checking the `:authority` pseudo header would be cleaner.
> test/jdk/java/net/httpclient/http2/UserInfoTest.java line 96:
>
>> 94: .loopback()
>> 95: .port(port)
>> 96: .build();
>
> Should we explicitly set the version to `HTTP_2` here to make it clear that
> we are interested in HTTP2 `:authority` header?
HTTP_2 is the default and our server is a TLS server supporting only HTTP/2 so
it doesn't seem necessary.
For the record we're using TLS here to avoid the upgrade, because for the
upgrade request the ":authority" pseudo header is reconstituted on the server
side by the test server from the HTTP/1.1 host header - and that's not the code
we want to test.
-------------
PR: https://git.openjdk.org/jdk/pull/10592
