<!-- Don't forget to update status.xml too! -->
<release version="3.5-beta4" date="2008-??-??">
+ <action dev="POI-DEVELOPERS" type="fix">Fixed problem with linking shared formulas when ranges overlap</action>
<action dev="POI-DEVELOPERS" type="fix">45784 - More fixes to SeriesTextRecord</action>
<action dev="POI-DEVELOPERS" type="fix">46033 - fixed TableCell to correctly set text type</action>
<action dev="POI-DEVELOPERS" type="fix">46122 - fixed Picture.draw to skip rendering if picture data was not found</action>
<!-- Don't forget to update changes.xml too! -->
<changes>
<release version="3.5-beta4" date="2008-??-??">
+ <action dev="POI-DEVELOPERS" type="fix">Fixed problem with linking shared formulas when ranges overlap</action>
<action dev="POI-DEVELOPERS" type="fix">45784 - More fixes to SeriesTextRecord</action>
<action dev="POI-DEVELOPERS" type="fix">46033 - fixed TableCell to correctly set text type</action>
<action dev="POI-DEVELOPERS" type="fix">46122 - fixed Picture.draw to skip rendering if picture data was not found</action>
return field_8_parsed_expr.getTokens();
}
+ public Formula getFormula() {
+ return field_8_parsed_expr;
+ }
+
public void setParsedExpression(Ptg[] ptgs) {
field_8_parsed_expr = Formula.create(ptgs);
}
result.field_7_parsed_expr = field_7_parsed_expr.copy();
return result;
}
+ public boolean isFormulaSame(SharedFormulaRecord other) {
+ return field_7_parsed_expr.isSame(other.field_7_parsed_expr);
+ }
}
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
+ (hasCachedStringFlag ? "" : "not ") + " set");
}
- _formulaRecord = formulaRec;
- _sharedValueManager = svm;
- _stringRecord = stringRec;
if (formulaRec.isSharedFormula()) {
- _sharedFormulaRecord = svm.linkSharedFormulaRecord(this);
- if (_sharedFormulaRecord == null) {
+ CellReference cr = new CellReference(formulaRec.getRow(), formulaRec.getColumn());
+
+ CellReference firstCell = formulaRec.getFormula().getExpReference();
+ if (firstCell == null) {
handleMissingSharedFormulaRecord(formulaRec);
+ } else {
+ _sharedFormulaRecord = svm.linkSharedFormulaRecord(firstCell, this);
}
}
+ _formulaRecord = formulaRec;
+ _sharedValueManager = svm;
+ _stringRecord = stringRec;
}
/**
* Sometimes the shared formula flag "seems" to be erroneously set (because the corresponding
public void visitContainedRecords(RecordVisitor rv) {
rv.visitRecord(_formulaRecord);
- // TODO - only bother with this if array or table formula
- Record sharedFormulaRecord = _sharedValueManager.getRecordForFirstCell(_formulaRecord);
- if (sharedFormulaRecord != null) {
- rv.visitRecord(sharedFormulaRecord);
+ 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);
+ }
}
if (_stringRecord != null) {
rv.visitRecord(_stringRecord);
package org.apache.poi.hssf.record.aggregates;
+import java.util.Arrays;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
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;
/**
* Manages various auxiliary records while constructing a
* <li>{@link ArrayRecord}s</li>
* <li>{@link TableRecord}s</li>
* </ul>
- *
+ *
* @author Josh Micich
*/
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 final FormulaRecordAggregate[] _frAggs;
+ private FormulaRecordAggregate[] _frAggs;
private int _numberOfFormulas;
public SharedValueGroup(SharedValueRecordBase svr) {
}
public void add(FormulaRecordAggregate agg) {
+ 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;
+ }
_frAggs[_numberOfFormulas++] = agg;
}
}
}
- public boolean isInRange(int rowIx, int columnIx) {
- return _svr.isInRange(rowIx, columnIx);
- }
-
public SharedValueRecordBase getSVR() {
return _svr;
}
* that is not the top-left corner of the range.
* @return <code>true</code> if this is the first formula cell in the group
*/
- public boolean isFirstCell(int row, int column) {
- // hack for the moment, just check against the first formula that
- // came in through the add() method.
- FormulaRecordAggregate fra = _frAggs[0];
- return fra.getRow() == row && fra.getColumn() == column;
+ 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("]");
+ return sb.toString();
}
-
}
public static final SharedValueManager EMPTY = new SharedValueManager(
_groupsBySharedFormulaRecord = m;
}
+ /**
+ * @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
+ * that this code does not inadvertently collect <tt>SharedFormulaRecord</tt>s from any other
+ * 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) {
/**
- * @return <code>null</code> if the specified formula does not have any corresponding
- * {@link SharedFormulaRecord}
+ * @param firstCell as extracted from the {@link ExpPtg} from the cell's formula.
+ * @return never <code>null</code>
*/
- public SharedFormulaRecord linkSharedFormulaRecord(FormulaRecordAggregate agg) {
- FormulaRecord formula = agg.getFormulaRecord();
- int row = formula.getRow();
- int column = formula.getColumn();
- // Traverse the list of shared formulas in
- // reverse order, and try to find the correct one
- // for us
-
- SharedValueGroup[] groups = getGroups();
+ public SharedFormulaRecord linkSharedFormulaRecord(CellReference firstCell, FormulaRecordAggregate agg) {
+
+ SharedValueGroup result = findGroup(getGroups(), firstCell);
+ result.add(agg);
+ return (SharedFormulaRecord) result.getSVR();
+ }
+
+ private static SharedValueGroup findGroup(SharedValueGroup[] 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 svr = groups[i];
- if (svr.isInRange(row, column)) {
- svr.add(agg);
- return (SharedFormulaRecord) svr.getSVR();
+ SharedValueGroup svg = groups[i];
+ if (svg.getSVR().isFirstCell(row, column)) {
+ return svg;
}
}
- return null;
+ // 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;
+ }
+
+ /**
+ * 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 <code>true</code> 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() {
if (_groups == null) {
SharedValueGroup[] groups = new SharedValueGroup[_groupsBySharedFormulaRecord.size()];
_groupsBySharedFormulaRecord.values().toArray(groups);
+ Arrays.sort(groups, SVGComparator); // make search behaviour more deterministic
_groups = groups;
-
}
return _groups;
}
+ 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();
+
+ int cmp;
+ cmp = rangeA.getFirstRow() - rangeB.getFirstRow();
+ if (cmp != 0) {
+ return cmp;
+ }
+ cmp = rangeA.getFirstColumn() - rangeB.getFirstColumn();
+ if (cmp != 0) {
+ return cmp;
+ }
+ return 0;
+ }
+ };
/**
- * Note - does not return SharedFormulaRecords currently, because the corresponding formula
- * records have been converted to 'unshared'. POI does not attempt to re-share formulas. On
- * the other hand, there is no such conversion for array or table formulas, so this method
- * returns the TABLE or ARRAY record (if it should be written after the specified
- * formulaRecord.
- *
- * @return the TABLE or ARRAY record for this formula cell, if it is the first cell of a
- * table or array region.
+ * 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.
+ *
+ * @return the SHRFMLA, TABLE or ARRAY record for this formula cell, if it is the first cell of a
+ * table or array region. <code>null</code> if
*/
- public SharedValueRecordBase getRecordForFirstCell(FormulaRecord formulaRecord) {
- int row = formulaRecord.getRow();
- int column = formulaRecord.getColumn();
- for (int i = 0; i < _tableRecords.length; i++) {
- TableRecord tr = _tableRecords[i];
- if (tr.isFirstCell(row, column)) {
- return tr;
+ public SharedValueRecordBase getRecordForFirstCell(CellReference firstCell, FormulaRecordAggregate agg) {
+ 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;
+ 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
}
SharedValueGroup[] groups = getGroups();
for (int i = 0; i < groups.length; i++) {
SharedValueGroup svg = groups[i];
- if (svg.isFirstCell(row, column)) {
- return svg.getSVR();
+ SharedValueRecordBase svr = svg.getSVR();
+ if (svr.isFirstCell(row, column)) {
+ if (svg.isFirstMember(agg)) {
+ return svr;
+ }
+ return null;
}
}
return null;
}
/**
- * Converts all {@link FormulaRecord}s handled by <tt>sharedFormulaRecord</tt>
+ * Converts all {@link FormulaRecord}s handled by <tt>sharedFormulaRecord</tt>
* to plain unshared formulas
*/
public void unlink(SharedFormulaRecord sharedFormulaRecord) {
package org.apache.poi.ss.formula;\r
\r
+import java.util.Arrays;\r
+\r
+import org.apache.poi.hssf.record.ArrayRecord;\r
+import org.apache.poi.hssf.record.SharedFormulaRecord;\r
+import org.apache.poi.hssf.record.TableRecord;\r
+import org.apache.poi.hssf.record.formula.ExpPtg;\r
import org.apache.poi.hssf.record.formula.Ptg;\r
+import org.apache.poi.hssf.record.formula.TblPtg;\r
+import org.apache.poi.hssf.util.CellReference;\r
+import org.apache.poi.util.LittleEndian;\r
import org.apache.poi.util.LittleEndianByteArrayInputStream;\r
import org.apache.poi.util.LittleEndianInput;\r
import org.apache.poi.util.LittleEndianOutput;\r
// OK to return this for the moment because currently immutable\r
return this;\r
}\r
+ \r
+ /**\r
+ * Gets the locator for the corresponding {@link SharedFormulaRecord}, {@link ArrayRecord} or\r
+ * {@link TableRecord} if this formula belongs to such a grouping. The {@link CellReference}\r
+ * returned by this method will match the top left corner of the range of that grouping. \r
+ * The return value is usually not the same as the location of the cell containing this formula.\r
+ * \r
+ * @return the firstRow & firstColumn of an array formula or shared formula that this formula\r
+ * belongs to. <code>null</code> if this formula is not part of an array or shared formula.\r
+ */\r
+ public CellReference getExpReference() {\r
+ byte[] data = _byteEncoding;\r
+ if (data.length != 5) {\r
+ // tExp and tTbl are always 5 bytes long, and the only ptg in the formula\r
+ return null;\r
+ }\r
+ switch (data[0]) {\r
+ case ExpPtg.sid:\r
+ break;\r
+ case TblPtg.sid:\r
+ break;\r
+ default:\r
+ return null;\r
+ }\r
+ int firstRow = LittleEndian.getUShort(data, 1);\r
+ int firstColumn = LittleEndian.getUShort(data, 3);\r
+ return new CellReference(firstRow, firstColumn);\r
+ }\r
+ public boolean isSame(Formula other) {\r
+ return Arrays.equals(_byteEncoding, other._byteEncoding);\r
+ }\r
}\r
_firstCol = firstCol;
_lastCol = lastCol;
}
- private static boolean isValid(int firstRow, int lastRow, int firstColumn, int lastColumn)
- {
+ private static boolean isValid(int firstRow, int lastRow, int firstColumn, int lastColumn) {
if(lastRow < 0 || lastRow > LAST_ROW_INDEX) {
return false;
}
}
public final String toString() {
- return getClass().getName() + " ["+_firstRow+", "+_lastRow+", "+_firstCol+", "+_lastCol+"]";
+ CellReference crA = new CellReference(_firstRow, _firstCol);
+ CellReference crB = new CellReference(_lastRow, _lastCol);
+ return getClass().getName() + " [" + crA.formatAsString() + ":" + crB.formatAsString() +"]";
}
}
result.addTestSuite(TestColumnInfoRecordsAggregate.class);\r
result.addTestSuite(TestFormulaRecordAggregate.class);\r
result.addTestSuite(TestRowRecordsAggregate.class);\r
+ result.addTestSuite(TestSharedValueManager.class);\r
result.addTestSuite(TestValueRecordsAggregate.class);\r
return result;\r
}\r
--- /dev/null
+/* ====================================================================
+ 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.record.aggregates;
+
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Collection;
+import java.util.HashMap;
+
+import junit.framework.AssertionFailedError;
+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.HSSFSheet;
+import org.apache.poi.hssf.usermodel.HSSFWorkbook;
+import org.apache.poi.hssf.usermodel.RecordInspector;
+
+/**
+ * Tests for {@link SharedValueManager}
+ *
+ * @author Josh Micich
+ */
+public final class TestSharedValueManager extends TestCase {
+
+ /**
+ * This Excel workbook contains two sheets that each have a pair of overlapping shared formula
+ * ranges. The first sheet has one row and one column shared formula ranges which intersect.
+ * The second sheet has two column shared formula ranges - one contained within the other.
+ * These shared formula ranges were created by fill-dragging a single cell formula across the
+ * desired region. The larger shared formula ranges were placed first.<br/>
+ *
+ * There are probably many ways to produce similar effects, but it should be noted that Excel
+ * is quite temperamental in this regard. Slight variations in technique can cause the shared
+ * formulas to spill out into plain formula records (which would make these tests pointless).
+ *
+ */
+ private static final String SAMPLE_FILE_NAME = "overlapSharedFormula.xls";
+ /**
+ * Some of these bugs are intermittent, and the test author couldn't think of a way to write
+ * test code to hit them bug deterministically. The reason for the unpredictability is that
+ * the bugs depended on the {@link SharedFormulaRecord}s being searched in a particular order.
+ * At the time of writing of the test, the order was being determined by the call to {@link
+ * Collection#toArray(Object[])} on {@link HashMap#values()} where the items in the map were
+ * using default {@link Object#hashCode()}<br/>
+ */
+ private static final int MAX_ATTEMPTS=5;
+
+ /**
+ * This bug happened when there were two or more shared formula ranges that overlapped. POI
+ * would sometimes associate formulas in the overlapping region with the wrong shared formula
+ */
+ public void testPartiallyOverlappingRanges() throws IOException {
+ Record[] records;
+
+ int attempt=1;
+ do {
+ HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook(SAMPLE_FILE_NAME);
+
+ HSSFSheet sheet = wb.getSheetAt(0);
+ RecordInspector.getRecords(sheet, 0);
+ assertEquals("1+1", sheet.getRow(2).getCell(0).getCellFormula());
+ if ("1+1".equals(sheet.getRow(3).getCell(0).getCellFormula())) {
+ throw new AssertionFailedError("Identified bug - wrong shared formula record chosen"
+ + " (attempt " + attempt + ")");
+ }
+ assertEquals("2+2", sheet.getRow(3).getCell(0).getCellFormula());
+ records = RecordInspector.getRecords(sheet, 0);
+ } while (attempt++ < MAX_ATTEMPTS);
+
+ int count=0;
+ for (int i = 0; i < records.length; i++) {
+ if (records[i] instanceof SharedFormulaRecord) {
+ count++;
+ }
+ }
+ assertEquals(2, count);
+ }
+
+ /**
+ * This bug occurs for similar reasons to the bug in {@link #testPartiallyOverlappingRanges()}
+ * but the symptoms are much uglier - serialization fails with {@link NullPointerException}.<br/>
+ */
+ public void testCompletelyOverlappedRanges() throws IOException {
+ Record[] records;
+
+ int attempt=1;
+ do {
+ HSSFWorkbook wb = HSSFTestDataSamples.openSampleWorkbook(SAMPLE_FILE_NAME);
+
+ HSSFSheet sheet = wb.getSheetAt(1);
+ try {
+ records = RecordInspector.getRecords(sheet, 0);
+ } catch (NullPointerException e) {
+ throw new AssertionFailedError("Identified bug " +
+ "- cannot reserialize completely overlapped shared formula"
+ + " (attempt " + attempt + ")");
+ }
+ } while (attempt++ < MAX_ATTEMPTS);
+
+ int count=0;
+ for (int i = 0; i < records.length; i++) {
+ if (records[i] instanceof SharedFormulaRecord) {
+ count++;
+ }
+ }
+ assertEquals(2, count);
+ }
+}
if (true) { // set to false to output test files
return;
}
+ System.setProperty("poi.keep.tmp.files", "true");
File file;
try {
file = TempFile.createTempFile(simpleFileName + "#", ".xls");
projects / poi.git / commitdiff