Fix for bug 45672 - prevent MissingRecordAwareHSSFListener generating multiple LastCellOfRowDummyRecords when shared formulas are present

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@688426 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">45672 - Fix for MissingRecordAwareHSSFListener to prevent multiple LastCellOfRowDummyRecords when shared formulas are present</action>
            <action dev="POI-DEVELOPERS" type="fix">45645 - Fix for HSSFSheet.autoSizeColumn() for widths exceeding Short.MAX_VALUE</action>
            <action dev="POI-DEVELOPERS" type="add">45623 - Support for additional HSSF header and footer fields, including bold and full file path</action>
            <action dev="POI-DEVELOPERS" type="add">45623 - Support stripping HSSF header and footer fields (eg page number) out of header and footer text if required</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">45672 - Fix for MissingRecordAwareHSSFListener to prevent multiple LastCellOfRowDummyRecords when shared formulas are present</action>
            <action dev="POI-DEVELOPERS" type="fix">45645 - Fix for HSSFSheet.autoSizeColumn() for widths exceeding Short.MAX_VALUE</action>
            <action dev="POI-DEVELOPERS" type="add">45623 - Support for additional HSSF header and footer fields, including bold and full file path</action>
            <action dev="POI-DEVELOPERS" type="add">45623 - Support stripping HSSF header and footer fields (eg page number) out of header and footer text if required</action>

src/java/org/apache/poi/hssf/eventusermodel/MissingRecordAwareHSSFListener.java

@@ -17,22 +17,15 @@
 
 package org.apache.poi.hssf.eventusermodel;
 
-import org.apache.poi.hssf.eventusermodel.HSSFListener;
 import org.apache.poi.hssf.eventusermodel.dummyrecord.LastCellOfRowDummyRecord;
 import org.apache.poi.hssf.eventusermodel.dummyrecord.MissingCellDummyRecord;
 import org.apache.poi.hssf.eventusermodel.dummyrecord.MissingRowDummyRecord;
 import org.apache.poi.hssf.record.BOFRecord;
-import org.apache.poi.hssf.record.BlankRecord;
+import org.apache.poi.hssf.record.CellValueRecordInterface;
-import org.apache.poi.hssf.record.BoolErrRecord;
-import org.apache.poi.hssf.record.BoundSheetRecord;
-import org.apache.poi.hssf.record.FormulaRecord;
-import org.apache.poi.hssf.record.LabelRecord;
-import org.apache.poi.hssf.record.LabelSSTRecord;
 import org.apache.poi.hssf.record.NoteRecord;
-import org.apache.poi.hssf.record.NumberRecord;
-import org.apache.poi.hssf.record.RKRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RowRecord;
+import org.apache.poi.hssf.record.SharedFormulaRecord;
 
 /**
  * <p>A HSSFListener which tracks rows and columns, and will
@@ -44,16 +37,16 @@ import org.apache.poi.hssf.record.RowRecord;
  *  file, or was skipped from being written as it was
  *  blank.
  */
