[engine] Review VELOCITY-926 fix:

deprecate velocimacro.arguments.preserve_literals in favor of the new velocimacro.enable_bc_mode flag,
which mimics 1.7 velocimacro behavior:

- preserve arguments literals:

    #macro(m $arg) $arg #end
    m($null)

will displays $arg w/o bc mode and $null with bc mode

- use global defaults for missing arguments:

    #macro(m $foo) $foo #end
    #set($foo='foo')
    #m()

will display $foo w/o bc mode and 'foo' with bc mode

The following use cases have been left aside from backward compatibility:

- preserving local macro scope values

    #macro(test) #set($foo = 'foo') $some_tool.change_foo_value_in_global_context_to_bar() $foo #end
    #test()

will always display 'bar', while 1.7 displayed 'foo'

- setting a null argument to null, while argument name exists in context, 

    #macro(setnull $foo) #set($foo = $null) #end
    #set($foo='foo')
    #setnull($null)
    $foo

Will always display 'foo' (w or w/o bc mode), while 1.7 did display $foo




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

11 files changed, 275 insertions, 70 deletions

diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java
index 2d876919..3789a76e 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java
@@ -274,4 +274,12 @@ public interface DeprecatedRuntimeConstants
      */
     String OLD_SPACE_GOBBLING = "space.gobbling";
 
+    /**
+     * When displaying null or invalid non-quiet references, use the argument literal reference
+     * instead of the one in the macro block. Defaults to false.
+     * @since 2.1
+     * @Deprecated since 2.2, see {@link RuntimeConstants#VM_ENABLE_BC_MODE}
+     **/
+    String OLD_VM_ENABLE_BC_MODE = "velocimacro.arguments.preserve_literals";
+
 }
diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java
index 9b640e8d..89d3c41f 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java
@@ -331,11 +331,21 @@ public interface RuntimeConstants extends DeprecatedRuntimeConstants
     String VM_ARGUMENTS_STRICT = "velocimacro.arguments.strict";
 
     /**
-     * When displaying null or invalid non-quiet references, use the argument literal reference
-     * instead of the one in the macro block. Defaults to false.
-     * @since 2.1
-     **/
-    String VM_PRESERVE_ARGUMENTS_LITERALS = "velocimacro.arguments.preserve_literals";
+     * This flag enable the 1.7 backward compatible mode for velocimacros (defaults to false):
+     * <ul>
+     *     <li>
+     *         preserve argument literals: when displaying null or invalid non-quiet references,
+     *         use the argument literal reference instead of the one in the macro block. Defaults to false.
+     *     </li>
+     *     <li>
+     *         use global values for missing arguments: when calling a macro with fewer arguments than declared,
+     *         if those arguments don't have an explicit default value in the macro definition, default values will
+     *         be looked for in the global context
+     *     </li>
+     * </ul>
+     * @since 2.2
+     */
+    String VM_ENABLE_BC_MODE = "velocimacro.enable_bc_mode";
 
     /**
      * Specify the maximum depth for macro calls
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 e8467fcf..ee4bddd3 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
@@ -56,7 +56,7 @@ public class VelocimacroProxy extends Directive
     private boolean strictArguments;
     private int maxCallDepth;
     private String bodyReference;
-    private boolean preserveArgumentsLiterals;
+    private boolean enableBCmode;
 
     private static final Object NULL_VALUE_MARKER = new Object();
 
@@ -99,7 +99,7 @@ public class VelocimacroProxy extends Directive
 
         // for performance reasons we precache these strings - they are needed in
         // "render literal if null" functionality
-        if (preserveArgumentsLiterals)
+        if (enableBCmode)
         {
             literalArgArray = new String[macroArgs.size()];
             for (int i = 0; i < macroArgs.size(); i++)
@@ -160,7 +160,7 @@ public class VelocimacroProxy extends Directive
         // get name of the reference that refers to AST block passed to block macro call
         bodyReference = rsvc.getString(RuntimeConstants.VM_BODY_REFERENCE, "bodyContent");
 
-        preserveArgumentsLiterals = rsvc.getBoolean(RuntimeConstants.VM_PRESERVE_ARGUMENTS_LITERALS, false);
+        enableBCmode = rsvc.getBoolean(RuntimeConstants.VM_ENABLE_BC_MODE, false);
     }
 
     /**
@@ -247,9 +247,10 @@ public class VelocimacroProxy extends Directive
             {
                 MacroArg macroArg = macroArgs.get(i);
                 current = context.get(macroArg.name);
-                if (current == values[(i-1) * 2 + 1])
+                Object given = values[(i-1) * 2 + 1];
+                Object old = values[(i-1) * 2];
+                if (current == given || current == null && given == NULL_VALUE_MARKER)
                 {
-                    Object old = values[(i-1) * 2];
                     if (old == null)
                     {
                         context.remove(macroArg.name);
@@ -264,11 +265,11 @@ public class VelocimacroProxy extends Directive
                     }
                 }
 
-                if (preserveArgumentsLiterals)
+                if (enableBCmode)
                 {
                     /* allow for nested calls */
                     Deque<String> literalsStack = (Deque<String>)context.get(literalArgArray[i]);
