[#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.

framework/src/play/data/binding/CachedBoundActionMethodArgs.java

@@ -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);
+    }
+
+}

framework/src/play/data/binding/ParamNode.java

@@ -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) {

framework/src/play/db/jpa/GenericModel.java

@@ -60,7 +60,9 @@ public static <T extends JPABase> T edit(Object o, String name, Map<String, Stri
     @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 @@ public static <T extends JPABase> T edit(ParamNode rootParamNode, String name, O
                             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 @@ public static <T extends JPABase> T edit(ParamNode rootParamNode, String name, O
                                     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 @@ public static <T extends JPABase> T edit(ParamNode rootParamNode, String name, O
             return (T) o;
         } catch (Exception e) {
             throw new UnexpectedException(e);
+        } finally {
+            // restoring changes to paramNode
+            ParamNode.restoreRemovedChildren( removedNodesList );
         }
     }
 

framework/src/play/mvc/ActionInvoker.java

@@ -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 @@ public static Object[] getActionMethodArgs(Method method, Object o) throws Excep
             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 @@ public static Object[] getActionMethodArgs(Method method, Object o) throws Excep
                         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;
     }
 }

framework/src/play/mvc/Scope.java

@@ -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;

framework/src/play/server/ServletWrapper.java

@@ -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();
         }
     }
 

samples-and-tests/just-test-cases/app/controllers/JPABinding.java

@@ -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);