Mailing-List: contact commits-help@velocity.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@velocity.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1752548 - in /velocity/engine/trunk/velocity-engine-core/src:
 main/java/org/apache/velocity/runtime/parser/node/
 test/java/org/apache/velocity/test/
Date: Wed, 13 Jul 2016 21:58:37 -0000
To: commits@velocity.apache.org
From: cbrisson@apache.org
Message-Id: <20160713215837.624DC3A0051@svn01-us-west.apache.org>
archived-at: Wed, 13 Jul 2016 21:58:43 -0000

Author: cbrisson
Date: Wed Jul 13 21:58:37 2016
New Revision: 1752548

URL: http://svn.apache.org/viewvc?rev=1752548&view=rev
Log:
invalid reference event should not be triggered by null values, or when testing a value inside #if/#elseif (fixes VELOCITY-553)

Modified:
    velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java
    velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
    velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/InvalidEventHandlerTestCase.java

Modified: velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java?rev=1752548&r1=1752547&r2=1752548&view=diff
==============================================================================
--- velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java (original)
+++ velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java Wed Jul 13 21:58:37 2016
@@ -32,6 +32,7 @@ import org.apache.velocity.runtime.direc
 import org.apache.velocity.runtime.parser.Parser;
 import org.apache.velocity.util.ClassUtils;
 import org.apache.velocity.util.introspection.Info;
