velocity-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From nbu...@apache.org
Subject svn commit: r686428 - in /velocity/engine/trunk/src: java/org/apache/velocity/runtime/parser/node/ test/org/apache/velocity/test/
Date Sat, 16 Aug 2008 00:54:36 GMT
Author: nbubna
Date: Fri Aug 15 17:54:35 2008
New Revision: 686428

URL: http://svn.apache.org/viewvc?rev=686428&view=rev
Log:
VELOCITY-531 make #if handle null values intuitively for ! == and != operations

Added:
    velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java   (with props)
Modified:
    velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java
    velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java
    velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTReference.java

Modified: velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java?rev=686428&r1=686427&r2=686428&view=diff
==============================================================================
--- velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java
(original)
+++ velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTEQNode.java
Fri Aug 15 17:54:35 2008
@@ -85,24 +85,6 @@
         Object right = jjtGetChild(1).value(context);
 
         /*
-         *  they could be null if they are references and not in the context
-         */
-
-        if (left == null || right == null)
-        {
-            log.error((left == null ? "Left" : "Right")
-                           + " side ("
-                           + jjtGetChild( (left == null? 0 : 1) ).literal()
-                           + ") of '==' operation "
-                           + "has null value. "
-                           + "If a reference, it may not be in the context."
-                           + " Operation not possible. "
-                           + context.getCurrentTemplateName() + " [line " + getLine()
-                           + ", column " + getColumn() + "]");
-            return false;
-        }
-
-        /*
          *  convert to Number if applicable
          */
         if (left instanceof TemplateNumber)
@@ -117,50 +99,67 @@
        /*
         * If comparing Numbers we do not care about the Class.
         */
-
        if (left instanceof Number && right instanceof Number)
        {
            return MathUtils.compare( (Number)left, (Number)right) == 0;
        }
 
-
-
-       /**
-        * assume that if one class is a subclass of the other
-        * that we should use the equals operator
-        */
-
-        if (left.getClass().isAssignableFrom(right.getClass()) ||
-                right.getClass().isAssignableFrom(left.getClass()) )
+        /**
+         * if both are not null, then assume that if one class
+         * is a subclass of the other that we should use the equals operator
+         */
+        if (left != null && right != null &&
+            (left.getClass().isAssignableFrom(right.getClass()) ||
+             right.getClass().isAssignableFrom(left.getClass())))
         {
             return left.equals( right );
         }
-        else
+
+        /*
+         * Ok, time to compare string values
+         */
+        left = (left == null) ? null : left.toString();
+        right = (right == null) ? null: right.toString();
+
+        if (left == null && right == null)
         {
-            /**
-             * Compare the String representations
-             */
-            if ((left.toString() == null) || (right.toString() == null))
+            if (log.isDebugEnabled())
             {
-        	boolean culprit =  (left.toString() == null);
-                log.error((culprit ? "Left" : "Right")
-                        + " string side "
-                        + "String representation ("
-                        + jjtGetChild((culprit ? 0 : 1) ).literal()
-                        + ") of '!=' operation has null value."
-                        + " Operation not possible. "
-                        + context.getCurrentTemplateName() + " [line " + getLine()
-                        + ", column " + getColumn() + "]");
-
-                return false;
+                log.debug("Both right (" + getLiteral(false) + " and left "
+                          + getLiteral(true) + " sides of '==' operation returned null."
+                          + "If references, they may not be in the context."
+                          + getLocation(context));
             }
-
-            else
+            return true;
+        }
+        else if (left == null || right == null)
+        {
+            if (log.isDebugEnabled())
             {
-                return left.toString().equals(right.toString());
+                log.debug((left == null ? "Left" : "Right")
+                        + " side (" + getLiteral(left == null)
+                        + ") of '==' operation has null value. If it is a "
+                        + "reference, it may not be in the context or its "
+                        + "toString() returned null. " + getLocation(context));
+
             }
+            return false;
         }
+        else
+        {
+            return left.equals(right);
+        }
+    }
 
