Add extra information when failing to parse contract (#1176)

velo · web-flow · commit a9e631ac9bee · 2020-03-09T22:07:29.000+13:00
* Add extra information when failing to parse contract

* Add warnings to other error messages

* Move check if processor are valid before if block
diff --git a/core/src/main/java/feign/Contract.java b/core/src/main/java/feign/Contract.java
@@ -52,14 +52,14 @@ public List<MethodMetadata> parseAndValidateMetadata(Class<?> targetType) {
             "Only single-level inheritance supported: %s",
             targetType.getSimpleName());
       }
-      Map<String, MethodMetadata> result = new LinkedHashMap<String, MethodMetadata>();
-      for (Method method : targetType.getMethods()) {
+      final Map<String, MethodMetadata> result = new LinkedHashMap<String, MethodMetadata>();
+      for (final Method method : targetType.getMethods()) {
         if (method.getDeclaringClass() == Object.class ||
             (method.getModifiers() & Modifier.STATIC) != 0 ||
             Util.isDefault(method)) {
           continue;
         }
-        MethodMetadata metadata = parseAndValidateMetadata(targetType, method);
+        final MethodMetadata metadata = parseAndValidateMetadata(targetType, method);
         checkState(!result.containsKey(metadata.configKey()), "Overrides unsupported: %s",
             metadata.configKey());
         result.put(metadata.configKey(), metadata);
@@ -79,7 +79,7 @@ public MethodMetadata parseAndValidateMetadata(Method method) {
      * Called indirectly by {@link #parseAndValidateMetadata(Class)}.
      */
     protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method method) {
-      MethodMetadata data = new MethodMetadata();
+      final MethodMetadata data = new MethodMetadata();
       data.targetType(targetType);
       data.method(method);
       data.returnType(Types.resolve(targetType, targetType, method.getGenericReturnType()));
@@ -91,20 +91,20 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
       processAnnotationOnClass(data, targetType);
 
 
-      for (Annotation methodAnnotation : method.getAnnotations()) {
+      for (final Annotation methodAnnotation : method.getAnnotations()) {
         processAnnotationOnMethod(data, methodAnnotation, method);
       }
       if (data.isIgnored()) {
         return data;
       }
       checkState(data.template().method() != null,
-          "Method %s not annotated with HTTP method type (ex. GET, POST)",
-          data.configKey());
-      Class<?>[] parameterTypes = method.getParameterTypes();
-      Type[] genericParameterTypes = method.getGenericParameterTypes();
+          "Method %s not annotated with HTTP method type (ex. GET, POST)%s",
+          data.configKey(), data.warnings());
+      final Class<?>[] parameterTypes = method.getParameterTypes();
+      final Type[] genericParameterTypes = method.getGenericParameterTypes();
 
-      Annotation[][] parameterAnnotations = method.getParameterAnnotations();
-      int count = parameterAnnotations.length;
+      final Annotation[][] parameterAnnotations = method.getParameterAnnotations();
+      final int count = parameterAnnotations.length;
       for (int i = 0; i < count; i++) {
         boolean isHttpAnnotation = false;
         if (parameterAnnotations[i] != null) {
@@ -120,11 +120,12 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
         } else if (!isHttpAnnotation && parameterTypes[i] != Request.Options.class) {
           if (data.isAlreadyProcessed(i)) {
             checkState(data.formParams().isEmpty() || data.bodyIndex() == null,
-                "Body parameters cannot be used with form parameters.");
+                "Body parameters cannot be used with form parameters.%s", data.warnings());
           } else {
             checkState(data.formParams().isEmpty(),
-                "Body parameters cannot be used with form parameters.");
-            checkState(data.bodyIndex() == null, "Method has too many Body parameters: %s", method);
+                "Body parameters cannot be used with form parameters.%s", data.warnings());
+            checkState(data.bodyIndex() == null,
+                "Method has too many Body parameters: %s%s", method, data.warnings());
             data.bodyIndex(i);
             data.bodyType(Types.resolve(targetType, targetType, genericParameterTypes[i]));
           }
@@ -156,17 +157,17 @@ private static void checkMapKeys(String name, Type genericType) {
 
       // assume our type parameterized
       if (ParameterizedType.class.isAssignableFrom(genericType.getClass())) {
-        Type[] parameterTypes = ((ParameterizedType) genericType).getActualTypeArguments();
+        final Type[] parameterTypes = ((ParameterizedType) genericType).getActualTypeArguments();
         keyClass = (Class<?>) parameterTypes[0];
       } else if (genericType instanceof Class<?>) {
         // raw class, type parameters cannot be inferred directly, but we can scan any extended
         // interfaces looking for any explict types
-        Type[] interfaces = ((Class) genericType).getGenericInterfaces();
+        final Type[] interfaces = ((Class) genericType).getGenericInterfaces();
         if (interfaces != null) {
-          for (Type extended : interfaces) {
+          for (final Type extended : interfaces) {
             if (ParameterizedType.class.isAssignableFrom(extended.getClass())) {
               // use the first extended interface we find.
-              Type[] parameterTypes = ((ParameterizedType) extended).getActualTypeArguments();
+              final Type[] parameterTypes = ((ParameterizedType) extended).getActualTypeArguments();
               keyClass = (Class<?>) parameterTypes[0];
               break;
             }
@@ -214,7 +215,7 @@ protected abstract boolean processAnnotationsOnParameter(MethodMetadata data,
      * links a parameter name to its index in the method signature.
      */
     protected void nameParam(MethodMetadata data, String name, int i) {
-      Collection<String> names =
+      final Collection<String> names =
           data.indexToName().containsKey(i) ? data.indexToName().get(i) : new ArrayList<String>();
       names.add(name);
       data.indexToName().put(i, names);
@@ -227,20 +228,20 @@ class Default extends DeclarativeContract {
 
     public Default() {
       super.registerClassAnnotation(Headers.class, (header, data) -> {
-        String[] headersOnType = header.value();
+        final String[] headersOnType = header.value();
         checkState(headersOnType.length > 0, "Headers annotation was empty on type %s.",
             data.configKey());
-        Map<String, Collection<String>> headers = toMap(headersOnType);
+        final Map<String, Collection<String>> headers = toMap(headersOnType);
         headers.putAll(data.template().headers());
         data.template().headers(null); // to clear
         data.template().headers(headers);
       });
       super.registerMethodAnnotation(RequestLine.class, (ann, data) -> {
-        String requestLine = ann.value();
+        final String requestLine = ann.value();
         checkState(emptyToNull(requestLine) != null,
             "RequestLine annotation was empty on method %s.", data.configKey());
 
-        Matcher requestLineMatcher = REQUEST_LINE_PATTERN.matcher(requestLine);
+        final Matcher requestLineMatcher = REQUEST_LINE_PATTERN.matcher(requestLine);
         if (!requestLineMatcher.find()) {
           throw new IllegalStateException(String.format(
               "RequestLine annotation didn't start with an HTTP verb on method %s",
@@ -254,7 +255,7 @@ public Default() {
             .collectionFormat(ann.collectionFormat());
       });
       super.registerMethodAnnotation(Body.class, (ann, data) -> {
-        String body = ann.value();
+        final String body = ann.value();
         checkState(emptyToNull(body) != null, "Body annotation was empty on method %s.",
             data.configKey());
         if (body.indexOf('{') == -1) {
@@ -264,17 +265,17 @@ public Default() {
         }
       });
       super.registerMethodAnnotation(Headers.class, (header, data) -> {
-        String[] headersOnMethod = header.value();
+        final String[] headersOnMethod = header.value();
         checkState(headersOnMethod.length > 0, "Headers annotation was empty on method %s.",
             data.configKey());
         data.template().headers(toMap(headersOnMethod));
       });
       super.registerParameterAnnotation(Param.class, (paramAnnotation, data, paramIndex) -> {
-        String name = paramAnnotation.value();
+        final String name = paramAnnotation.value();
         checkState(emptyToNull(name) != null, "Param annotation was empty on param %s.",
             paramIndex);
         nameParam(data, name, paramIndex);
-        Class<? extends Param.Expander> expander = paramAnnotation.expander();
+        final Class<? extends Param.Expander> expander = paramAnnotation.expander();
         if (expander != Param.ToStringExpander.class) {
           data.indexToExpanderClass().put(paramIndex, expander);
         }
@@ -296,11 +297,11 @@ public Default() {
     }
 
     private static Map<String, Collection<String>> toMap(String[] input) {
-      Map<String, Collection<String>> result =
+      final Map<String, Collection<String>> result =
           new LinkedHashMap<String, Collection<String>>(input.length);
-      for (String header : input) {
-        int colon = header.indexOf(':');
-        String name = header.substring(0, colon);
+      for (final String header : input) {
+        final int colon = header.indexOf(':');
+        final String name = header.substring(0, colon);
         if (!result.containsKey(name)) {
           result.put(name, new ArrayList<String>(1));
         }
diff --git a/core/src/main/java/feign/DeclarativeContract.java b/core/src/main/java/feign/DeclarativeContract.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 The Feign Authors
  *
  * 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
@@ -15,8 +15,10 @@
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
+import java.lang.reflect.Parameter;
 import java.util.*;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 import feign.Contract.BaseContract;
 
 /**
@@ -25,9 +27,9 @@
  */
 public abstract class DeclarativeContract extends BaseContract {
 
-  private List<GuardedAnnotationProcessor> classAnnotationProcessors = new ArrayList<>();
-  private List<GuardedAnnotationProcessor> methodAnnotationProcessors = new ArrayList<>();
-  Map<Class<Annotation>, DeclarativeContract.ParameterAnnotationProcessor<Annotation>> parameterAnnotationProcessors =
+  private final List<GuardedAnnotationProcessor> classAnnotationProcessors = new ArrayList<>();
+  private final List<GuardedAnnotationProcessor> methodAnnotationProcessors = new ArrayList<>();
+  private final Map<Class<Annotation>, DeclarativeContract.ParameterAnnotationProcessor<Annotation>> parameterAnnotationProcessors =
       new HashMap<>();
 
   @Override
@@ -45,10 +47,33 @@ public final List<MethodMetadata> parseAndValidateMetadata(Class<?> targetType)
    */
   @Override
   protected final void processAnnotationOnClass(MethodMetadata data, Class<?> targetType) {
-    Arrays.stream(targetType.getAnnotations())
-        .forEach(annotation -> classAnnotationProcessors.stream()
-            .filter(processor -> processor.test(annotation))
-            .forEach(processor -> processor.process(annotation, data)));
+    final List<GuardedAnnotationProcessor> processors = Arrays.stream(targetType.getAnnotations())
+        .flatMap(annotation -> classAnnotationProcessors.stream()
+            .filter(processor -> processor.test(annotation)))
+        .collect(Collectors.toList());
+
+    if (!processors.isEmpty()) {
+      Arrays.stream(targetType.getAnnotations())
+          .forEach(annotation -> processors.stream()
+              .filter(processor -> processor.test(annotation))
+              .forEach(processor -> processor.process(annotation, data)));
+    } else {
+      if (targetType.getAnnotations().length == 0) {
+        data.addWarning(String.format(
+            "Class %s has no annotations, it may affect contract %s",
+            targetType.getSimpleName(),
+            getClass().getSimpleName()));
+      } else {
+        data.addWarning(String.format(
+            "Class %s has annotations %s that are not used by contract %s",
+            targetType.getSimpleName(),
+            Arrays.stream(targetType.getAnnotations())
+                .map(annotation -> annotation.annotationType()
+                    .getSimpleName())
+                .collect(Collectors.toList()),
+            getClass().getSimpleName()));
+      }
+    }
   }
 
   /**
@@ -60,9 +85,20 @@ protected final void processAnnotationOnClass(MethodMetadata data, Class<?> targ
   protected final void processAnnotationOnMethod(MethodMetadata data,
                                                  Annotation annotation,
                                                  Method method) {
-    methodAnnotationProcessors.stream()
+    List<GuardedAnnotationProcessor> processors = methodAnnotationProcessors.stream()
         .filter(processor -> processor.test(annotation))
-        .forEach(processor -> processor.process(annotation, data));
+        .collect(Collectors.toList());
+
+    if (!processors.isEmpty()) {
+      processors.forEach(processor -> processor.process(annotation, data));
+    } else {
+      data.addWarning(String.format(
+          "Method %s has an annotation %s that is not used by contract %s",
+          method.getName(),
+          annotation.annotationType()
+              .getSimpleName(),
+          getClass().getSimpleName()));
+    }
   }
 
 
@@ -78,12 +114,37 @@ protected final void processAnnotationOnMethod(MethodMetadata data,
   protected final boolean processAnnotationsOnParameter(MethodMetadata data,
                                                         Annotation[] annotations,
                                                         int paramIndex) {
-    Arrays.stream(annotations)
+    List<Annotation> matchingAnnotations = Arrays.stream(annotations)
         .filter(
             annotation -> parameterAnnotationProcessors.containsKey(annotation.annotationType()))
-        .forEach(annotation -> parameterAnnotationProcessors
-            .getOrDefault(annotation.annotationType(), ParameterAnnotationProcessor.DO_NOTHING)
-            .process(annotation, data, paramIndex));
+        .collect(Collectors.toList());
+
+    if (!matchingAnnotations.isEmpty()) {
+      matchingAnnotations.forEach(annotation -> parameterAnnotationProcessors
+          .getOrDefault(annotation.annotationType(), ParameterAnnotationProcessor.DO_NOTHING)
+          .process(annotation, data, paramIndex));
+
+    } else {
+      final Parameter parameter = data.method().getParameters()[paramIndex];
+      String parameterName = parameter.isNamePresent()
+          ? parameter.getName()
+          : parameter.getType().getSimpleName();
+      if (annotations.length == 0) {
+        data.addWarning(String.format(
+            "Parameter %s has no annotations, it may affect contract %s",
+            parameterName,
+            getClass().getSimpleName()));
+      } else {
+        data.addWarning(String.format(
+            "Parameter %s has annotations %s that are not used by contract %s",
+            parameterName,
+            Arrays.stream(annotations)
+                .map(annotation -> annotation.annotationType()
+                    .getSimpleName())
+                .collect(Collectors.toList()),
+            getClass().getSimpleName()));
+      }
+    }
     return false;
   }
 
@@ -177,8 +238,8 @@ public interface ParameterAnnotationProcessor<E extends Annotation> {
   private class GuardedAnnotationProcessor
       implements Predicate<Annotation>, DeclarativeContract.AnnotationProcessor<Annotation> {
 
-    private Predicate<Annotation> predicate;
-    private DeclarativeContract.AnnotationProcessor<Annotation> processor;
+    private final Predicate<Annotation> predicate;
+    private final DeclarativeContract.AnnotationProcessor<Annotation> processor;
 
     @SuppressWarnings({"rawtypes", "unchecked"})
     private GuardedAnnotationProcessor(Predicate predicate,
diff --git a/core/src/main/java/feign/MethodMetadata.java b/core/src/main/java/feign/MethodMetadata.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2019 The Feign Authors
+ * Copyright 2012-2020 The Feign Authors
  *
  * 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
@@ -17,6 +17,7 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Type;
 import java.util.*;
+import java.util.stream.Collectors;
 import feign.Param.Expander;
 
 public final class MethodMetadata implements Serializable {
@@ -30,18 +31,19 @@ public final class MethodMetadata implements Serializable {
   private Integer queryMapIndex;
   private boolean queryMapEncoded;
   private transient Type bodyType;
-  private RequestTemplate template = new RequestTemplate();
-  private List<String> formParams = new ArrayList<String>();
-  private Map<Integer, Collection<String>> indexToName =
+  private final RequestTemplate template = new RequestTemplate();
+  private final List<String> formParams = new ArrayList<String>();
+  private final Map<Integer, Collection<String>> indexToName =
       new LinkedHashMap<Integer, Collection<String>>();
-  private Map<Integer, Class<? extends Expander>> indexToExpanderClass =
+  private final Map<Integer, Class<? extends Expander>> indexToExpanderClass =
       new LinkedHashMap<Integer, Class<? extends Expander>>();
-  private Map<Integer, Boolean> indexToEncoded = new LinkedHashMap<Integer, Boolean>();
+  private final Map<Integer, Boolean> indexToEncoded = new LinkedHashMap<Integer, Boolean>();
   private transient Map<Integer, Expander> indexToExpander;
   private BitSet parameterToIgnore = new BitSet();
   private boolean ignored;
   private transient Class<?> targetType;
   private transient Method method;
+  private transient final List<String> warnings = new ArrayList<>();
 
   MethodMetadata() {
     template.methodMetadata(this);
@@ -240,4 +242,13 @@ public Method method() {
     return method;
   }
 
+  public void addWarning(String warning) {
+    warnings.add(warning);
+  }
+
+  public String warnings() {
+    return warnings.stream()
+        .collect(Collectors.joining("\n- ", "\nWarnings:\n- ", ""));
+  }
+
 }
diff --git a/core/src/test/java/feign/DefaultContractTest.java b/core/src/test/java/feign/DefaultContractTest.java
