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, <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] www.forio.com
