[engine][VELOCITY-904] Handle nested macro calls in 'velocimacro.arguments.preserve_literals' backward compatibility mode

git-svn-id: https://svn.apache.org/repos/asf/velocity/engine/trunk@1871332 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 53 insertions, 17 deletions

diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
index 43373a29..9fca114c 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
@@ -32,6 +32,8 @@ import org.apache.velocity.util.StringUtils;
 
 import java.io.IOException;
 import java.io.Writer;
+import java.util.Deque;
+import java.util.LinkedList;
 import java.util.List;
 
 /**
@@ -45,7 +47,7 @@ import java.util.List;
 public class VelocimacroProxy extends Directive
 {
     private String macroName;
-    private List<Macro.MacroArg> macroArgs = null;
+    private List<MacroArg> macroArgs = null;
     private String[] literalArgArray = null;
     private SimpleNode nodeTree = null;
     private int numMacroArgs = 0;
@@ -54,6 +56,8 @@ public class VelocimacroProxy extends Directive
     private String bodyReference;
     private boolean preserveArgumentsLiterals;
 
+    private static final Object NULL_VALUE_MARKER = new Object();
+
     /**
      * Return name of this Velocimacro.
      * @return The name of this Velocimacro.
@@ -244,23 +248,29 @@ public class VelocimacroProxy extends Directive
                 if (current == values[(i-1) * 2 + 1])
                 {
                     Object old = values[(i-1) * 2];
-                    if (old != null)
+                    if (old == null)
                     {
-                        context.put(macroArg.name, old);
+                        context.remove(macroArg.name);
+                    }
+                    else if (old == NULL_VALUE_MARKER)
+                    {
+                        context.put(macroArg.name, null);
                     }
                     else
                     {
-                        context.remove(macroArg.name);
+                        context.put(macroArg.name, old);
                     }
                 }
-            }
 
-            if (preserveArgumentsLiterals)
-            {
-                for (String literalKey : literalArgArray)
+                if (preserveArgumentsLiterals)
                 {
-                    // The behavior is not recursive.
-                    context.remove(literalKey);
+                    /* allow for nested calls */
+                    Deque<String> literalsStack = (Deque<String>)context.get(literalArgArray[i]);
+                    literalsStack.removeFirst();
+                    if (literalsStack.size() == 0)
+                    {
+                        context.remove(literalArgArray[i]);
+                    }
                 }
             }
         }
@@ -343,7 +353,11 @@ public class VelocimacroProxy extends Directive
         for (int i = 1; i < macroArgs.size(); i++)
         {
             MacroArg macroArg = macroArgs.get(i);
-            values[(i-1) * 2] = context.get(macroArg.name);
+            Object oldVal = context.get(macroArg.name);
+            values[(i-1) * 2] =
+                oldVal == null
+                ? context.containsKey(macroArg.name) ? NULL_VALUE_MARKER : null
+                : oldVal;
 
             // put the new value in
             Object newVal = null;
@@ -389,12 +403,18 @@ public class VelocimacroProxy extends Directive
                we still expect the passed argument literal to be displayed to be fully backward compatible. */
             if (preserveArgumentsLiterals && /* newVal == null && */ argNode != null)
             {
-                context.put(literalArgArray[i], argNode);
+                /* allow nested macro calls for B.C. */
+                Deque<String> literalsStack = (Deque<String>)context.get(literalArgArray[i]);
+                if (literalsStack == null)
+                {
+                    literalsStack = new LinkedList();
+                    context.put(literalArgArray[i], literalsStack);
+                }
+                literalsStack.addFirst(argNode.literal());
             }
         }
 
         // return the array of replaced and new values
         return values;
     }
-
 }
diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
index 2dd97b4c..527de725 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
@@ -28,7 +28,6 @@ import org.apache.velocity.io.Filter;
 import org.apache.velocity.runtime.Renderable;
 import org.apache.velocity.runtime.RuntimeConstants;
 import org.apache.velocity.runtime.directive.Block.Reference;
-import org.apache.velocity.runtime.parser.LogContext;
 import org.apache.velocity.runtime.parser.Parser;
 import org.apache.velocity.runtime.parser.Token;
 import org.apache.velocity.util.ClassUtils;
@@ -41,6 +40,7 @@ import org.apache.velocity.util.introspection.VelPropertySet;
 import java.io.IOException;
 import java.io.Writer;
 import java.lang.reflect.InvocationTargetException;
+import java.util.Deque;
 
 /**
  * This class is responsible for handling the references in
@@ -65,6 +65,7 @@ public class ASTReference extends SimpleNode
 
     private int referenceType;
     private String nullString;
+    private String alternateNullStringKey;
     private String rootString;
     private boolean escaped = false;
     private boolean computableReference = true;
@@ -165,6 +166,11 @@ public class ASTReference extends SimpleNode
          */
 
         rootString = rsvc.useStringInterning() ? getRoot().intern() : getRoot();
+        if (lookupAlternateLiteral)
+        {
+            /* cache alternate null tring key */
+            alternateNullStringKey = ".literal." + nullString;
+        }
 
         numChildren = jjtGetNumChildren();
 
@@ -644,10 +650,10 @@ public class ASTReference extends SimpleNode
 
         if (lookupAlternateLiteral)
         {
-            Node callingArgument = (Node)context.get(".literal." + nullString);
-            if (callingArgument != null)
+            Deque<String> alternateLiteralsStack = (Deque<String>)context.get(alternateNullStringKey);
+            if (alternateLiteralsStack != null && alternateLiteralsStack.size() > 0)
             {
-                ret = ((Node) callingArgument).literal();
+                ret = alternateLiteralsStack.peekFirst();
             }
         }
         return ret;
diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity904TestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity904TestCase.java
index 4e647072..04d78373 100755
--- a/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity904TestCase.java
+++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity904TestCase.java
@@ -58,4 +58,14 @@ public class Velocity904TestCase extends BaseTestCase
         assertEvalEquals("$variable", "#macro(mymacro $input)#set($input = $null)$input#end#set($variable = 'value')#mymacro($variable)");
     }
 
+    public void testSubMacroNoPreserve()
+    {
+        assertEvalEquals("$return$return$return", "#macro(macro1 $return)$return#macro2($param2)$return#end#macro(macro2 $return)$return#end#macro1($param)");
+    }
+
+    public void testSubMacroPreserve()
+    {
+        assertEvalEquals("$param$param2$param", "#macro(macro1 $return)$return#macro2($param2)$return#end#macro(macro2 $return)$return#end#macro1($param)");
+    }
+
 }