+import org.apache.velocity.util.introspection.IntrospectionCacheData;
 import org.apache.velocity.util.introspection.VelMethod;
 
 /**
@@ -161,7 +162,25 @@ public class ASTMethod extends SimpleNod
 
         VelMethod method = ClassUtils.getMethod(methodName, params, paramClasses,
             o, context, this, strictRef);
-        if (method == null) return null;
+
+        /*
+         * The parent class (typically ASTReference) uses the icache entry
+         * under 'this' key to distinguish a valid null result from a non-existent method.
+         * So update this dummy cache value if necessary.
+         */
+        IntrospectionCacheData prevICD = context.icacheGet(this);
+        if (method == null)
+        {
+            if (prevICD != null)
+            {
+                context.icachePut(this, null);
+            }
+            return null;
+        }
+        else if (prevICD == null)
+        {
+            context.icachePut(this, new IntrospectionCacheData()); // no need to fill in its members
+        }
 
         try
         {

Modified: velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java?rev=1752548&r1=1752547&r2=1752548&view=diff
==============================================================================
--- velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java (original)
+++ velocity/engine/trunk/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java Wed Jul 13 21:58:37 2016
@@ -213,7 +213,7 @@ public class ASTReference extends Simple
     /**
      *   gets an Object that 'is' the value of the reference
      *
-     *   @param o   unused Object parameter
+     *   @param o Object parameter, unused per se, but non-null by convention inside an #if/#elseif evaluation
      *   @param context context used to generate value
      * @return The execution result.
      * @throws MethodInvocationException
@@ -221,6 +221,14 @@ public class ASTReference extends Simple
     public Object execute(Object o, InternalContextAdapter context)
         throws MethodInvocationException
     {
+        /*
+         *  The only case where 'o' is not null is when this method is called by evaluate().
+         *  Its value is not used, but it is a convention meant to allow statements like
+         *  #if($invalidReference) *not* to trigger an invalid reference event.
+         *  Statements like #if($invalidReference.prop) should *still* trigger an invalid reference event.
+         *  Statements like #if($validReference.invalidProp) should not.
+         */
+        boolean onlyTestingReference = (o != null);
 
         if (referenceType == RUNT)
             return null;
@@ -233,8 +241,15 @@ public class ASTReference extends Simple
 
         if (result == null && !strictRef)
         {
-            return EventHandlerUtil.invalidGetMethod(rsvc, context, 
-                    "$" + rootString, null, null, uberInfo);
+            /*
+             * do not trigger an invalid reference if the reference is present, but with a null value
+             * don't either inside an #if/#elseif evaluation context
+             */
+            if (!context.containsKey(rootString) && !onlyTestingReference)
+            {
+                return EventHandlerUtil.invalidGetMethod(rsvc, context,
+                        "$" + rootString, null, null, uberInfo);
+            }
         }
 
         /*
@@ -273,8 +288,15 @@ public class ASTReference extends Simple
                 if (result == null && !strictRef)  // If strict and null then well catch this
                                                    // next time through the loop
                 {
-                    failedChild = i;
-                    break;
+                    // do not call bad reference handler if the getter is present
+                    // (it means the getter has been called and returned null)
+                    // do not either if the *last* child failed while testing the reference
+                    Object getter = context.icacheGet(jjtGetChild(i));
+                    if (getter == null && (!onlyTestingReference || i < jjtGetNumChildren() - 1))
+                    {
+                        failedChild = i;
+                        break;
+                    }
                 }
             }
 
@@ -282,8 +304,15 @@ public class ASTReference extends Simple
             {
                 if (failedChild == -1)
                 {
-                    result = EventHandlerUtil.invalidGetMethod(rsvc, context, 
-                            "$" + rootString, previousResult, null, uberInfo);                    
+                    /*
+                     * do not trigger an invalid reference if the reference is present, but with a null value
+                     * don't either inside an #if/#elseif evaluation context when there's no child
+                     */
+                    if (!context.containsKey(rootString) && (!onlyTestingReference || jjtGetNumChildren() > 0))
+                    {
+                        result = EventHandlerUtil.invalidGetMethod(rsvc, context,
+                                "$" + rootString, previousResult, null, uberInfo);
+                    }
                 }
                 else
                 {
@@ -304,7 +333,7 @@ public class ASTReference extends Simple
                     if (jjtGetChild(failedChild) instanceof ASTMethod)
                     {
                         String methodName = ((ASTMethod) jjtGetChild(failedChild)).getMethodName();
-                        result = EventHandlerUtil.invalidMethod(rsvc, context, 
+                        result = EventHandlerUtil.invalidMethod(rsvc, context,
                                 name.toString(), previousResult, methodName, uberInfo);                                                                
                     }
                     else
@@ -526,7 +555,7 @@ public class ASTReference extends Simple
     public boolean evaluate(InternalContextAdapter context)
         throws MethodInvocationException
     {
-        Object value = execute(null, context);
+        Object value = execute(this, context); // non-null object as first parameter by convention for 'evaluate'
         if (value == null)
         {
             return false;

Modified: velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/InvalidEventHandlerTestCase.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/InvalidEventHandlerTestCase.java?rev=1752548&r1=1752547&r2=1752548&view=diff
==============================================================================
--- velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/InvalidEventHandlerTestCase.java (original)
+++ velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/InvalidEventHandlerTestCase.java Wed Jul 13 21:58:37 2016
@@ -46,6 +46,42 @@ import org.apache.velocity.util.introspe
 public class InvalidEventHandlerTestCase
 extends TestCase
 {
+    // @@ VELOCITY-553
+    public class TestObject {
+        private String nullValueAttribute = null;
+        
+        public String getNullValueAttribute() {
+            return nullValueAttribute;
+        }	
+
+        public String getRealString() {
+            return new String("helloFooRealStr");
+        }	
+        
+        public String getString() {
+            return new String("helloFoo");
+        }
+
+        public String getNullString() {
+            return null;
+        }	
+        
+        public java.util.Date getNullDate() {
+            return null;
+        }
+
+        @Override
+        public String toString() {
+            StringBuilder builder = new StringBuilder();
+            builder.append("TestObject [nullValueAttribute=");
+            builder.append(nullValueAttribute);
+            builder.append("]");
+            return builder.toString();
+        }
+    }
+    // @@ VELOCITY-553
+
+    
     /**
      * Default constructor.
      */
@@ -86,6 +122,7 @@ extends TestCase
         ec.addEventHandler(te);
         ec.attachToContext( inner );
         
+        doTestInvalidReferenceEventHandler0(ve, inner);
         doTestInvalidReferenceEventHandler1(ve, inner);
         doTestInvalidReferenceEventHandler2(ve, inner);
         doTestInvalidReferenceEventHandler3(ve, inner);
@@ -103,6 +140,7 @@ extends TestCase
         ve.setProperty(RuntimeConstants.EVENTHANDLER_INVALIDREFERENCES, TestEventCartridge.class.getName());
 
         ve.init();
+        doTestInvalidReferenceEventHandler0(ve, null);
         doTestInvalidReferenceEventHandler1(ve, null);
         doTestInvalidReferenceEventHandler2(ve, null);
         doTestInvalidReferenceEventHandler3(ve, null);
@@ -133,11 +171,11 @@ extends TestCase
         // show work fine
         s = "$tree.Field $tree.field $tree.child.Field";
         w = new StringWriter();
-        ve.evaluate( context, w, "mystring", s );
+        ve.evaluate(context, w, "mystring", s);
         
         s = "$tree.x $tree.field.x $tree.child.y $tree.child.Field.y";
         w = new StringWriter();
-        ve.evaluate( context, w, "mystring", s );
+        ve.evaluate(context, w, "mystring", s);
         
     }
     
@@ -151,8 +189,8 @@ extends TestCase
     throws Exception
     {
         VelocityContext context = new VelocityContext(vc);
-        context.put("a1",new Integer(5));
-        context.put("a4",new Integer(5));
+        context.put("a1", new Integer(5));
+        context.put("a4", new Integer(5));
         context.put("b1","abc");
         
         String s;
@@ -235,7 +273,7 @@ extends TestCase
         // normal - should be no calls to handler
         String s = "$a1 $a1.intValue() $b1 $b1.length() #set($c1 = '5')";
         Writer w = new StringWriter();
-        ve.evaluate( context, w, "mystring", s );
+        ve.evaluate(context, w, "mystring", s);
         
         // good object, bad property
         s = "$a1.foobar";
@@ -244,7 +282,14 @@ extends TestCase
             ve.evaluate( context, w, "mystring", s );
             fail("Expected exception.");
         } catch (RuntimeException e) {}
-        
+
+        // same one inside an #if statement should not fail
+        s = "#if($a1.foobar)yes#{else}no#end";
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+        assertEquals("no",w.toString());
+
+
         // bad object, bad property            
         s = "$a2.foobar";
         w = new StringWriter();
@@ -252,7 +297,15 @@ extends TestCase
             ve.evaluate( context, w, "mystring", s );
             fail("Expected exception.");
         } catch (RuntimeException e) {}
-        
+
+        // same one inside an #if statement should still fail
+        s = "#if($a2.foobar)yes#{else}no#end";
+        w = new StringWriter();
+        try {
+            ve.evaluate( context, w, "mystring", s );
+            fail("Expected exception.");
+        } catch (RuntimeException e) {}
+
         // bad object, no property            
         s = "$a3";
         w = new StringWriter();
@@ -260,7 +313,14 @@ extends TestCase
             ve.evaluate( context, w, "mystring", s );
             fail("Expected exception.");
         } catch (RuntimeException e) {}
-        
+
+        // bad object, no property as #if condition should not fail
+        s = "#if($a3)yes#{else}no#end";
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+        result = w.toString();
+        assertEquals("no", result);
+
         // good object, bad property; change the value
         s = "$a4.foobar";
         w = new StringWriter();
@@ -269,8 +329,95 @@ extends TestCase
         assertEquals("zzz", result);
         
     }
-    
-    
+
+    /**
+     * Test invalidGetMethod
+     *
+     * Test behaviour (which should be the same) of
+     * $objRef.myAttribute and $objRef.getMyAttribute()
+     *
+     * @param ve
+     * @param vc
+     * @throws Exception
+     */
+    private void doTestInvalidReferenceEventHandler0(VelocityEngine ve, VelocityContext vc)
+            throws Exception
+    {
+        String result;
+        Writer w;
+        String s;
+        boolean rc;
+
+        VelocityContext context = new VelocityContext(vc);
+        context.put("propertyAccess", new String("lorem ipsum"));
+        context.put("objRef", new TestObject());
+        java.util.ArrayList arrayList = new java.util.ArrayList();
+        arrayList.add("firstOne");
+        arrayList.add(null);
+        java.util.HashMap hashMap = new java.util.HashMap();
+        hashMap.put(41, "41 is not 42");
+
+        context.put("objRefArrayList", arrayList);
+        context.put("objRefHashMap", hashMap);
+
+        // good object, good property (returns non null value)
+        s = "#set($resultVar = $propertyAccess.bytes)"; // -> getBytes()
+        w = new StringWriter();
+        rc = ve.evaluate( context, w, "mystring", s );
+
+        // good object, good property accessor method (returns non null value)
+        s = "#set($resultVar = $propertyAccess.getBytes())"; // -> getBytes()
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+        // good object, good property (returns non null value)
+        s = "$objRef.getRealString()";
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+        // good object, good property accessor method (returns null value)
+        // No exception shall be thrown, as returning null should be valid
+        s = "$objRef.getNullValueAttribute()";
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+        // good object, good property (returns null value)
+        // No exception shall be thrown, as returning null should be valid
+        s = "$objRef.nullValueAttribute";   // -> getNullValueAttribute()
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+        // good object, good accessor method which returns a non-null object reference
+        // Test removing a hashmap element which exists
+        s = "$objRefHashMap.remove(41)";
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+
+        // good object, good accessor method which returns null
+        // Test removing a hashmap element which DOES NOT exist
+        // Expected behaviour: Returning null as a value should be
+        // OK and not result in an exception
+        s = "$objRefHashMap.remove(42)";   // will return null, as the key does not exist
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+        // good object, good method invocation (returns non-null object reference)
+        s = "$objRefArrayList.get(0)";   // element 0 is NOT NULL
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+
+        // good object, good method invocation (returns null value)
+        // Expected behaviour: Returning null as a value should be
+        // OK and not result in an exception
+        s = "$objRefArrayList.get(1)";   // element 1 is null
+        w = new StringWriter();
+        ve.evaluate( context, w, "mystring", s );
+
+    }
+
+
 
     /**
      * Test assigning the event handlers via properties