From: Josh Micich Date: Sun, 14 Sep 2008 18:55:28 +0000 (+0000) Subject: Fix for bug 45978 - removed eager initialisation of SheetReferences X-Git-Tag: REL_3_2_FINAL~57 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=866ae7562a51de72161c4f46beb0280987e5818f;p=poi.git 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 --- diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 5f7c09520a..5ce7097f26 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 45978 - Fixed IOOBE in Ref3DPtg.toFormulaString() due eager initialisation of SheetReferences Made HSSFFormulaEvaluator no longer require initialisation with sheet or row Extended support for cached results of formula cells 45639 - Fixed AIOOBE due to bad index logic in ColumnInfoRecordsAggregate diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index e62842ba3a..926986edbb 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 45978 - Fixed IOOBE in Ref3DPtg.toFormulaString() due eager initialisation of SheetReferences Made HSSFFormulaEvaluator no longer require initialisation with sheet or row Extended support for cached results of formula cells 45639 - Fixed AIOOBE due to bad index logic in ColumnInfoRecordsAggregate diff --git a/src/java/org/apache/poi/hssf/model/LinkTable.java b/src/java/org/apache/poi/hssf/model/LinkTable.java index a64f134fed..2caab0fb56 100755 --- a/src/java/org/apache/poi/hssf/model/LinkTable.java +++ b/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); diff --git a/src/java/org/apache/poi/hssf/model/Workbook.java b/src/java/org/apache/poi/hssf/model/Workbook.java index 7976588d34..f8a426108a 100644 --- a/src/java/org/apache/poi/hssf/model/Workbook.java +++ b/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 - * @return sheet name + * @param externSheetIndex extern sheet index + * @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); } diff --git a/src/java/org/apache/poi/hssf/record/formula/Area3DPtg.java b/src/java/org/apache/poi/hssf/record/formula/Area3DPtg.java index f68aa4d3ff..55c14cdb75 100644 --- a/src/java/org/apache/poi/hssf/record/formula/Area3DPtg.java +++ b/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 diff --git a/src/java/org/apache/poi/hssf/record/formula/Ref3DPtg.java b/src/java/org/apache/poi/hssf/record/formula/Ref3DPtg.java index d1133d9801..e6a8a959e3 100644 --- a/src/java/org/apache/poi/hssf/record/formula/Ref3DPtg.java +++ b/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( '!' ); diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java index a708bd16c1..e751d9b38a 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java +++ b/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); } diff --git a/src/java/org/apache/poi/hssf/util/SheetReferences.java b/src/java/org/apache/poi/hssf/util/SheetReferences.java deleted file mode 100644 index 9fe8dd1f3d..0000000000 --- a/src/java/org/apache/poi/hssf/util/SheetReferences.java +++ /dev/null @@ -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)); - } - -} diff --git a/src/testcases/org/apache/poi/hssf/HSSFTests.java b/src/testcases/org/apache/poi/hssf/HSSFTests.java index ff15491d90..1edaf7e151 100644 --- a/src/testcases/org/apache/poi/hssf/HSSFTests.java +++ b/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.TestCellReference; -import org.apache.poi.hssf.util.TestRKUtil; -import org.apache.poi.hssf.util.TestRangeAddress; -import org.apache.poi.hssf.util.TestSheetReferences; +import org.apache.poi.hssf.util.AllHSSFUtilTests; /** * Test Suite for all sub-packages of org.apache.poi.hssf
@@ -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"); - // $JUnit-BEGIN$ + TestSuite suite = new TestSuite(HSSFTests.class.getName()); 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)); - suite.addTest(new TestSuite(TestCellReference.class)); - suite.addTest(new TestSuite(TestRangeAddress.class)); - suite.addTest(new TestSuite(TestRKUtil.class)); - suite.addTest(new TestSuite(TestSheetReferences.class)); + if (false) { // TODO - hook this test up + suite.addTest(new TestSuite(TestExcelExtractor.class)); + } suite.addTest(new TestSuite(TestEventRecordFactory.class)); suite.addTest(new TestSuite(TestModelFactory.class)); - // $JUnit-END$ return suite; } } diff --git a/src/testcases/org/apache/poi/hssf/data/ex45978-extraLinkTableSheets.xls b/src/testcases/org/apache/poi/hssf/data/ex45978-extraLinkTableSheets.xls new file mode 100644 index 0000000000..5e496142cb Binary files /dev/null and b/src/testcases/org/apache/poi/hssf/data/ex45978-extraLinkTableSheets.xls differ diff --git a/src/testcases/org/apache/poi/hssf/eventusermodel/TestEventWorkbookBuilder.java b/src/testcases/org/apache/poi/hssf/eventusermodel/TestEventWorkbookBuilder.java index c8beb7b78b..e88d274eac 100644 --- a/src/testcases/org/apache/poi/hssf/eventusermodel/TestEventWorkbookBuilder.java +++ b/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(); - assertEquals("Sh3", ref.getSheetName(0)); - assertEquals("Sheet1", ref.getSheetName(1)); - assertEquals("S2", ref.getSheetName(2)); + Workbook ref = listener.getStubWorkbook(); + assertEquals("Sh3", ref.findSheetNameFromExternSheet(0)); + assertEquals("Sheet1", ref.findSheetNameFromExternSheet(1)); + assertEquals("S2", ref.findSheetNameFromExternSheet(2)); } public void testFormulas() { diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestLinkTable.java b/src/testcases/org/apache/poi/hssf/usermodel/TestLinkTable.java index d8b4e95212..e8542edd8f 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestLinkTable.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestLinkTable.java @@ -6,7 +6,7 @@ import junit.framework.TestCase; import org.apache.poi.hssf.HSSFTestDataSamples; /** * Tests for LinkTable - * + * * @author Josh Micich */ public final class TestLinkTable extends TestCase { @@ -14,9 +14,9 @@ public final class TestLinkTable extends TestCase { /** * The example file attached to bugzilla 45046 is a clear example of Name records being present * without an External Book (SupBook) record. Excel has no trouble reading this file.
- * TODO get OOO documentation updated to reflect this (that EXTERNALBOOK is optional). - * - * It's not clear what exact steps need to be taken in Excel to create such a workbook + * TODO get OOO documentation updated to reflect this (that EXTERNALBOOK is optional). + * + * It's not clear what exact steps need to be taken in Excel to create such a workbook */ public void testLinkTableWithoutExternalBookRecord_bug45046() { HSSFWorkbook wb; @@ -32,16 +32,16 @@ public final class TestLinkTable extends TestCase { // some other sanity checks assertEquals(3, wb.getNumberOfSheets()); String formula = wb.getSheetAt(0).getRow(4).getCell(13).getCellFormula(); - + if ("ipcSummenproduktIntern($P5,N$6,$A$9,N$5)".equals(formula)) { - // The reported symptom of this bugzilla is an earlier bug (already fixed) - throw new AssertionFailedError("Identified bug 41726"); + // The reported symptom of this bugzilla is an earlier bug (already fixed) + throw new AssertionFailedError("Identified bug 41726"); // This is observable in version 3.0 } - + assertEquals("ipcSummenproduktIntern($C5,N$2,$A$9,N$1)", formula); } - + public void testMultipleExternSheetRecords_bug45698() { HSSFWorkbook wb; @@ -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); + } } diff --git a/src/testcases/org/apache/poi/hssf/util/AllHSSFUtilTests.java b/src/testcases/org/apache/poi/hssf/util/AllHSSFUtilTests.java index ec88e5c439..45c96e6c42 100755 --- a/src/testcases/org/apache/poi/hssf/util/AllHSSFUtilTests.java +++ b/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; } } diff --git a/src/testcases/org/apache/poi/hssf/util/TestSheetReferences.java b/src/testcases/org/apache/poi/hssf/util/TestSheetReferences.java deleted file mode 100644 index 4464d768c5..0000000000 --- a/src/testcases/org/apache/poi/hssf/util/TestSheetReferences.java +++ /dev/null @@ -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")); - } -}