Checking for inherited Subscribe/Produce annotated methods

square · Jan 5, 2015 · d0b816f · d0b816f
1 parent 46237e8
commit d0b816f
Show file tree

Hide file tree

Showing 3 changed files with 172 additions and 55 deletions.
diff --git a/otto/src/main/java/com/squareup/otto/AnnotatedHandlerFinder.java b/otto/src/main/java/com/squareup/otto/AnnotatedHandlerFinder.java
@@ -19,6 +19,8 @@
 
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -49,74 +51,107 @@ 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.");
-        }
-
-        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);
-      } 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<?> currentClass = listenerClass;
+    while (shouldCheck(currentClass)) {
+      for (Method method : currentClass.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 = 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.");
+        boolean containsMethod = false;
+        for (Set<Method> methods : subscriberMethods.values()) {
+          if (containsMethod(method, methods)) {
+            containsMethod = true;
+            break;
+          }
         }
 
-        if ((method.getModifiers() & Modifier.PUBLIC) == 0) {
-          throw new IllegalArgumentException("Method " + method + " has @Produce annotation on " + eventType
-              + " but is not 'public'.");
+        //if this method has been overridden in a subclass, skip it
+        if (containsMethod || containsMethod(method, producerMethods.values())) {
+          continue;
         }
 
-        if (producerMethods.containsKey(eventType)) {
-          throw new IllegalArgumentException("Producer for type " + eventType + " has already been registered.");
+        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);
+        } 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);
         }
-        producerMethods.put(eventType, method);
       }
+      currentClass = currentClass.getSuperclass();
     }
 
     PRODUCERS_CACHE.put(listenerClass, producerMethods);
     SUBSCRIBERS_CACHE.put(listenerClass, subscriberMethods);
   }
 
+  static boolean shouldCheck(Class<?> clazz) {
+    String name = clazz.getName();
+    return !name.startsWith("java.") && !name.startsWith("android.");
+  }
+
+  static boolean containsMethod(Method check, Collection<Method> methods) {
+    for (Method method : methods) {
+      if (check.getName().equals(method.getName())
+          && Arrays.equals(check.getParameterTypes(), method.getParameterTypes())) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /** This implementation finds all methods marked with a {@link Produce} annotation. */
   static Map<Class<?>, EventProducer> findAllProducers(Object listener) {
     final Class<?> listenerClass = listener.getClass();

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);
+    }
+  }
+
 }