Bugzilla 47312 - Fixed formula parser to properly reject cell references with a '0' row component

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@781616 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/status.xml

@@ -35,6 +35,7 @@
         <release version="3.5-beta7" date="2009-??-??">
         </release>
         <release version="3.5-beta6" date="2009-06-11">
+           <action dev="POI-DEVELOPERS" type="fix">47312 - Fixed formula parser to properly reject cell references with a '0' row component</action>
            <action dev="POI-DEVELOPERS" type="fix">47199 - Fixed PageSettingsBlock/Sheet to tolerate margin records after other non-PSB records</action>
            <action dev="POI-DEVELOPERS" type="fix">47069 - Fixed HSSFSheet#getFirstRowNum and HSSFSheet#getLastRowNum to return correct values after removal of all rows</action>
            <action dev="POI-DEVELOPERS" type="fix">47278 - Fixed XSSFCell to avoid generating xsi:nil entries in shared string table</action>

src/java/org/apache/poi/ss/util/CellReference.java

@@ -21,7 +21,6 @@ import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import org.apache.poi.hssf.record.formula.SheetNameFormatter;
-import org.apache.poi.hssf.record.formula.function.FunctionMetadataRegistry;
 import org.apache.poi.ss.SpreadsheetVersion;
 
 /**
@@ -68,11 +67,11 @@ public class CellReference {
 	//private static final String BIFF8_LAST_ROW = String.valueOf(SpreadsheetVersion.EXCEL97.getMaxRows());
 	//private static final int BIFF8_LAST_ROW_TEXT_LEN = BIFF8_LAST_ROW.length();
 
-    private final int _rowIndex;
+	private final int _rowIndex;
-    private final int _colIndex;
+	private final int _colIndex;
-    private final String _sheetName;
+	private final String _sheetName;
-    private final boolean _isRowAbs;
+	private final boolean _isRowAbs;
-    private final boolean _isColAbs;
+	private final boolean _isColAbs;
 
 	/**
 	 * Create an cell ref from a string representation.  Sheet names containing special characters should be
@@ -204,7 +203,7 @@ public class CellReference {
 		}
 		String lettersGroup = cellRefPatternMatcher.group(1);
 		String digitsGroup = cellRefPatternMatcher.group(2);
-        if (cellReferenceIsWithinRange(lettersGroup, digitsGroup, ssVersion)) {
+		if (cellReferenceIsWithinRange(lettersGroup, digitsGroup, ssVersion)) {
 			// valid cell reference
 			return NameType.CELL;
 		}
@@ -275,30 +274,23 @@ public class CellReference {
 		if (!isColumnWithnRange(colStr, ssVersion)) {
 			return false;
 		}
-        String lastRow = String.valueOf(ssVersion.getMaxRows());
-        int lastRowLen = lastRow.length();
 
-        int nDigits = rowStr.length();
+		int rowNum = Integer.parseInt(rowStr);
-		if(nDigits > lastRowLen) {
+
+		if (rowNum < 0) {
+			throw new IllegalStateException("Invalid rowStr '" + rowStr + "'.");
+		}
+		if (rowNum == 0) {
+			// execution gets here because caller does first pass of discriminating
+			// potential cell references using a simplistic regex pattern.
 			return false;
 		}
-		
+		return rowNum <= ssVersion.getMaxRows();
-		if(nDigits == lastRowLen) {
-			// ASCII comparison is valid if digit count is same
-			if(rowStr.compareTo(lastRow) > 0) {
-				return false;
-			}
-		} else {
-			// apparent row has less chars than max
-			// no need to check range
-		}
-		
-		return true;
 	}
 
 	public static boolean isColumnWithnRange(String colStr, SpreadsheetVersion ssVersion) {
-        String lastCol = ssVersion.getLastColumnName();
+		String lastCol = ssVersion.getLastColumnName();
-        int lastColLength = lastCol.length();
+		int lastColLength = lastCol.length();
 
 		int numberOfLetters = colStr.length();
 		if(numberOfLetters > lastColLength) {
@@ -343,9 +335,9 @@ public class CellReference {
 			}
 		}
 		return new String[] {
-		   sheetName,
+			sheetName,
-		   reference.substring(start,loc),
+			reference.substring(start,loc),
-		   reference.substring(loc),
+			reference.substring(loc),
 		};
 	}
 

src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java

@@ -68,6 +68,7 @@ import org.apache.poi.hssf.usermodel.TestHSSFName;
 import org.apache.poi.ss.formula.FormulaParser;
 import org.apache.poi.ss.formula.FormulaParserTestHelper;
 import org.apache.poi.ss.usermodel.BaseTestBugzillaIssues;
+import org.apache.poi.ss.usermodel.Name;
 
 /**
  * Test the low level formula parser functionality. High level tests are to
@@ -1229,4 +1230,46 @@ public final class TestFormulaParser extends TestCase {
 		assertNotNull("Ptg array should not be null", result);
 		confirmTokenClasses(result, new Class[] { IntPtg.class, NamePtg.class, AddPtg.class,});
 	}
+
+	/**
+	 * Zero is not a valid row number so cell references like 'A0' are not valid.
+	 * Actually, they should be treated like defined names.
+	 * <br/>
+	 * In addition, leading zeros (on the row component) should be removed from cell
+	 * references during parsing.
+	 */
+	public void testZeroRowRefs() {
+		String badCellRef = "B0"; // bad because zero is not a valid row number
+		String leadingZeroCellRef = "B000001"; // this should get parsed as "B1"
+		HSSFWorkbook wb = new HSSFWorkbook();
+
+		try {
+			HSSFFormulaParser.parse(badCellRef, wb);
+			throw new AssertionFailedError("Identified bug 47312b - Shouldn't be able to parse cell ref '"
+					+ badCellRef + "'.");
+		} catch (RuntimeException e) {
+			// expected during successful test
+			FormulaParserTestHelper.confirmParseException(e, "Specified named range '"
+					+ badCellRef + "' does not exist in the current workbook.");
+		}
+
+		Ptg[] ptgs;
+		try {
+			ptgs = HSSFFormulaParser.parse(leadingZeroCellRef, wb);
+			assertEquals("B1", ((RefPtg) ptgs[0]).toFormulaString());
+		} catch (RuntimeException e) {
+			FormulaParserTestHelper.confirmParseException(e, "Specified named range '"
+					+ leadingZeroCellRef + "' does not exist in the current workbook.");
+			// close but no cigar
+			throw new AssertionFailedError("Identified bug 47312c - '"
+					+ leadingZeroCellRef + "' should parse as 'B1'.");
+		}
+
+		// create a defined name called 'B0' and try again
+		Name n = wb.createName();
+		n.setNameName("B0");
+		n.setRefersToFormula("1+1");
+		ptgs = HSSFFormulaParser.parse("B0", wb);
+		assertEquals(NamePtg.class, ptgs[0].getClass());
+	}
 }

