allow multiple inheritance on the interface level with current restrictions intact.

Author: silkentrance

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

@@ -13,15 +13,14 @@
  */
 package feign;
 
-import static feign.Util.checkState;
-import static feign.Util.emptyToNull;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.*;
 import java.net.URI;
 import java.util.*;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import feign.Request.HttpMethod;
+import static feign.Util.*;
 
 /**
  * Defines what annotations and values are valid on interfaces.
@@ -43,15 +42,7 @@ abstract class BaseContract implements Contract {
      */
     @Override
     public List<MethodMetadata> parseAndValidateMetadata(Class<?> targetType) {
-      checkState(targetType.getTypeParameters().length == 0, "Parameterized types unsupported: %s",
-          targetType.getSimpleName());
-      checkState(targetType.getInterfaces().length <= 1, "Only single inheritance supported: %s",
-          targetType.getSimpleName());
-      if (targetType.getInterfaces().length == 1) {
-        checkState(targetType.getInterfaces()[0].getInterfaces().length == 0,
-            "Only single-level inheritance supported: %s",
-            targetType.getSimpleName());
-      }
+      validateTargetType(targetType);
       final Map<String, MethodMetadata> result = new LinkedHashMap<String, MethodMetadata>();
       for (final Method method : targetType.getMethods()) {
         if (method.getDeclaringClass() == Object.class ||
@@ -60,8 +51,20 @@ public List<MethodMetadata> parseAndValidateMetadata(Class<?> targetType) {
           continue;
         }
         final MethodMetadata metadata = parseAndValidateMetadata(targetType, method);
-        checkState(!result.containsKey(metadata.configKey()), "Overrides unsupported: %s",
-            metadata.configKey());
+        if (result.containsKey(metadata.configKey())) {
+          // there are two different scenarios here: getMethods() returns the overridden method
+          // prior
+          // to the inherited generic one or vice versa, e.g.
+          // java.lang.Object get(...) may come before of after java.lang.String get(...)
+          if (metadata.returnType().equals(Object.class)) {
+            // ignore generic override
+            continue;
+          } else if (result.get(metadata.configKey()).returnType().equals(Object.class)) {
+            // replace generic override
+            result.put(metadata.configKey(), metadata);
+            continue;
+          }
+        }
         result.put(metadata.configKey(), metadata);
       }
       return new ArrayList<>(result.values());
@@ -146,6 +149,84 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
       return data;
     }
 
+    private void validateTargetType(Class<?> targetType) {
+      checkState(targetType.getTypeParameters().length == 0, "Parameterized types unsupported: %s",
+          targetType.getSimpleName());
+      checkState(targetType.getInterfaces().length <= 1, "Only single inheritance supported: %s",
+          targetType.getSimpleName());
+      // original contract: single level, single inheritance
+      if (targetType.getInterfaces().length == 0
+          || targetType.getInterfaces()[0].getInterfaces().length == 0) {
+        return;
+      }
+
+      // recursively collect all inherited interfaces starting from superType and
+      // validate these for offending annotations
+      Class<?> superType = targetType.getInterfaces()[0];
+
+      Stack<Class<?>> candidateTypes = new Stack<>();
+      List<Class<?>> inheritedTypes = new ArrayList<>();
+
+      candidateTypes.push(superType);
+      while (!candidateTypes.empty()) {
+        Class<?> inheritedType = candidateTypes.pop();
+        for (Class<?> type : inheritedType.getInterfaces()) {
+          candidateTypes.push(type);
+        }
+        if (superType.equals(inheritedType)) {
+          continue;
+        }
+        inheritedTypes.add(inheritedType);
+      }
+
+      if (!inheritedTypes.isEmpty()) {
+        for (Class<?> inheritedType : inheritedTypes) {
+          validateInheritedType(targetType, inheritedType);
+        }
+      }
+    }
+
+    private void validateInheritedType(Class<?> targetType, Class<?> inheritedType) {
+      for (Class<? extends Annotation> annotationClass : getNonInheritableTypeAnnotations()) {
+        checkState(inheritedType.getAnnotationsByType(annotationClass).length == 0,
+            "Multiple inheritance supported for plain types only: targetType: %s, inheritedType: %s, not inheritable: %s",
+            targetType.getSimpleName(), inheritedType.getSimpleName(),
+            annotationClass.getSimpleName());
+        validateInheritedMethods(targetType, inheritedType);
+      }
+    }
+
+    private void validateInheritedMethods(Class<?> targetType, Class<?> inheritedType) {
+      for (Method method : inheritedType.getDeclaredMethods()) {
+        for (Class<? extends Annotation> annotationClass : getNonInheritableMethodAnnotations()) {
+          checkState(method.getAnnotationsByType(annotationClass).length == 0,
+              "Multiple inheritance supported for plain types only: targetType: %s, inheritedType: %s, method: %s, not inheritable: %s",
+              targetType.getSimpleName(), inheritedType.getSimpleName(), method.getName(),
+              annotationClass.getSimpleName());
+          validateInheritedMethodParameters(targetType, inheritedType, method);
+        }
+      }
+    }
+
+    private void validateInheritedMethodParameters(Class<?> targetType,
+                                                   Class<?> inheritedType,
+                                                   Method method) {
+      for (Parameter parameter : method.getParameters()) {
+        for (Class<? extends Annotation> annotationClass : getNonInheritableParameterAnnotations()) {
+          checkState(parameter.getAnnotationsByType(annotationClass).length == 0,
+              "Multiple inheritance supported for plain types only: targetType: %s, inheritedType: %s, method: %s, parameter: %s, not inheritable: %s",
+              targetType.getSimpleName(), inheritedType.getSimpleName(), method.getName(),
+              parameter.getName(), annotationClass.getSimpleName());
+        }
+      }
+    }
+
+    protected abstract List<Class<? extends Annotation>> getNonInheritableTypeAnnotations();
+
+    protected abstract List<Class<? extends Annotation>> getNonInheritableMethodAnnotations();
+
+    protected abstract List<Class<? extends Annotation>> getNonInheritableParameterAnnotations();
+
     private static void checkMapString(String name, Class<?> type, Type genericType) {
       checkState(Map.class.isAssignableFrom(type),
           "%s parameter must be a Map: %s", name, type);
@@ -310,5 +391,19 @@ private static Map<String, Collection<String>> toMap(String[] input) {
       return result;
     }
 
+    @Override
+    protected List<Class<? extends Annotation>> getNonInheritableTypeAnnotations() {
+      return Collections.singletonList(Headers.class);
+    }
+
+    @Override
+    protected List<Class<? extends Annotation>> getNonInheritableMethodAnnotations() {
+      return Arrays.asList(Headers.class, RequestLine.class, Body.class);
+    }
+
+    @Override
+    protected List<Class<? extends Annotation>> getNonInheritableParameterAnnotations() {
+      return Arrays.asList(HeaderMap.class, Param.class, QueryMap.class);
+    }
   }
 }