-                    if (literalsStack != null) /* may be null if argument was missing in macro call */
+                    if (literalsStack != null) /* shouldn't be null */
                     {
                         literalsStack.removeFirst();
                         if (literalsStack.size() == 0)
@@ -354,6 +355,8 @@ public class VelocimacroProxy extends Directive
     	// Changed two dimensional array to single dimensional to optimize memory lookups
         Object[] values = new Object[macroArgs.size() * 2];
 
+        boolean warnedMissingArguments = false;
+
         // Move arguments into the macro's context. Start at one to skip macro name
         for (int i = 1; i < macroArgs.size(); i++)
         {
@@ -394,18 +397,26 @@ public class VelocimacroProxy extends Directive
             }
             else
             {
-                // Backward compatibility logging, Mainly for MacroForwardDefinedTestCase
-                log.debug("VM #{}: too few arguments to macro. Wanted {} got {}",
-                          macroArgs.get(0).name, macroArgs.size() - 1, callArgNum);
-                break;
+                if (!warnedMissingArguments)
+                {
+                    // Backward compatibility logging, Mainly for MacroForwardDefinedTestCase
+                    log.debug("VM #{}: too few arguments to macro. Wanted {} got {}",
+                        macroArgs.get(0).name, macroArgs.size() - 1, callArgNum);
+                    warnedMissingArguments = true;
+                }
+                if (enableBCmode)
+                {
+                    // use the global context value as default
+                    newVal = oldVal;
+                }
             }
 
             values[(i-1) * 2 + 1] = newVal;
 
-            /* when preserveArgumentsLiterals is true, we still store the actual reference passed to the macro
+            /* when enableBCmode is true, we still store the actual reference passed to the macro
                even if the value is not null, because *if* the argument is set to null *during* the macro rendering
                we still expect the passed argument literal to be displayed to be fully backward compatible. */
-            if (preserveArgumentsLiterals && /* newVal == null && */ argNode != null)
+            if (enableBCmode && /* newVal == null && */ argNode != null)
             {
                 /* allow nested macro calls for B.C. */
                 Deque<String> literalsStack = (Deque<String>)context.get(literalArgArray[i]);
@@ -415,7 +426,7 @@ public class VelocimacroProxy extends Directive
                     context.put(literalArgArray[i], literalsStack);
                 }
                 /* Reflects the strange 1.7 behavor... */
-                if (argNode instanceof ASTReference || argNode instanceof ASTStringLiteral)
+                if (argNode != null && (argNode instanceof ASTReference || argNode instanceof ASTStringLiteral))
                 {
                     literalsStack.addFirst(argNode.literal());
                 }
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 527de725..747780dd 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
@@ -157,7 +157,7 @@ public class ASTReference extends SimpleNode
 
         strictEscape = rsvc.getBoolean(RuntimeConstants.RUNTIME_REFERENCES_STRICT_ESCAPE, false);
         strictRef = rsvc.getBoolean(RuntimeConstants.RUNTIME_REFERENCES_STRICT, false);
-        lookupAlternateLiteral = rsvc.getBoolean(RuntimeConstants.VM_PRESERVE_ARGUMENTS_LITERALS, false);
+        lookupAlternateLiteral = rsvc.getBoolean(RuntimeConstants.VM_ENABLE_BC_MODE, false);
 
         /*
          *  the only thing we can do in init() is getRoot()
diff --git a/velocity-engine-core/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties b/velocity-engine-core/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties
index 30119a24..3d96b636 100644
--- a/velocity-engine-core/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties
+++ b/velocity-engine-core/src/main/resources/org/apache/velocity/runtime/defaults/velocity.properties
@@ -118,14 +118,16 @@ velocimacro.arguments.strict = false
 velocimacro.body_reference = bodyContent
 
 # ----------------------------------------------------------------------------
-# VELOCIMACRO PRESERVE ARGUMENTS LITERALS
+# VELOCIMACRO ENABLE BC MODE
 # ----------------------------------------------------------------------------
-# if true, when a macro has to render a null or invalid argument reference
+# Backward compatibility for 1.7 macros behavior.
+# If true, when a macro has to render a null or invalid argument reference
 # which is not quiet, it will print the provided literal reference instead
-# of the one found in the body of the macro
+# of the one found in the body of the macro ; and if a macro argument is
+# without an explicit default value is missing from the macro call, its value
+# will be looked up in the global context
 # ----------------------------------------------------------------------------
-velocimacro.arguments.preserve_literals = false
-
+velocimacro.enable_bc_mode = false
 
 # ----------------------------------------------------------------------------
 # STRICT REFERENCE MODE
diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/PreserveArgumentsLiteralsTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/PreserveArgumentsLiteralsTestCase.java
deleted file mode 100644
index ff627e18..00000000
--- a/velocity-engine-core/src/test/java/org/apache/velocity/test/PreserveArgumentsLiteralsTestCase.java
+++ /dev/null
@@ -1,45 +0,0 @@
-package org.apache.velocity.test;
-
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-import org.apache.velocity.app.VelocityEngine;
-import org.apache.velocity.runtime.RuntimeConstants;
-
-/**
- * This class tests the mode where velocimacros do preserve arguments literals
- */
-
-public class PreserveArgumentsLiteralsTestCase extends BaseTestCase
-{
-    public PreserveArgumentsLiteralsTestCase(final String name)
-    {
-        super(name);
-    }
-
-    protected void setUpEngine(VelocityEngine engine)
-    {
-        engine.setProperty(RuntimeConstants.VM_PRESERVE_ARGUMENTS_LITERALS, true);
-    }
-
-    public void testPreserveLiterals()
-    {
-        assertEvalEquals("$bar","#macro(m $foo)$foo#end#m($bar)");
-    }
-}
diff --git a/velocity-engine-core/src/test/java/org/apache/velocity/test/VelocimacroBCModeTestCase.java b/velocity-engine-core/src/test/java/org/apache/velocity/test/VelocimacroBCModeTestCase.java
new file mode 100644
index 00000000..72a27c15
--- /dev/null
+++ b/velocity-engine-core/src/test/java/org/apache/velocity/test/VelocimacroBCModeTestCase.java
@@ -0,0 +1,99 @@
+package org.apache.velocity.test;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.velocity.Template;
+import org.apache.velocity.VelocityContext;
+import org.apache.velocity.app.VelocityEngine;
+import org.apache.velocity.runtime.RuntimeConstants;
+
+import java.io.BufferedWriter;
+import java.io.FileOutputStream;
+import java.io.OutputStreamWriter;
+import java.io.Writer;
+
+/**
+ * This class tests the mode where velocimacros do preserve arguments literals
+ */
+
+public class VelocimacroBCModeTestCase extends BaseTestCase
+{
+    private static final String BASE_DIR = TEST_COMPARE_DIR + "/bc_mode";
+    private static final String CMP_DIR = BASE_DIR + "/compare";
+    private static final String RESULTS_DIR = TEST_RESULT_DIR + "/bc_mode";
+
+    public VelocimacroBCModeTestCase(final String name)
+    {
+        super(name);
+    }
+
+    protected void setUpEngine(VelocityEngine engine)
+    {
+        boolean bcMode = !getName().contains("NoPreserve");
+        engine.setProperty(RuntimeConstants.VM_ENABLE_BC_MODE, bcMode);
+        engine.setProperty("file.resource.loader.path", TEST_COMPARE_DIR + "/bc_mode");
+    }
+
+    public void testPreserveLiterals()
+    {
+        assertEvalEquals("$bar","#macro(m $foo)$foo#end#m($bar)");
+    }
+
+    public void testGlobalDefaults()
+    {
+        assertEvalEquals("foo","#macro(m $foo)$foo#end#set($foo='foo')#m()");
+    }
+
+    public void testVariousCasesPreserve() throws Exception
+    {
+        doTestVariousCases("bc_mode_enabled");
+    }
+
+    public void testVariousCasesNoPreserve() throws Exception
+    {
+        doTestVariousCases("bc_mode_disabled");
+    }
+
+    private void doTestVariousCases(String compare_ext) throws Exception
+    {
+        assureResultsDirectoryExists(RESULTS_DIR);
+        String basefilename = "test_bc_mode";
+        Template template = engine.getTemplate( getFileName(null, basefilename, "vtl") );
+        context = new VelocityContext();
+        FileOutputStream fos;
+        Writer fwriter;
+
+        fos = new FileOutputStream (getFileName(RESULTS_DIR, basefilename, RESULT_FILE_EXT));
+
+        fwriter = new BufferedWriter( new OutputStreamWriter(fos) );
+
+        template.merge(context, fwriter);
+        fwriter.flush();
+        fwriter.close();
+
+        if (!isMatch(RESULTS_DIR, CMP_DIR, basefilename, RESULT_FILE_EXT, compare_ext))
+        {
+            String result = getFileContents(RESULTS_DIR, basefilename, RESULT_FILE_EXT);
+            String compare = getFileContents(CMP_DIR, basefilename, compare_ext);
+
+            assertEquals(compare, result);
+        }
+    }
+}
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 b22e198b..86caa7cf 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
@@ -35,6 +35,7 @@ public class Velocity904TestCase extends BaseTestCase
     @Override
     protected void setUpEngine(VelocityEngine engine)
     {
+        // that will also test the deprecation of velocimacro.arguments.preserve_literals towards velocimacro.enable_bc_mode
         engine.setProperty("velocimacro.arguments.preserve_literals", getName().contains("NoPreserve") ? "false" : "true");
     }
 
diff --git a/velocity-engine-core/src/test/resources/bc_mode/compare/test_bc_mode.bc_mode_disabled b/velocity-engine-core/src/test/resources/bc_mode/compare/test_bc_mode.bc_mode_disabled
new file mode 100644
index 00000000..59d87824
--- /dev/null
+++ b/velocity-engine-core/src/test/resources/bc_mode/compare/test_bc_mode.bc_mode_disabled
@@ -0,0 +1,40 @@
+
+
+
+
+A) Null Values
+1. missing argument
+foo=$foo => disp miss [foo=$foo] => foo=$foo
+foo=$foo => setn miss [foo=$foo] => foo=$foo
+foo=$foo => setv miss [foo=inn] => foo=inn
+2. null argument
+foo=$foo => disp null [foo=$foo] => foo=$foo
+foo=$foo => setn null [foo=$foo] => foo=$foo
+foo=$foo => setv null [foo=inn] => foo=inn
+3. non-colliding argument
+foo=$foo => disp ncol [foo=$foo] => foo=$foo
+foo=$foo => setn ncol [foo=$foo] => foo=$foo
+foo=$foo => setv ncol [foo=inn] => foo=inn
+4. colliding argument
+foo=$foo => disp coll [foo=$foo] => foo=$foo
+foo=$foo => setn coll [foo=$foo] => foo=$foo
+foo=$foo => setv coll [foo=inn] => foo=inn
+
+B) Non-null Values
+1. missing argument
+foo=foo => disp miss [foo=$foo] => foo=foo
+foo=foo => setn miss [foo=$foo] => foo=foo
+foo=foo => setv miss [foo=inn] => foo=inn
+2. null argument
+foo=foo => disp null [foo=$foo] => foo=foo
+foo=foo => setn null [foo=$foo] => foo=foo
+foo=foo => setv null [foo=inn] => foo=inn
+3. non-colliding argument
+foo=foo => disp ncol [foo=bar] => foo=foo
+foo=foo => setn ncol [foo=$foo] => foo=$foo
+foo=foo => setv ncol [foo=bar] => foo=foo
+4. colliding argument
+foo=foo => disp coll [foo=foo] => foo=foo
+foo=foo => setn coll [foo=$foo] => foo=$foo
+foo=foo => setv coll [foo=foo] => foo=foo
+
diff --git a/velocity-engine-core/src/test/resources/bc_mode/compare/test_bc_mode.bc_mode_enabled b/velocity-engine-core/src/test/resources/bc_mode/compare/test_bc_mode.bc_mode_enabled
new file mode 100644
index 00000000..934d659e
--- /dev/null
+++ b/velocity-engine-core/src/test/resources/bc_mode/compare/test_bc_mode.bc_mode_enabled
@@ -0,0 +1,40 @@
+
+
+
+
+A) Null Values
+1. missing argument
+foo=$foo => disp miss [foo=$foo] => foo=$foo
+foo=$foo => setn miss [foo=$foo] => foo=$foo
+foo=$foo => setv miss [foo=inn] => foo=inn
+2. null argument
+foo=$foo => disp null [foo=$null] => foo=$foo
+foo=$foo => setn null [foo=$foo] => foo=$foo
+foo=$foo => setv null [foo=inn] => foo=inn
+3. non-colliding argument
+foo=$foo => disp ncol [foo=$bar] => foo=$foo
+foo=$foo => setn ncol [foo=$foo] => foo=$foo
+foo=$foo => setv ncol [foo=inn] => foo=inn
+4. colliding argument
+foo=$foo => disp coll [foo=$foo] => foo=$foo
+foo=$foo => setn coll [foo=$foo] => foo=$foo
+foo=$foo => setv coll [foo=inn] => foo=inn
+
+B) Non-null Values
+1. missing argument
+foo=foo => disp miss [foo=foo] => foo=foo
+foo=foo => setn miss [foo=$foo] => foo=$foo
+foo=foo => setv miss [foo=foo] => foo=foo
+2. null argument
+foo=foo => disp null [foo=$null] => foo=foo
+foo=foo => setn null [foo=$foo] => foo=foo
+foo=foo => setv null [foo=inn] => foo=inn
+3. non-colliding argument
+foo=foo => disp ncol [foo=bar] => foo=foo
+foo=foo => setn ncol [foo=$foo] => foo=$foo
+foo=foo => setv ncol [foo=bar] => foo=foo
+4. colliding argument
+foo=foo => disp coll [foo=foo] => foo=foo
+foo=foo => setn coll [foo=$foo] => foo=$foo
+foo=foo => setv coll [foo=foo] => foo=foo
+
diff --git a/velocity-engine-core/src/test/resources/bc_mode/test_bc_mode.vtl b/velocity-engine-core/src/test/resources/bc_mode/test_bc_mode.vtl
new file mode 100644
index 00000000..8ab1050b
--- /dev/null
+++ b/velocity-engine-core/src/test/resources/bc_mode/test_bc_mode.vtl
@@ -0,0 +1,39 @@
+#macro(store)#set($foo_ = $foo)#set($bar_ = $bar)#end##
+#macro(reset)#set($foo = $foo_)#set($bar = $bar_)#end##
+#macro(state)foo=$foo#end##
+
+#macro(disp $foo)[foo=$foo]#end
+#macro(setn $foo)#set($foo=$null)#disp($foo)#end
+#macro(setv $foo)#if(!$foo)#set($foo='inn')#end#disp($foo)#end
+#macro(sub)#set($foo='sub')#end
+
+#macro(test)
+1. missing argument
+#store#state => disp miss #disp() => #state#reset
+#store#state => setn miss #setn() => #state#reset
+#store#state => setv miss #setv() => #state#reset
+2. null argument
+#store#state => disp null #disp($null) => #state#reset
+#store#state => setn null #setn($null) => #state#reset
+#store#state => setv null #setv($null) => #state#reset
+3. non-colliding argument
+#store#state => disp ncol #disp($bar) => #state#reset
+#store#state => setn ncol #setn($bar) => #state#reset
+#store#state => setv ncol #setv($bar) => #state#reset
+4. colliding argument
+#store#state => disp coll #disp($foo) => #state#reset
+#store#state => setn coll #setn($foo) => #state#reset
+#store#state => setv coll #setv($foo) => #state#reset
+#end
+
+
+A) Null Values
+#set($foo = $null)
+#set($bar = $null)
+#test()
+
+B) Non-null Values
+#set($foo = 'foo')
+#set($bar = 'bar')
+#test()
+