-public class MissingRecordAwareHSSFListener implements HSSFListener {
+public final class MissingRecordAwareHSSFListener implements HSSFListener {
 	private HSSFListener childListener;
 	
 	// Need to have different counters for cell rows and
 	//  row rows, as you sometimes get a RowRecord in the
 	//  middle of some cells, and that'd break everything
-	private int lastRowRow = -1;
+	private int lastRowRow;
 	
-	private int lastCellRow = -1;
+	private int lastCellRow;
-	private int lastCellColumn = -1;
+	private int lastCellColumn;
 	
 	/**
 	 * Constructs a new MissingRecordAwareHSSFListener, which
@@ -62,46 +55,41 @@ public class MissingRecordAwareHSSFListener implements HSSFListener {
 	 * @param listener The HSSFListener to pass records on to
 	 */
 	public MissingRecordAwareHSSFListener(HSSFListener listener) {
+		resetCounts();
 		childListener = listener;
 	}
 
 	public void processRecord(Record record) {
-		int thisRow = -1;
+		int thisRow;
-		int thisColumn = -1;
+		int thisColumn;
 
-		switch (record.getSid())
+
-        {
+		if (record instanceof CellValueRecordInterface) {
-            // the BOFRecord can represent either the beginning of a sheet or the workbook
+			CellValueRecordInterface valueRec = (CellValueRecordInterface) record;
+			thisRow = valueRec.getRow();
+			thisColumn = valueRec.getColumn();
+		} else {
+			thisRow = -1;
+			thisColumn = -1;
+
+			switch (record.getSid()) {
+				// the BOFRecord can represent either the beginning of a sheet or
+				// the workbook
 				case BOFRecord.sid:
 					BOFRecord bof = (BOFRecord) record;
-                if (bof.getType() == bof.TYPE_WORKBOOK)
+					if (bof.getType() == bof.TYPE_WORKBOOK || bof.getType() == bof.TYPE_WORKSHEET) {
-                {
+						// Reset the row and column counts - new workbook / worksheet
-                	// Reset the row and column counts - new workbook
+						resetCounts();
-                	lastRowRow = -1;
-                	lastCellRow = -1;
-                	lastCellColumn = -1;
-                    //System.out.println("Encountered workbook");
-                } else if (bof.getType() == bof.TYPE_WORKSHEET)
-                {
-                	// Reset the row and column counts - new sheet
-                	lastRowRow = -1;
-                	lastCellRow = -1;
-                	lastCellColumn = -1;
-                    //System.out.println("Encountered sheet reference");
 					}
 					break;
-            case BoundSheetRecord.sid:
-                BoundSheetRecord bsr = (BoundSheetRecord) record;
-                //System.out.println("New sheet named: " + bsr.getSheetname());
-                break;
 				case RowRecord.sid:
 					RowRecord rowrec = (RowRecord) record;
 					//System.out.println("Row " + rowrec.getRowNumber() + " found, first column at "
 					//        + rowrec.getFirstCol() + " last column at " + rowrec.getLastCol());
 
 					// If there's a jump in rows, fire off missing row records
-                if(lastRowRow+1 < rowrec.getRowNumber()) {
+					if (lastRowRow + 1 < rowrec.getRowNumber()) {
-                	for(int i=(lastRowRow+1); i<rowrec.getRowNumber(); i++) {
+						for (int i = (lastRowRow + 1); i < rowrec.getRowNumber(); i++) {
 							MissingRowDummyRecord dr = new MissingRowDummyRecord(i);
 							childListener.processRecord(dr);
 						}
@@ -111,79 +99,36 @@ public class MissingRecordAwareHSSFListener implements HSSFListener {
 					lastRowRow = rowrec.getRowNumber();
 					break;
 
-                
+				case SharedFormulaRecord.sid:
-            // These are all the "cell" records
+					// SharedFormulaRecord occurs after the first FormulaRecord of the cell range.
-                
+					// There are probably (but not always) more cell records after this
-            case BlankRecord.sid:
+					// - so don't fire off the LastCellOfRowDummyRecord yet
-            	BlankRecord brec = (BlankRecord) record;
+					childListener.processRecord(record);
-                thisRow = brec.getRow();
+					return;
-                thisColumn = brec.getColumn();
-                break;
-            case BoolErrRecord.sid:
-            	BoolErrRecord berec = (BoolErrRecord) record;
-                thisRow = berec.getRow();
-                thisColumn = berec.getColumn();
-                break;
-            case FormulaRecord.sid:
-            	FormulaRecord frec = (FormulaRecord) record;
-            	thisRow = frec.getRow();
-            	thisColumn = frec.getColumn();
-                break;
-            case LabelRecord.sid:
-            	LabelRecord lrec = (LabelRecord) record;
-                thisRow = lrec.getRow();
-                thisColumn = lrec.getColumn();
-                //System.out.println("Cell found containing String "
-                //        + " at row " + lrec.getRow() + " and column " + lrec.getColumn());
-                break;
-            case LabelSSTRecord.sid:
-            	LabelSSTRecord lsrec = (LabelSSTRecord) record;
-                thisRow = lsrec.getRow();
-                thisColumn = lsrec.getColumn();
-                //System.out.println("Cell found containing String "
-                //        + " at row " + lsrec.getRow() + " and column " + lsrec.getColumn());
-                break;
 				case NoteRecord.sid:
 					NoteRecord nrec = (NoteRecord) record;
 					thisRow = nrec.getRow();
 					thisColumn = nrec.getColumn();
 					break;
-            case NumberRecord.sid:
-                NumberRecord numrec = (NumberRecord) record;
-                thisRow = numrec.getRow();
-                thisColumn = numrec.getColumn();
-                //System.out.println("Cell found with value " + numrec.getValue()
-                //        + " at row " + numrec.getRow() + " and column " + numrec.getColumn());
-                break;
-            case RKRecord.sid:
-            	RKRecord rkrec = (RKRecord) record;
-            	thisRow = rkrec.getRow();
-            	thisColumn = rkrec.getColumn();
-                break;
-            default:
-            	//System.out.println(record.getClass());
-            	break;
 			}
-		
+		}
 		// If we're on cells, and this cell isn't in the same
 		//  row as the last one, then fire the 
-		//  dummy end-of-row records?
+		//  dummy end-of-row records
 		if(thisRow != lastCellRow && lastCellRow > -1) {
 			for(int i=lastCellRow; i<thisRow; i++) {
 				int cols = -1;
 				if(i == lastCellRow) {
 					cols = lastCellColumn;
 				}
-				LastCellOfRowDummyRecord r = new LastCellOfRowDummyRecord(i, cols);
+				childListener.processRecord(new LastCellOfRowDummyRecord(i, cols));
-				childListener.processRecord(r);
 			}
 		}
 		
 		// If we've just finished with the cells, then fire the
 		// final dummy end-of-row record
 		if(lastCellRow != -1 && lastCellColumn != -1 && thisRow == -1) {
-			LastCellOfRowDummyRecord r = new LastCellOfRowDummyRecord(lastCellRow, lastCellColumn);
+			childListener.processRecord(new LastCellOfRowDummyRecord(lastCellRow, lastCellColumn));
-			childListener.processRecord(r);
 			
 			lastCellRow = -1;
 			lastCellColumn = -1;
@@ -196,11 +141,10 @@ public class MissingRecordAwareHSSFListener implements HSSFListener {
 		}
 		
 		// If there's a gap in the cells, then fire
-		//  the dummy cell records?
+		//  the dummy cell records
-		if(lastCellColumn != (thisColumn-1)) {
+		if(lastCellColumn != thisColumn-1) {
 			for(int i=lastCellColumn+1; i<thisColumn; i++) {
-				MissingCellDummyRecord r = new MissingCellDummyRecord(thisRow, i);
+				childListener.processRecord(new MissingCellDummyRecord(thisRow, i));
-				childListener.processRecord(r);
 			}
 		}
 		
@@ -212,4 +156,10 @@ public class MissingRecordAwareHSSFListener implements HSSFListener {
 
 		childListener.processRecord(record);
 	}
+
+	private void resetCounts() {
+		lastRowRow = -1;
+		lastCellRow = -1;
+		lastCellColumn = -1;
+	}
 }

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

@@ -21,6 +21,7 @@ import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.List;
 
+import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
@@ -31,6 +32,7 @@ import org.apache.poi.hssf.record.BOFRecord;
 import org.apache.poi.hssf.record.LabelSSTRecord;
 import org.apache.poi.hssf.record.Record;
 import org.apache.poi.hssf.record.RowRecord;
+import org.apache.poi.hssf.record.SharedFormulaRecord;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 /**
  * Tests for MissingRecordAwareHSSFListener
@@ -39,44 +41,29 @@ public final class TestMissingRecordAwareHSSFListener extends TestCase {
 	
 	private Record[] r;
 
+	private void readRecords(String sampleFileName) {
+		HSSFRequest req = new HSSFRequest();
+		MockHSSFListener mockListen = new MockHSSFListener();
+		MissingRecordAwareHSSFListener listener = new MissingRecordAwareHSSFListener(mockListen);
+		req.addListenerForAllRecords(listener);
+		
+		HSSFEventFactory factory = new HSSFEventFactory();
+		try {
+			InputStream is = HSSFTestDataSamples.openSampleFileStream(sampleFileName);
+			POIFSFileSystem fs = new POIFSFileSystem(is);
+			factory.processWorkbookEvents(req, fs);
+		} catch (IOException e) {
+			throw new RuntimeException(e);
+		}
+		
+		r = mockListen.getRecords();
+		assertTrue(r.length > 100);
+	} 
 	public void openNormal() {
-		HSSFRequest req = new HSSFRequest();
+		readRecords("MissingBits.xls");
-		MockHSSFListener mockListen = new MockHSSFListener();
-		MissingRecordAwareHSSFListener listener = new MissingRecordAwareHSSFListener(mockListen);
-		req.addListenerForAllRecords(listener);
-		
-		HSSFEventFactory factory = new HSSFEventFactory();
-		try {
-			InputStream is = HSSFTestDataSamples.openSampleFileStream("MissingBits.xls");
-			POIFSFileSystem fs = new POIFSFileSystem(is);
-			factory.processWorkbookEvents(req, fs);
-		} catch (IOException e) {
-			throw new RuntimeException(e);
 	}
 	
-		r = mockListen.getRecords();
+	public void testMissingRowRecords() {
-		assertTrue(r.length > 100);
-	} 
-	public void openAlt() {
-		HSSFRequest req = new HSSFRequest();
-		MockHSSFListener mockListen = new MockHSSFListener();
-		MissingRecordAwareHSSFListener listener = new MissingRecordAwareHSSFListener(mockListen);
-		req.addListenerForAllRecords(listener);
-		
-		HSSFEventFactory factory = new HSSFEventFactory();
-		try {
-			InputStream is = HSSFTestDataSamples.openSampleFileStream("MRExtraLines.xls");
-			POIFSFileSystem fs = new POIFSFileSystem(is);
-			factory.processWorkbookEvents(req, fs);
-		} catch (IOException e) {
-			throw new RuntimeException(e);
-		}
-		
-		r = mockListen.getRecords();
-		assertTrue(r.length > 100);
-	} 
-	
-	public void testMissingRowRecords() throws Exception {
 		openNormal();
 		
 		// We have rows 0, 1, 2, 20 and 21
@@ -126,7 +113,7 @@ public final class TestMissingRecordAwareHSSFListener extends TestCase {
 		assertEquals(19, mr.getRowNumber());
 	}
 	
-	public void testEndOfRowRecords() throws Exception {
+	public void testEndOfRowRecords() {
 		openNormal();
 		
 		// Find the cell at 0,0
@@ -248,7 +235,7 @@ public final class TestMissingRecordAwareHSSFListener extends TestCase {
 	}
 	
 	
-	public void testMissingCellRecords() throws Exception {
+	public void testMissingCellRecords() {
 		openNormal();
 		
 		// Find the cell at 0,0
@@ -350,29 +337,21 @@ public final class TestMissingRecordAwareHSSFListener extends TestCase {
 	
 	// Make sure we don't put in any extra new lines
 	//  that aren't already there
-	public void testNoExtraNewLines() throws Exception {
+	public void testNoExtraNewLines() {
 		// Load a different file
-		openAlt();
-		
-		
 		// This file has has something in lines 1-33
-		List lcor = new ArrayList();
+		readRecords("MRExtraLines.xls");
+		
+		int rowCount=0;
 		for(int i=0; i<r.length; i++) {
-			if(r[i] instanceof LastCellOfRowDummyRecord)
+			if(r[i] instanceof LastCellOfRowDummyRecord) {
-				lcor.add( (LastCellOfRowDummyRecord)r[i] );
+				LastCellOfRowDummyRecord eor = (LastCellOfRowDummyRecord) r[i];
+				assertEquals(rowCount, eor.getRow());
+				rowCount++;
+			}
 		}
-		
 		// Check we got the 33 rows
-		assertEquals(33, lcor.size());
+		assertEquals(33, rowCount);
-		LastCellOfRowDummyRecord[] rowEnds = (LastCellOfRowDummyRecord[])
-			lcor.toArray(new LastCellOfRowDummyRecord[lcor.size()]);
-		assertEquals(33, rowEnds.length);
-		
-		// And check they have the right stuff in them,
-		//  no repeats
-		for(int i=0; i<rowEnds.length; i++) {
-			assertEquals(i, rowEnds[i].getRow());
-		}
 	}
 
 	private static final class MockHSSFListener implements HSSFListener {
@@ -418,4 +397,29 @@ public final class TestMissingRecordAwareHSSFListener extends TestCase {
 			return result;
 		}
 	}
+	
+	/**
+	 * Make sure that the presence of shared formulas does not cause extra 
+	 * end-of-row records.
+	 */
+	public void testEndOfRow_bug45672() {
+		readRecords("ex45672.xls");
+		Record[] rr = r;
+		int eorCount=0;
+		int sfrCount=0;
+		for (int i = 0; i < rr.length; i++) {
+			Record record = rr[i];
+			if (record instanceof SharedFormulaRecord) {
+				sfrCount++;
+			}
+			if (record instanceof LastCellOfRowDummyRecord) {
+				eorCount++;
+			}
+		}
+		if (eorCount == 2) {
+			throw new AssertionFailedError("Identified bug 45672");
+		}
+		assertEquals(1, eorCount);
+		assertEquals(1, sfrCount);
+	}
 }