You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Will Glass-Husain <wg...@gmail.com> on 2008/08/16 05:27:25 UTC
Re: svn commit: r686428 - in /velocity/engine/trunk/src: java/org/apache/velocity/runtime/parser/node/ test/org/apache/velocity/test/
Nice. Makes a lot of sense to me. Give a warning but still return true for
null==null.
WILL
On Fri, Aug 15, 2008 at 5:54 PM, <nb...@apache.org> wrote:
> 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
>
>
>
--
Forio Business Simulations
Will Glass-Husain
wglass@forio.com
www.forio.com