RFR: 8265123: Add static factory methods to com.sun.net.httpserver.Filter [v2]

[Daniel Fuchs]

On Mon, 26 Apr 2021 14:25:50 GMT, Julia Boes <jboes at 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