Refactor Interceptor into Function and introduce RequestEntity (#58)

* Refactor Interceptor into Function and introduce RequestEntity

This changes changes `RequestInterceptor` from a `Consumer` to a
`Function`, to simplify Request Specification management during method execution,
specifically during Asynchronous or Reactive execution.  Modifying
the `RequestSpecification` through side-effects is difficult to reason over
and is one of the areas that existing Feign struggles with.

Additionally, `RequestEncoder` has been modified to return a `RequestEntity` to
more closely match current Feign and to make updating a Request body, content type,
content length, and charset more explicit.

Additional Changes
* Correcting Circle CI Codecov Orb Version
* Add StringRequestEntity and refactor Test Cases for coverage

.circleci/config.yml

@@ -14,7 +14,7 @@ defaults: &defaults
 
 version: 2.1
 orbs:
-  codecov: codecov/codecov@1.0.4
+  codecov: codecov/codecov@1.1.1
 jobs:
   build:
     <<: *defaults

core/src/main/java/feign/RequestEncoder.java

@@ -19,11 +19,19 @@
 import feign.http.RequestSpecification;
 
 /**
- * Consumer responsible for encoding the provided object for use on the Request.
+ * Prepares a {@link RequestEntity} for use.
  */
 @FunctionalInterface
 public interface RequestEncoder {
 
-  void apply(Object content, RequestSpecification requestSpecification);
+  /**
+   * Encode the request content for use.  May return {@code null} if the entity should be
+   * ignored.
+   *
+   * @param content to be encoded.
+   * @param requestSpecification containing the current {@link RequestSpecification}.
+   * @return a {@link RequestEntity} instance, or {@code null}.
+   */
+  RequestEntity apply(Object content, RequestSpecification requestSpecification);
 
 }

core/src/main/java/feign/RequestEntity.java

@@ -0,0 +1,54 @@
+/*
+ * Copyright 2019-2020 OpenFeign Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package feign;
+
+import java.nio.charset.Charset;
+import java.util.Optional;
+
+/**
+ * Represents the content of a {@link Request}.
+ */
+public interface RequestEntity {
+
+  /**
+   * Character Set this entity is encoded in.
+   *
+   * @return the entity {@link Charset}.
+   */
+  Optional<Charset> getCharset();
+
+  /**
+   * Length of the entity data.
+   *
+   * @return entity data length.
+   */
+  int getContentLength();
+
+  /**
+   * Content Type of the entity.
+   *
+   * @return String representation of a MIME-Type or any other acceptable Content Type.
+   */
+  String getContentType();
+
+  /**
+   * Entity Data.
+   *
+   * @return entity data.
+   */
+  byte[] getData();
+}

core/src/main/java/feign/RequestInterceptor.java

@@ -17,11 +17,15 @@
 package feign;
 
 import feign.http.RequestSpecification;
-import java.util.function.Consumer;
+import java.util.function.Function;
 
 /**
- * Consumer that can be used to modify a Request before being processed.
+ * Function used to act on a {@link RequestSpecification} during request processing.  It is possible
+ * to return an entirely new {@link RequestSpecification} from this component.
  */
-public interface RequestInterceptor extends Consumer<RequestSpecification> {
+public interface RequestInterceptor extends Function<RequestSpecification, RequestSpecification> {
 
+  static RequestInterceptor identity() {
+    return (t) -> t;
+  }
 }

core/src/main/java/feign/encoder/StringEncoder.java

@@ -17,8 +17,8 @@
 package feign.encoder;
 
 import feign.RequestEncoder;
+import feign.RequestEntity;
 import feign.http.RequestSpecification;
-import java.nio.charset.StandardCharsets;
 
 /**
  * HttpRequest Encoder that encodes the request content as a String.  This implementation uses
@@ -27,10 +27,11 @@
 public class StringEncoder implements RequestEncoder {
 
   @Override
-  public void apply(Object content, RequestSpecification requestSpecification) {
+  public RequestEntity apply(Object content, RequestSpecification requestSpecification) {
     if (content != null) {
-      /* use the content objects toString() method to populate the request */
-      requestSpecification.content(content.toString().getBytes(StandardCharsets.UTF_8));
+      return new StringRequestEntity(content.toString());
     }
+    return null;
   }