+    private String getLiteral(boolean left)
+    {
+        return jjtGetChild(left ? 0 : 1).literal();
+    }
+
+    private String getLocation(InternalContextAdapter context)
+    {
+        return context.getCurrentTemplateName() + " [line " + getLine()
+            + ", column " + getColumn() + "]";
     }
 
     /**

Modified: velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java?rev=686428&r1=686427&r2=686428&view=diff
==============================================================================
--- velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java
(original)
+++ velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTNENode.java
Fri Aug 15 17:54:35 2008
@@ -70,23 +70,6 @@
         Object right = jjtGetChild(1).value( context );
 
         /*
-         *  null check
-         */
-
-        if ( left == null || right == null)
-        {
-            log.error((left == null ? "Left" : "Right")
-                           + " side ("
-                           + jjtGetChild( (left == null? 0 : 1) ).literal()
-                           + ") of '!=' operation has null value."
-                           + " Operation not possible. "
-                           + context.getCurrentTemplateName() + " [line " + getLine()
-                           + ", column " + getColumn() + "]");
-            return false;
-
-        }
-
-        /*
          *  convert to Number if applicable
          */
         if (left instanceof TemplateNumber)
@@ -106,43 +89,62 @@
             return MathUtils.compare ( (Number)left,(Number)right) != 0;
        }
 
-
-       /**
-        * assume that if one class is a subclass of the other
-        * that we should use the equals operator
-        */
-
-        if (left.getClass().isAssignableFrom(right.getClass()) ||
-                right.getClass().isAssignableFrom(left.getClass()) )
+        /**
+         * if both are not null, then assume that if one class
+         * is a subclass of the other that we should use the equals operator
+         */
+        if (left != null && right != null &&
+            (left.getClass().isAssignableFrom(right.getClass()) ||
+             right.getClass().isAssignableFrom(left.getClass())))
         {
             return !left.equals( right );
         }
-        else
+
+        /*
+         * Ok, time to compare string values
+         */
+        left = (left == null) ? null : left.toString();
+        right = (right == null) ? null: right.toString();
+
+        if (left == null && right == null)
         {
-            /**
-             * Compare the String representations
-             */
-            if ((left.toString() == null) || (right.toString() == null))
+            if (log.isDebugEnabled())
             {
-        	boolean culprit =  (left.toString() == null);
-                log.error((culprit ? "Left" : "Right")
-                        + " string side "
-                        + "String representation ("
-                        + jjtGetChild((culprit ? 0 : 1) ).literal()
-                        + ") of '!=' operation has null value."
-                        + " Operation not possible. "
-                        + context.getCurrentTemplateName() + " [line " + getLine()
-                        + ", column " + getColumn() + "]");
-                return false;
+                log.debug("Both right (" + getLiteral(false) + " and left "
+                          + getLiteral(true) + " sides of '!=' operation returned null."
+                          + "If references, they may not be in the context."
+                          + getLocation(context));
             }
-
-            else
+            return false;
+        }
+        else if (left == null || right == null)
+        {
+            if (log.isDebugEnabled())
             {
-                return !left.toString().equals(right.toString());
-            }
+                log.debug((left == null ? "Left" : "Right")
+                        + " side (" + getLiteral(left == null)
+                        + ") of '!=' operation has null value. If it is a "
+                        + "reference, it may not be in the context or its "
+                        + "toString() returned null. " + getLocation(context));
 
+            }
+            return true;
         }
+        else
+        {
+            return !left.equals(right);
+        }
+    }
 