src/testcases/org/apache/poi/ss/util/TestCellReference.java

@@ -17,15 +17,17 @@
 
 package org.apache.poi.ss.util;
 
+import org.apache.poi.ss.SpreadsheetVersion;
 import org.apache.poi.ss.util.CellReference;
 
+import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 
 /**
  * Tests that the common CellReference works as we need it to
  */
-public class TestCellReference extends TestCase {
+public final class TestCellReference extends TestCase {
 	
 	public void testGetCellRefParts() {
 		CellReference cellReference;
@@ -168,4 +170,35 @@ public class TestCellReference extends TestCase {
 		String collRef4 = new CellReference(0, col4).formatAsString();
 		assertEquals("CBA1", collRef4);
 	}
+
+	public void testBadRowNumber() {
+		SpreadsheetVersion v97 = SpreadsheetVersion.EXCEL97;
+		SpreadsheetVersion v2007 = SpreadsheetVersion.EXCEL2007;
+
+		confirmCrInRange(true, "A", "1", v97);
+		confirmCrInRange(true, "IV", "65536", v97);
+		confirmCrInRange(false, "IV", "65537", v97);
+		confirmCrInRange(false, "IW", "65536", v97);
+
+		confirmCrInRange(true, "A", "1", v2007);
+		confirmCrInRange(true, "XFD", "1048576", v2007);
+		confirmCrInRange(false, "XFD", "1048577", v2007);
+		confirmCrInRange(false, "XFE", "1048576", v2007);
+
+		if (CellReference.cellReferenceIsWithinRange("B", "0", v97)) {
+			throw new AssertionFailedError("Identified bug 47312a");
+		}
+
+		confirmCrInRange(false, "A", "0", v97);
+		confirmCrInRange(false, "A", "0", v2007);
+	}
+
+	private static void confirmCrInRange(boolean expResult, String colStr, String rowStr,
+			SpreadsheetVersion sv) {
+		if (expResult == CellReference.cellReferenceIsWithinRange(colStr, rowStr, sv)) {
+			return;
+		}
+		throw new AssertionFailedError("expected (c='" + colStr + "', r='" + rowStr + "' to be "
+				+ (expResult ? "within" : "out of") + " bounds for version " + sv.name());
+	}
 }