Hi Michael
Further feedback is in-line (please note that, to improve readability, I
took the liberty to discard points without further feedback).
On 25/03/2015 18:33, Michael McMahon wrote:
* how is compression (the
Accept-Encoding/Content-Encoding/Transfer-Encoding headers) handled?
Is this handled transparently by the HttpClient? For example, if I
request a text document, will the client automatically send a header
"Accept-Encoding: gzip, deflate" (this is the current default in
Firefox)? And once I receive the response, transparently decompress
it, so I can just do "response.body(asString())"?
It wouldn't be transparent as such. But, it should be possible to
implement it using HttpRequestBodyProcessor and
HttpResponseBodyProcessor, but I wasn't
planning to do it as part of this work initially. Someone could define
a class like: [...]
After reading up on the different headers, I agree that
Accept-Encoding/Content-Encoding must not be handled by the API.
However, I believe that the TE/Transfer-Encoding headers should be fully
supported, by adding a "compression(boolean enable)" method to
HttpClient.Builder and HttpRequest.Builder. This method would allow
transparent compression by supporting at least one of the standardized
algorithms (preferably gzip) [
http://www.iana.org/assignments/http-parameters/http-parameters.xhtml#transfer-coding
].
Basically, I believe that the API should offer full support for the
"low-level" RFC 7230 (Message Syntax and Routing), in which the
"Transfer Codings" section specifies these headers. And since
TE/Transfer-Encoding is the correct way for transparent on-the-fly
compression, it seems logical to include support for it in the API (to
me, it's on the same level as pipelining support, which is included in
the API already).
* HttpHeaders: for the proposed first*() methods, please also
consider the signature: Optional<T> firstValueAsT(String name). This
way, the application developer can decide whether to use a default
value or to throw an exception (or anything else) when the header is
absent.
How would that be implemented without knowing the type <T> ?
Sorry for the confusion, this wasn't meant to represent a generic
method. I just meant to say: please consider returning an Optional
instead of the current behavior of returning null or taking a "default
value" parameter. So instead of the current methods:
String firstValue(String name)
long firstValueAsLong(String name, long defaultValue)
consider these methods instead:
Optional<String> firstValue(String name)
OptionalLong firstValueAsLong(String name)
The advantage of the latter methods would be that you can say for example:
headers.firstValue("Content-Type").orElse("application/octet-stream")
headers.firstValueAsLong("Content-Length").orElseThrow(() -> new
IllegalStateException())
* HttpRequest::fromString and HttpResponse::asString should take a
java.nio.charset.Charset as parameter
instead of the String name of the Charset? Will consider.
I'd just like to point out that my main motivation for this is
java.nio.charset.StandardCharsets. So for charsets such as UTF-8, UTF-16
and US-ASCII, one can use a static import & simply write e.g.
asString(UTF_8). Of course, if it's estimated that most of the uses of
this method will use a non-standard charset, then it may be undesirable
having to write asString(Charset.forName("Windows-1252")) instead.
On a sidenote, the method asString(String charset) in HttpResponse
should specify to throw an UnsupportedCharsetException. And in the
method fromString(String s, String charset) in HttpRequest, the
parameter "s" should be renamed to "body".
* HttpResponse: in my opinion, "asFile" should take a Path as
parameter & the parameter name should be "file", instead of "filename"
same as above
An advantage of taking a Path, would be that you don't have to embed
(OS-specific) file separators to construct the desired argument:
asFile(downloadDirectory.resolve(Paths.get("documents", "foo.pdf"));
* please consider adding a HttpResponseBodyProcessor implementation
"asDefined()", which uses the same mechanism as
java.net.URLConnection::getContent (i.e. using the Content-Type
header & ContentHandlers) to determine the object to return. This
would allow for easy migration from the old API to the new API. (the
"defined" in the method name refers to the fact that the value of the
Content-Type header is used)
How would you determine the type of object to return? I believe that
was one limitation of that API in URLConnection
This would be a HttpResponseBodyProcessor<Object>, and the type of
object would just be whatever type the chosen ContentHandler returns
(where the ContentHandler is chosen according to the algorithm specfied
in java.net.ContentHandler).
After giving this some more thought, I think the proposed
HttpResponseBodyProcessor isn't what I'm after. Actually, what I like
about the old API, is that I can write ContentHandlers, specify the
java.content.handler.pkgs property, and then, for example, automatically
have urlConnection.getContent() return an instance of
org.w3c.dom.Document for text/html documents.
To bring this idea to the new API, I would like to propose something
like the following for consideration:
* use the java.util.ServiceLoader mechanism to find
HttpResponseBodyProcessor implementations
* in HttpResponseBodyProcessor, add the methods:
boolean canProcess(HttpHeaders headers)
Class<T> getBodyClass()
* in HttpResponse, add the following method:
<T> T body(Class<T> bodyClass) {
Iterator<HttpResponseBodyProcessor> processors =
serviceLoader.iterator();
Predicate filter = p -> p.canProcess(headers) &&
bodyClass.isAssignableFrom(p.getBodyClass());
// convert the Iterator to a Stream
HttpResponseBodyProcessor p =
processorsStream.filter(filter).findFirst().orElseThrow(() -> new
UnsupportedOperationException());
return body(p);
}
So then I'd create a .jar with all my HttpResponseBodyProcessor
implementations, and simply write:
HttpResponse response = client.request(new
URI("http://www.foo.com/index.html";)).get().send();
Document homePage = response.body(Document.class);
Many thanks for your comprehensive review!
You're welcome,
Anthony
Michael
=== documentation ===
* as I'm sure you are aware, the package Javadoc should be updated to
document the new API. I also think it would be good to clarify its
relation to the old API, JAX-RS and Java API for WebSocket (the
latter 2 of which will, I assume, in future versions use this API as
the basis for their Client API implementations).
* HttpClient: "Request builders are then created by calling
request(URI) if associated with an application created client."
rephrase as "Request builders that are associated with an
application-created client, are created by calling request(URI)."
* HttpClient.Builder: "If set, the first request to an origin server
using "http" URLs will attempt to upgrade to HTTP version 2. If that
fails, then following requests will not attempt to upgrade again."
** make "origin server" a hyperlink to
https://tools.ietf.org/html/rfc6454#section-4
** explicitly state "following requests to the same origin server"
* HttpRequest: "A request's uri, headers and body can be set."
change "uri" to "URI" & link it to java.net.URI
* HttpRequest: currently, all examples specify ".body(noBody())".
This gives the impression it's actually required. I propose to remove
this from all examples of GET requests (especially the one in the
"Two simple, example HTTP interactions" at the top).
* HttpRequest::fromString and HttpResponse::asString: replace
"ISO8859_1" with ISO-8859-1 and link to
java.nio.charset.StandardCharsets.ISO_8859_1
* HttpResponse: "such as String, byte arrays, Files."
change "Files" to "files", since "Files" suggests that it returns
java.io.File, when instead it's java.nio.file.Path
* HttpResponseBodyProcessor: "write responses to String, byte[],
File, Consumer<byte[]>"
change "File" to "file", since "File" suggests that it returns
java.io.File, when instead it's java.nio.file.Path
* HttpResponseBodyProcessor: "If an exact content length was provided
in onRequestStart(), then that number of bytes must be provided."
explicitly add "before returning null" at the end, so: "If an exact
content length was provided in onRequestStart(), then that number of
bytes must be provided before returning null."
* WebSocket: add a note that using a WebSocket in a
try-with-resources statement will cause the close to be done by
closing the underlying TCP connection & what the possible
implications are.
=== spelling ===
* HttpHeaders: "read only"
should be "read-only"
* HttpRequest: "any type as body,"
either has missing text after the comma, or the comma should be a point
* HttpRequest: "client initiated"
should be "client-initiated"
* HttpRequest: "The response body can then be received (either
synchronous or asynchronously)"
should be "The response body can then be received (either
synchronously or asynchronously)."
* HttpRequest: "Path body = r2.body(asFile("/tmp/response.txt));"
should be "Path body = r2.body(asFile("/tmp/response.txt"));"
* HttpRequest: "All of above examples"
should be "All of the above examples"
* HttpRequest: "CompletableFuture<String>; last ="
should be "CompletableFuture<String> last ="
* HttpRequest: "Returns the HttpClient.requestHttp2() setting for
this request."
should note that this setting may have been overridden using
HttpRequest.Builder.requestHttp2(), and therefore is not always equal
to HttpClient.requestHttp2()
* HttpRequest.Builder: "A builder of HttpRequests"
should be "A builder of HttpRequests." (if you look at the package
overview, all classes except this one have a point at the end)
* HttpResponseBodyProcessor: "return null and the body"
has missing text after body
* MultiResponseProcessor: "provides the"
should be "Provides the"
* WebSocketMessage: "A WebSocketMessages (Binary or Text)"
should be "A WebSocketMessage (Binary or Text)"
On 9/03/2015 17:27, Michael McMahon wrote:
Hi,
JEP 110 HTTP 2 client
in JDK 9, is defining and implementing a new API for HTTP which also
supports
the new HTTP version 2 that has recently been working its way
through the IETF.
The work also includes support for websockets (RFC 6455).
In fact, the majority of the API is agnostic about the HTTP protocol
version, with only minor
configuration settings, and support for multiple responses (Http
server push) having any direct impact.
The HTTP API is defined around three main types (HttpClient, which
is the central
point for configuration of SSL, executor service cookie management
etc), HttpRequest
and HttpResponse (which should be self explanatory).
Requests are sent/received either synchronously (blocking) or in a
non-blocking (asynchronous)
mode using java.util.future.CompletableFuture which is a powerful
new framework for
asynchronous execution introduced in JDK 8.
The API docs can be seen at the link below:
http://cr.openjdk.java.net/~michaelm/httpclient/01/
All new classes and interfaces belong to the java.net package.
A prototype implementation of this API supporting HTTP/1.1 only, is
available and will
be uploaded into the JDK 9 sandbox forest in the coming day or two.
Comments welcome!
Thanks,
Michael.
