On Mon, 26 Apr 2021 14:25:50 GMT, Julia Boes <jb...@openjdk.org> wrote:
>> Add two static factory methods to com.sun.net.httpserver.Filter that
>> facilitate the creation of pre- and post-processing Filters:
>>
>> `public static Filter beforeResponse(String description,
>> Consumer<HttpExchange> filterImpl) {}`
>> `public static Filter afterResponse(String description,
>> Consumer<HttpExchange> filterImpl) {}`
>
> Julia Boes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update specs and add test
src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 149:
> 147: * operation.
> 148: *
> 149: * <p>The {@code description} string describes the returned filter.
> The
Maybe you could `{@linkplain #description() describes}` in this sentence.
src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 162:
> 160: * executed. The response is commonly sent by the exchange handler.
> 161: * The filter is only expected to send the response in the uncommon
> case
> 162: * that the exchange will not be handled after the filter is
> executed.
Maybe this could be rephrased as:
* The filter {@code operation} is typically not expected to handle the request
or
* {@linkplain HttpExchange#sendResponseHeaders(int, long) send response
headers}, since
* the next filter in the chain (or the exchange handler) will eventually be
invoked after the
* operation completes.
Alternatively we could use a `Predicate<HttpExchange>` instead of a consumer to
tell whether the next filter in the chain should be invoked - but that would be
another kind of worms. I believe `Consumer<HttpExchange>` is the API we want.
src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 213:
> 211: * executed and the response has commonly been sent. The filter is
> only
> 212: * expected to send the response in the uncommon case that the
> exchange
> 213: * has not been handled before the filter is executed.
Maybe we could rephrase it here too:
* executed and the response has commonly been sent. The filter
* {@code operation} is typically not expected to handle the exchange or
* {@linkplain HttpExchange#sendResponseHeaders(int, long) send the response
headers}
* as doing so is likely to fail, since the exchange is expected to have
already been handled
* at this point.
I would like to make it more obvious that `afterHandler` and `beforeHandler`
are not the right API to handle the exchange. I have the feeling that saying
that "it is only expected to send ... " does not convey that strongly enough.
src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 234:
> 232: * context.getFilters().add(filter1);
> 233: * server.start();
> 234: * }</pre>
I don't think "note the ordering" is sufficiently explanatory here: I feel some
better description of the sequence of calls that explain that filter2's
operation will be executed after filter1's if registered before would be in
order. Maybe something like:
* <p> Example of adding a sequence of afterHandler filters to a context.<br>
* Because the filter operation is invoked after the next filter in the chain
has been invoked,
* the operation of the first after filter registered will be invoked
<b>after</b> the operation
* of the second after filter registered (note the ordering):
* <pre>...</pre>
* <p>The operation of {@code filter2} which has been registered before {@code
filter1}
* will be invoked after {@code filter1} operation has been invoked.
Maybe renaming `filter1` into `a1Get` and `filter2` into `a1Set` would be less
confusing too.
test/jdk/com/sun/net/httpserver/FilterTest.java line 92:
> 90: var filter = Filter.beforeHandler("Add x-foo response header",
> 91: (var e) -> e.getResponseHeaders().set("x-foo", "bar"));
> 92: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback please?
test/jdk/com/sun/net/httpserver/FilterTest.java line 114:
> 112: var filter2 = Filter.beforeHandler("Update x-foo response
> header",
> 113: (var e) -> e.getResponseHeaders().set("x-foo",
> "barbar"));
> 114: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback?
test/jdk/com/sun/net/httpserver/FilterTest.java line 146:
> 144: }
> 145: });
> 146: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback?
test/jdk/com/sun/net/httpserver/FilterTest.java line 167:
> 165: var filter = Filter.afterHandler("Log response code",
> 166: (var e) -> respCode.set(e.getResponseCode()));
> 167: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback?
test/jdk/com/sun/net/httpserver/FilterTest.java line 190:
> 188: var filter2 = Filter.afterHandler("Read attribute",
> 189: (var e) -> attr.set((String)
> e.getAttribute("test-attr")));
> 190: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback?
test/jdk/com/sun/net/httpserver/FilterTest.java line 222:
> 220: }
> 221: });
> 222: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback?
test/jdk/com/sun/net/httpserver/FilterTest.java line 244:
> 242: var afterFilter = Filter.afterHandler("Log response code",
> 243: (var e) -> respCode.set(e.getResponseCode()));
> 244: var server = HttpServer.create(new InetSocketAddress(0), 10);
Can we use the loopback?
test/jdk/com/sun/net/httpserver/FilterTest.java line 275:
> 273: OutputStream os = exchange.getResponseBody()) {
> 274: var len = is.transferTo(os);
> 275: exchange.sendResponseHeaders(200, len);
Technically this should be:
exchange.sendResponseHeaders(200, len == 0 ? -1 : len);
-------------
PR: https://git.openjdk.java.net/jdk/pull/3468
