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

@@ -37,6 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.5-beta4" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46213 - Fixed FormulaRecordAggregate to gracefully ignore extra StringRecords</action>
            <action dev="POI-DEVELOPERS" type="fix">46174 - Fixed HSSFName to handle general formulas (not just area references)</action>
            <action dev="POI-DEVELOPERS" type="add">46189 - added chart records: CHARTFRTINFO, STARTBLOCK, ENDBLOCK, STARTOBJECT, ENDOBJECT, and CATLAB</action>
            <action dev="POI-DEVELOPERS" type="fix">46199 - More tweaks to EmbeddedObjectRefSubRecord</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.5-beta4" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">46213 - Fixed FormulaRecordAggregate to gracefully ignore extra StringRecords</action>
            <action dev="POI-DEVELOPERS" type="fix">46174 - Fixed HSSFName to handle general formulas (not just area references)</action>
            <action dev="POI-DEVELOPERS" type="add">46189 - added chart records: CHARTFRTINFO, STARTBLOCK, ENDBLOCK, STARTOBJECT, ENDOBJECT, and CATLAB</action>
            <action dev="POI-DEVELOPERS" type="fix">46199 - More tweaks to EmbeddedObjectRefSubRecord</action>

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;
+		if (formulaRec.hasCachedResultString()) {
-		boolean hasCachedStringFlag = formulaRec.hasCachedResultString();
+			if (stringRec == null) {
-		if (hasStringRec != hasCachedStringFlag) {
+				throw new RecordFormatException("Formula record flag is set but String record was not found");
-			throw new RecordFormatException("String record was "
+			}
-					+ (hasStringRec ? "": "not ") + " supplied but formula record flag is "
+			_stringRecord = stringRec;
-					+ (hasCachedStringFlag ? "" : "not ") + " set");
+		} 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

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 {
+	public void testBasic() throws Exception {
-        FormulaRecord f = new FormulaRecord();
+		FormulaRecord f = new FormulaRecord();
-        f.setCachedResultTypeString();
+		f.setCachedResultTypeString();
-        StringRecord s = new StringRecord();
+		StringRecord s = new StringRecord();
-        s.setString("abc");
+		s.setString("abc");
-        FormulaRecordAggregate fagg = new FormulaRecordAggregate(f, s, SharedValueManager.EMPTY);
+		FormulaRecordAggregate fagg = new FormulaRecordAggregate(f, s, SharedValueManager.EMPTY);
-        assertEquals("abc", fagg.getStringValue());
+		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
+	 * <i>can</i> evaluate to a text value.<br/>
+	 * 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]);
+	}
 }

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<Record> _list;
 
 		public RecordCollector() {
-			_list = new ArrayList(128);
+			_list = new ArrayList<Record>(128);
 		}
 
 		public void visitRecord(Record r) {