From 225d659a7e6f1e68f46b6d0e8feb58956738f596 Mon Sep 17 00:00:00 2001 From: Andrew Heuermann Date: Mon, 5 Jan 2015 00:50:55 -0600 Subject: [PATCH] Checking for inherited Subscribe/Produce annotated methods --- .../squareup/otto/AnnotatedHandlerFinder.java | 170 ++++++++++++------ .../outside/AnnotatedHandlerFinderTest.java | 64 ++++++- .../outside/AnnotatedProducerFinderTest.java | 20 +++ 3 files changed, 197 insertions(+), 57 deletions(-) diff --git a/otto/src/main/java/com/squareup/otto/AnnotatedHandlerFinder.java b/otto/src/main/java/com/squareup/otto/AnnotatedHandlerFinder.java index c7fb155..57db0e3 100644 --- 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, Set> subscriberMethods = new HashMap, Set>(); Map, Method> producerMethods = new HashMap, 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 identifiers = new HashSet(); + + List> 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 methods = subscriberMethods.get(eventType); + if (methods == null) { + methods = new HashSet(); + 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 methods = subscriberMethods.get(eventType); - if (methods == null) { - methods = new HashSet(); - 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> getSuperClasses(Class clazz) { + List> classes = new ArrayList>(); + 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> 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 index 66194eb..3479e7b 100644 --- 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 { + + static class SuperClass { + final List annotatedInSuperclass = new ArrayList(); + final List overriddenInSubclass = new ArrayList(); + final List differentParametersInSubclass = new ArrayList(); + + @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 overriddenInSubclass2 = new ArrayList(); + final List differentParametersInSubclass2 = new ArrayList(); + + @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 { 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 index 37b9295..71e8562 100644 --- 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); + } + } + }