On Wed, 13 Oct 2021 22:04:23 GMT, Mahendra Chhipa <d...@openjdk.java.net> wrote:
> There are some regression tests depending on sun.net.www.MessageHeader, the
> internal API dependency should be removed. Some of other internal API
> dependancies are removed in following issues :
> JDK-8273142
> JDK-8268464
> JDK-8268133
It may be that the new library HttpHeaderParser works for the small use cases
in which it is used, but its parsing of header fields is approximative. I am a
bit uncomfortable by replacing a well tested parser (MessageHeader) with
something that obviously doesn't implement the spec correctly - even though
it's possible that the cases where it would give back a wrong answer do not
occur in the scenario where it is used.
What is the underlying rationale for wanting to stop using MessageHeader in
tests?
test/jdk/sun/net/www/protocol/http/NTLMTest.java line 163:
> 161:
> 162: for (int i=start; i<end; i++) {
> 163: MessageHeader header = new MessageHeader
> (s.getInputStream());
This change introduces a subtle behaviour difference in the test that may
result in spurious "Connection Reset" exception. I would prefer if the existing
logic was kept - and the test continued parsing the request headers before
writing to the output.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 47:
> 45: while(true) {
> 46: headerString = br.readLine();
> 47: if(headerString == null || headerString.isBlank()) {
This is not completely correct, a blank line would be an error, I think, or
possibly an empty continuation line. isBlank() should be replaced by isEmpty().
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 54:
> 52: List <String> values = new ArrayList<>();
> 53: String headerValue =
> headerString.substring(headerString.indexOf(": ") + 2);
> 54: String [] valueArray = headerValue.split(",");
This is not right either - at least that's not how MessageHeader (or the
HttpClient or HttpServer header parsers work). Multi-valued header correspond
to header fields whose name appear on several lines.
foo: bar, baz
=> single value "bar, baz"
foo: bar
foo: baz
multiple values => "bar", "baz"
Usually - analyzing the value - and possibly splitting it around `,` or `;` is
done in the second step at a higher level.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 62:
> 60: } else {
> 61: requestDetails = headerString.trim();
> 62: }
This is not right: the request line - or status line - is the first line which
is read - not just any line that doesn't contain `:`
FWIW, a request line may contain the full URL and therefore may contain `:`
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 63:
> 61: requestDetails = headerString.trim();
> 62: }
> 63: }
You should also take into account continuation lines (lines that begin with a
space) - and IIRC a continuation would be \r followed by a space - so I'm not
100% sure that using readline() is completely appropriate as it will consume
\r, \n, or \r\n equally.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5937
