From 48eb5c521bb8f257c4c4ba14626aed0954003deb Mon Sep 17 00:00:00 2001 From: Javen O'Neal Date: Mon, 13 Jun 2016 08:58:55 +0000 Subject: [PATCH] improve unit test coverage on CellReference. Add note that _sheetName can be null or empty string, depending on the entry-point. Throw an exception when parsing a string if sheet name is not quoted and contains a space. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1748144 13f79535-47bb-0310-9956-ffa450edef68 --- .../org/apache/poi/ss/util/CellReference.java | 34 ++++---- .../apache/poi/ss/util/TestCellReference.java | 86 +++++++++++++++++++ 2 files changed, 104 insertions(+), 16 deletions(-) diff --git a/src/java/org/apache/poi/ss/util/CellReference.java b/src/java/org/apache/poi/ss/util/CellReference.java index 235f58307..6adc53007 100644 --- a/src/java/org/apache/poi/ss/util/CellReference.java +++ b/src/java/org/apache/poi/ss/util/CellReference.java @@ -91,9 +91,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(); + // FIXME: _sheetName may be null, depending on the entry point. + // Perhaps it would be better to declare _sheetName is never null, using an empty string to represent a 2D reference. + private final String _sheetName; private final int _rowIndex; private final int _colIndex; - private final String _sheetName; private final boolean _isRowAbs; private final boolean _isColAbs; @@ -353,17 +355,8 @@ public class CellReference { } public static boolean isRowWithinRange(String rowStr, SpreadsheetVersion ssVersion) { - int rowNum = Integer.parseInt(rowStr); - - 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(); + int rowNum = Integer.parseInt(rowStr) - 1; + return 0 <= rowNum && rowNum <= ssVersion.getLastRowIndex(); } private static final class CellRefParts { @@ -408,11 +401,16 @@ public class CellReference { boolean isQuoted = reference.charAt(0) == SPECIAL_NAME_DELIMITER; if(!isQuoted) { - return reference.substring(0, indexOfSheetNameDelimiter); + // sheet names with spaces must be quoted + if (reference.indexOf(' ') == -1) { + return reference.substring(0, indexOfSheetNameDelimiter); + } else { + throw new IllegalArgumentException("Sheet names containing spaces must be quoted: (" + reference + ")"); + } } int lastQuotePos = indexOfSheetNameDelimiter-1; if(reference.charAt(lastQuotePos) != SPECIAL_NAME_DELIMITER) { - throw new RuntimeException("Mismatched quotes: (" + reference + ")"); + throw new IllegalArgumentException("Mismatched quotes: (" + reference + ")"); } // TODO - refactor cell reference parsing logic to one place. @@ -438,7 +436,7 @@ public class CellReference { continue; } } - throw new RuntimeException("Bad sheet name quote escaping: (" + reference + ")"); + throw new IllegalArgumentException("Bad sheet name quote escaping: (" + reference + ")"); } return sb.toString(); } @@ -555,7 +553,10 @@ public class CellReference { return _rowIndex == cr._rowIndex && _colIndex == cr._colIndex && _isRowAbs == cr._isRowAbs - && _isColAbs == cr._isColAbs; + && _isColAbs == cr._isColAbs + && ((_sheetName == null) + ? (cr._sheetName == null) + : _sheetName.equals(cr._sheetName)); } @Override @@ -565,6 +566,7 @@ public class CellReference { result = 31 * result + _colIndex; result = 31 * result + (_isRowAbs ? 1 : 0); result = 31 * result + (_isColAbs ? 1 : 0); + result = 31 * result + (_sheetName == null ? 0 : _sheetName.hashCode()); return result; } } diff --git a/src/testcases/org/apache/poi/ss/util/TestCellReference.java b/src/testcases/org/apache/poi/ss/util/TestCellReference.java index 08aa50674..d2f2e462a 100644 --- a/src/testcases/org/apache/poi/ss/util/TestCellReference.java +++ b/src/testcases/org/apache/poi/ss/util/TestCellReference.java @@ -317,4 +317,90 @@ public final class TestCellReference { assertTrue("row absolute", ref.isRowAbsolute()); //assertFalse("column absolute/relative is undefined", ref.isColAbsolute()); } + + @Test + public void getSheetName() { + assertEquals(null, new CellReference("A5").getSheetName()); + assertEquals(null, new CellReference(null, 0, 0, false, false).getSheetName()); + // FIXME: CellReference is inconsistent + assertEquals("", new CellReference("", 0, 0, false, false).getSheetName()); + assertEquals("Sheet1", new CellReference("Sheet1!A5").getSheetName()); + assertEquals("Sheet 1", new CellReference("'Sheet 1'!A5").getSheetName()); + } + + @Test + public void testToString() { + CellReference ref = new CellReference("'Sheet 1'!A5"); + assertEquals("org.apache.poi.ss.util.CellReference ['Sheet 1'!A5]", ref.toString()); + } + + @Test + public void testEqualsAndHashCode() { + CellReference ref1 = new CellReference("'Sheet 1'!A5"); + CellReference ref2 = new CellReference("Sheet 1", 4, 0, false, false); + assertEquals("equals", ref1, ref2); + assertEquals("hash code", ref1.hashCode(), ref2.hashCode()); + + assertFalse("null", ref1.equals(null)); + assertFalse("3D vs 2D", ref1.equals(new CellReference("A5"))); + assertFalse("type", ref1.equals(new Integer(0))); + } + + @Test + public void isRowWithinRange() { + SpreadsheetVersion ss = SpreadsheetVersion.EXCEL2007; + assertFalse("1 before first row", CellReference.isRowWithinRange("0", ss)); + assertTrue("first row", CellReference.isRowWithinRange("1", ss)); + assertTrue("last row", CellReference.isRowWithinRange("1048576", ss)); + assertFalse("1 beyond last row", CellReference.isRowWithinRange("1048577", ss)); + } + + @Test + public void isColWithinRange() { + SpreadsheetVersion ss = SpreadsheetVersion.EXCEL2007; + assertTrue("(empty)", CellReference.isColumnWithinRange("", ss)); + assertTrue("first column (A)", CellReference.isColumnWithinRange("A", ss)); + assertTrue("last column (XFD)", CellReference.isColumnWithinRange("XFD", ss)); + assertFalse("1 beyond last column (XFE)", CellReference.isColumnWithinRange("XFE", ss)); + } + + @Test(expected=IllegalArgumentException.class) + public void unquotedSheetName() { + new CellReference("'Sheet 1!A5"); + } + @Test(expected=IllegalArgumentException.class) + public void mismatchedQuotesSheetName() { + new CellReference("Sheet 1!A5"); + } + + @Test + public void escapedSheetName() { + String escapedName = "'Don''t Touch'!A5"; + String unescapedName = "'Don't Touch'!A5"; + new CellReference(escapedName); + try { + new CellReference(unescapedName); + fail("Sheet names containing apostrophe's must be escaped via a repeated apostrophe"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().startsWith("Bad sheet name quote escaping: ")); + } + } + + @Test(expected=IllegalArgumentException.class) + public void negativeRow() { + new CellReference("sheet", -2, 0, false, false); + } + @Test(expected=IllegalArgumentException.class) + public void negativeColumn() { + new CellReference("sheet", 0, -2, false, false); + } + + @Test(expected=IllegalArgumentException.class) + public void classifyEmptyStringCellReference() { + CellReference.classifyCellReference("", SpreadsheetVersion.EXCEL2007); + } + @Test(expected=IllegalArgumentException.class) + public void classifyInvalidFirstCharCellReference() { + CellReference.classifyCellReference("!A5", SpreadsheetVersion.EXCEL2007); + } }