+
 }

core/src/main/java/feign/encoder/StringRequestEntity.java

@@ -0,0 +1,59 @@
+/*
+ * Copyright 2019-2020 OpenFeign Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package feign.encoder;
+
+import feign.RequestEntity;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import java.util.Optional;
+
+/**
+ * Simple Request Entity backed by a String.
+ */
+public class StringRequestEntity implements RequestEntity {
+
+  public static final String TEXT_PLAIN = "text/plain";
+  private final byte[] data;
+  private final int length;
+  private final Charset charset = StandardCharsets.UTF_8;
+
+  public StringRequestEntity(String content) {
+    this.data = content.getBytes(StandardCharsets.UTF_8);
+    this.length = data.length;
+  }
+
+  @Override
+  public Optional<Charset> getCharset() {
+    return Optional.of(this.charset);
+  }
+
+  @Override
+  public int getContentLength() {
+    return this.length;
+  }
+
+  @Override
+  public String getContentType() {
+    return TEXT_PLAIN;
+  }
+
+  @Override
+  public byte[] getData() {
+    return Arrays.copyOf(this.data, this.data.length);
+  }
+}

core/src/main/java/feign/impl/AbstractTargetMethodHandler.java

@@ -21,6 +21,7 @@
 import feign.Logger;
 import feign.Request;
 import feign.RequestEncoder;
+import feign.RequestEntity;
 import feign.RequestInterceptor;
 import feign.Response;
 import feign.ResponseDecoder;
@@ -51,14 +52,14 @@ public abstract class AbstractTargetMethodHandler implements TargetMethodHandler
 
   protected final org.slf4j.Logger log = LoggerFactory.getLogger(this.getClass());
   protected TargetMethodDefinition targetMethodDefinition;
-  private RequestEncoder encoder;
-  private List<RequestInterceptor> interceptors;
-  private Client client;
-  private ResponseDecoder decoder;
-  private ExceptionHandler exceptionHandler;
-  private Logger logger;
-  private Executor executor;
-  private Retry retry;
+  private final RequestEncoder encoder;
+  private final List<RequestInterceptor> interceptors;
+  private final Client client;
+  private final ResponseDecoder decoder;
+  private final ExceptionHandler exceptionHandler;
+  private final Logger logger;
+  private final Executor executor;
+  private final Retry retry;
 
   /**
    * Creates a new Abstract Target HttpMethod Handler.
@@ -106,9 +107,6 @@ protected AbstractTargetMethodHandler(
   @Override
   public Object execute(final Object[] arguments) {
 
-    /* prepare the request specification */
-    final RequestSpecification requestSpecification = this.resolve(arguments);
-
     /* create an asynchronous chain, using the provided executor.  this implementation
      * is not non-blocking, however, it does try to take advantage of the executor provided.
      *
@@ -117,9 +115,10 @@ public Object execute(final Object[] arguments) {
      * this call effectively synchronous.
      */
     CompletableFuture<Object> result =