+    private String getLiteral(boolean left)
+    {
+        return jjtGetChild(left ? 0 : 1).literal();
+    }
+
+    private String getLocation(InternalContextAdapter context)
+    {
+        return context.getCurrentTemplateName() + " [line " + getLine()
+            + ", column " + getColumn() + "]";
     }
 
     /**

Modified: velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTReference.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTReference.java?rev=686428&r1=686427&r2=686428&view=diff
==============================================================================
--- velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTReference.java
(original)
+++ velocity/engine/trunk/src/java/org/apache/velocity/runtime/parser/node/ASTReference.java
Fri Aug 15 17:54:35 2008
@@ -427,6 +427,10 @@
             else
                 return false;
         }
+        else if (value.toString() == null)
+        {
+            return false;
+        }
         else
             return true;
     }

Added: velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java?rev=686428&view=auto
==============================================================================
--- velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java (added)
+++ velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java Fri Aug 15
17:54:35 2008
@@ -0,0 +1,141 @@
+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 java.io.StringWriter;
+import java.lang.reflect.Array;
+import java.util.Arrays;
+import java.util.ArrayList;
+import java.util.List;
+import junit.framework.Test;
+import junit.framework.TestCase;
+import junit.framework.TestSuite;
+import org.apache.velocity.VelocityContext;
+import org.apache.velocity.app.VelocityEngine;
+import org.apache.velocity.runtime.RuntimeConstants;
+import org.apache.velocity.runtime.log.SystemLogChute;
+
+/**
+ * Used to check that nulls are properly handled in #if statements
+ */
+public class IfNullTestCase extends TestCase
+{
+    private VelocityEngine engine;
+    private VelocityContext context;
+
+    public IfNullTestCase(final String name)
+    {
+        super(name);
+    }
+
+    public static Test suite ()
+    {
+        return new TestSuite(IfNullTestCase.class);
+    }
+
+    public void setUp() throws Exception
+    {
+        engine = new VelocityEngine();
+
+        // make the engine's log output go to the test-report
+        SystemLogChute log = new SystemLogChute();
+        log.setEnabledLevel(SystemLogChute.INFO_ID);
+        log.setSystemErrLevel(SystemLogChute.WARN_ID);
+        engine.setProperty(RuntimeConstants.RUNTIME_LOG_LOGSYSTEM, log);
+
+        context = new VelocityContext();
+        context.put("nullToString", new NullToString());
+        context.put("notnull", new Object());
+    }
+
+    public void tearDown()
+    {
+        engine = null;
+        context = null;
+    }
+
+    public void testIfEquals()
+    {
+        // both null
+        assertEvalEquals("foo", "#if( $null == $otherNull )foo#{else}bar#end");
+        assertEvalEquals("foo", "#if( $null == $nullToString )foo#{else}bar#end");
+        assertEvalEquals("foo", "#if( $nullToString == $null )foo#{else}bar#end");
+        // left null, right not
+        assertEvalEquals("bar", "#if( $nullToString == $notnull )foo#{else}bar#end");
+        assertEvalEquals("bar", "#if( $null == $notnull )foo#{else}bar#end");
+        // right null, left not
+        assertEvalEquals("bar", "#if( $notnull == $nullToString )foo#{else}bar#end");
+        assertEvalEquals("bar", "#if( $notnull == $null )foo#{else}bar#end");
+    }
+
+    public void testIfNotEquals()
+    {
+        // both null
+        assertEvalEquals("bar", "#if( $null != $otherNull )foo#{else}bar#end");
+        assertEvalEquals("bar", "#if( $null != $nullToString )foo#{else}bar#end");
+        assertEvalEquals("bar", "#if( $nullToString != $null )foo#{else}bar#end");
+        // left null, right not
+        assertEvalEquals("foo", "#if( $nullToString != $notnull )foo#{else}bar#end");
+        assertEvalEquals("foo", "#if( $null != $notnull )foo#{else}bar#end");
+        // right null, left not
+        assertEvalEquals("foo", "#if( $notnull != $nullToString )foo#{else}bar#end");
+        assertEvalEquals("foo", "#if( $notnull != $null )foo#{else}bar#end");
+    }
+
+    public void testIfValue()
+    {
+        assertEvalEquals("bar", "#if( $null )foo#{else}bar#end");
+        assertEvalEquals("bar", "#if( $nullToString )foo#{else}bar#end");
+        assertEvalEquals("foo", "#if( !$null )foo#{else}bar#end");
+        assertEvalEquals("foo", "#if( !$nullToString )foo#{else}bar#end");
+    }
+
+    protected void assertEvalEquals(String expected, String template)
+    {
+        try
+        {
+            String result = evaluate(template);
+            assertEquals(expected, result);
+        }
+        catch (Exception e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private String evaluate(String template) throws Exception
+    {
+        StringWriter writer = new StringWriter();
+        // use template as its own name, since our templates are short
+        engine.evaluate(context, writer, template, template);
+        return writer.toString();
+    }
+
+    public static class NullToString
+    {
+        public String toString()
+        {
+            return null;
+        }
+    }
+
+}
+
+

Propchange: velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java
------------------------------------------------------------------------------
    svn:eol-style = native

Propchange: velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java
------------------------------------------------------------------------------
    svn:executable = *

Propchange: velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java
------------------------------------------------------------------------------
    svn:keywords = Revision

Propchange: velocity/engine/trunk/src/test/org/apache/velocity/test/IfNullTestCase.java
------------------------------------------------------------------------------
    svn:mime-type = text/plain



Mime
View raw message