Declarative contracts (#1060)

* Declarative contracts

* Actually using the data structure to read declaritve contracts

* Using declarative contract for jaxrs contracts

* Make possible for contracts to declare parameters as ignored

* Using predicate to decide if an AnnotationProcessor should be invoked

* Restore environment variable for GITHUB_TOKEN

Author: velo

.travis.yml

@@ -32,4 +32,9 @@ jobs:
       jdk: openjdk8
       install: true
       script:
-        - ./mvnw -B -nsu -s ./travis/settings.xml -P release -pl -:feign-benchmark -DskipTests=true deploy
+        - ./mvnw -B -nsu -s ./travis/settings.xml -P release -pl -:feign-benchmark -DskipTests=true deploy
+
+env:
+  global:
+    # Ex. travis encrypt GITHUB_TOKEN=token_for_tests
+    - secure: "H4PuppuPE3lkvVQ1osulhgWeZmpIkDKj/z74lx4MUeDPNtcuqpwmTVWtL5Zyjf8CxlALX2djx4RIBshaQAu4GtKarPLONinNLZ/TCtoK8dF08/ESxLEiLQzwGkS+geWoEFiZncB5Px2T7ZbUfVFO3crVY9CLn35znR8k1uidocL0JlyVPGwCwuBxFmDhs3BZh3JvbwSikAVRvlCRU6BbREFQbSK1EamuUju/rlo+dx7W5tiiuEJJ50c8vpgatTFyy821YP82fMRrhuBDpS4/rsL9DmLhQTEbCjZW+22DhEFPRlo0XIfidC7APybXnu3oO+jFuGaFKiQdy7sjB03g/Bz5H7jAIAkbl8UpbjN+IoeUU/OgMuBYf5wJjPDYUEdI3CXqywPn0xYZwVsOcSg+UkQGYdW9ux/U+nKsYLXLWWhst2QMFzbmO94KCrpgCW4mshr/5WP4XU6cEJwDsKMAUPWuOk0KMMjIufSgvPvteWZwT9akZwzEMuGaUQ5kLr1X6xTPv1cKXTreitaoOLQs28kmPVfTwVEdareaSVXcRqeflJJBSXkAgBqGhV5CAEUaUgt9/QD0Jj5RGyRPllFcydXVLTPeg62X/L5COswlvJhPkvfNnkbMpDQZYojKKPmAf+UqZJmVYPpOoNEXygldueKeunWkna/wYkMj0YnOkM8="

core/src/main/java/feign/Contract.java

@@ -15,11 +15,7 @@
 
 import java.io.Serializable;
 import java.lang.reflect.Type;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import feign.Param.Expander;
 
 public final class MethodMetadata implements Serializable {
@@ -41,6 +37,7 @@ public final class MethodMetadata implements Serializable {
       new LinkedHashMap<Integer, Class<? extends Expander>>();
   private Map<Integer, Boolean> indexToEncoded = new LinkedHashMap<Integer, Boolean>();
   private transient Map<Integer, Expander> indexToExpander;
+  private BitSet parameterToIgnore = new BitSet();
 
   MethodMetadata() {}
 
@@ -163,4 +160,50 @@ public MethodMetadata indexToExpander(Map<Integer, Expander> indexToExpander) {
   public Map<Integer, Expander> indexToExpander() {
     return indexToExpander;
   }
+
+  /**
+   * @param i individual parameter that should be ignored
+   * @return this instance
+   */
+  public MethodMetadata ignoreParamater(int i) {
+    this.parameterToIgnore.set(i);
+    return this;
+  }
+
+  public BitSet parameterToIgnore() {
+    return parameterToIgnore;
+  }
+
+  public MethodMetadata parameterToIgnore(BitSet parameterToIgnore) {
+    this.parameterToIgnore = parameterToIgnore;
+    return this;
+  }
+
+  /**
+   * @param i individual parameter to check if should be ignored
+   * @return true when field should not be processed by feign
+   */
+  public boolean shouldIgnoreParamater(int i) {
+    return parameterToIgnore.get(i);
+  }
+
+  /**
+   * @param index
+   * @return true if the parameter {@code index} was already consumed by a any
+   *         {@link MethodMetadata} holder
+   */
+  public boolean isAlreadyProcessed(Integer index) {
+    return index.equals(urlIndex)
+        || index.equals(bodyIndex)
+        || index.equals(headerMapIndex)
+        || index.equals(queryMapIndex)
+        || indexToName.containsKey(index)
+        || indexToExpanderClass.containsKey(index)
+        || indexToEncoded.containsKey(index)
+        || (indexToExpander != null && indexToExpander.containsKey(index))
+        || parameterToIgnore.get(index);
+  }
+
+
+
 }

core/src/main/java/feign/MethodMetadata.java

@@ -187,8 +187,9 @@ Options findOptions(Object[] argv) {
     if (argv == null || argv.length == 0) {
       return this.options;
     }
-    return (Options) Stream.of(argv)
-        .filter(o -> o instanceof Options)
+    return Stream.of(argv)
+        .filter(Options.class::isInstance)
+        .map(Options.class::cast)
         .findFirst()
         .orElse(this.options);
   }

core/src/main/java/feign/SynchronousMethodHandler.java

@@ -82,7 +82,7 @@ Contract contract(BeanFactory beanFactory) {
     }
   }
 
-  static class ContractWithRuntimeInjection extends Contract.Default {
+  static class ContractWithRuntimeInjection implements Contract {
     final BeanFactory beanFactory;
 
     ContractWithRuntimeInjection(BeanFactory beanFactory) {
@@ -94,7 +94,7 @@ static class ContractWithRuntimeInjection extends Contract.Default {
      */
     @Override
     public List<MethodMetadata> parseAndValidatateMetadata(Class<?> targetType) {
-      List<MethodMetadata> result = super.parseAndValidatateMetadata(targetType);
+      List<MethodMetadata> result = new Contract.Default().parseAndValidatateMetadata(targetType);
       for (MethodMetadata md : result) {
         Map<Integer, Param.Expander> indexToExpander = new LinkedHashMap<Integer, Param.Expander>();
         for (Map.Entry<Integer, Class<? extends Param.Expander>> entry : md.indexToExpanderClass()

core/src/test/java/feign/ContractWithRuntimeInjectionTest.java

@@ -814,7 +814,7 @@ interface MissingMethod {
   public void missingMethod() throws Exception {
     thrown.expect(IllegalStateException.class);
     thrown.expectMessage(
-        "RequestLine annotation didn't start with an HTTP verb on method updateSharing");
+        "RequestLine annotation didn't start with an HTTP verb on method MissingMethod#updateSharing");
 
     contract.parseAndValidatateMetadata(MissingMethod.class);
   }

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

@@ -93,4 +93,19 @@ public RequestTemplateAssert hasNoHeader(final String encoded) {
     objects.assertNull(info, actual.headers().get(encoded));
     return this;
   }
+
+  public RequestTemplateAssert noRequestBody() {
+    isNotNull();
+    if (actual.requestBody() != null) {
+      if (actual.requestBody().bodyTemplate() != null) {
+        failWithMessage("\nExpecting requestBody.bodyTemplate to be null, but was:<%s>",
+            actual.requestBody().bodyTemplate());
+      }
+      if (actual.requestBody().asBytes() != null) {
+        failWithMessage("\nExpecting requestBody.data to be null, but was:<%s>",
+            actual.requestBody().asString());
+      }
+    }
+    return this;
+  }
 }

core/src/test/java/feign/assertj/RequestTemplateAssert.java

@@ -13,25 +13,22 @@
  */
 package feign.jaxrs;
 
-import feign.Contract;
-import feign.MethodMetadata;
-import feign.Request;
-import javax.ws.rs.*;
-import java.lang.annotation.Annotation;
-import java.lang.reflect.Method;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
 import static feign.Util.checkState;
 import static feign.Util.emptyToNull;
 import static feign.Util.removeValues;
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
+import java.util.Collections;
+import javax.ws.rs.*;
+import feign.Contract.DeclarativeContract;
+import feign.MethodMetadata;
+import feign.Request;
 
 /**
  * Please refer to the <a href="https://github.com/Netflix/feign/tree/master/feign-jaxrs">Feign
  * JAX-RS README</a>.
  */
-public class JAXRSContract extends Contract.BaseContract {
+public class JAXRSContract extends DeclarativeContract {
 
   static final String ACCEPT = "Accept";
   static final String CONTENT_TYPE = "Content-Type";
@@ -44,52 +41,49 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
     return super.parseAndValidateMetadata(targetType, method);
   }
 
-  @Override
-  protected void processAnnotationOnClass(MethodMetadata data, Class<?> clz) {
-    Path path = clz.getAnnotation(Path.class);
-    if (path != null && !path.value().isEmpty()) {
-      String pathValue = path.value();
-      if (!pathValue.startsWith("/")) {
-        pathValue = "/" + pathValue;
-      }
-      if (pathValue.endsWith("/")) {
-        // Strip off any trailing slashes, since the template has already had slashes appropriately
-        // added
-        pathValue = pathValue.substring(0, pathValue.length() - 1);
+  public JAXRSContract() {
+    super.registerClassAnnotation(Path.class, (path, data) -> {
+      if (path != null && !path.value().isEmpty()) {
+        String pathValue = path.value();
+        if (!pathValue.startsWith("/")) {
+          pathValue = "/" + pathValue;
+        }
+        if (pathValue.endsWith("/")) {
+          // Strip off any trailing slashes, since the template has already had slashes
+          // appropriately
+          // added
+          pathValue = pathValue.substring(0, pathValue.length() - 1);
+        }
+        // jax-rs allows whitespace around the param name, as well as an optional regex. The
+        // contract
+        // should
+        // strip these out appropriately.
+        pathValue = pathValue.replaceAll("\\{\\s*(.+?)\\s*(:.+?)?\\}", "\\{$1\\}");
+        data.template().uri(pathValue);
       }
-      // jax-rs allows whitespace around the param name, as well as an optional regex. The contract
-      // should
-      // strip these out appropriately.
-      pathValue = pathValue.replaceAll("\\{\\s*(.+?)\\s*(:.+?)?\\}", "\\{$1\\}");
-      data.template().uri(pathValue);
-    }
-    Consumes consumes = clz.getAnnotation(Consumes.class);
-    if (consumes != null) {
-      handleConsumesAnnotation(data, consumes, clz.getName());
-    }
-    Produces produces = clz.getAnnotation(Produces.class);
-    if (produces != null) {
-      handleProducesAnnotation(data, produces, clz.getName());
-    }
-  }
+    });
+    super.registerClassAnnotation(Consumes.class, this::handleConsumesAnnotation);
+    super.registerClassAnnotation(Produces.class, this::handleProducesAnnotation);
 
-  @Override
-  protected void processAnnotationOnMethod(MethodMetadata data,
-                                           Annotation methodAnnotation,
-                                           Method method) {
-    Class<? extends Annotation> annotationType = methodAnnotation.annotationType();
-    HttpMethod http = annotationType.getAnnotation(HttpMethod.class);
-    if (http != null) {
+    registerMethodAnnotation(methodAnnotation -> {
+      Class<? extends Annotation> annotationType = methodAnnotation.annotationType();
+      HttpMethod http = annotationType.getAnnotation(HttpMethod.class);
+      return http != null;
+    }, (methodAnnotation, data) -> {
+      Class<? extends Annotation> annotationType = methodAnnotation.annotationType();
+      HttpMethod http = annotationType.getAnnotation(HttpMethod.class);
       checkState(data.template().method() == null,
-          "Method %s contains multiple HTTP methods. Found: %s and %s", method.getName(),
+          "Method %s contains multiple HTTP methods. Found: %s and %s", data.configKey(),
           data.template().method(), http.value());
       data.template().method(Request.HttpMethod.valueOf(http.value()));
-    } else if (annotationType == Path.class) {
-      String pathValue = emptyToNull(Path.class.cast(methodAnnotation).value());
+    });
+
+    super.registerMethodAnnotation(Path.class, (path, data) -> {
+      final String pathValue = emptyToNull(path.value());
       if (pathValue == null) {
         return;
       }
-      String methodAnnotationValue = Path.class.cast(methodAnnotation).value();
+      String methodAnnotationValue = path.value();
       if (!methodAnnotationValue.startsWith("/") && !data.template().url().endsWith("/")) {
         methodAnnotationValue = "/" + methodAnnotationValue;
       }
@@ -99,83 +93,62 @@ protected void processAnnotationOnMethod(MethodMetadata data,
       methodAnnotationValue =
           methodAnnotationValue.replaceAll("\\{\\s*(.+?)\\s*(:.+?)?\\}", "\\{$1\\}");
       data.template().uri(methodAnnotationValue, true);
-    } else if (annotationType == Produces.class) {
-      handleProducesAnnotation(data, (Produces) methodAnnotation, "method " + method.getName());
-    } else if (annotationType == Consumes.class) {
-      handleConsumesAnnotation(data, (Consumes) methodAnnotation, "method " + method.getName());
-    }
+    });
+    super.registerMethodAnnotation(Consumes.class, this::handleConsumesAnnotation);
+    super.registerMethodAnnotation(Produces.class, this::handleProducesAnnotation);
+
+    // trying to minimize the diff
+    registerParamAnnotations();
   }
 
-  private void handleProducesAnnotation(MethodMetadata data, Produces produces, String name) {
-    String[] serverProduces =
+  private void handleProducesAnnotation(Produces produces, MethodMetadata data) {
+    final String[] serverProduces =
         removeValues(produces.value(), (mediaType) -> emptyToNull(mediaType) == null, String.class);
-    checkState(serverProduces.length > 0, "Produces.value() was empty on %s", name);
+    checkState(serverProduces.length > 0, "Produces.value() was empty on %s", data.configKey());
     data.template().header(ACCEPT, Collections.emptyList()); // remove any previous produces
     data.template().header(ACCEPT, serverProduces);
   }
 
-  private void handleConsumesAnnotation(MethodMetadata data, Consumes consumes, String name) {
-    String[] serverConsumes =
+  private void handleConsumesAnnotation(Consumes consumes, MethodMetadata data) {
+    final String[] serverConsumes =
         removeValues(consumes.value(), (mediaType) -> emptyToNull(mediaType) == null, String.class);
-    checkState(serverConsumes.length > 0, "Consumes.value() was empty on %s", name);
+    checkState(serverConsumes.length > 0, "Consumes.value() was empty on %s", data.configKey());
     data.template().header(CONTENT_TYPE, Collections.emptyList()); // remove any previous consumes
     data.template().header(CONTENT_TYPE, serverConsumes[0]);
   }
 
-  /**
-   * Allows derived contracts to specify unsupported jax-rs parameter annotations which should be
-   * ignored. Required for JAX-RS 2 compatibility.
-   */
-  protected boolean isUnsupportedHttpParameterAnnotation(Annotation parameterAnnotation) {
-    return false;
-  }
-
-  @Override
-  protected boolean processAnnotationsOnParameter(MethodMetadata data,
-                                                  Annotation[] annotations,
-                                                  int paramIndex) {
-    boolean isHttpParam = false;
-    for (Annotation parameterAnnotation : annotations) {
-      Class<? extends Annotation> annotationType = parameterAnnotation.annotationType();
-      // masc20180327. parameter with unsupported jax-rs annotations should not be passed as body
-      // params.
-      // this will prevent interfaces from becoming unusable entirely due to single (unsupported)
-      // endpoints.
-      // https://github.com/OpenFeign/feign/issues/669
-      if (this.isUnsupportedHttpParameterAnnotation(parameterAnnotation)) {
-        isHttpParam = true;
-      } else if (annotationType == PathParam.class) {
-        String name = PathParam.class.cast(parameterAnnotation).value();
+  protected void registerParamAnnotations() {
+    {
+      registerParameterAnnotation(PathParam.class, (param, data, paramIndex) -> {
+        final String name = param.value();
         checkState(emptyToNull(name) != null, "PathParam.value() was empty on parameter %s",
             paramIndex);
         nameParam(data, name, paramIndex);
-        isHttpParam = true;
-      } else if (annotationType == QueryParam.class) {
-        String name = QueryParam.class.cast(parameterAnnotation).value();
+      });
+      registerParameterAnnotation(QueryParam.class, (param, data, paramIndex) -> {
+        final String name = param.value();
         checkState(emptyToNull(name) != null, "QueryParam.value() was empty on parameter %s",
             paramIndex);
-        String query = addTemplatedParam(name);
+        final String query = addTemplatedParam(name);
         data.template().query(name, query);
         nameParam(data, name, paramIndex);
-        isHttpParam = true;
-      } else if (annotationType == HeaderParam.class) {
-        String name = HeaderParam.class.cast(parameterAnnotation).value();
+      });
+      registerParameterAnnotation(HeaderParam.class, (param, data, paramIndex) -> {
+        final String name = param.value();
         checkState(emptyToNull(name) != null, "HeaderParam.value() was empty on parameter %s",
             paramIndex);
-        String header = addTemplatedParam(name);
+        final String header = addTemplatedParam(name);
         data.template().header(name, header);
         nameParam(data, name, paramIndex);
-        isHttpParam = true;
-      } else if (annotationType == FormParam.class) {
-        String name = FormParam.class.cast(parameterAnnotation).value();
+      });
+      registerParameterAnnotation(FormParam.class, (param, data, paramIndex) -> {
+        final String name = param.value();
         checkState(emptyToNull(name) != null, "FormParam.value() was empty on parameter %s",
             paramIndex);
         data.formParams().add(name);
         nameParam(data, name, paramIndex);
-        isHttpParam = true;
-      }
+      });
     }
-    return isHttpParam;
   }
 
   // Not using override as the super-type's method is deprecated and will be removed.