-        CompletableFuture.runAsync(() -> intercept(requestSpecification), this.executor)
-            .thenRunAsync(() -> encode(requestSpecification, arguments), this.executor)
-            .thenApplyAsync(nothing -> requestSpecification.build(), this.executor)
+        CompletableFuture.supplyAsync(() -> this.resolve(arguments), this.executor)
+            .thenApplyAsync(this::intercept, this.executor)
+            .thenApplyAsync(specification -> encode(specification, arguments), this.executor)
+            .thenApplyAsync(RequestSpecification::build, this.executor)
             .thenApplyAsync(this::request, this.executor)
             .handleAsync((response, throwable) -> {
               if (throwable != null) {
@@ -171,11 +170,19 @@ protected RequestSpecification resolve(Object[] arguments) {
    *
    * @param requestSpecification to intercept.
    */
-  protected void intercept(RequestSpecification requestSpecification) {
-    log.debug("Applying interceptors");
-    for (RequestInterceptor interceptor : this.interceptors) {
-      interceptor.accept(requestSpecification);
+  protected RequestSpecification intercept(RequestSpecification requestSpecification) {
+    if (!this.interceptors.isEmpty()) {
+      log.debug("Applying interceptors");
+      RequestSpecification result = requestSpecification;
+      for (RequestInterceptor interceptor : this.interceptors) {
+        /* apply all of the interceptors, in order */
+        result = interceptor.apply(result);
+      }
+      return result;
     }
+
+    /* no op */
+    return requestSpecification;
   }
 
   /**
@@ -197,12 +204,17 @@ protected Optional<Object> getRequestBody(Object[] arguments) {
    * @param requestSpecification to receive the encoded result.
    * @param arguments that may contain the request body.
    */
-  protected void encode(RequestSpecification requestSpecification, Object[] arguments) {
+  protected RequestSpecification encode(RequestSpecification requestSpecification,
+      Object[] arguments) {
     this.getRequestBody(arguments)
         .ifPresent(body -> {
           log.debug("Encoding Request Body: {}", body.getClass().getSimpleName());
-          this.encoder.apply(body, requestSpecification);
+          RequestEntity entity = this.encoder.apply(body, requestSpecification);
+          if (entity != null) {
+            requestSpecification.content(entity.getData());
+          }
         });
+    return requestSpecification;
   }
 
   /**
@@ -215,7 +227,7 @@ protected Response request(final Request request) {
     try {
       this.logRequest(targetMethodDefinition.getTag(), request);
       final Response response =
-          this.retry.execute(targetMethodDefinition.getTag(), request, req -> client.request(req));
+          this.retry.execute(targetMethodDefinition.getTag(), request, client::request);
       this.logResponse(targetMethodDefinition.getTag(), response);
       return response;
     } catch (Throwable th) {
@@ -262,15 +274,13 @@ private Object decode(Response response, TypeDefinition typeDefinition) throws E
      * certain types that act as containers.  when decoding these types, we want to
      * decode the 'contained' type and then wrap the result in the desired container,
      */
-    try {
+    try (response) {
       if (typeDefinition.isContainer()) {
         /* we want to decode the actual type */
         return this.decoder.decode(response, typeDefinition.getActualType());
       } else {
         return this.decoder.decode(response, typeDefinition.getType());
       }
-    } finally {
-      response.close();
     }
   }
 

core/src/test/java/feign/FeignTests.java

@@ -255,7 +255,10 @@ void create_withConfiguration() {
         .encoder(new StringEncoder())
         .decoder(new StringDecoder())
         .exceptionHandler(exceptionHandler)
-        .interceptor(requestSpecification -> System.out.println("intercept"))
+        .interceptor(requestSpecification -> {
+          System.out.println("intercept");
+          return requestSpecification;
+        })
         .executor(Executors.newSingleThreadExecutor())
         .logger(logger)
         .retry(retry)

core/src/test/java/feign/encoder/StringEncoderTest.java

@@ -16,13 +16,11 @@
 
 package feign.encoder;
 
-import static org.mockito.ArgumentMatchers.any;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyZeroInteractions;
 
 import feign.RequestEncoder;
+import feign.RequestEntity;
 import feign.http.RequestSpecification;
 import org.junit.jupiter.api.Test;
 
@@ -32,15 +30,16 @@ class StringEncoderTest {
   void apply_toString() {
     RequestEncoder encoder = new StringEncoder();
     RequestSpecification requestSpecification = mock(RequestSpecification.class);
-    encoder.apply("content", requestSpecification);
-    verify(requestSpecification, times(1)).content(any(byte[].class));
+    RequestEntity entity = encoder.apply("content", requestSpecification);
+    assertThat(entity).isNotNull();
+    assertThat(entity.getData()).isNotEmpty();
   }
 
   @Test
   void apply_withNullSkips() {
     RequestEncoder encoder = new StringEncoder();
     RequestSpecification requestSpecification = mock(RequestSpecification.class);
-    encoder.apply(null, requestSpecification);
-    verifyZeroInteractions(requestSpecification);
+    RequestEntity entity = encoder.apply(null, requestSpecification);
+    assertThat(entity).isNull();
   }
 }