From 54ae9ed2e31b9f40120e81984e8b2ba5c7e8adca Mon Sep 17 00:00:00 2001 From: Kieran Kelleher Date: Thu, 16 Aug 2012 18:20:39 -0400 Subject: [PATCH] Adds logic to verify that we are not inadvertently adding identical pattern/method routes that call different actions. A developer may inadvertently declare the same url pattern and method on different REST actions. The result is that the first one found in the handler's routes array will be used. This change checks for this illegal state at startup. If you have duplicate declarations mapping to different REST actions this will error on first launch. --- Frameworks/EOF/ERRest/.classpath | 2 + .../rest/routes/ERXRouteRequestHandler.java | 54 ++++ .../routes/ERXRouteRequestHandlerTest.java | 234 ++++++++++++++++++ 3 files changed, 290 insertions(+) create mode 100644 Frameworks/EOF/ERRest/Tests/er/rest/routes/ERXRouteRequestHandlerTest.java diff --git a/Frameworks/EOF/ERRest/.classpath b/Frameworks/EOF/ERRest/.classpath index 0de8cee797f..76e6f06ebef 100644 --- a/Frameworks/EOF/ERRest/.classpath +++ b/Frameworks/EOF/ERRest/.classpath @@ -1,6 +1,7 @@ + @@ -10,6 +11,7 @@ + diff --git a/Frameworks/EOF/ERRest/Sources/er/rest/routes/ERXRouteRequestHandler.java b/Frameworks/EOF/ERRest/Sources/er/rest/routes/ERXRouteRequestHandler.java index 1e833f2b251..759e3024498 100644 --- a/Frameworks/EOF/ERRest/Sources/er/rest/routes/ERXRouteRequestHandler.java +++ b/Frameworks/EOF/ERRest/Sources/er/rest/routes/ERXRouteRequestHandler.java @@ -3,6 +3,7 @@ import java.lang.annotation.Annotation; import java.lang.reflect.Method; +import org.apache.commons.lang.ObjectUtils; import org.apache.log4j.Logger; import com.webobjects.appserver.WOAction; @@ -309,6 +310,7 @@ public ERXRouteRequestHandler(NameFormat entityNameFormat) { * the route to insert */ public void insertRoute(ERXRoute route) { + verifyRoute(route); _routes.insertObjectAtIndex(route, 0); } @@ -322,6 +324,7 @@ public void addRoute(ERXRoute route) { if (log.isDebugEnabled()) { log.debug("adding route " + route); } + verifyRoute(route); _routes.addObject(route); } @@ -708,6 +711,57 @@ public ERXRoute routeForMethodAndPath(String method, String path, NSMutableDicti return matchingRoute; } + + /** + * @param method + * @param pattern + * @return the first route matching method and pattern. + */ + protected ERXRoute routeForMethodAndPattern(ERXRoute.Method method, String urlPattern) { + for (ERXRoute route : _routes) { + if (route.method().equals(method) && route.routePattern().pattern().equals(urlPattern)) { + return route; + } + } + return null; + } + + /** + * Checks for an existing route that matches the {@link ERXRoute.Method} and {@link ERXRoute#routePattern()} of route and + * yet has a different controller or action mapping. + * @param route + */ + protected void verifyRoute(ERXRoute route) { + ERXRoute duplicateRoute = routeForMethodAndPattern(route.method(), route.routePattern().pattern()); + if (duplicateRoute != null) { + boolean isDifferentController = ObjectUtils.notEqual(duplicateRoute.controller(), route.controller()); + boolean isDifferentAction = ObjectUtils.notEqual(duplicateRoute.action(), route.action()); + if (isDifferentController || isDifferentAction) { + // We have a problem whereby two routes with same url pattern and http method map to different direct actions + StringBuilder message = new StringBuilder(); + message.append("The route <"); + message.append(route); + message.append("> conflicts with existing route <"); + message.append(duplicateRoute); + message.append(">."); + if (isDifferentController) { + message.append(" The controller class <"); + message.append(route.controller()); + message.append("> is different to <"); + message.append(duplicateRoute.controller()); + message.append(">."); + } + if (isDifferentAction) { + message.append(" The action <"); + message.append(route.action()); + message.append("> is different to <"); + message.append(duplicateRoute.action()); + message.append(">."); + } + throw new IllegalStateException(message.toString()); + } + } + } /** * Sets up the request userInfo for the given request for a request of the given method and path. diff --git a/Frameworks/EOF/ERRest/Tests/er/rest/routes/ERXRouteRequestHandlerTest.java b/Frameworks/EOF/ERRest/Tests/er/rest/routes/ERXRouteRequestHandlerTest.java new file mode 100644 index 00000000000..7b331225836 --- /dev/null +++ b/Frameworks/EOF/ERRest/Tests/er/rest/routes/ERXRouteRequestHandlerTest.java @@ -0,0 +1,234 @@ +package er.rest.routes; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.junit.Test; + +import com.webobjects.appserver.WOActionResults; +import com.webobjects.appserver.WORequest; +import com.webobjects.foundation.NSArray; + +import er.rest.routes.jsr311.DELETE; +import er.rest.routes.jsr311.GET; +import er.rest.routes.jsr311.POST; +import er.rest.routes.jsr311.PUT; +import er.rest.routes.jsr311.Path; +import er.rest.routes.jsr311.Paths; + +public class ERXRouteRequestHandlerTest { + + @Test + public void testRouteForMethodAndPattern() { + ERXRouteRequestHandler handler = new ERXRouteRequestHandler(); + ERXRoute route1 = new ERXRoute("SomeEntity", "somepattern", ERXRoute.Method.Post); + handler.addRoute(route1); + + // We create this object just to get the constructor-generated url pattern + ERXRoute route2 = new ERXRoute("SomeEntity", "somepattern"); + + ERXRoute route3 = handler.routeForMethodAndPattern(ERXRoute.Method.Post, route2.routePattern().pattern()); + assertTrue(route3 != null); + assertTrue(route3 == route1); + } + + // Checks error on verify conflict + @Test + public void testVerifyRouteConflict() { + ERXRouteRequestHandler handler = new ERXRouteRequestHandler(); + ERXRoute route1 = new ERXRoute("SomeEntity", "somepattern"); + handler.addRoute(route1); + + ERXRoute route2 = new ERXRoute("SomeEntity", "somepattern", SomeController.class, "someAction"); + + try { + handler.verifyRoute(route2); + fail("Expected IllegalStateException"); + } + catch (IllegalStateException e) { + System.out.println(e.getMessage()); + } + } + + // No error thrown when routes and controller/action are identical + @Test + public void testAddRouteSameNoConflict() { + ERXRouteRequestHandler handler = new ERXRouteRequestHandler(); + ERXRoute route1 = new ERXRoute("SomeEntity", "somepattern", SomeController.class, "someAction"); + System.out.println("route1 = " + route1); + handler.addRoute(route1); + + ERXRoute route2 = new ERXRoute("SomeEntity", "somepattern", SomeController.class, "someAction"); + System.out.println("route2 = " + route2); + + handler.addRoute(route2); + + // Two identical routes mapping to same controller and action exist. + assertEquals(handler.routes().count(), 2); + } + + // Ensures declared methods are added correctly + @Test + public void testAddDeclaredMethods() { + ERXRouteRequestHandler handler = new ERXRouteRequestHandler(); + handler.addDeclaredRoutes("SomeEntity", SomeController.class, true); + NSArray routes = handler.routes(); + assertEquals(routes.count(), 6); + + boolean didFindRoute1 = false; + boolean didFindRoute2 = false; + boolean didFindRoute3 = false; + boolean didFindRoute4 = false; + boolean didFindRouteXPut = false; + boolean didFindRouteXDefaultGet = false; + for (ERXRoute route : routes) { + System.out.println(route); + if (route.routePattern().pattern().contains("thing1")) { + assertEquals(route.method(), ERXRoute.Method.Get); + // Note that ERRest chops off trailing "Action" + assertEquals(route.action(), "some"); + didFindRoute1 = true; + } + if (route.routePattern().pattern().contains("thing2")) { + assertEquals(route.method(), ERXRoute.Method.Post); + assertEquals(route.action(), "someAction2"); + didFindRoute2 = true; + } + if (route.routePattern().pattern().contains("thing3")) { + assertEquals(route.method(), ERXRoute.Method.Put); + assertEquals(route.action(), "someAction3"); + didFindRoute3 = true; + } + if (route.routePattern().pattern().contains("thing4")) { + assertEquals(route.method(), ERXRoute.Method.Delete); + assertEquals(route.action(), "someAction4"); + didFindRoute4 = true; + } + if (route.routePattern().pattern().contains("thingX")) { + String routeAction = route.action(); + if (routeAction.equals("someAction3")) { + assertEquals(route.method(), ERXRoute.Method.Put); + didFindRouteXPut = true; + } else if (routeAction.equals("someActionX")) { + assertEquals(route.method(), ERXRoute.Method.Get); + didFindRouteXDefaultGet = true; + } + } + } + assertTrue(didFindRoute1); + assertTrue(didFindRoute2); + assertTrue(didFindRoute3); + assertTrue(didFindRoute4); + assertTrue(didFindRouteXPut); + assertTrue(didFindRouteXDefaultGet); + } + + // Throws error where actions have conflicting route declarations + @Test + public void testAddDeclaredMethodsWithConflicts() { + ERXRouteRequestHandler handler = new ERXRouteRequestHandler(); + try { + handler.addDeclaredRoutes("FaultyEntity", FaultyController.class, true); + fail("Expected an IllegalStateException"); + } + catch (IllegalStateException e) { + ; + } + } + + // Throws error where actions have conflicting route declarations + @Test + public void testAddDeclaredMethodsWithConflicts2() { + ERXRouteRequestHandler handler = new ERXRouteRequestHandler(); + try { + handler.addDeclaredRoutes("FaultyEntity", FaultyController2.class, true); + fail("Expected an IllegalStateException"); + } + catch (IllegalStateException e) { + ; + } + } + + // A dummy controller for testing with valid declarations + @SuppressWarnings("unused") + private static class SomeController extends ERXRouteController { + public SomeController(WORequest request) { + super(request); + } + + + @GET + @Path("/somethings/thing1") + public WOActionResults someAction() { + return null; + } + + @POST + @Path("/somethings/thing2") + public WOActionResults someAction2() { + return null; + } + + @PUT + @Paths({@Path("/somethings/thing3"), @Path("/somethings/thingX")}) + public WOActionResults someAction3() { + return null; + } + + @DELETE + @Path("/somethings/thing4") + public WOActionResults someAction4() { + return null; + } + + // No http method declaration should default to @GET + @Path("/somethings/thingX") + public WOActionResults someActionX() { + return null; + } + } + + // A dummy controller for testing + // This class deliberately has two duplicate declared route conflicts + @SuppressWarnings("unused") + private static class FaultyController extends ERXRouteController { + public FaultyController(WORequest request) { + super(request); + } + + @GET + @Path("/somethings/thing1") + public WOActionResults someAction() { + return null; + } + + // This will default to @GET and so should conflict with "someAction" + @Path("/somethings/thing1") + public WOActionResults someAction2() { + return null; + } + } + + // A dummy controller for testing + // This class deliberately has a duplicate declared route conflict + @SuppressWarnings("unused") + private static class FaultyController2 extends ERXRouteController { + public FaultyController2(WORequest request) { + super(request); + } + + @PUT + @Paths({@Path("/somethings/thing3"), @Path("/somethings/thingX")}) + public WOActionResults someAction3() { + return null; + } + + // This should conflict with the same method/path declaration on someAction3 + @PUT + @Path("/somethings/thingX") + public WOActionResults someAction4() { + return null; + } + } +}