Fix for bug 45978 - removed eager initialisation of SheetReferences

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

src/documentation/content/xdocs/changes.xml

@@ -37,6 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1.1-alpha1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">45978 - Fixed IOOBE in Ref3DPtg.toFormulaString() due eager initialisation of SheetReferences</action>
            <action dev="POI-DEVELOPERS" type="add">Made HSSFFormulaEvaluator no longer require initialisation with sheet or row</action>
            <action dev="POI-DEVELOPERS" type="add">Extended support for cached results of formula cells</action>
            <action dev="POI-DEVELOPERS" type="fix">45639 - Fixed AIOOBE due to bad index logic in ColumnInfoRecordsAggregate</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1.1-alpha1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">45978 - Fixed IOOBE in Ref3DPtg.toFormulaString() due eager initialisation of SheetReferences</action>
            <action dev="POI-DEVELOPERS" type="add">Made HSSFFormulaEvaluator no longer require initialisation with sheet or row</action>
            <action dev="POI-DEVELOPERS" type="add">Extended support for cached results of formula cells</action>
            <action dev="POI-DEVELOPERS" type="fix">45639 - Fixed AIOOBE due to bad index logic in ColumnInfoRecordsAggregate</action>

src/java/org/apache/poi/hssf/model/LinkTable.java

@@ -348,10 +348,6 @@ final class LinkTable {
 		return -1;
 	}
 
