On Thu, 11 Nov 2021 12:05:49 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review for this change which implements the enhancement > noted in https://bugs.openjdk.java.net/browse/JDK-8276559? > > The commit in this PR introduces a new `HEAD()` method on the > `HttpRequest.Builder` interface. Given that this is a public interface on a > public class of a public module, I decided to add this new method as a > `default` method to prevent any potential breakages of any non-JDK > implementations of this interface. The internal > `jdk.internal.net.http.HttpRequestBuilderImpl` which implements this > interface, overrides this default implementation to use an implementation of > its own. > > Existing tests have been updated to include coverage for this new method. > > P.S: Slightly unrelated question - is it intentional that the `Builder` > interface specifies the visibility modifiers on the interface methods. For > example: `public abstract Builder timeout(Duration duration);`, `public > Builder headers(String... headers);` and so on? Maybe `test/jdk/java/net/httpclient/HeadTest.java` could acquire a new dedicated method where `HttpRequest.Builder.HEAD()` is used. The test currently tests `method("HEAD", BodyPublishers.noBody());` src/java.net.http/share/classes/java/net/http/HttpRequest.java line 382: > 380: case "GET" -> builder.GET(); > 381: case "DELETE" -> builder.DELETE(); > 382: case "HEAD" -> builder.HEAD(); The comment above (line 376) mentions: // otherwise, the body is absent, special case for GET/DELETE, // or else use empty body maybe HEAD should be added there as well... test/jdk/java/net/httpclient/HttpRequestNewBuilderTest.java line 124: > 122: // dedicated method > 123: { > HttpRequest.newBuilder(URI.create("https://method-1/";)).GET().build() }, > 124: { > HttpRequest.newBuilder(URI.create("https://method-1/";)).HEAD().build() }, I'd suggest to change the URI path to `/method-0/` and move the HEAD case to just after line 122 (before GET) ------------- PR: https://git.openjdk.java.net/jdk/pull/6348
