Hi Chris, Tobias,
Apologies for the delayed reply. I have spent a significant amount of
time on this feedback, both prototyping and determining any potential
impact on the API. There are a number of proposals in this email that
address some of the comments, as well as explanations for those that do
not require changes. It would be really helpful if you could acknowledge
each, after which I will file JIRA issues and get them resolved.
On 08/05/18 20:34, Chris Povirk wrote:
...
* I second the idea of making HttpHeaders a final class, as proposed
as part of JDK-8184274
<https://bugs.openjdk.java.net/browse/JDK-8184274>. While "final
class" was part of that suggestion, it doesn't seem to have been
specifically shot down. Tobias suggested that the goal of an
extensible class was to let users plug in a lazily parsed map for
HTTP 2, but he didn't seem to think this was likely to work, given
the HTTP 2 spec. The pros I see for a final class, besides the usual
advantages of simplicity, are: It could ensure that instances use a
Map that retains key order and doesn't contain nulls. It could also
implement case insensitivity, which the current implementation
claims to do but doesn't. (Maybe it's relying on internal callers to
do normalize strings before calling it? But that doesn't help the
external caller who asks for accept-encoding instead of
Accept-Encoding.)
For HTTP/2, to keep the client and server HPACK indexing tables
consistent it is required that all header block fragments be decoded.
HTTP/1.1, obviously, does not have such a restriction. A lazy parsing
implementation of HTTP/1.1 headers is not likely to be common, but
possible in the current design.
HttpHeaders instances are required, by specification, to be immutable,
but I accept that a final class, and implementation, is a stronger
contract. Ordering of any elements of the HttpHeaders is not something
that we want to guarantee, and would likely lead to hard to diagnose
issues if some aspect of user code depended on it, since the HTTP
protocol does not provide any such ordering guarantees, nor the
iteration order of the returned map.
All that said, on balance I think it may be better to make HttpHeaders
final. Given the requirement for other client implementations to be
able to create an HttpHeaders, then it will need a first-class public
API for its construction, even though direct construction of an
HttpHeaders is not something that we necessarily want to promote in the
API. Rather HttpHeaders are returned from an instance of HttpRequest or
HttpResponse.
Proposal: Make HttpHeaders final, as follows:
1) Change from abstract to final class ( obviously )
2) Remove its protected constructor
3) Remove the "default implementation" notes ( since there will be
just one implementation )
4) Provide a public factory method:
/**
* Returns an HTTP headers from the given map. The given map's key
* represents the header name, and its value the list of header
* values for that header name.
*
* @param headerMap the map containing the header names and values
* @param filter a filter that can be used to inspect each entry in
* the given map to determine if it should, or should
* not, be added to the to the HTTP headers
* @return an HTTP headers instance containing the given headers
* @throws NullPointerException if any of: {@code headerMap}, a key
* or value in the given map, or an entry in the map's value
* list, or {@code filter}, is {@code null}
*/
public static HttpHeaders of(Map<String, List<String>> headerMap,
BiPredicate<String, List<String>>
filter)
After much prototyping, a filter argument in the API will significant
reduce the need to re-creating Maps just for the purpose of feeding
into an HttpHeaders ( and a defensive copy of the Map must be performed
by the factory to ensure structural immutability ). The above also
addresses the nullability concern.
---
From the HttpHeaders class-level description:
"The methods of this class ( that accept a String header name ), and
the Map returned by the map method, operate without regard to case
when retrieving the header value."
The following demonstrates the case insensitivity.
$ ~/binaries/jdk-11.jdk/Contents/Home/bin/jshell
| Welcome to JShell -- Version 11-ea
| For an introduction type: /help intro
jshell> import java.net.http.*;
jshell> URI uri = URI.create("http://foo.com";);
uri ==> http://foo.com
jshell> HttpRequest req =
HttpRequest.newBuilder(uri).header("Accept-Encoding", "gzip,
deflate").build();
req ==> http://foo.com GET
jshell> HttpHeaders headers = req.headers();
headers ==> jdk.internal.net.http.ImmutableHeaders@d9233844 { ...
encoding=[gzip, deflate]} }
jshell> System.out.println(headers.firstValue("accept-encoding").get());
gzip, deflate
If you do not find that this is the case, then that's a bug. Please
create a short example, or otherwise describe how to reproduce the
issue you encounter.
* The HttpHeaders doc could be clearer about what it does with
comma-separated header values. I assume that the caller just gets
the whole string as a single value, since HttpHeaders would have to
know whether a given header is comma-separated or not, but some
users might expect it to be split, especially given the List<String>
return type and the interchangeability of comma-separated values and
multiple headers in some cases.
HttpHeaders does not do any interpretation of header values.
Proposal: Add a minor specification clarification to avoid any future
potential confusion on this point, such as:
"Header values are uninterpreted strings."
* Could HttpClient drop its getters? They feel like implementation
details of the default HttpClient implementation, not properties of
an HTTP client in the abstract. (e.g., even if my alternative
implementation were to support cookies, it might not use the JDK's
cookie API.) It feels kind of like if Future had executor() and
runnable() getters: *Many* Futures will have reasonable values for
them, but not all, so I'd hope most users would avoid using them,
anyway, to be safe. Or, if the getters stick around, I could see
putting them on some kind of DefaultHttpClient class (to which the
current HttpClient.Builder might move).
The getters are necessary and are part of the abstract client. If one
has an HttpClient, whose construction is unknown, then it may be
necessary to determine the client's configuration. For example, if a
cookie handler has been configured, then the code performing exchanges
through the client need not explicitly process cookie related headers,
that can be left to the cookie handler. Equally, the calling code should
be able to determine if a cookie handler is not configured, so that it
can take appropriate action. Similar with redirects and authentication.
The Executor is a little different. To provide the most flexibility and
allow for better use of resources, to build user-level chains of
asynchronous dependent actions, the executor is required. Again, this is
when the construction-site and use-site of the client do not have
explicit knowledge of each other.
Providing finer grain abstractions than the existing HttpClient seems
not worthwhile.
* I'm a little confused by HttpRequest. At first, I thought it was a
value type like HttpHeaders, so I was going to have the same
suggestion of making it a final type (and also its builder). But the
implementation appears to be mutable and to contain additional
fields. It feels like the type is pulling double duty as both a
value type in the public API (which could be nice to make final) and
as a stateful internal type (which might contain an instance of the
public type), and it's not clear if different implementations could
successfully interchange HttpRequest instances. (Tobias said that
he'd raised this point earlier, and it sounds like it may
realistically have to stay how it is, but since this was my reaction
and it turned out to match what he'd said, I wanted to at least
mention it again.)
From an API perspective HttpRequest is immutable, and all
implementations returned from the client exhibit this behaviour. If not,
then it's a bug. I think, since you clearly looked at the
implementation, that you noticed that the HTTP request implementation
has some mutable state that it carries, that is required for certain
parts of the default implementation. That is not unusual or any cause
for alarm, and does not have any impact on the exposed state.
There can be different HttpRequest implementations. For example, the
default implementation disallows requests with the CONNECT method -
which is used for tunneling - and there are restrictions around its use
when performing security manager permission checks. Another
implementation may not have such a restriction. There are a few other
small constraints and restrictions specific to the default client that
another implementation may choose to do differently.
It is for these reasons that neither HttpRequest or its Builder are
final. More generally, there is a clear tension between extensibility
and finality. We favour the former to be more friendly to other
implementations, and to support scenarios like offline testing ( and to
a lesser extent mocking ).
* In the HttpRequest.Builder docs, it would be nice to call out that
GET is the default.
Agreed. The fact that the default request method is GET could be made
clearer.
Proposal: to HttpRequest.Builder add:
* <p> The builder can be used to configure per-request state, like:
* the request URI, the request method (default is GET unless
* explicitly set ), specific request headers, etc.
-Chris.
