Merge pull request #3454 from kumarprabhashanand/implement-http-query-verb

Add HTTP QUERY method support (RFC 10008)

Author: velo

CHANGELOG.md

@@ -1,3 +1,9 @@
+### Version 13.14
+
+* Add support for the HTTP QUERY method (RFC 10008) — safe, idempotent, and cacheable with a
+  request body. `HttpCacheInterceptor` includes QUERY in its default cacheable set and
+  incorporates a body hash into the cache key to reduce cross-body collisions.
+
 ### Version 13.12
 
 * `UrlencodedFormContentProcessor` now honors `CollectionFormat` from `@RequestLine`/`RequestTemplate` for array and

core/src/main/java/feign/Request.java

@@ -44,7 +44,8 @@ public enum HttpMethod {
     CONNECT,
     OPTIONS,
     TRACE,
-    PATCH(true);
+    PATCH(true),
+    QUERY(true);
 
     private final boolean withBody;
 

core/src/main/java/feign/RequestLine.java

@@ -35,9 +35,10 @@
    *
    * <p>The string must begin with a valid {@linkplain feign.Request.HttpMethod HTTP method name}
    * (e.g. {@linkplain feign.Request.HttpMethod#GET GET}, {@linkplain feign.Request.HttpMethod#POST
-   * POST}, {@linkplain feign.Request.HttpMethod#PUT PUT}), followed by a space and a URI template.
-   * If only the HTTP method is specified (e.g. {@code "DELETE"}), the request will use the base URL
-   * defined for the client.
+   * POST}, {@linkplain feign.Request.HttpMethod#PUT PUT}, {@linkplain
+   * feign.Request.HttpMethod#QUERY QUERY}), followed by a space and a URI template. If only the
+   * HTTP method is specified (e.g. {@code "DELETE"}), the request will use the base URL defined for
+   * the client.
    *
    * <p>Example:
    *

core/src/test/java/feign/DefaultContractTest.java

@@ -59,6 +59,8 @@ void httpMethods() throws Exception {
     assertThat(parseAndValidateMetadata(Methods.class, "get").template()).hasMethod("GET");
 
     assertThat(parseAndValidateMetadata(Methods.class, "delete").template()).hasMethod("DELETE");
+
+    assertThat(parseAndValidateMetadata(Methods.class, "query").template()).hasMethod("QUERY");
   }
 
   @Test
@@ -422,6 +424,9 @@ interface Methods {
 
     @RequestLine("DELETE /")
     void delete();
+
+    @RequestLine("QUERY /")
+    void query();
   }
 
   interface BodyParams {

core/src/test/java/feign/client/AbstractClientTest.java

@@ -243,6 +243,28 @@ public void noResponseBodyForPatch() {
     api.noPatchBody();
   }
 
+  /**
+   * Some client implementation tests should override this test if the QUERY operation is
+   * unsupported.
+   */
+  @Test
+  public void query() throws Exception {
+    server.enqueue(new MockResponse().setBody("foo"));
+    server.enqueue(new MockResponse());
+
+    TestInterface api =
+        newBuilder().target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    assertThat(api.query("body")).isEqualTo("foo");
+
+    MockWebServerAssertions.assertThat(server.takeRequest())
+        .hasHeaders(
+            entry("Accept", Collections.singletonList("text/plain")),
+            entry("Content-Type", Collections.singletonList("application/json")),
+            entry("Content-Length", Collections.singletonList("4")))
+        .hasMethod("QUERY");
+  }
+
   @Test
   public void parsesResponseMissingLength() throws IOException {
     server.enqueue(new MockResponse().setChunkedBody("foo", 1));
@@ -583,6 +605,10 @@ public interface TestInterface {
     @Headers("Accept: text/plain")
     String patch(String body);
 
+    @RequestLine("QUERY /")
+    @Headers({"Accept: text/plain", "Content-Type: application/json"})
+    String query(String body);
+
     @RequestLine("POST")
     String noPostBody();
 

core/src/test/java/feign/client/DefaultClientTest.java

@@ -146,6 +146,18 @@ public void noResponseBodyForPatch() {
     assertThat(exception).hasCauseInstanceOf(ProtocolException.class);
   }
 
+  /**
+   * {@link java.net.HttpURLConnection} does not support the QUERY method. For now, prefer okhttp.
+   *
+   * @see java.net.HttpURLConnection#setRequestMethod
+   */
+  @Test
+  @Override
+  public void query() throws Exception {
+    RetryableException exception = assertThrows(RetryableException.class, super::query);
+    assertThat(exception).hasCauseInstanceOf(ProtocolException.class);
+  }
+
   @Test
   void canOverrideHostnameVerifier() throws IOException, InterruptedException {
     server.useHttps(TrustingSSLSocketFactory.get("bad.example.com"), false);

googlehttpclient/src/test/java/feign/googlehttpclient/GoogleHttpClientTest.java

@@ -44,6 +44,10 @@ public void noResponseBodyForPatch() {}
   @Override
   public void patch() {}
 
+  // NetHttpTransport is backed by HttpURLConnection, which does not support QUERY.
+  @Override
+  public void query() throws Exception {}
+
   @Override
   public void parsesUnauthorizedResponseBody() {}
 

http-cache/src/main/java/feign/cache/HttpCacheInterceptor.java

@@ -23,6 +23,7 @@
 import feign.interceptor.Invocation;
 import feign.interceptor.MethodInterceptor;
 import java.time.Instant;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Map;
 import java.util.function.Function;
@@ -36,7 +37,8 @@
  * <p>Successful responses (2xx) carrying an {@code ETag} or {@code Last-Modified} header are
  * stored. Responses with {@code Cache-Control: no-store} are skipped.
  *
- * <p>Default scope is HTTP {@code GET} and {@code HEAD}; override via {@link #cacheable(Function)}.
+ * <p>Default scope is HTTP {@code GET}, {@code HEAD}, and {@code QUERY}; override via {@link
+ * #cacheable(Function)}.
  *
  * <p><b>Important:</b> 304 detection relies on the configured {@link feign.codec.ErrorDecoder}
  * raising a {@link FeignException} for non-2xx responses (the default behaviour). If a custom error
@@ -129,12 +131,17 @@ private void maybeStore(String key, Object result, Response response) {
 
   private static String defaultKey(Invocation invocation) {
     RequestTemplate template = invocation.requestTemplate();
-    return invocation.methodMetadata().configKey() + "|" + template.method() + " " + template.url();
+    String base =
+        invocation.methodMetadata().configKey() + "|" + template.method() + " " + template.url();
+    byte[] body = template.body();
+    return body != null ? base + "|" + Arrays.hashCode(body) : base;
   }
 
   private static Boolean defaultCacheable(RequestTemplate template) {
     String method = template.method();
-    return "GET".equalsIgnoreCase(method) || "HEAD".equalsIgnoreCase(method);
+    return "GET".equalsIgnoreCase(method)
+        || "HEAD".equalsIgnoreCase(method)
+        || "QUERY".equalsIgnoreCase(method);
   }
 
   private static boolean containsNoStore(Map<String, Collection<String>> headers) {

http-cache/src/test/java/feign/cache/HttpCacheInterceptorTest.java

@@ -18,10 +18,18 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import feign.Client;
 import feign.Feign;
 import feign.FeignException;
 import feign.Param;
 import feign.RequestLine;
+import feign.Response;
+import feign.Util;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 import okhttp3.mockwebserver.MockResponse;
 import okhttp3.mockwebserver.MockWebServer;
 import okhttp3.mockwebserver.RecordedRequest;
@@ -44,6 +52,9 @@ interface Api {
 
     @RequestLine("POST /things")
     String create(String body);
+
+    @RequestLine("QUERY /things")
+    String query(String body);
   }
 
   private Api api() {
@@ -120,6 +131,47 @@ void postRequestsBypassCache() throws Exception {
     assertThat(recorded.getHeader("If-None-Match")).isNull();
   }
 
+  @Test
+  void queryRequestsParticipateInCache() {
+    AtomicInteger calls = new AtomicInteger();
+    Client client =
+        (request, options) -> {
+          calls.incrementAndGet();
+          if (request.headers().containsKey("If-None-Match")) {
+            return Response.builder()
+                .status(304)
+                .headers(Collections.emptyMap())
+                .request(request)
+                .build();
+          }
+          String body = request.body() != null ? new String(request.body(), Util.UTF_8) : "";
+          Map<String, Collection<String>> headers = new HashMap<>();
+          headers.put("ETag", Collections.singletonList("\"" + body + "\""));
+          return Response.builder()
+              .status(200)
+              .headers(headers)
+              .body("result-" + body, Util.UTF_8)
+              .request(request)
+              .build();
+        };
+
+    Api api =
+        Feign.builder()
+            .client(client)
+            .methodInterceptor(new HttpCacheInterceptor(store))
+            .target(Api.class, "http://localhost:0");
+
+    String first = api.query("q1");
+    String second = api.query("q2");
+    String third = api.query("q1");
+
+    assertThat(first).isEqualTo("result-q1");
+    assertThat(second).isEqualTo("result-q2");
+    assertThat(third).isEqualTo("result-q1");
+    assertThat(calls.get()).isEqualTo(3);
+    assertThat(store.size()).isEqualTo(2);
+  }
+
   @Test
   void noStoreCacheControlPreventsStorage() throws Exception {
     server.enqueue(

jaxrs2/src/test/java/feign/jaxrs2/AbstractJAXRSClientTest.java

@@ -44,6 +44,15 @@ public void patch() throws Exception {
     }
   }
 
+  @Override
+  public void query() throws Exception {
+    try {
+      super.query();
+    } catch (final RuntimeException _) {
+      Assumptions.assumeFalse(false, "JaxRS client do not support QUERY requests");
+    }
+  }
+
   @Override
   public void noResponseBodyForPut() throws Exception {
     try {