-	public int getNumberOfREFStructures() {
-		return _externSheetRecord.getNumOfRefs();
-	}
-
 	public String resolveNameXText(int refIndex, int definedNameIndex) {
 		int extBookIndex = _externSheetRecord.getExtbookIndexFromRefIndex(refIndex);
 		return _externalBookBlocks[extBookIndex].getNameText(definedNameIndex);

src/java/org/apache/poi/hssf/model/Workbook.java

@@ -26,7 +26,6 @@ import org.apache.poi.ddf.*;
 import org.apache.poi.hssf.record.*;
 import org.apache.poi.hssf.record.formula.NameXPtg;
 import org.apache.poi.hssf.util.HSSFColor;
-import org.apache.poi.hssf.util.SheetReferences;
 import org.apache.poi.util.POILogFactory;
 import org.apache.poi.util.POILogger;
 
@@ -1902,34 +1901,23 @@ public final class Workbook implements Model {
         return linkTable;
     }
 
-    public SheetReferences getSheetReferences() {
-        SheetReferences refs = new SheetReferences();
-        
-        if (linkTable != null) {
-            int numRefStructures = linkTable.getNumberOfREFStructures();
-            for (short k = 0; k < numRefStructures; k++) {
-                
-                String sheetName = findSheetNameFromExternSheet(k);
-                refs.addSheetReference(sheetName, k);
-                
-            }
-        }
-        return refs;
-    }
-
     /** finds the sheet name by his extern sheet index
-     * @param num extern sheet index
+     * @param externSheetIndex extern sheet index
-     * @return sheet name
+     * @return sheet name.
      */
-    public String findSheetNameFromExternSheet(short num){
+    public String findSheetNameFromExternSheet(int externSheetIndex){
 
-        int indexToSheet = linkTable.getIndexToSheet(num);
+        int indexToSheet = linkTable.getIndexToSheet(externSheetIndex);
         
         if (indexToSheet < 0) {
             // TODO - what does '-1' mean here?
             //error check, bail out gracefully!
             return "";
         }
+        if (indexToSheet >= boundsheets.size()) {
+            // Not sure if this can ever happen (See bug 45798)
+            return ""; // Seems to be what excel would do in this case
+        }
         return getSheetName(indexToSheet);
     }
 

src/java/org/apache/poi/hssf/record/formula/Area3DPtg.java

@@ -90,7 +90,7 @@ public final class Area3DPtg extends AreaPtgBase {
 	public String toFormulaString(HSSFWorkbook book) {
 		// First do the sheet name
 		StringBuffer retval = new StringBuffer();
-		String sheetName = Ref3DPtg.getSheetName(book, field_1_index_extern_sheet);
+		String sheetName = book.findSheetNameFromExternSheet(field_1_index_extern_sheet);
 		if(sheetName != null) {
 			if(sheetName.length() == 0) {
 				// What excel does if sheet has been deleted

src/java/org/apache/poi/hssf/record/formula/Ref3DPtg.java

@@ -21,7 +21,6 @@ import org.apache.poi.hssf.record.RecordInputStream;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
 import org.apache.poi.hssf.util.CellReference;
 import org.apache.poi.hssf.util.RangeAddress;
-import org.apache.poi.hssf.util.SheetReferences;
 import org.apache.poi.util.BitField;
 import org.apache.poi.util.BitFieldFactory;
 import org.apache.poi.util.LittleEndian;
@@ -160,20 +159,6 @@ public final class Ref3DPtg extends OperandPtg {
 
     }
 
-    // TODO - find a home for this method
-    // There is already a method on HSSFWorkbook called getSheetName but it seems to do something different.
-	static String getSheetName(HSSFWorkbook book, int externSheetIndex) {
-        // TODO - there are 3 ways this method can return null. Is each valid?
-        if (book == null) {
-            return null;
-        }
-
-        SheetReferences refs = book.getSheetReferences();
-        if (refs == null) {
-            return null;
-        }
-        return refs.getSheetName(externSheetIndex);
-    }
     /**
      * @return text representation of this cell reference that can be used in text 
      * formulas. The sheet name will get properly delimited if required.
@@ -181,7 +166,7 @@ public final class Ref3DPtg extends OperandPtg {
     public String toFormulaString(HSSFWorkbook book)
     {
         StringBuffer retval = new StringBuffer();
-        String sheetName = getSheetName(book, field_1_index_extern_sheet);
+        String sheetName = book.findSheetNameFromExternSheet(field_1_index_extern_sheet);
         if(sheetName != null) {
             SheetNameFormatter.appendFormat(retval, sheetName);
             retval.append( '!' );

src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java

@@ -35,7 +35,6 @@ import org.apache.poi.ddf.EscherBlipRecord;
 import org.apache.poi.ddf.EscherRecord;
 import org.apache.poi.hssf.model.Sheet;
 import org.apache.poi.hssf.model.Workbook;
-import org.apache.poi.hssf.model.DrawingManager2;
 import org.apache.poi.hssf.record.AbstractEscherHolderRecord;
 import org.apache.poi.hssf.record.BackupRecord;
 import org.apache.poi.hssf.record.DrawingGroupRecord;
@@ -60,7 +59,6 @@ import org.apache.poi.hssf.record.formula.Ref3DPtg;
 import org.apache.poi.hssf.record.formula.UnionPtg;
 import org.apache.poi.hssf.usermodel.HSSFRow.MissingCellPolicy;
 import org.apache.poi.hssf.util.CellReference;
-import org.apache.poi.hssf.util.SheetReferences;
 import org.apache.poi.poifs.filesystem.DirectoryNode;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 import org.apache.poi.util.POILogFactory;
@@ -858,8 +856,13 @@ public class HSSFWorkbook extends POIDocument
         return retval;
     }
 
-    public SheetReferences getSheetReferences() {
+    /**
-        return workbook.getSheetReferences();
+     * @deprecated for POI internal use only (formula rendering).  This method is likely to
+     * be removed in future versions of POI.
+     */
+    public String findSheetNameFromExternSheet(int externSheetIndex){
+        // TODO - don't expose internal ugliness like externSheet indexes to the user model API
+        return workbook.findSheetNameFromExternSheet(externSheetIndex);
     }
 
     /**
@@ -1370,12 +1373,15 @@ public class HSSFWorkbook extends POIDocument
     }
 
     /**
-     * TODO - make this less cryptic / move elsewhere
+     * @deprecated for POI internal use only (formula rendering).  This method is likely to
+     * be removed in future versions of POI.
+     * 
      * @param refIndex Index to REF entry in EXTERNSHEET record in the Link Table
      * @param definedNameIndex zero-based to DEFINEDNAME or EXTERNALNAME record
      * @return the string representation of the defined or external name
      */
     public String resolveNameXText(int refIndex, int definedNameIndex) {
+        // TODO - make this less cryptic / move elsewhere
         return workbook.resolveNameXText(refIndex, definedNameIndex);
     }
 

src/java/org/apache/poi/hssf/util/SheetReferences.java

@@ -1,46 +0,0 @@
-/* ====================================================================
-   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.
-==================================================================== */
-
-package org.apache.poi.hssf.util;
-
-import java.util.HashMap;
-import java.util.Map;
-
-/**
- * Holds a collection of Sheet names and their associated
- * reference numbers.
- *
- * @author Andrew C. Oliver (acoliver at apache dot org)
- *
- */
-public class SheetReferences
-{
-    Map map;
-    public SheetReferences()
-    {
-      map = new HashMap(5);
-    }
- 
-    public void addSheetReference(String sheetName, int number) {
-       map.put(new Integer(number), sheetName);
-    } 
-
-    public String getSheetName(int number) {
-       return (String)map.get(new Integer(number));
-    }
-
-}

src/testcases/org/apache/poi/hssf/HSSFTests.java

@@ -23,14 +23,11 @@ import junit.framework.TestSuite;
 import org.apache.poi.hssf.eventmodel.TestEventRecordFactory;
 import org.apache.poi.hssf.eventmodel.TestModelFactory;
 import org.apache.poi.hssf.eventusermodel.AllEventUserModelTests;
+import org.apache.poi.hssf.extractor.TestExcelExtractor;
 import org.apache.poi.hssf.model.AllModelTests;
 import org.apache.poi.hssf.record.AllRecordTests;
 import org.apache.poi.hssf.usermodel.AllUserModelTests;
-import org.apache.poi.hssf.util.TestAreaReference;
+import org.apache.poi.hssf.util.AllHSSFUtilTests;
-import org.apache.poi.hssf.util.TestCellReference;
-import org.apache.poi.hssf.util.TestRKUtil;
-import org.apache.poi.hssf.util.TestRangeAddress;
-import org.apache.poi.hssf.util.TestSheetReferences;
 
 /**
  * Test Suite for all sub-packages of org.apache.poi.hssf<br/>
@@ -41,27 +38,20 @@ import org.apache.poi.hssf.util.TestSheetReferences;
  */
 public final class HSSFTests {
 
-    public static void main(String[] args) {
-        junit.textui.TestRunner.run(suite());
-    }
-
     public static Test suite() {
-        TestSuite suite = new TestSuite("Tests for org.apache.poi.hssf");
+        TestSuite suite = new TestSuite(HSSFTests.class.getName());
-        // $JUnit-BEGIN$
 
         suite.addTest(AllEventUserModelTests.suite());
         suite.addTest(AllModelTests.suite());
         suite.addTest(AllUserModelTests.suite());
         suite.addTest(AllRecordTests.suite());
+        suite.addTest(AllHSSFUtilTests.suite());
 
-        suite.addTest(new TestSuite(TestAreaReference.class));
+        if (false) { // TODO - hook this test up
-        suite.addTest(new TestSuite(TestCellReference.class));
+            suite.addTest(new TestSuite(TestExcelExtractor.class));
-        suite.addTest(new TestSuite(TestRangeAddress.class));
+        }
-        suite.addTest(new TestSuite(TestRKUtil.class));
-        suite.addTest(new TestSuite(TestSheetReferences.class));
         suite.addTest(new TestSuite(TestEventRecordFactory.class));
         suite.addTest(new TestSuite(TestModelFactory.class));
-        // $JUnit-END$
         return suite;
     }
 }

src/testcases/org/apache/poi/hssf/eventusermodel/TestEventWorkbookBuilder.java

@@ -16,6 +16,7 @@
 ==================================================================== */
 
 package org.apache.poi.hssf.eventusermodel;
+
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
@@ -32,7 +33,6 @@ import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.formula.Ptg;
 import org.apache.poi.hssf.record.formula.Ref3DPtg;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
-import org.apache.poi.hssf.util.SheetReferences;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 /**
  * Tests for {@link EventWorkbookBuilder}
@@ -66,9 +66,6 @@ public final class TestEventWorkbookBuilder extends TestCase {
 	public void testGetStubWorkbooks() {
 		assertNotNull(listener.getStubWorkbook());
 		assertNotNull(listener.getStubHSSFWorkbook());
-		
-		assertNotNull(listener.getStubWorkbook().getSheetReferences());
-		assertNotNull(listener.getStubHSSFWorkbook().getSheetReferences());
 	}
 	
 	public void testContents() {
@@ -78,10 +75,10 @@ public final class TestEventWorkbookBuilder extends TestCase {
 		
 		assertEquals(3, listener.getStubWorkbook().getNumSheets());
 		
-		SheetReferences ref = listener.getStubWorkbook().getSheetReferences();
+		Workbook ref = listener.getStubWorkbook();
-		assertEquals("Sh3", ref.getSheetName(0));
+		assertEquals("Sh3", ref.findSheetNameFromExternSheet(0));
-		assertEquals("Sheet1", ref.getSheetName(1));
+		assertEquals("Sheet1", ref.findSheetNameFromExternSheet(1));
-		assertEquals("S2", ref.getSheetName(2));
+		assertEquals("S2", ref.findSheetNameFromExternSheet(2));
 	}
 	
 	public void testFormulas() {

src/testcases/org/apache/poi/hssf/usermodel/TestLinkTable.java

@@ -56,4 +56,45 @@ public final class TestLinkTable extends TestCase {
 		// some other sanity checks
 		assertEquals(7, wb.getNumberOfSheets());
 	}
+
+	public void testExtraSheetRefs_bug45978() {
+		HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("ex45978-extraLinkTableSheets.xls");
+		/*
+		ex45978-extraLinkTableSheets.xls is a cut-down version of attachment 22561.
+		The original file produces the same error.
+
+		This bug was caused by a combination of invalid sheet indexes in the EXTERNSHEET
+		record, and eager initialisation of the extern sheet references. Note - the worbook
+		has 2 sheets, but the EXTERNSHEET record refers to sheet indexes 0, 1 and 2.
+
+		Offset 0x3954 (14676)
+		recordid = 0x17, size = 32
+		[EXTERNSHEET]
+		   numOfRefs	 = 5
+		refrec		 #0: extBook=0 firstSheet=0 lastSheet=0
+		refrec		 #1: extBook=1 firstSheet=2 lastSheet=2
+		refrec		 #2: extBook=2 firstSheet=1 lastSheet=1
+		refrec		 #3: extBook=0 firstSheet=-1 lastSheet=-1
+		refrec		 #4: extBook=0 firstSheet=1 lastSheet=1
+		[/EXTERNSHEET]
+
+		As it turns out, the formula in question doesn't even use externSheetIndex #1 - it
+		uses #4, which resolves to sheetIndex 1 -> 'Data'.
+
+		It is not clear exactly what externSheetIndex #4 would refer to.  Excel seems to
+		display such a formula as "''!$A2", but then complains of broken link errors.
+		*/
+
+		HSSFCell cell = wb.getSheetAt(0).getRow(1).getCell(1);
+		String cellFormula;
+		try {
+			cellFormula = cell.getCellFormula();
+		} catch (IndexOutOfBoundsException e) {
+			if (e.getMessage().equals("Index: 2, Size: 2")) {
+				throw new AssertionFailedError("Identified bug 45798");
+			}
+			throw e;
+		}
+		assertEquals("Data!$A2", cellFormula);
+	}
 }

src/testcases/org/apache/poi/hssf/util/AllHSSFUtilTests.java

@@ -34,7 +34,6 @@ public class AllHSSFUtilTests {
 		result.addTestSuite(TestHSSFColor.class);
 		result.addTestSuite(TestRangeAddress.class);
 		result.addTestSuite(TestRKUtil.class);
-		result.addTestSuite(TestSheetReferences.class);
 		return result;
 	}
 }

src/testcases/org/apache/poi/hssf/util/TestSheetReferences.java

@@ -1,51 +0,0 @@
-
-/* ====================================================================
-   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.
-==================================================================== */
-        
-
-package org.apache.poi.hssf.util;
-
-import junit.framework.TestCase;
-
-/**
- * Tests the SheetReferences class.
- * @author Andrew C. Oliver (acoliver at apache dot org)
- */
-public class TestSheetReferences
-        extends TestCase
-{
-    public TestSheetReferences(String s)
-    {
-        super(s);
-    }
-
-    /**
-     * Test that the SheetReferences class can add references and give them
-     * out
-     */
-    public void testSheetRefs()
-            throws Exception
-    {
-        SheetReferences refs = new SheetReferences();
-        refs.addSheetReference("A", 0);
-        refs.addSheetReference("B", 1);
-        refs.addSheetReference("C", 3);
-        assertTrue("ref 0 == A", refs.getSheetName(0).equals("A"));
-        assertTrue("ref 1 == B", refs.getSheetName(1).equals("B"));
-        assertTrue("ref 3 == C", refs.getSheetName(3).equals("C"));
-    }
-}