Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
* [#1934] feat(controller): Call all non-static controller methods on…
… the same instance in the scope of a request instead of creating a new one each time.

* [#1934] feat(controller):Support for non-static controller methods - they are easier to mock/unit-test
  • Loading branch information
xael-fry committed Dec 25, 2015
1 parent eac6035 commit deb7d53
Show file tree
Hide file tree
Showing 3 changed files with 176 additions and 63 deletions.
139 changes: 76 additions & 63 deletions framework/src/play/mvc/ActionInvoker.java
@@ -1,10 +1,23 @@
package play.mvc;

import com.jamonapi.Monitor;
import com.jamonapi.MonitorFactory;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Stack;
import java.util.concurrent.Future;

import org.apache.commons.javaflow.Continuation;
import org.apache.commons.javaflow.bytecode.StackRecorder;
import org.apache.commons.lang.StringUtils;

import com.jamonapi.Monitor;
import com.jamonapi.MonitorFactory;

import play.Invoker.Suspend;
import play.Logger;
import play.Play;
Expand All @@ -31,19 +44,6 @@
import play.utils.Java;
import play.utils.Utils;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.InputStream;
import java.lang.annotation.Annotation;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Stack;
import java.util.concurrent.Future;

/**
* Invoke an action after an HTTP request.
*/
Expand Down Expand Up @@ -117,7 +117,8 @@ public static void invoke(Http.Request request, Http.Response response) {

// add parameters from the URI query string
String encoding = Http.Request.current().encoding;
Scope.Params.current()._mergeWith(UrlEncodedParser.parseQueryString(new ByteArrayInputStream(request.querystring.getBytes(encoding))));
Scope.Params.current()
._mergeWith(UrlEncodedParser.parseQueryString(new ByteArrayInputStream(request.querystring.getBytes(encoding))));

// 2. Easy debugging ...
if (Play.mode == Play.Mode.DEV) {
Expand Down Expand Up @@ -160,7 +161,7 @@ public static void invoke(Http.Request request, Http.Response response) {
ControllerInstrumentation.initActionCall();
try {
inferResult(invokeControllerMethod(actionMethod));
} catch(Result result) {
} catch (Result result) {
actionResult = result;
// Cache it if needed
if (cacheKey != null) {
Expand All @@ -177,13 +178,13 @@ public static void invoke(Http.Request request, Http.Response response) {

} else {
// @Catch
Object[] args = new Object[]{ex.getTargetException()};
Object[] args = new Object[] { ex.getTargetException() };
List<Method> catches = Java.findAllAnnotatedMethods(Controller.getControllerClass(), Catch.class);
ControllerInstrumentation.stopActionCall();
for (Method mCatch : catches) {
Class[] exceptions = mCatch.getAnnotation(Catch.class).value();
if (exceptions.length == 0) {
exceptions = new Class[]{Exception.class};
exceptions = new Class[] { Exception.class };
}
for (Class exception : exceptions) {
if (exception.isInstance(args[0])) {
Expand Down Expand Up @@ -227,7 +228,8 @@ public static void invoke(Http.Request request, Http.Response response) {
}
StackTraceElement element = PlayException.getInterestingStackTraceElement(ex.getTargetException());
if (element != null) {
throw new JavaExecutionException(Play.classes.getApplicationClass(element.getClassName()), element.getLineNumber(), ex.getTargetException());
throw new JavaExecutionException(Play.classes.getApplicationClass(element.getClassName()), element.getLineNumber(),
ex.getTargetException());
}
throw new JavaExecutionException(Http.Request.current().action, ex);
}
Expand Down Expand Up @@ -352,16 +354,20 @@ private static void handleAfters(Http.Request request) throws Exception {
}

/**
* Checks and calla all methods in controller annotated with @Finally.
* The caughtException-value is sent as argument to @Finally-method if method has one argument which is Throwable
* Checks and calla all methods in controller annotated with @Finally. The
* caughtException-value is sent as argument to @Finally-method if method
* has one argument which is Throwable
*
* @param request
* @param caughtException If @Finally-methods are called after an error, this variable holds the caught error
* @param caughtException
* If @Finally-methods are called after an error, this variable
* holds the caught error
* @throws PlayException
*/
static void handleFinallies(Http.Request request, Throwable caughtException) throws PlayException {

if (Controller.getControllerClass() == null) {
//skip it
// skip it
return;
}

Expand Down Expand Up @@ -395,21 +401,24 @@ static void handleFinallies(Http.Request request, Throwable caughtException) thr
if (!skip) {
aFinally.setAccessible(true);

//check if method accepts Throwable as only parameter
// check if method accepts Throwable as only parameter
Class[] parameterTypes = aFinally.getParameterTypes();
if (parameterTypes.length == 1 && parameterTypes[0] == Throwable.class) {
//invoking @Finally method with caughtException as parameter
invokeControllerMethod(aFinally, new Object[]{caughtException});
// invoking @Finally method with caughtException as
// parameter
invokeControllerMethod(aFinally, new Object[] { caughtException });
} else {
//invoce @Finally-method the regular way without caughtException
// invoce @Finally-method the regular way without
// caughtException
invokeControllerMethod(aFinally, null);
}
}
}
} catch (InvocationTargetException ex) {
StackTraceElement element = PlayException.getInterestingStackTraceElement(ex.getTargetException());
if (element != null) {
throw new JavaExecutionException(Play.classes.getApplicationClass(element.getClassName()), element.getLineNumber(), ex.getTargetException());
throw new JavaExecutionException(Play.classes.getApplicationClass(element.getClassName()), element.getLineNumber(),
ex.getTargetException());
}
throw new JavaExecutionException(Http.Request.current().action, ex);
} catch (Exception e) {
Expand Down Expand Up @@ -451,36 +460,44 @@ public static Object invokeControllerMethod(Method method) throws Exception {
}

public static Object invokeControllerMethod(Method method, Object[] forceArgs) throws Exception {
if (Modifier.isStatic(method.getModifiers()) && !method.getDeclaringClass().getName().matches("^controllers\\..*\\$class$")) {
return invoke(method, null, forceArgs == null ? getActionMethodArgs(method, null) : forceArgs);
} else if (Modifier.isStatic(method.getModifiers())) {
Object[] args = getActionMethodArgs(method, null);
args[0] = Http.Request.current().controllerClass.getDeclaredField("MODULE$").get(null);
return invoke(method, null, args);
} else {
Object instance = null;
boolean isStatic = Modifier.isStatic(method.getModifiers());
String declaringClassName = method.getDeclaringClass().getName();
boolean isProbablyScala = declaringClassName.contains("$");

Http.Request request = Http.Request.current();

if (!isStatic && request.controllerInstance == null) {
request.controllerInstance = request.controllerClass.newInstance();
}

Object[] args = forceArgs != null ? forceArgs : getActionMethodArgs(method, request.controllerInstance);

if (isProbablyScala) {
try {
instance = method.getDeclaringClass().getDeclaredField("MODULE$").get(null);
} catch (Exception e) {
Annotation[] annotations = method.getDeclaredAnnotations();
String annotation = Utils.getSimpleNames(annotations);
if (!StringUtils.isEmpty(annotation)) {
throw new UnexpectedException("Method public static void " + method.getName() + "() annotated with " + annotation + " in class " + method.getDeclaringClass().getName() + " is not static.");
Object scalaInstance = request.controllerClass.getDeclaredField("MODULE$").get(null);
if (declaringClassName.endsWith("$class")) {
args[0] = scalaInstance; // Scala trait method
} else {
request.controllerInstance = (Controller) scalaInstance; // Scala
// object
// method
}
// TODO: Find a better error report
throw new ActionNotFoundException(Http.Request.current().action, e);
} catch (NoSuchFieldException e) {
// not Scala
}
return invoke(method, instance, forceArgs == null ? getActionMethodArgs(method, instance) : forceArgs);
}

return invoke(method, request.controllerInstance, args);
}

static Object invoke(Method method, Object instance, Object[] realArgs) throws Exception {
if(isActionMethod(method)) {
if (isActionMethod(method)) {
return invokeWithContinuation(method, instance, realArgs);
} else {
return method.invoke(instance, realArgs);
}
}

static final String C = "__continuation";
static final String A = "__callback";
static final String F = "__future";
Expand Down Expand Up @@ -573,32 +590,32 @@ public static Object[] getActionMethod(String fullAction) {
// Try the scala way
controllerClass = Play.classloader.getClassIgnoreCase(controller + "$");
if (!ControllerSupport.class.isAssignableFrom(controllerClass)) {
throw new ActionNotFoundException(fullAction, new Exception("class " + controller + " does not extend play.mvc.Controller"));
throw new ActionNotFoundException(fullAction,
new Exception("class " + controller + " does not extend play.mvc.Controller"));
}
}
actionMethod = Java.findActionMethod(action, controllerClass);
if (actionMethod == null) {
throw new ActionNotFoundException(fullAction, new Exception("No method public static void " + action + "() was found in class " + controller));
throw new ActionNotFoundException(fullAction,
new Exception("No method public static void " + action + "() was found in class " + controller));
}
} catch (PlayException e) {
throw e;
} catch (Exception e) {
throw new ActionNotFoundException(fullAction, e);
}
return new Object[]{controllerClass, actionMethod};
return new Object[] { controllerClass, actionMethod };
}


public static Object[] getActionMethodArgs(Method method, Object o) throws Exception {
String[] paramsNames = Java.parameterNames(method);
if (paramsNames == null && method.getParameterTypes().length > 0) {
throw new UnexpectedException("Parameter names not found for method " + method);
}


// Check if we have already performed the bind operation
Object[] rArgs = CachedBoundActionMethodArgs.current().retrieveActionMethodArgs(method);
if ( rArgs != null) {
if (rArgs != null) {
// We have already performed the binding-operation for this method
// in this request.
return rArgs;
Expand All @@ -608,24 +625,20 @@ public static Object[] getActionMethodArgs(Method method, Object o) throws Excep
for (int i = 0; i < method.getParameterTypes().length; i++) {

Class<?> type = method.getParameterTypes()[i];
Map<String, String[]> params = new HashMap<String, String[]> ();
Map<String, String[]> params = new HashMap<String, String[]>();

// In case of simple params, we don't want to parse the body.
if (type.equals(String.class) || Number.class.isAssignableFrom(type) || type.isPrimitive()) {
params.put(paramsNames[i], Scope.Params.current().getAll(paramsNames[i]));
} else {
params.putAll(Scope.Params.current().all());
}
Logger.trace("getActionMethodArgs name [" + paramsNames[i] + "] annotation [" + Utils.join(method.getParameterAnnotations()[i], " ") + "]");
Logger.trace("getActionMethodArgs name [" + paramsNames[i] + "] annotation ["
+ Utils.join(method.getParameterAnnotations()[i], " ") + "]");

RootParamNode root = ParamNode.convert(params);
rArgs[i] = Binder.bind(
root,
paramsNames[i],
method.getParameterTypes()[i],
method.getGenericParameterTypes()[i],
method.getParameterAnnotations()[i],
new Binder.MethodAndParamInfo(o, method, i + 1));
rArgs[i] = Binder.bind(root, paramsNames[i], method.getParameterTypes()[i], method.getGenericParameterTypes()[i],
method.getParameterAnnotations()[i], new Binder.MethodAndParamInfo(o, method, i + 1));
}

CachedBoundActionMethodArgs.current().storeActionMethodArgs(method, rArgs);
Expand Down
4 changes: 4 additions & 0 deletions framework/src/play/mvc/Http.java
Expand Up @@ -266,6 +266,10 @@ public static class Request implements Serializable {
* The invoked controller class
*/
public transient Class<? extends Controller> controllerClass;
/**
* The instance of invoked controller in case it uses non-static action methods.
*/
public transient Controller controllerInstance;
/**
* Free space to store your request specific data
*/
Expand Down
96 changes: 96 additions & 0 deletions framework/test-src/play/mvc/ActionInvokerTest.java
@@ -0,0 +1,96 @@
package play.mvc;

import org.junit.*;
import org.junit.Before;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertSame;

public class ActionInvokerTest {
private Object[] noArgs = new Object[0];

@Before
public void setUp() throws Exception {
Http.Request.current.set(new Http.Request());
}

@Test
public void invokeStaticJavaMethod() throws Exception {
Http.Request.current().controllerClass = TestController.class;
assertEquals("static", ActionInvoker.invokeControllerMethod(TestController.class.getMethod("staticJavaMethod"), noArgs));
}

@Test
public void invokeNonStaticJavaMethod() throws Exception {
Http.Request.current().controllerClass = TestController.class;
assertEquals("non-static", ActionInvoker.invokeControllerMethod(TestController.class.getMethod("nonStaticJavaMethod"), noArgs));
}

@Test
public void invokeScalaObjectMethod() throws Exception {
Http.Request.current().controllerClass = TestScalaObject$.class;
assertEquals("non-static", ActionInvoker.invokeControllerMethod(TestScalaObject$.class.getMethod("objectMethod"), noArgs));
}

@Test
public void invokeScalaTraitMethod() throws Exception {
Http.Request.current().controllerClass = TestScalaObject$.class;
assertEquals("static-with-object", ActionInvoker.invokeControllerMethod(TestScalaTrait$class.class.getMethod("traitMethod", Object.class), new Object[] {null}));
}

@Test
public void controllerInstanceIsPreservedForAllControllerMethodInvocations() throws Exception {
Http.Request.current().controllerClass = FullCycleTestController.class;

Controller controllerInstance = (Controller) ActionInvoker.invokeControllerMethod(FullCycleTestController.class.getMethod("before"), noArgs);
assertSame(controllerInstance, Http.Request.current().controllerInstance);

controllerInstance = (Controller) ActionInvoker.invokeControllerMethod(FullCycleTestController.class.getMethod("action"), noArgs);
assertSame(controllerInstance, Http.Request.current().controllerInstance);

controllerInstance = (Controller) ActionInvoker.invokeControllerMethod(FullCycleTestController.class.getMethod("after"), noArgs);
assertSame(controllerInstance, Http.Request.current().controllerInstance);
}

public static class TestController extends Controller {
public static String staticJavaMethod() {
return "static";
}

public String nonStaticJavaMethod() {
return "non-static";
}
}

public static class FullCycleTestController extends Controller {
@play.mvc.Before public Controller before() {
return this;
}

public Controller action() {
return this;
}

@play.mvc.After public Controller after() {
return this;
}
}

public static class TestScalaObject$ extends Controller {
public static final TestScalaObject$ MODULE$ = new TestScalaObject$();

public String objectMethod() {
return "non-static";
}

@Override public String toString() {
return "object";
}
}

public abstract static class TestScalaTrait$class {
public static String traitMethod(Object that) {
return "static-with-" + that;
}
}
}

0 comments on commit deb7d53

Please sign in to comment.