Checking for inherited Subscribe/Produce annotated methods

square · Jan 7, 2015 · 225d659 · 225d659
1 parent 46237e8
commit 225d659
Show file tree

Hide file tree

Showing 3 changed files with 197 additions and 57 deletions.
diff --git a/otto/src/main/java/com/squareup/otto/AnnotatedHandlerFinder.java b/otto/src/main/java/com/squareup/otto/AnnotatedHandlerFinder.java
@@ -19,8 +19,11 @@
 
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -48,73 +51,128 @@ final class AnnotatedHandlerFinder {
   private static void loadAnnotatedMethods(Class<?> listenerClass) {
     Map<Class<?>, Set<Method>> subscriberMethods = new HashMap<Class<?>, Set<Method>>();
     Map<Class<?>, Method> producerMethods = new HashMap<Class<?>, Method>();
-
-    for (Method method : listenerClass.getDeclaredMethods()) {
-      // The compiler sometimes creates synthetic bridge methods as part of the
-      // type erasure process. As of JDK8 these methods now include the same
-      // annotations as the original declarations. They should be ignored for
-      // subscribe/produce.
-      if (method.isBridge()) {
-        continue;
-      }
-      if (method.isAnnotationPresent(Subscribe.class)) {
-        Class<?>[] parameterTypes = method.getParameterTypes();
-        if (parameterTypes.length != 1) {
-          throw new IllegalArgumentException("Method " + method + " has @Subscribe annotation but requires "
-              + parameterTypes.length + " arguments.  Methods must require a single argument.");
+    Set<MethodIdentifier> identifiers = new HashSet<MethodIdentifier>();
+
+    List<Class<?>> supers = getSuperClasses(listenerClass);
+    for (Class<?> clazz : supers) {
+      for (Method method : clazz.getDeclaredMethods()) {
+        // The compiler sometimes creates synthetic bridge methods as part of the
+        // type erasure process. As of JDK8 these methods now include the same
+        // annotations as the original declarations. They should be ignored for
+        // subscribe/produce.
+        if (method.isBridge()) {
+            continue;
         }
 
-        Class<?> eventType = parameterTypes[0];
-        if (eventType.isInterface()) {
-          throw new IllegalArgumentException("Method " + method + " has @Subscribe annotation on " + eventType
-              + " which is an interface.  Subscription must be on a concrete class type.");
+        MethodIdentifier identifier = new MethodIdentifier(method);
+        if (identifiers.contains(identifier)) {
+          continue;
         }
 
-        if ((method.getModifiers() & Modifier.PUBLIC) == 0) {
-          throw new IllegalArgumentException("Method " + method + " has @Subscribe annotation on " + eventType
-              + " but is not 'public'.");
+        if (method.isAnnotationPresent(Subscribe.class)) {
+          Class<?>[] parameterTypes = method.getParameterTypes();
+          if (parameterTypes.length != 1) {
+            throw new IllegalArgumentException("Method " + method + " has @Subscribe annotation but requires "
+                + parameterTypes.length + " arguments.  Methods must require a single argument.");
+          }
+
+          Class<?> eventType = parameterTypes[0];
+          if (eventType.isInterface()) {
+            throw new IllegalArgumentException("Method " + method + " has @Subscribe annotation on " + eventType
+                + " which is an interface.  Subscription must be on a concrete class type.");
+          }
+
+          if ((method.getModifiers() & Modifier.PUBLIC) == 0) {
+            throw new IllegalArgumentException("Method " + method + " has @Subscribe annotation on " + eventType
+                + " but is not 'public'.");
+          }
+
+          Set<Method> methods = subscriberMethods.get(eventType);
+          if (methods == null) {
+            methods = new HashSet<Method>();
+            subscriberMethods.put(eventType, methods);
+          }
+          methods.add(method);
+          identifiers.add(identifier);
+        } else if (method.isAnnotationPresent(Produce.class)) {
+          Class<?>[] parameterTypes = method.getParameterTypes();
+          if (parameterTypes.length != 0) {
+            throw new IllegalArgumentException("Method " + method + "has @Produce annotation but requires "
+                + parameterTypes.length + " arguments.  Methods must require zero arguments.");
+          }
+          if (method.getReturnType() == Void.class) {
+            throw new IllegalArgumentException("Method " + method
+                + " has a return type of void.  Must declare a non-void type.");
+          }
+
+          Class<?> eventType = method.getReturnType();
+          if (eventType.isInterface()) {
+            throw new IllegalArgumentException("Method " + method + " has @Produce annotation on " + eventType
+                + " which is an interface.  Producers must return a concrete class type.");
+          }
+          if (eventType.equals(Void.TYPE)) {
+            throw new IllegalArgumentException("Method " + method + " has @Produce annotation but has no return type.");
+          }
+
+          if ((method.getModifiers() & Modifier.PUBLIC) == 0) {
+            throw new IllegalArgumentException("Method " + method + " has @Produce annotation on " + eventType
+                + " but is not 'public'.");
+          }
+
+          if (producerMethods.containsKey(eventType)) {
+            throw new IllegalArgumentException("Producer for type " + eventType + " has already been registered.");
+          }
+          producerMethods.put(eventType, method);
+          identifiers.add(identifier);
         }
+      }
+    }
 
-        Set<Method> methods = subscriberMethods.get(eventType);
-        if (methods == null) {
-          methods = new HashSet<Method>();
-          subscriberMethods.put(eventType, methods);
-        }
-        methods.add(method);
-      } else if (method.isAnnotationPresent(Produce.class)) {
-        Class<?>[] parameterTypes = method.getParameterTypes();
-        if (parameterTypes.length != 0) {
-          throw new IllegalArgumentException("Method " + method + "has @Produce annotation but requires "
-              + parameterTypes.length + " arguments.  Methods must require zero arguments.");
-        }
-        if (method.getReturnType() == Void.class) {
-          throw new IllegalArgumentException("Method " + method
-              + " has a return type of void.  Must declare a non-void type.");
-        }
+    PRODUCERS_CACHE.put(listenerClass, producerMethods);
+    SUBSCRIBERS_CACHE.put(listenerClass, subscriberMethods);
+  }
 
-        Class<?> eventType = method.getReturnType();
-        if (eventType.isInterface()) {
-          throw new IllegalArgumentException("Method " + method + " has @Produce annotation on " + eventType
-              + " which is an interface.  Producers must return a concrete class type.");
-        }
-        if (eventType.equals(Void.TYPE)) {
-          throw new IllegalArgumentException("Method " + method + " has @Produce annotation but has no return type.");
-        }
+  /** Get super classes, stopping once a class in "java." or "android." package is reached. */
+  static List<Class<?>> getSuperClasses(Class<?> clazz) {
+    List<Class<?>> classes = new ArrayList<Class<?>>();
+    while (shouldCheck(clazz)) {
+      classes.add(clazz);
+      clazz = clazz.getSuperclass();
+    }
+    return classes;
+  }
 
-        if ((method.getModifiers() & Modifier.PUBLIC) == 0) {
-          throw new IllegalArgumentException("Method " + method + " has @Produce annotation on " + eventType
-              + " but is not 'public'.");
-        }
+  static boolean shouldCheck(Class<?> clazz) {
+    if (clazz == null) {
+      return false;
+    }
+    String name = clazz.getName();
+    return !name.startsWith("java.") && !name.startsWith("android.");
+  }
 
-        if (producerMethods.containsKey(eventType)) {
-          throw new IllegalArgumentException("Producer for type " + eventType + " has already been registered.");
-        }
-        producerMethods.put(eventType, method);
-      }
+  private static final class MethodIdentifier {
+
+    private final String name;
+    private final List<Class<?>> parameterTypes;
+
+    MethodIdentifier(Method method) {
+      this.name = method.getName();
+      this.parameterTypes = Arrays.asList(method.getParameterTypes());
     }
 
-    PRODUCERS_CACHE.put(listenerClass, producerMethods);
-    SUBSCRIBERS_CACHE.put(listenerClass, subscriberMethods);
+    @Override
+    public int hashCode() {
+      return Arrays.hashCode(new Object[]{name, parameterTypes});
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (o instanceof MethodIdentifier) {
+        MethodIdentifier ident = (MethodIdentifier) o;
+        return name.equals(ident.name) && parameterTypes.equals(ident.parameterTypes);
+      }
+      return false;
+    }
   }
 
   /** This implementation finds all methods marked with a {@link Produce} annotation. */

diff --git a/otto/src/test/java/com/squareup/otto/outside/AnnotatedHandlerFinderTest.java b/otto/src/test/java/com/squareup/otto/outside/AnnotatedHandlerFinderTest.java
@@ -57,10 +57,12 @@ H getHandler() {
       return handler;
     }
 
+    protected Bus bus;
+
     @Before
     public void setUp() throws Exception {
       handler = createHandler();
-      Bus bus = new Bus(ThreadEnforcer.ANY);
+      bus = new Bus(ThreadEnforcer.ANY);
       bus.register(handler);
       bus.post(EVENT);
     }
@@ -139,6 +141,66 @@ public void overriddenAndAnnotatedInSubclass(Object o) {
     }
   }
 
+  public static class AnnotatedInSuperclassTest
+            extends AbstractEventBusTest<AnnotatedInSuperclassTest.SubClass> {
+
+    static class SuperClass {
+      final List<Object> annotatedInSuperclass = new ArrayList<Object>();
+      final List<Object> overriddenInSubclass = new ArrayList<Object>();
+      final List<Object> differentParametersInSubclass = new ArrayList<Object>();
+
+      @Subscribe
+      public void annotatedInSuperclass(Object o){
+        annotatedInSuperclass.add(o);
+      }
+
+      @Subscribe
+      public void overriddenInSubclass(Object o){
+        overriddenInSubclass.add(o);
+      }
+
+      @Subscribe
+      public void differentSignatureInSubclass(Object o){
+        differentParametersInSubclass.add(o);
+      }
+    }
+
+    static class SubClass extends SuperClass {
+      final List<Object> overriddenInSubclass2 = new ArrayList<Object>();
+      final List<Object> differentParametersInSubclass2 = new ArrayList<Object>();
+
+      @Subscribe
+      public void overriddenInSubclass(Object o){
+        overriddenInSubclass2.add(o);
+      }
+
+      @Subscribe
+      public void differentSignatureInSubclass(String o){
+        differentParametersInSubclass2.add(o);
+      }
+    }
+
+    @Test public void annotatedInSuperclass() {
+      assertThat(getHandler().annotatedInSuperclass).containsExactly(EVENT);
+    }
+
+    @Test public void overriddenInSubclass() {
+      assertThat(getHandler().overriddenInSubclass).isEmpty();
+      assertThat(getHandler().overriddenInSubclass2).containsExactly(EVENT);
+    }
+
+    @Test public void differentSignatureInSubclass() {
+      String stringEvent = "event";
+      bus.post(stringEvent);
+      assertThat(getHandler().differentParametersInSubclass).containsExactly(EVENT, stringEvent);
+      assertThat(getHandler().differentParametersInSubclass2).containsExactly(stringEvent);
+    }
+
+    @Override SubClass createHandler() {
+      return new SubClass();
+    }
+  }
+
   public static class NeitherAbstractNorAnnotatedInSuperclassTest
       extends AbstractEventBusTest<NeitherAbstractNorAnnotatedInSuperclassTest.SubClass> {
     static class SuperClass {

diff --git a/otto/src/test/java/com/squareup/otto/outside/AnnotatedProducerFinderTest.java b/otto/src/test/java/com/squareup/otto/outside/AnnotatedProducerFinderTest.java
@@ -80,4 +80,24 @@ static class SimpleProducer {
     bus.register(new Subscriber());
     assertThat(producer.produceCalled).isEqualTo(2);
   }
+
+
+  public static class ExtendedProducerTest {
+
+    static class ExtendedProducer extends SimpleProducer {
+    }
+
+    @Test public void subclassedProducer() {
+      Bus bus = new Bus(ThreadEnforcer.ANY);
+      Subscriber subscriber = new Subscriber();
+      ExtendedProducer producer = new ExtendedProducer();
+
+      bus.register(producer);
+      assertThat(producer.produceCalled).isEqualTo(0);
+      bus.register(subscriber);
+      assertThat(producer.produceCalled).isEqualTo(1);
+      assertEquals(Arrays.asList(SimpleProducer.VALUE), subscriber.events);
+    }
+  }
+
 }