Thrown an error when a private method has MVC annotations fix #372

Author: jknack

jooby/src/main/java/org/jooby/internal/mvc/MvcRoutes.java

@@ -21,6 +21,7 @@
 import java.lang.annotation.Annotation;
 import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -29,6 +30,8 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
 import java.util.function.Function;
 
 import org.jooby.Env;
@@ -71,31 +74,27 @@ public class MvcRoutes {
       .add(Consumes.class)
       .build();
 
-  @SuppressWarnings({"unchecked", "rawtypes" })
   public static List<Route.Definition> routes(final Env env, final RouteMetadata classInfo,
       final String rpath, final Class<?> routeClass) {
 
+    // check and fail fast
+    methods(routeClass, methods -> {
+      routes(methods, (m, a) -> {
+        if (!Modifier.isPublic(m.getModifiers())) {
+          throw new IllegalArgumentException("Not a public method: " + m);
+        }
+      });
+    });
+
     RequestParamProvider provider = new RequestParamProviderImpl(
         new RequestParamNameProviderImpl(classInfo));
 
     String[] rootPaths = path(routeClass);
     String[] rootExcludes = excludes(routeClass, EMPTY);
 
+    // we are good, now collect them
     Map<Method, List<Class<?>>> methods = new HashMap<>();
-    for (Method method : routeClass.getMethods()) {
-      List<Class<?>> annotations = new ArrayList<>();
-      for (Class annotationType : VERBS) {
-        Annotation annotation = method.getAnnotation(annotationType);
-        if (annotation != null) {
-          annotations.add(annotationType);
-        }
-      }
-      if (annotations.size() > 0) {
-        methods.put(method, annotations);
-      } else if (method.isAnnotationPresent(Path.class)) {
-        methods.put(method, Arrays.asList(GET.class));
-      }
-    }
+    routes(routeClass.getMethods(), methods::put);
 
     List<Definition> definitions = new ArrayList<>();
     Map<String, Object> attrs = attrs(routeClass.getAnnotations());
@@ -145,6 +144,32 @@ public static List<Route.Definition> routes(final Env env, final RouteMetadata c
     return definitions;
   }
 
+  private static void methods(final Class<?> clazz, final Consumer<Method[]> callback) {
+    if (clazz != Object.class) {
+      callback.accept(clazz.getDeclaredMethods());
+      methods(clazz.getSuperclass(), callback);
+    }
+  }
+
+  @SuppressWarnings({"rawtypes", "unchecked" })
+  private static void routes(final Method[] methods,
+      final BiConsumer<Method, List<Class<?>>> consumer) {
+    for (Method method : methods) {
+      List<Class<?>> annotations = new ArrayList<>();
+      for (Class annotationType : VERBS) {
+        Annotation annotation = method.getAnnotation(annotationType);
+        if (annotation != null) {
+          annotations.add(annotationType);
+        }
+      }
+      if (annotations.size() > 0) {
+        consumer.accept(method, annotations);
+      } else if (method.isAnnotationPresent(Path.class)) {
+        consumer.accept(method, Arrays.asList(GET.class));
+      }
+    }
+  }
+
   private static Map<String, Object> attrs(final Annotation[] annotations) {
     Map<String, Object> result = new LinkedHashMap<>();
     for (Annotation annotation : annotations) {

jooby/src/test/java/org/jooby/issues/Issue372.java

@@ -0,0 +1,54 @@
+package org.jooby.issues;
+
+import static org.easymock.EasyMock.expect;
+
+import org.jooby.Env;
+import org.jooby.Result;
+import org.jooby.Results;
+import org.jooby.internal.RouteMetadata;
+import org.jooby.internal.mvc.MvcRoutes;
+import org.jooby.mvc.GET;
+import org.jooby.mvc.Path;
+import org.jooby.test.MockUnit;
+import org.junit.Test;
+
+public class Issue372 {
+
+  public static class PingRoute {
+    @Path("/ping")
+    @GET
+    private Result ping() {
+      return Results.ok();
+    }
+  }
+
+  public static class Ext extends PingRoute {
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void shouldFailFastOnPrivateMvcRoutes() throws Exception {
+    new MockUnit(Env.class)
+        .expect(unit -> {
+          Env env = unit.get(Env.class);
+          expect(env.name()).andReturn("dev").times(2);
+        })
+        .run(unit -> {
+          Env env = unit.get(Env.class);
+          MvcRoutes.routes(env, new RouteMetadata(env), "", PingRoute.class);
+        });
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void shouldFailFastOnPrivateMvcRoutesExt() throws Exception {
+    new MockUnit(Env.class)
+        .expect(unit -> {
+          Env env = unit.get(Env.class);
+          expect(env.name()).andReturn("dev").times(2);
+        })
+        .run(unit -> {
+          Env env = unit.get(Env.class);
+          MvcRoutes.routes(env, new RouteMetadata(env), "", Ext.class);
+        });
+  }
+
+}