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

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);
+        int rowNum = Integer.parseInt(rowStr) - 1;
-
+        return 0 <= rowNum && rowNum <= ssVersion.getLastRowIndex();
-        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();
     }
 
     private static final class CellRefParts {
@@ -408,11 +401,16 @@ public class CellReference {
 
         boolean isQuoted = reference.charAt(0) == SPECIAL_NAME_DELIMITER;
         if(!isQuoted) {
+            // 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;
     }
 }

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);
+    }
 }