core/src/main/java/feign/DeclarativeContract.java

@@ -13,13 +13,13 @@
  */
 package feign;
 
+import feign.Contract.BaseContract;
 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;
 
 /**
  * {@link Contract} base implementation that works by declaring witch annotations should be

core/src/main/java/feign/Util.java

@@ -104,6 +104,12 @@ public static void checkArgument(boolean expression,
     }
   }
 
+  public static void checkFail(String errorMessageTemplate,
+                               Object... errorMessageArgs) {
+    throw new IllegalStateException(
+        format(errorMessageTemplate, errorMessageArgs));
+  }
+
   /**
    * Copy of {@code com.google.common.base.Preconditions#checkNotNull}.
    */

core/src/test/java/feign/DefaultContractInheritanceTest.java

@@ -0,0 +1,184 @@
+/**
+ * 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
+ *
+ * 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 org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import java.util.List;
+import static feign.assertj.FeignAssertions.assertThat;
+import static java.util.Arrays.asList;
+import static org.assertj.core.data.MapEntry.entry;
+
+/**
+ * Tests interface inheritance defined per {@link Contract.Default} are interpreted into expected
+ * {@link feign .RequestTemplate template} instances.
+ */
+public class DefaultContractInheritanceTest {
+
+  @Rule
+  public final ExpectedException thrown = ExpectedException.none();
+
+  Contract.Default contract = new Contract.Default();
+
+  @Headers("Foo: Bar")
+  interface SimpleParameterizedBaseApi<M> {
+
+    @RequestLine("GET /api/{zoneId}")
+    M get(@Param("key") String key);
+  }
+
+  interface SimpleParameterizedApi extends SimpleParameterizedBaseApi<String> {
+
+  }
+
+  @Test
+  public void simpleParameterizedBaseApi() throws Exception {
+    final List<MethodMetadata> md = contract.parseAndValidateMetadata(SimpleParameterizedApi.class);
+
+    assertThat(md).hasSize(1);
+
+    assertThat(md.get(0).configKey())
+        .isEqualTo("SimpleParameterizedApi#get(String)");
+    assertThat(md.get(0).returnType())
+        .isEqualTo(String.class);
+    assertThat(md.get(0).template())
+        .hasHeaders(entry("Foo", asList("Bar")));
+  }
+
+  @Test
+  public void parameterizedApiUnsupported() throws Exception {
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("Parameterized types unsupported: SimpleParameterizedBaseApi");
+    contract.parseAndValidateMetadata(SimpleParameterizedBaseApi.class);
+  }
+
+  interface OverrideParameterizedApi extends SimpleParameterizedBaseApi<String> {
+
+    @Override
+    @RequestLine("GET /api/{zoneId}")
+    String get(@Param("key") String key);
+  }
+
+  @Headers("Foo: Bar")
+  interface SimpleBaseApi {
+
+    @RequestLine("GET /api/{zoneId}")
+    String get(@Param("key") String key);
+  }
+
+  interface OverrideSimpleApi extends SimpleBaseApi {
+
+    @RequestLine("GET /api/v2/{zoneId}")
+    String get(@Param("key") String key);
+  }
+
+  @Test
+  public void overrideParameterizedApiSupported() {
+    contract.parseAndValidateMetadata(OverrideParameterizedApi.class);
+  }
+
+  @Test
+  public void overrideSimpleApiSupported() {
+    contract.parseAndValidateMetadata(OverrideSimpleApi.class);
+  }
+
+  interface Child<T> extends SimpleParameterizedBaseApi<List<T>> {
+
+  }
+
+  interface GrandChild extends Child<String> {
+
+  }
+
+  interface SingleInheritanceChild extends SimpleParameterizedBaseApi<String> {
+  }
+
+  interface FirstServiceBaseApi<T> {
+    T get(String key);
+  }
+
+  interface SecondServiceBaseApi<T> {
+    void update(String key, T value);
+  }
+
+  interface MultipleInheritanceDoneWrong
+      extends SimpleParameterizedBaseApi<String>, FirstServiceBaseApi<String>,
+      SecondServiceBaseApi<String> {
+  }
+
+  @Headers("Foo: Bar")
+  interface MultipleInheritanceParameterizedBaseApi<T>
+      extends FirstServiceBaseApi<T>, SecondServiceBaseApi<T> {
+
+    @Override
+    @RequestLine("GET /api/{zoneId}")
+    T get(@Param("key") String key);
+
+    @Override
+    @RequestLine("POST /api/{zoneId}")
+    @Body("%7B\"value\": \"{value}\"%7D")
+    void update(@Param("key") String key, @Param("value") T value);
+  }
+
+  interface MultipleInheritanceDoneCorrectly
+      extends MultipleInheritanceParameterizedBaseApi<String> {
+  }
+
+  interface ServiceApi<T> extends FirstServiceBaseApi<T>, SecondServiceBaseApi<T> {
+  }
+
+  @Headers("Foo: Bar")
+  interface MultipleInheritanceApi extends ServiceApi<String> {
+
+    @Override
+    @RequestLine("GET /api/{zoneId}")
+    String get(@Param("key") String key);
+
+    @Override
+    @RequestLine("POST /api/{zoneId}")
+    @Body("%7B\"value\": \"{value}\"%7D")
+    void update(@Param("key") String key, @Param("value") String value);
+  }
+
+  @Test
+  public void singleInheritance() {
+    contract.parseAndValidateMetadata(SingleInheritanceChild.class);
+  }
+
+  @Test
+  public void multipleInheritanceDoneWrong() {
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("Only single inheritance supported: MultipleInheritanceDoneWrong");
+    contract.parseAndValidateMetadata(MultipleInheritanceDoneWrong.class);
+  }
+
+  @Test
+  public void multipleInheritanceDoneCorrectly() {
+    contract.parseAndValidateMetadata(MultipleInheritanceDoneCorrectly.class);
+  }
+
+  @Test
+  public void multipleInheritanceDoneCorrectly2() {
+    contract.parseAndValidateMetadata(MultipleInheritanceApi.class);
+  }
+
+  @Test
+  public void multipleInheritanceForPlainTypesOnly() throws Exception {
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage(
+        "Multiple inheritance supported for plain types only: targetType: GrandChild, inheritedType: SimpleParameterizedBaseApi, not inheritable: Headers");
+    contract.parseAndValidateMetadata(GrandChild.class);
+  }
+}