From faa8b69a1e0aceae59e1ef9649d468be31d5c6b1 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Fri, 18 Sep 2009 00:33:18 +0000 Subject: [PATCH] Bugzilla 47747 - fixed logic for locating shared formula records git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@816417 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/status.xml | 3 +- .../poi/hssf/model/RowBlocksReader.java | 35 ++- .../aggregates/FormulaRecordAggregate.java | 25 +- .../record/aggregates/SharedValueManager.java | 232 +++++++++--------- .../aggregates/TestSharedValueManager.java | 49 ++++ .../spreadsheet/ex47747-sharedFormula.xls | Bin 0 -> 24576 bytes 6 files changed, 197 insertions(+), 147 deletions(-) create mode 100755 test-data/spreadsheet/ex47747-sharedFormula.xls diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index c4c9288550..53dbe1e3d6 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,8 +33,9 @@ + 47747 - fixed logic for locating shared formula records 47809 - Improved work with user-defined functions - 47581 - fixed XSSFSheet.setColumnWidth to produce XML compatible with Mac Excel 2008 + 47581 - fixed XSSFSheet.setColumnWidth to produce XML compatible with Mac Excel 2008 47734 - removed unnecessary svn:executable flag from files in SVN trunk 47543 - added javadoc how to avoid Excel crash when creating too many HSSFRichTextString cells 47813 - fixed problems with XSSFWorkbook.removeSheetAt when workbook contains chart diff --git a/src/java/org/apache/poi/hssf/model/RowBlocksReader.java b/src/java/org/apache/poi/hssf/model/RowBlocksReader.java index 038304916a..24498267c8 100644 --- a/src/java/org/apache/poi/hssf/model/RowBlocksReader.java +++ b/src/java/org/apache/poi/hssf/model/RowBlocksReader.java @@ -21,17 +21,19 @@ import java.util.ArrayList; import java.util.List; import org.apache.poi.hssf.record.ArrayRecord; +import org.apache.poi.hssf.record.FormulaRecord; import org.apache.poi.hssf.record.MergeCellsRecord; import org.apache.poi.hssf.record.Record; import org.apache.poi.hssf.record.SharedFormulaRecord; import org.apache.poi.hssf.record.TableRecord; import org.apache.poi.hssf.record.aggregates.MergedCellsTable; import org.apache.poi.hssf.record.aggregates.SharedValueManager; +import org.apache.poi.ss.util.CellReference; /** - * Segregates the 'Row Blocks' section of a single sheet into plain row/cell records and + * Segregates the 'Row Blocks' section of a single sheet into plain row/cell records and * shared formula records. - * + * * @author Josh Micich */ public final class RowBlocksReader { @@ -47,45 +49,56 @@ public final class RowBlocksReader { public RowBlocksReader(RecordStream rs) { List plainRecords = new ArrayList(); List shFrmRecords = new ArrayList(); + List firstCellRefs = new ArrayList(); List arrayRecords = new ArrayList(); List tableRecords = new ArrayList(); List mergeCellRecords = new ArrayList(); + Record prevRec = null; while(!RecordOrderer.isEndOfRowBlock(rs.peekNextSid())) { // End of row/cell records for the current sheet - // Note - It is important that this code does not inadvertently add any sheet - // records from a subsequent sheet. For example, if SharedFormulaRecords + // Note - It is important that this code does not inadvertently add any sheet + // records from a subsequent sheet. For example, if SharedFormulaRecords // are taken from the wrong sheet, this could cause bug 44449. if (!rs.hasNext()) { throw new RuntimeException("Failed to find end of row/cell records"); - + } Record rec = rs.getNext(); List dest; switch (rec.getSid()) { case MergeCellsRecord.sid: dest = mergeCellRecords; break; - case SharedFormulaRecord.sid: dest = shFrmRecords; break; + case SharedFormulaRecord.sid: dest = shFrmRecords; + if (!(prevRec instanceof FormulaRecord)) { + throw new RuntimeException("Shared formula record should follow a FormulaRecord"); + } + FormulaRecord fr = (FormulaRecord)prevRec; + firstCellRefs.add(new CellReference(fr.getRow(), fr.getColumn())); + break; case ArrayRecord.sid: dest = arrayRecords; break; case TableRecord.sid: dest = tableRecords; break; default: dest = plainRecords; } dest.add(rec); + prevRec = rec; } SharedFormulaRecord[] sharedFormulaRecs = new SharedFormulaRecord[shFrmRecords.size()]; + CellReference[] firstCells = new CellReference[firstCellRefs.size()]; ArrayRecord[] arrayRecs = new ArrayRecord[arrayRecords.size()]; TableRecord[] tableRecs = new TableRecord[tableRecords.size()]; shFrmRecords.toArray(sharedFormulaRecs); + firstCellRefs.toArray(firstCells); arrayRecords.toArray(arrayRecs); tableRecords.toArray(tableRecs); - + _plainRecords = plainRecords; - _sfm = SharedValueManager.create(sharedFormulaRecs, arrayRecs, tableRecs); + _sfm = SharedValueManager.create(sharedFormulaRecs, firstCells, arrayRecs, tableRecs); _mergedCellsRecords = new MergeCellsRecord[mergeCellRecords.size()]; mergeCellRecords.toArray(_mergedCellsRecords); } - + /** - * Some unconventional apps place {@link MergeCellsRecord}s within the row block. They + * Some unconventional apps place {@link MergeCellsRecord}s within the row block. They * actually should be in the {@link MergedCellsTable} which is much later (see bug 45699). * @return any loose MergeCellsRecords found */ @@ -97,7 +110,7 @@ public final class RowBlocksReader { return _sfm; } /** - * @return a {@link RecordStream} containing all the non-{@link SharedFormulaRecord} + * @return a {@link RecordStream} containing all the non-{@link SharedFormulaRecord} * non-{@link ArrayRecord} and non-{@link TableRecord} Records. */ public RecordStream getPlainRecordStream() { 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 73869e193b..0f4e976d9a 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/FormulaRecordAggregate.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/FormulaRecordAggregate.java @@ -58,10 +58,12 @@ public final class FormulaRecordAggregate extends RecordAggregate implements Cel } 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). + // ignores it (see bug 46213). _stringRecord = null; } + _formulaRecord = formulaRec; + _sharedValueManager = svm; if (formulaRec.isSharedFormula()) { CellReference firstCell = formulaRec.getFormula().getExpReference(); if (firstCell == null) { @@ -70,24 +72,22 @@ public final class FormulaRecordAggregate extends RecordAggregate implements Cel _sharedFormulaRecord = svm.linkSharedFormulaRecord(firstCell, this); } } - _formulaRecord = formulaRec; - _sharedValueManager = svm; } /** * Sometimes the shared formula flag "seems" to be erroneously set (because the corresponding * {@link SharedFormulaRecord} does not exist). Normally this would leave no way of determining - * the {@link Ptg} tokens for the formula. However as it turns out in these + * the {@link Ptg} tokens for the formula. However as it turns out in these * cases, Excel encodes the unshared {@link Ptg} tokens in the right place (inside the {@link * FormulaRecord}). So the the only thing that needs to be done is to ignore the erroneous * shared formula flag.
- * + * * This method may also be used for setting breakpoints to help diagnose issues regarding the - * abnormally-set 'shared formula' flags. + * abnormally-set 'shared formula' flags. * (see TestValueRecordsAggregate.testSpuriousSharedFormulaFlag()).

*/ private static void handleMissingSharedFormulaRecord(FormulaRecord formula) { // make sure 'unshared' formula is actually available - Ptg firstToken = formula.getParsedExpression()[0]; + Ptg firstToken = formula.getParsedExpression()[0]; if (firstToken instanceof ExpPtg) { throw new RecordFormatException( "SharedFormulaRecord not found for FormulaRecord with (isSharedFormula=true)"); @@ -138,14 +138,9 @@ public final class FormulaRecordAggregate extends RecordAggregate implements Cel public void visitContainedRecords(RecordVisitor rv) { rv.visitRecord(_formulaRecord); - CellReference sharedFirstCell = _formulaRecord.getFormula().getExpReference(); - // perhaps this could be optimised by consulting the (somewhat unreliable) isShared flag - // and/or distinguishing between tExp and tTbl. - if (sharedFirstCell != null) { - Record sharedFormulaRecord = _sharedValueManager.getRecordForFirstCell(sharedFirstCell, this); - if (sharedFormulaRecord != null) { - rv.visitRecord(sharedFormulaRecord); - } + Record sharedFormulaRecord = _sharedValueManager.getRecordForFirstCell(this); + if (sharedFormulaRecord != null) { + rv.visitRecord(sharedFormulaRecord); } if (_formulaRecord.hasCachedResultString() && _stringRecord != null) { rv.visitRecord(_stringRecord); diff --git a/src/java/org/apache/poi/hssf/record/aggregates/SharedValueManager.java b/src/java/org/apache/poi/hssf/record/aggregates/SharedValueManager.java index f007ebe6e9..8c4d19d2b8 100644 --- a/src/java/org/apache/poi/hssf/record/aggregates/SharedValueManager.java +++ b/src/java/org/apache/poi/hssf/record/aggregates/SharedValueManager.java @@ -28,9 +28,8 @@ import org.apache.poi.hssf.record.SharedFormulaRecord; import org.apache.poi.hssf.record.SharedValueRecordBase; import org.apache.poi.hssf.record.TableRecord; import org.apache.poi.hssf.record.formula.ExpPtg; -import org.apache.poi.hssf.record.formula.TblPtg; import org.apache.poi.hssf.util.CellRangeAddress8Bit; -import org.apache.poi.hssf.util.CellReference; +import org.apache.poi.ss.util.CellReference; /** * Manages various auxiliary records while constructing a @@ -45,26 +44,38 @@ import org.apache.poi.hssf.util.CellReference; */ public final class SharedValueManager { - // This class should probably be generalised to handle array and table groups too - private static final class SharedValueGroup { - private final SharedValueRecordBase _svr; - private FormulaRecordAggregate[] _frAggs; + private static final class SharedFormulaGroup { + private final SharedFormulaRecord _sfr; + private final FormulaRecordAggregate[] _frAggs; private int _numberOfFormulas; + /** + * Coordinates of the first cell having a formula that uses this shared formula. + * This is often but not always the top left cell in the range covered by + * {@link #_sfr} + */ + private final CellReference _firstCell; - public SharedValueGroup(SharedValueRecordBase svr) { - _svr = svr; - int width = svr.getLastColumn() - svr.getFirstColumn() + 1; - int height = svr.getLastRow() - svr.getFirstRow() + 1; + public SharedFormulaGroup(SharedFormulaRecord sfr, CellReference firstCell) { + if (!sfr.isInRange(firstCell.getRow(), firstCell.getCol())) { + throw new IllegalArgumentException("First formula cell " + firstCell.formatAsString() + + " is not shared formula range " + sfr.getRange().toString() + "."); + } + _sfr = sfr; + _firstCell = firstCell; + int width = sfr.getLastColumn() - sfr.getFirstColumn() + 1; + int height = sfr.getLastRow() - sfr.getFirstRow() + 1; _frAggs = new FormulaRecordAggregate[width * height]; _numberOfFormulas = 0; } public void add(FormulaRecordAggregate agg) { + if (_numberOfFormulas == 0) { + if (_firstCell.getRow() != agg.getRow() || _firstCell.getCol() != agg.getColumn()) { + throw new IllegalStateException("shared formula coding error"); + } + } if (_numberOfFormulas >= _frAggs.length) { - // this probably shouldn't occur - problems with sample file "15228.xls" - FormulaRecordAggregate[] temp = new FormulaRecordAggregate[_numberOfFormulas*2]; - System.arraycopy(_frAggs, 0, temp, 0, _frAggs.length); - _frAggs = temp; + throw new RuntimeException("Too many formula records for shared formula group"); } _frAggs[_numberOfFormulas++] = agg; } @@ -75,49 +86,55 @@ public final class SharedValueManager { } } - public SharedValueRecordBase getSVR() { - return _svr; + public SharedFormulaRecord getSFR() { + return _sfr; } - /** - * Note - Sometimes the first formula in a group is not present (because the range - * is sparsely populated), so this method can return true for a cell - * that is not the top-left corner of the range. - * @return true if this is the first formula cell in the group - */ - public boolean isFirstMember(FormulaRecordAggregate agg) { - return agg == _frAggs[0]; - } public final String toString() { StringBuffer sb = new StringBuffer(64); sb.append(getClass().getName()).append(" ["); - sb.append(_svr.getRange().toString()); + sb.append(_sfr.getRange().toString()); sb.append("]"); return sb.toString(); } + + /** + * Note - the 'first cell' of a shared formula group is not always the top-left cell + * of the enclosing range. + * @return true if the specified coordinates correspond to the 'first cell' + * of this shared formula group. + */ + public boolean isFirstCell(int row, int column) { + return _firstCell.getRow() == row && _firstCell.getCol() == column; + } } public static final SharedValueManager EMPTY = new SharedValueManager( - new SharedFormulaRecord[0], new ArrayRecord[0], new TableRecord[0]); + new SharedFormulaRecord[0], new CellReference[0], new ArrayRecord[0], new TableRecord[0]); private final ArrayRecord[] _arrayRecords; private final TableRecord[] _tableRecords; - private final Map _groupsBySharedFormulaRecord; + private final Map _groupsBySharedFormulaRecord; /** cached for optimization purposes */ - private SharedValueGroup[] _groups; + private SharedFormulaGroup[] _groups; private SharedValueManager(SharedFormulaRecord[] sharedFormulaRecords, - ArrayRecord[] arrayRecords, TableRecord[] tableRecords) { + CellReference[] firstCells, ArrayRecord[] arrayRecords, TableRecord[] tableRecords) { + int nShF = sharedFormulaRecords.length; + if (nShF != firstCells.length) { + throw new IllegalArgumentException("array sizes don't match: " + nShF + "!=" + firstCells.length + "."); + } _arrayRecords = arrayRecords; _tableRecords = tableRecords; - Map m = new HashMap(sharedFormulaRecords.length * 3 / 2); - for (int i = 0; i < sharedFormulaRecords.length; i++) { + Map m = new HashMap(nShF * 3 / 2); + for (int i = 0; i < nShF; i++) { SharedFormulaRecord sfr = sharedFormulaRecords[i]; - m.put(sfr, new SharedValueGroup(sfr)); + m.put(sfr, new SharedFormulaGroup(sfr, firstCells[i])); } _groupsBySharedFormulaRecord = m; } /** + * @param firstCells * @param recs list of sheet records (possibly contains records for other parts of the Excel file) * @param startIx index of first row/cell record for current sheet * @param endIx one past index of last row/cell record for current sheet. It is important @@ -125,11 +142,11 @@ public final class SharedValueManager { * sheet (which could happen if endIx is chosen poorly). (see bug 44449) */ public static SharedValueManager create(SharedFormulaRecord[] sharedFormulaRecords, - ArrayRecord[] arrayRecords, TableRecord[] tableRecords) { - if (sharedFormulaRecords.length + arrayRecords.length + tableRecords.length < 1) { + CellReference[] firstCells, ArrayRecord[] arrayRecords, TableRecord[] tableRecords) { + if (sharedFormulaRecords.length + firstCells.length + arrayRecords.length + tableRecords.length < 1) { return EMPTY; } - return new SharedValueManager(sharedFormulaRecords, arrayRecords, tableRecords); + return new SharedValueManager(sharedFormulaRecords, firstCells, arrayRecords, tableRecords); } @@ -139,68 +156,30 @@ public final class SharedValueManager { */ public SharedFormulaRecord linkSharedFormulaRecord(CellReference firstCell, FormulaRecordAggregate agg) { - SharedValueGroup result = findGroup(getGroups(), firstCell); + SharedFormulaGroup result = findFormulaGroup(getGroups(), firstCell); result.add(agg); - return (SharedFormulaRecord) result.getSVR(); + return result.getSFR(); } - private static SharedValueGroup findGroup(SharedValueGroup[] groups, CellReference firstCell) { + private static SharedFormulaGroup findFormulaGroup(SharedFormulaGroup[] groups, CellReference firstCell) { int row = firstCell.getRow(); int column = firstCell.getCol(); // Traverse the list of shared formulas and try to find the correct one for us // perhaps this could be optimised to some kind of binary search for (int i = 0; i < groups.length; i++) { - SharedValueGroup svg = groups[i]; - if (svg.getSVR().isFirstCell(row, column)) { + SharedFormulaGroup svg = groups[i]; + if (svg.isFirstCell(row, column)) { return svg; } } - // else - no SharedFormulaRecord was found with the specified firstCell. - // This is unusual, but one sample file exhibits the anomaly: "ex45046-21984.xls" - // Excel seems to handle the problem OK, and doesn't even correct it. Perhaps POI should. - - // search for shared formula by range - SharedValueGroup result = null; - for (int i = 0; i < groups.length; i++) { - SharedValueGroup svg = groups[i]; - if (svg.getSVR().isInRange(row, column)) { - if (result != null) { - // This happens in sample file "15228.xls" - if (sharedFormulasAreSame(result, svg)) { - // hopefully this is OK - just use the first one since they are the same - // not quite - // TODO - fix file "15228.xls" so it opens in Excel after rewriting with POI - } else { - throw new RuntimeException("This cell is in the range of more than one distinct shared formula"); - } - } else { - result = svg; - } - } - } - if (result == null) { - throw new RuntimeException("Failed to find a matching shared formula record"); - } - return result; + // TODO - fix file "15228.xls" so it opens in Excel after rewriting with POI + throw new RuntimeException("Failed to find a matching shared formula record"); } - /** - * Handles the ugly situation (seen in example "15228.xls") where a shared formula cell is - * covered by more than one shared formula range, but the formula cell's {@link ExpPtg} - * doesn't identify any of them. - * @return true if the underlying shared formulas are the same - */ - private static boolean sharedFormulasAreSame(SharedValueGroup grpA, SharedValueGroup grpB) { - // safe to cast here because this findGroup() is never called for ARRAY or TABLE formulas - SharedFormulaRecord sfrA = (SharedFormulaRecord) grpA.getSVR(); - SharedFormulaRecord sfrB = (SharedFormulaRecord) grpB.getSVR(); - return sfrA.isFormulaSame(sfrB); - } - - private SharedValueGroup[] getGroups() { + private SharedFormulaGroup[] getGroups() { if (_groups == null) { - SharedValueGroup[] groups = new SharedValueGroup[_groupsBySharedFormulaRecord.size()]; + SharedFormulaGroup[] groups = new SharedFormulaGroup[_groupsBySharedFormulaRecord.size()]; _groupsBySharedFormulaRecord.values().toArray(groups); Arrays.sort(groups, SVGComparator); // make search behaviour more deterministic _groups = groups; @@ -208,11 +187,11 @@ public final class SharedValueManager { return _groups; } - private static final Comparator SVGComparator = new Comparator() { + private static final Comparator SVGComparator = new Comparator() { - public int compare(Object a, Object b) { - CellRangeAddress8Bit rangeA = ((SharedValueGroup)a).getSVR().getRange(); - CellRangeAddress8Bit rangeB = ((SharedValueGroup)b).getSVR().getRange(); + public int compare(SharedFormulaGroup a, SharedFormulaGroup b) { + CellRangeAddress8Bit rangeA = a.getSFR().getRange(); + CellRangeAddress8Bit rangeB = b.getSFR().getRange(); int cmp; cmp = rangeA.getFirstRow() - rangeB.getFirstRow(); @@ -228,44 +207,57 @@ public final class SharedValueManager { }; /** - * The {@link SharedValueRecordBase} record returned by this method - * @param firstCell the cell coordinates as read from the {@link ExpPtg} or {@link TblPtg} - * of the current formula. Note - this is usually not the same as the cell coordinates - * of the formula's cell. + * Gets the {@link SharedValueRecordBase} record if it should be encoded immediately after the + * formula record contained in the specified {@link FormulaRecordAggregate} agg. Note - the + * shared value record always appears after the first formula record in the group. For arrays + * and tables the first formula is always the in the top left cell. However, since shared + * formula groups can be sparse and/or overlap, the first formula may not actually be in the + * top left cell. * - * @return the SHRFMLA, TABLE or ARRAY record for this formula cell, if it is the first cell of a - * table or array region. null if + * @return the SHRFMLA, TABLE or ARRAY record for the formula cell, if it is the first cell of + * a table or array region. null if the formula cell is not shared/array/table, + * or if the specified formula is not the the first in the group. */ - public SharedValueRecordBase getRecordForFirstCell(CellReference firstCell, FormulaRecordAggregate agg) { + public SharedValueRecordBase getRecordForFirstCell(FormulaRecordAggregate agg) { + CellReference firstCell = agg.getFormulaRecord().getFormula().getExpReference(); + // perhaps this could be optimised by consulting the (somewhat unreliable) isShared flag + // and/or distinguishing between tExp and tTbl. + if (firstCell == null) { + // not a shared/array/table formula + return null; + } + + int row = firstCell.getRow(); int column = firstCell.getCol(); - boolean isTopLeft = agg.getRow() == row && agg.getColumn() == column; - if (isTopLeft) { - for (int i = 0; i < _tableRecords.length; i++) { - TableRecord tr = _tableRecords[i]; - if (tr.isFirstCell(row, column)) { - return tr; - } - } - for (int i = 0; i < _arrayRecords.length; i++) { - ArrayRecord ar = _arrayRecords[i]; - if (ar.isFirstCell(row, column)) { - return ar; - } - } - } else { - // Since arrays and tables cannot be sparse (all cells in range participate) - // no need to search arrays and tables if agg is not the top left cell + if (agg.getRow() != row || agg.getColumn() != column) { + // not the first formula cell in the group + return null; } - SharedValueGroup[] groups = getGroups(); + SharedFormulaGroup[] groups = getGroups(); for (int i = 0; i < groups.length; i++) { - SharedValueGroup svg = groups[i]; - SharedValueRecordBase svr = svg.getSVR(); - if (svr.isFirstCell(row, column)) { - if (svg.isFirstMember(agg)) { - return svr; - } - return null; + // note - logic for finding correct shared formula group is slightly + // more complicated since the first cell + SharedFormulaGroup sfg = groups[i]; + if (sfg.isFirstCell(row, column)) { + return sfg.getSFR(); + } + } + + // Since arrays and tables cannot be sparse (all cells in range participate) + // The first cell will be the top left in the range. So we can match the + // ARRAY/TABLE record directly. + + for (int i = 0; i < _tableRecords.length; i++) { + TableRecord tr = _tableRecords[i]; + if (tr.isFirstCell(row, column)) { + return tr; + } + } + for (int i = 0; i < _arrayRecords.length; i++) { + ArrayRecord ar = _arrayRecords[i]; + if (ar.isFirstCell(row, column)) { + return ar; } } return null; @@ -276,7 +268,7 @@ public final class SharedValueManager { * to plain unshared formulas */ public void unlink(SharedFormulaRecord sharedFormulaRecord) { - SharedValueGroup svg = (SharedValueGroup) _groupsBySharedFormulaRecord.remove(sharedFormulaRecord); + SharedFormulaGroup svg = _groupsBySharedFormulaRecord.remove(sharedFormulaRecord); _groups = null; // be sure to reset cached value if (svg == null) { throw new IllegalStateException("Failed to find formulas for shared formula"); diff --git a/src/testcases/org/apache/poi/hssf/record/aggregates/TestSharedValueManager.java b/src/testcases/org/apache/poi/hssf/record/aggregates/TestSharedValueManager.java index 9f169cada8..66e1c84f03 100644 --- a/src/testcases/org/apache/poi/hssf/record/aggregates/TestSharedValueManager.java +++ b/src/testcases/org/apache/poi/hssf/record/aggregates/TestSharedValueManager.java @@ -26,6 +26,7 @@ import junit.framework.TestCase; import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.hssf.record.Record; import org.apache.poi.hssf.record.SharedFormulaRecord; +import org.apache.poi.hssf.usermodel.HSSFCell; import org.apache.poi.hssf.usermodel.HSSFSheet; import org.apache.poi.hssf.usermodel.HSSFWorkbook; import org.apache.poi.hssf.usermodel.RecordInspector; @@ -120,4 +121,52 @@ public final class TestSharedValueManager extends TestCase { } assertEquals(2, count); } + + /** + * Tests fix for a bug in the way shared formula cells are associated with shared formula + * records. Prior to this fix, POI would attempt to use the upper left corner of the + * shared formula range as the locator cell. The correct cell to use is the 'first cell' + * in the shared formula group which is not always the top left cell. This is possible + * because shared formula groups may be sparse and may overlap.
+ * + * Two existing sample files (15228.xls and ex45046-21984.xls) had similar issues. + * These were not explored fully, but seem to be fixed now. + */ + public void testRecalculateFormulas47747() { + + /* + * ex47747-sharedFormula.xls is a heavily cut-down version of the spreadsheet from + * the attachment (id=24176) in Bugzilla 47747. This was done to make the sample + * file smaller, which hopefully allows the special data encoding condition to be + * seen more easily. Care must be taken when modifying this file since the + * special conditions are easily destroyed (which would make this test useless). + * It seems that removing the worksheet protection has made this more so - if the + * current file is re-saved in Excel(2007) the bug condition disappears. + * + * + * Using BiffViewer, one can see that there are two shared formula groups representing + * the essentially same formula over ~20 cells. The shared group ranges overlap and + * are A12:Q20 and A20:Q27. The locator cell ('first cell') for the second group is + * Q20 which is not the top left cell of the enclosing range. It is this specific + * condition which caused the bug to occur + */ + HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook("ex47747-sharedFormula.xls"); + + // pick out a cell from within the second shared formula group + HSSFCell cell = wb.getSheetAt(0).getRow(23).getCell(0); + String formulaText; + try { + formulaText = cell.getCellFormula(); + // succeeds if the formula record has been associated + // with the second shared formula group + } catch (RuntimeException e) { + // bug occurs if the formula record has been associated + // with the first shared formula group + if ("Shared Formula Conversion: Coding Error".equals(e.getMessage())) { + throw new AssertionFailedError("Identified bug 47747"); + } + throw e; + } + assertEquals("$AF24*A$7", formulaText); + } } diff --git a/test-data/spreadsheet/ex47747-sharedFormula.xls b/test-data/spreadsheet/ex47747-sharedFormula.xls new file mode 100755 index 0000000000000000000000000000000000000000..716ffa10c3371b142ccfa68316e046a476a365fb GIT binary patch literal 24576 zcmeHPdvIJ=dH+^gN$W>!$!O`60v3%joseJy-B{Zy;WlGou z>T{v`d`zJ}qds@Z0(qtCRXKO_T^nxzLAvens@to(GSV!a&#VuamCq zSngCIw@q}_tmvvmQB?`qF3-?JL4paH6;)LQj!KCC+Ycu;H73^}YOnOv-_dUk-|21CgyFQxZPeQ4rGsaZ+6dvA{H=V{%1y7A&C)J% zlA>(R*pihWPc$pQiA_7pfTorv$bGVT_epSRl}-2qs)@HbVldI$*NgH)qS4M+I`3poV z$9Q03;Nz(=xmG?djoubMDyrFa^02f)e$uw})}m>-kL6nGu4U)Rmfa^?8k1Y)6P(6R zwz$ns825yG+V7G_Fo-;iAOPs8ewT@8h|3Q8au{xxPs+6hLmS(NYU|m-YnM+k$8ZE~ z)l3~U#UX&zDxX%)WOON{d!~Hb&b;x5(cpRUFaP()xdz74o{$W?SH849;U`)T zPnN;Sk0|{I;h&CxKN10dGy?uW1pKiG_@^S^e;NUQZv;F_o+$cHC_1wP`uT7KT+0(| zujL8CKNdmfvk~woBjA4>0e?>f{0kBA*DKHG6rDK%{b)Zy2Ce<-j~g29n{(eBN#3j4 zU*o_>M!vNAORFXMw1OuPztwpB85*d|9`B!sfIkuee>fCA4>ZHZdmZ4faAoO>GxP2C z&bZKij+Kn2^DQ}o+tpY(DES+_aHvJY7r5{YZb8-u_wnUgI7}Yo0-CR*Pd#21+3;`4 zUASFt(?<-}?H&0)spP~gpy8Jq8kWpi0=wL=;7e>c=xe)ZdojfhvkT?`4fn~9yhOvl zC0QiDj((7i1lqqNwMgDUft+i%JhN-}ZRb;aX6h)|EwMCc|J5aDbP zO@zL4RU%h<;;lo6s(?tmb?AK+5Q(=AU9|!t@z%jWtb|CsbyieHB;Gn$r5TdoK|;n( zdq=!=u!^gMNW68h;HZR1ymb&!Dj^bYolIp!;;pl$G9vNT!NT|I-4Thm&f3a|#9L=w zWklkwv%WGS@z&W;8IgGFY^;n(ymgu#B3wsYlG;*W?69Kr)xRsb;qimM$!il@)Tzmf zZGUlI!il{6^2-j9I!tJ6e9t}ilrrTj6#|=Oz|dS>vqNC#Q+}Z~qpcQ-Gb*ppo;~!? zLjj>WJ39kHnJ`-@!Zc~_G3gGOu=A-FpQf;YN4Gi6+nx6a6`azsw^ANGL_o$7uG+8s z*tCy7xxj{!F$Ad&4_E4)!1y89Q3;V!XFQSZl@RfVYQGY%0}-u6N*(A<&=synGuJ;m zJMRuxV(H4C1YF^b^H0A~uo+}nHZW-H!BCn>I6?*8<3#00K2|DSanxgv(#Icvyd-5JwqOcYR*yX(Oqe|&92jT^O|uhF(=?q9Zzu1iQ035} zc-umiN2KCy3sr_G-nLNYh_r<=VZl+l3ml^zEO9<{)ThC>03jKjy;EXifGJ`5VZ#vr zl(0cbIvq)rX?rP$P3BR_oUyWwmW`q^v`ve_KNM#nM5rp_Aq!3+N$ z&SsIH&2>RGtubsE>B_cA!8ZT#pW$p4``PRWvT2WDv%_UG%NqIRzdaYuW(nDBkN33I zV6(jfPrK6d`77TKXS3ALCdAX$#;`Fx?MlzTe&O@sY|?%A7^#&nCpvI%3$Eo_3|@ z)o(u;&SoXq?1=ZY4A|_bz|-10KJ>)1nho5XhYFmSw#v^Y#M9QrurWQYz2mw2J`~QT z(a$Et)0$)0n4Z?&@i+f?e>j`fel{VVwl#*0>1pj9k3aa;a5fn~n-EWHi(zAWT6@RE zZ%_F_xMAx}Q;Xi(&>wQE`pAmWBFK-Vgy51!UZ}Ah|9lZOB9~qC%Vl?)U>|Hf8X03Ml>bOOu_bMhcBdV z^jbc#qEsz=vD_MVrp6j9^|%aXcEo$GmCKQ&9^A@xo#!Kx*t zIno7!V@SNF@R>#Gm6JSDFebpvBuJEbuFZf;0>z+KkPD-d$#w#b&8L*-X_n{dV%2<{ zCct}{u2E~~Os2~k8@rIY$0yg)(rm~ziezv|&LB1H!rd7DOsTj=R!N~bNNuT@O4mA4 z=_gw%{bWm}pDtoEd&^99=|U>w6{V&cOuC?wv-rz`l?dBzovB6JW?$P{oT_u%X54v^&uDrY*M211^AV^N&z5r+rjLww%I18CR7Uu`@dAR#dFR8@xQc@0p zNZ#l0+*hzAg2=W+^RSx0zS2P}JDeLooxzI3*WK!R*ykXm;0U4=ry-eMX(&g+uv>lT zye3D7u7hMcoL;P&CmkknL{LBRh7jU9ibfDGbv}IgpV1K#-)<5|B=r+t5JFr>(+J`^ z!KhNiuQQ1wqWZ#0^}-P1jH=r1b*5VA`#WY*LEo=RDI(!ko-NnIh2+rq#1x!x%4_DT zMbN<^SYraCfbQl79`1eEhUNyzuNipian=P9u_rfv#u_e1o?Cswq!AH&$}Xy1*Nr{Z z&Ny(QpdI(PAyNxv&y^b?zZ|xviiIo!n{P@;ti+UX7#_%;7T|{rX?iWCA~|+M?EYTD zwuv*;WJaL!Ca=4iW z6+3ovZ^Vu0`pJ%+`pJ%+`srfT9N8(@pzfP0Dg(dW$8<4B_o*%iO>E9B48gfkRj zC+aIWAQBg=X5*|ON#u((xNnoAdYK|Yg==Fcl@EC~GN^Yu)ac&YPxPn;oIJ!^{bSj} zIHQ`5rfcro?2x;7oK@ZpsW0!w4VIi=y@DliYG@-teOQ9vU)fZPxX`bv7S%4vJUtRgL69sE>(i+~AD=3R# z*K8QIENO&rT+3qH4-^YlacC51G&oCI_)bHvG0^s9b7RxYT)L(T7Mk)sIZIldo0o-A zO@{}D3fT!Du&*p>1b$r0;}Kk$_}YZ!YK(gF(7_}a@jL}f8kq<#96WF?rxV?U+z`{l zW7)|fdjoG%C5@b8T={s+ge8p>4_AE{>Vb~EQ@OEWFmhYBuyv@~4iqeF+*TV; z^RUFJ%|&pB|_DKwo?U+TDl4L zlr#7foc~NCJ;Sv)y0POZe0R%|#<;?@@@gn))f&~~*g%`3oPcP2xgeRKGL@7b zG?(dIH7JN3F_bh@S3Tf3OO00I#P-p}aoNpQo=#LIk&=!Owl_C62F{ze+>#T(*nw9!H~xhYbfkV9S;Kd4XaaKC~Q6uQeNiqS3-* zW_S1AEY_k!41-Ph(`~olTN?a*>+W_VJuFcJ!E9dE0I*g7gC#9I)*y3(OuF+0 zCC(ZZx#$&9Emc<`_*(pF!NcQsZ|mb@ZlbVMBDd>p;Jpq=ER`tCMRXaj z2QEvl{eyYHPIScYEX~jt980xVRb%nkYagD~3#_z*$nKMsW zPS@+;3-0mjsc30Z5pt?|R*f{kzzjalNP>ED0AZhkwOvV<&99Qp%8qtCYCKW>xbwlA3Y@tLri!bB=~!OdeQ5 z(*FvH2^gynn{N+F) zKQ*z>nkeo}pj`>?^QyrxrtRVES*zT(3j1Ga(8iUbUdk7hOOxNE_tHYBp}WIVYKsT` zWeQiz-C89j(-%n@T^$(BjTF%gt+@NfR$oq}P_SbAQ9@o-3MLYyhde16N-aUr5Pov@ z;MFSkVps28qh{|967m3G4;V0VcpqTzGhoE#{H_0gLA~ey7tE<*cBgyFOoG=>Zp9$) zVYuc`{O96x7^_6yH{g>IKP9hz;-zo)o;uQbQo%PX_!jl~2MRJ{^lLqKk)f2tJ5lrH z8jd;6rtea%|4@Bya9Y3E$)8c{>&4Dy=l&<1n~eKj*#VNh*d-mp7HwW8QAV&^nvvt! zPaVMbK0x>i_AIs>dHyYf?c4&|bDzFYrjL!#+UMnXb#;mTPsk5v=TpP@o{xWcz-NBq z?FKN49=tO8uk=Ox@ZvYA^P79G`0`x)4jn$)B=w1&>bdguZ~m7pvZsyxz|2WsHg)^? z&F}v&8Za$ikB5QY!CTmOWM_$AXPl^4jC*tA(%skJJJ5HuziYqr?;qIL(<4X6vG$wf z-J$WKRT#6fcVo3Kecj!1=xp}1H8?$C$t_3rALzA)rLT~~OsY;z$dSJO!Tvpm2Bj-M zhJzHALg}|A^J7!oHkZB$er=N@*$JyKko!GLdiDj0+$?jO!Xdj5_nfxYY8?!gSORolG zYA-gJ*j$NC1DeWf7aO)EHd-l}7jlEGUZw*w(+5nli!@o{kt`lP)o>4+EUW1) zCiY@4HXc3Ia3m9(eq~}W@nYlAQw^6hvFU*(_EIl49zE4?P7|BHYGS9o*m(3*!;MXB zdbNqY%!`djPc?LJV$=Uk?B!l;JbJ3(cQ*F3Tpb|9ja41Pkpvd^kCC$pl>L!zt%8j@ z(eznF;AX(yf*)5rr9$Iw)nZb8z89JLdyTlyoNyJyLDI*I$u~ky>BlOFUk}I^6YAg0 z4UAe=aog$CQaNp4BWGnQ`L02o?(_c5>kZUEKh_jHc#^>pKdvcwtnc+)OQc2_h$3+w zj=O&H!w}XMrGjZslb@-QWSn|N1%2#Nk5a45oyu50# zvf_%0=Qi1Qo-rFyG7;h)bk(hcpUvRUfx+y8A_QB6LtZR{YXYmf*C)>n;BWW6QvTG} z&vfd8;i%7|PT=s>DS2DCurW~T|Ml``R6tT8nz0mfzehNfG;nlDq0)HYgZDk88Ea5ru%``;?D?0`z@ZX!Ou^qwZOp4J+xa#E9 zy#XI?MC}_H$qiXLux8rZ+Bz~$bV4eRA0y>PMs84t!&K~TRr!4D4f4v3kN+;o*_X}v z&F|j?4RA`K)#(`gKi-@;Xie^JaZaaKakq-I5$8BgJ=|^L$l~0_-6PKJoZmP$xSr>L zWsE01Mm&DMdHrT;z_dRzk2li(oJwhbPA8nWxEsfi%@l^aYZ;U^D4eyJy)bcEkHWY^ YpJ^_CZ^ri)l&$4w*1hRdZ;U_v56y~#L;wH) literal 0 HcmV?d00001