Return-Path: Delivered-To: apmail-velocity-commits-archive@locus.apache.org Received: (qmail 61772 invoked from network); 16 Aug 2008 00:54:57 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 16 Aug 2008 00:54:57 -0000 Received: (qmail 71802 invoked by uid 500); 16 Aug 2008 00:54:56 -0000 Delivered-To: apmail-velocity-commits-archive@velocity.apache.org Received: (qmail 71766 invoked by uid 500); 16 Aug 2008 00:54:55 -0000 Mailing-List: contact commits-help@velocity.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@velocity.apache.org Delivered-To: mailing list commits@velocity.apache.org Received: (qmail 71757 invoked by uid 99); 16 Aug 2008 00:54:55 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 15 Aug 2008 17:54:55 -0700 X-ASF-Spam-Status: No, hits=-2000.0 required=10.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 16 Aug 2008 00:54:07 +0000 Received: by eris.apache.org (Postfix, from userid 65534) id 5144D2388988; Fri, 15 Aug 2008 17:54:36 -0700 (PDT) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit 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 -0000 To: commits@velocity.apache.org From: nbubna@apache.org X-Mailer: svnmailer-1.0.8 Message-Id: <20080816005436.5144D2388988@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org 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