From d32368be4a318ffb06b6cd891ed9fffd7762eb96 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Sat, 15 Nov 2008 17:25:32 +0000 Subject: [PATCH] Fix for bug 46213 - FormulaRecordAggregate should gracefully ignore extra StringRecords git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@717883 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + .../aggregates/FormulaRecordAggregate.java | 20 +++---- .../TestFormulaRecordAggregate.java | 57 +++++++++++++++---- .../poi/hssf/usermodel/RecordInspector.java | 8 +-- 5 files changed, 62 insertions(+), 25 deletions(-) diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 8257ad5ef..bb45c6369 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,6 +37,7 @@ + 46213 - Fixed FormulaRecordAggregate to gracefully ignore extra StringRecords 46174 - Fixed HSSFName to handle general formulas (not just area references) 46189 - added chart records: CHARTFRTINFO, STARTBLOCK, ENDBLOCK, STARTOBJECT, ENDOBJECT, and CATLAB 46199 - More tweaks to EmbeddedObjectRefSubRecord diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index d3f1ed911..57a0616d7 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,6 +34,7 @@ + 46213 - Fixed FormulaRecordAggregate to gracefully ignore extra StringRecords 46174 - Fixed HSSFName to handle general formulas (not just area references) 46189 - added chart records: CHARTFRTINFO, STARTBLOCK, ENDBLOCK, STARTOBJECT, ENDOBJECT, and CATLAB 46199 - More tweaks to EmbeddedObjectRefSubRecord diff --git a/src/java/org/apache/poi/hssf/record/aggregates/FormulaRecordAggregate.java b/src/java/org/apache/poi/hssf/record/aggregates/FormulaRecordAggregate.java index 18653e952..83a4ce7e0 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/FormulaRecordAggregate.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/FormulaRecordAggregate.java @@ -26,7 +26,6 @@ import org.apache.poi.hssf.record.StringRecord; import org.apache.poi.hssf.record.formula.ExpPtg; import org.apache.poi.hssf.record.formula.Ptg; import org.apache.poi.hssf.util.CellReference; -import org.apache.poi.ss.formula.Formula; /** * The formula record aggregate is used to join together the formula record and it's @@ -51,17 +50,19 @@ public final class FormulaRecordAggregate extends RecordAggregate implements Cel if (svm == null) { throw new IllegalArgumentException("sfm must not be null"); } - boolean hasStringRec = stringRec != null; - boolean hasCachedStringFlag = formulaRec.hasCachedResultString(); - if (hasStringRec != hasCachedStringFlag) { - throw new RecordFormatException("String record was " - + (hasStringRec ? "": "not ") + " supplied but formula record flag is " - + (hasCachedStringFlag ? "" : "not ") + " set"); + if (formulaRec.hasCachedResultString()) { + if (stringRec == null) { + throw new RecordFormatException("Formula record flag is set but String record was not found"); + } + _stringRecord = stringRec; + } else { + // Usually stringRec is null here (in agreement with what the formula rec says). + // In the case where an extra StringRecord is erroneously present, Excel (2007) + // ignores it (see bug 46213). + _stringRecord = null; } if (formulaRec.isSharedFormula()) { - CellReference cr = new CellReference(formulaRec.getRow(), formulaRec.getColumn()); - CellReference firstCell = formulaRec.getFormula().getExpReference(); if (firstCell == null) { handleMissingSharedFormulaRecord(formulaRec); @@ -71,7 +72,6 @@ public final class FormulaRecordAggregate extends RecordAggregate implements Cel } _formulaRecord = formulaRec; _sharedValueManager = svm; - _stringRecord = stringRec; } /** * Sometimes the shared formula flag "seems" to be erroneously set (because the corresponding diff --git a/src/testcases/org/apache/poi/hssf/record/aggregates/TestFormulaRecordAggregate.java b/src/testcases/org/apache/poi/hssf/record/aggregates/TestFormulaRecordAggregate.java index 978f400fc..a93bed20c 100644 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/TestFormulaRecordAggregate.java +++ b/src/testcases/org/apache/poi/hssf/record/aggregates/TestFormulaRecordAggregate.java @@ -17,23 +17,58 @@ package org.apache.poi.hssf.record.aggregates; +import junit.framework.AssertionFailedError; import junit.framework.TestCase; import org.apache.poi.hssf.record.FormulaRecord; +import org.apache.poi.hssf.record.Record; +import org.apache.poi.hssf.record.RecordFormatException; import org.apache.poi.hssf.record.StringRecord; +import org.apache.poi.hssf.usermodel.RecordInspector.RecordCollector; /** - * - * @author avik + * + * @author avik */ public final class TestFormulaRecordAggregate extends TestCase { - - public void testBasic() throws Exception { - FormulaRecord f = new FormulaRecord(); - f.setCachedResultTypeString(); - StringRecord s = new StringRecord(); - s.setString("abc"); - FormulaRecordAggregate fagg = new FormulaRecordAggregate(f, s, SharedValueManager.EMPTY); - assertEquals("abc", fagg.getStringValue()); - } + + public void testBasic() throws Exception { + FormulaRecord f = new FormulaRecord(); + f.setCachedResultTypeString(); + StringRecord s = new StringRecord(); + s.setString("abc"); + FormulaRecordAggregate fagg = new FormulaRecordAggregate(f, s, SharedValueManager.EMPTY); + assertEquals("abc", fagg.getStringValue()); + } + + /** + * Sometimes a {@link StringRecord} appears after a {@link FormulaRecord} even though the + * formula has evaluated to a text value. This might be more likely to occur when the formula + * can evaluate to a text value.
+ * Bug 46213 attachment 22874 has such an extra {@link StringRecord} at stream offset 0x5765. + * This file seems to open in Excel (2007) with no trouble. When it is re-saved, Excel omits + * the extra record. POI should do the same. + */ + public void testExtraStringRecord_bug46213() { + FormulaRecord fr = new FormulaRecord(); + fr.setValue(2.0); + StringRecord sr = new StringRecord(); + sr.setString("NA"); + SharedValueManager svm = SharedValueManager.EMPTY; + FormulaRecordAggregate fra; + + try { + fra = new FormulaRecordAggregate(fr, sr, svm); + } catch (RecordFormatException e) { + if ("String record was supplied but formula record flag is not set".equals(e.getMessage())) { + throw new AssertionFailedError("Identified bug 46213"); + } + throw e; + } + RecordCollector rc = new RecordCollector(); + fra.visitContainedRecords(rc); + Record[] vraRecs = rc.getRecords(); + assertEquals(1, vraRecs.length); + assertEquals(fr, vraRecs[0]); + } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/RecordInspector.java b/src/testcases/org/apache/poi/hssf/usermodel/RecordInspector.java index 867c13db4..c47fca762 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/RecordInspector.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/RecordInspector.java @@ -24,7 +24,7 @@ import org.apache.poi.hssf.record.Record; import org.apache.poi.hssf.record.aggregates.RecordAggregate.RecordVisitor; /** - * Test utility class to get {@link Record}s out HSSF objects + * Test utility class to get {@link Record}s out of HSSF objects * * @author Josh Micich */ @@ -34,12 +34,12 @@ public final class RecordInspector { // no instances of this class } - private static final class RecordCollector implements RecordVisitor { + public static final class RecordCollector implements RecordVisitor { - private List _list; + private List _list; public RecordCollector() { - _list = new ArrayList(128); + _list = new ArrayList(128); } public void visitRecord(Record r) {