Permalink
Browse files

[#1195] Fixes regression introduced after new Binder impl by making J…

…PA binding not modifying the ParamNode-structure.

This commit also caches the main ActionInvoker http-to-method-args bind-operation pr request. This prevent us from
performing the (heavy) bind-operation twice pr request when using Validation.
  • Loading branch information...
1 parent 9bff894 commit 602e53db3cf5d856f3aa2376a055346d954f35d4 @mbknor mbknor committed Oct 30, 2011
@@ -0,0 +1,36 @@
+package play.data.binding;
+
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+// ActionInvoker.getActionMethodArgs() is called twice when using validation
+// so we use this threadlocal cache to store the binding-result pr method pr request.
+// This way we don't have to do it twice.
+public class CachedBoundActionMethodArgs {
+
+ private static ThreadLocal<CachedBoundActionMethodArgs> current = new ThreadLocal<CachedBoundActionMethodArgs>();
+
+ private Map<Method, Object[]> preBoundActionMethodArgs = new HashMap<Method, Object[]>(1);
+
+ public static void init() {
+ current.set( new CachedBoundActionMethodArgs());
+ }
+
+ public static void clear() {
+ current.remove();
+ }
+
+ public static CachedBoundActionMethodArgs current() {
+ return current.get();
+ }
+
+ public void storeActionMethodArgs( Method method, Object[] rArgs) {
+ preBoundActionMethodArgs.put(method, rArgs);
+ }
+
+ public Object[] retrieveActionMethodArgs( Method method) {
+ return preBoundActionMethodArgs.get(method);
+ }
+
+}
@@ -4,6 +4,7 @@
import java.util.Collection;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -63,8 +64,40 @@ public ParamNode getChild(String name, boolean returnEmptyChildIfNotFound) {
return child;
}
- public boolean removeChild(String name) {
- return _children.remove(name) != null;
+ public static class RemovedNode {
+ public final ParamNode removedFrom;
+ public final ParamNode removedNode;
+
+ public RemovedNode(ParamNode removedFrom, ParamNode removedNode) {
+ this.removedFrom = removedFrom;
+ this.removedNode = removedNode;
+ }
+ }
+
+ /**
+ * Removes a child from this node, but stores what is removed to list.
+ * The we can later call which will add it back again.
+ * This is a "hack" related to #1195 which makes it possible to reuse the RootParamsNode-structure
+ * if you want to perform the bind-operation multiple times.
+ *
+ * @param name the name of the child-node in this paramNode which should be removed.
+ * @param removedNodesList a list where info about what is removed where is stored.
+ * @return true if anything was removed.
+ */
+ public boolean removeChild(String name, List<RemovedNode> removedNodesList) {
+ ParamNode removedNode = _children.remove(name);
+ if ( removedNode != null) {
+ removedNodesList.add( new RemovedNode(this, removedNode));
+ return true;
+ } else {
+ return false;
+ }
+ }
+
+ public static void restoreRemovedChildren( List<RemovedNode> removedNodesList ) {
+ for ( RemovedNode rn : removedNodesList) {
+ rn.removedFrom._children.put( rn.removedNode.name, rn.removedNode);
+ }
}
private ParamNode getChild(String[] nestedNames) {
@@ -60,7 +60,9 @@
@SuppressWarnings("deprecation")
public static <T extends JPABase> T edit(ParamNode rootParamNode, String name, Object o, Annotation[] annotations) {
ParamNode paramNode = rootParamNode.getChild(name, true);
-
+ // #1195 - Needs to keep track of whick keys we remove so that we can restore it before
+ // returning from this method.
+ List<ParamNode.RemovedNode> removedNodesList = new ArrayList<ParamNode.RemovedNode>();
try {
BeanWrapper bw = new BeanWrapper(o.getClass());
// Start with relations
@@ -103,7 +105,7 @@
String[] ids = fieldParamNode.getChild(keyName, true).getValues();
if (ids != null) {
// Remove it to prevent us from finding it again later
- fieldParamNode.removeChild(keyName);
+ fieldParamNode.removeChild(keyName, removedNodesList);
for (String _id : ids) {
if (_id.equals("")) {
continue;
@@ -129,24 +131,24 @@
Object to = q.getSingleResult();
edit(paramNode, field.getName(), to, field.getAnnotations());
// Remove it to prevent us from finding it again later
- paramNode.removeChild( field.getName());
+ paramNode.removeChild( field.getName(), removedNodesList);
bw.set(field.getName(), o, to);
} catch (NoResultException e) {
Validation.addError(fieldParamNode.getOriginalKey(), "validation.notFound", ids[0]);
// Remove only the key to prevent us from finding it again later
// This how the old impl does it..
- fieldParamNode.removeChild(keyName);
+ fieldParamNode.removeChild(keyName, removedNodesList);
if (fieldParamNode.getAllChildren().size()==0) {
// remove the whole node..
- paramNode.removeChild( field.getName());
+ paramNode.removeChild( field.getName(), removedNodesList);
}
}
} else if (ids != null && ids.length > 0 && ids[0].equals("")) {
bw.set(field.getName(), o, null);
// Remove it to prevent us from finding it again later
- paramNode.removeChild(field.getName());
+ paramNode.removeChild(field.getName(), removedNodesList);
}
}
}
@@ -157,6 +159,9 @@
return (T) o;
} catch (Exception e) {
throw new UnexpectedException(e);
+ } finally {
+ // restoring changes to paramNode
+ ParamNode.restoreRemovedChildren( removedNodesList );
}
}
@@ -19,6 +19,7 @@
import play.classloading.enhancers.ControllersEnhancer.ControllerInstrumentation;
import play.classloading.enhancers.ControllersEnhancer.ControllerSupport;
import play.data.binding.Binder;
+import play.data.binding.CachedBoundActionMethodArgs;
import play.data.binding.RootParamNode;
import play.data.parsing.UrlEncodedParser;
import play.data.validation.Validation;
@@ -64,6 +65,7 @@ public static void resolve(Http.Request request, Http.Response response) {
Scope.RouteArgs.current.set(new Scope.RouteArgs());
Scope.Session.current.set(Scope.Session.restore());
Scope.Flash.current.set(Scope.Flash.restore());
+ CachedBoundActionMethodArgs.init();
ControllersEnhancer.currentAction.set(new Stack<String>());
@@ -614,9 +616,17 @@ static Object invokeWithContinuation(Method method, Object instance, Object[] re
throw new UnexpectedException("Parameter names not found for method " + method);
}
- Object[] rArgs = new Object[method.getParameterTypes().length];
+ // Check if we have already performed the bind operation
+ Object[] rArgs = CachedBoundActionMethodArgs.current().retrieveActionMethodArgs( method );
+ if ( rArgs != null) {
+ // We have already performed the binding-operation for this method
+ // in this request.
+ return rArgs;
+ }
+
+ rArgs = new Object[method.getParameterTypes().length];
if (method.getParameterTypes().length>0) {
-
+
RootParamNode root = Scope.Params.current().getRootParamNode();
for (int i = 0; i < method.getParameterTypes().length; i++) {
@@ -636,6 +646,10 @@ static Object invokeWithContinuation(Method method, Object instance, Object[] re
new Binder.MethodAndParamInfo(o, method, i + 1));
}
}
+
+ // Store the bind-result in case we need it again in the same request
+ CachedBoundActionMethodArgs.current().storeActionMethodArgs(method, rArgs);
+
return rArgs;
}
}
@@ -1,6 +1,7 @@
package play.mvc;
import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
import java.net.URLDecoder;
import java.net.URLEncoder;
import java.util.HashMap;
@@ -5,6 +5,7 @@
import play.Invoker.InvocationContext;
import play.Logger;
import play.Play;
+import play.data.binding.CachedBoundActionMethodArgs;
import play.data.validation.Validation;
import play.exceptions.PlayException;
import play.exceptions.UnexpectedException;
@@ -162,6 +163,7 @@ protected void service(HttpServletRequest httpServletRequest, HttpServletRespons
Scope.Flash.current.remove();
Scope.RenderArgs.current.remove();
Scope.RouteArgs.current.remove();
+ CachedBoundActionMethodArgs.clear();
}
}
@@ -14,7 +14,8 @@ public static void index() {
render();
}
- public static void create(Project project) {
+ // #1195 Use @Valid to trigger the validation before the action-invoking
+ public static void create(@Valid Project project) {
System.out.println(project);
project.create();
show(project.id);

0 comments on commit 602e53d

Please sign in to comment.