From: Greg Woolsey Date: Wed, 26 Jul 2017 22:19:58 +0000 (+0000) Subject: Fix data validation value list evaluation X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=e1f37388fc8787cba6b88e1cf3a662225d7124b7;p=poi.git Fix data validation value list evaluation One of my users found that my initial implementation was lacking a core distinction - most evaluations expect a single result, and "unwrap" 2/3D ValueEval results to a single value based on the input row/column. However, data validation list formulas explicitly are expected to return a 2D ValueEval. This worked when the formula was simple and evaluated to a single Ptg, but only returned one value when the formula was more complex, or referenced a named range defined as a complex formula. This change teaches WorkbookEvaluator about the distinction, by way of a new attribute for FormulaType. There is room for discussion over how it is implemented, but this works for me. Includes the failing workbook we had as a new unit test. While I was in FormulaType I went ahead and removed the deprecated, unused, and redundant code marked for removal in 3.17. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1803121 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/src/java/org/apache/poi/ss/formula/DataValidationEvaluator.java b/src/java/org/apache/poi/ss/formula/DataValidationEvaluator.java index dd8f0faae2..46ebe13304 100644 --- a/src/java/org/apache/poi/ss/formula/DataValidationEvaluator.java +++ b/src/java/org/apache/poi/ss/formula/DataValidationEvaluator.java @@ -196,7 +196,8 @@ public class DataValidationEvaluator { } } else if (formula != null) { // evaluate formula for cell refs then get their values - ValueEval eval = context.getEvaluator().getWorkbookEvaluator().evaluate(formula, context.getTarget(), context.getRegion()); + // note this should return the raw formula result, not the "unwrapped" version that returns a single value. + ValueEval eval = context.getEvaluator().getWorkbookEvaluator().evaluateList(formula, context.getTarget(), context.getRegion()); // formula is a StringEval if the validation is by a fixed list. Use the explicit list later. // there is no way from the model to tell if the list is fixed values or formula based. if (eval instanceof TwoDEval) { @@ -344,6 +345,7 @@ public class DataValidationEvaluator { * @see org.apache.poi.ss.formula.DataValidationEvaluator.ValidationEnum#isValidValue(org.apache.poi.ss.usermodel.Cell, org.apache.poi.ss.usermodel.DataValidationConstraint, org.apache.poi.ss.formula.WorkbookEvaluator) */ public boolean isValidValue(Cell cell, DataValidationContext context) { + // unwrapped single value ValueEval comp = context.getEvaluator().getWorkbookEvaluator().evaluate(context.getFormula1(), context.getTarget(), context.getRegion()); if (comp instanceof RefEval) { comp = ((RefEval) comp).getInnerValueEval(((RefEval) comp).getFirstSheetIndex()); @@ -419,6 +421,7 @@ public class DataValidationEvaluator { } catch (NumberFormatException e) { // must be an expression, then. Overloading by Excel in the file formats. } + // note the call to the "unwrapped" version, which returns a single value ValueEval eval = context.getEvaluator().getWorkbookEvaluator().evaluate(formula, context.getTarget(), context.getRegion()); if (eval instanceof RefEval) { eval = ((RefEval) eval).getInnerValueEval(((RefEval) eval).getFirstSheetIndex()); diff --git a/src/java/org/apache/poi/ss/formula/FormulaType.java b/src/java/org/apache/poi/ss/formula/FormulaType.java index 45d26ae885..fa154e6d42 100644 --- a/src/java/org/apache/poi/ss/formula/FormulaType.java +++ b/src/java/org/apache/poi/ss/formula/FormulaType.java @@ -27,7 +27,7 @@ import org.apache.poi.util.Internal; @Internal public enum FormulaType { /** Regular cell formula */ - CELL(0), + CELL(true), /** * A Shared Formula ("{=SUM(A1:E1*{1,2,3,4,5}}") @@ -35,46 +35,52 @@ public enum FormulaType { * Similar to an array formula, but stored in a SHAREDFMLA instead of ARRAY record as a file size optimization. * See Section 4.8 of https://www.openoffice.org/sc/excelfileformat.pdf */ - SHARED(1), + SHARED(true), /** * An Array formula ("{=SUM(A1:E1*{1,2,3,4,5}}") * https://support.office.com/en-us/article/Guidelines-and-examples-of-array-formulas-7D94A64E-3FF3-4686-9372-ECFD5CAA57C7 */ - ARRAY(2), + ARRAY(false), /** Conditional formatting */ - CONDFORMAT(3), + CONDFORMAT(true), /** Named range */ - NAMEDRANGE(4), + NAMEDRANGE(false), /** * This constant is currently very specific. The exact differences from general data * validation formulas or conditional format formulas is not known yet */ - DATAVALIDATION_LIST(5); + DATAVALIDATION_LIST(false); - /** @deprecated POI 3.15 beta 3. */ - private final int code; + /** formula is expected to return a single value vs. multiple values */ + private final boolean isSingleValue ; /** * @since POI 3.15 beta 3. - * @deprecated POI 3.15 beta 3. - * Formula type code doesn't mean anything. Only in this class for transitioning from a class with int constants to a true enum. - * Remove hard-coded numbers from the enums above. */ - private FormulaType(int code) { - this.code = code; + */ + private FormulaType(boolean singleValue) { + this.isSingleValue = singleValue; + } + + /** + * @return true if this formula type only returns single values, false if it can return multiple values (arrays, ranges, etc.) + */ + public boolean isSingleValue() { + return isSingleValue; } /** + * Used to transition from ints (possibly stored in the Excel file) to FormulaTypes. + * @param code + * @return FormulaType + * @throws IllegalArgumentException if code is out of range * @since POI 3.15 beta 3. - * @deprecated POI 3.15 beta 3. Used to transition code from ints to FormulaTypes. */ public static FormulaType forInt(int code) { - for (FormulaType type : values()) { - if (type.code == code) { - return type; - } + if (code >= 0 && code < values().length) { + return values()[code]; } throw new IllegalArgumentException("Invalid FormulaType code: " + code); } diff --git a/src/java/org/apache/poi/ss/formula/OperationEvaluationContext.java b/src/java/org/apache/poi/ss/formula/OperationEvaluationContext.java index d7345f7cbe..5deb34febe 100644 --- a/src/java/org/apache/poi/ss/formula/OperationEvaluationContext.java +++ b/src/java/org/apache/poi/ss/formula/OperationEvaluationContext.java @@ -53,15 +53,22 @@ public final class OperationEvaluationContext { private final int _columnIndex; private final EvaluationTracker _tracker; private final WorkbookEvaluator _bookEvaluator; - + private final boolean _isSingleValue; + public OperationEvaluationContext(WorkbookEvaluator bookEvaluator, EvaluationWorkbook workbook, int sheetIndex, int srcRowNum, int srcColNum, EvaluationTracker tracker) { + this(bookEvaluator, workbook, sheetIndex, srcRowNum, srcColNum, tracker, true); + } + + public OperationEvaluationContext(WorkbookEvaluator bookEvaluator, EvaluationWorkbook workbook, int sheetIndex, int srcRowNum, + int srcColNum, EvaluationTracker tracker, boolean isSingleValue) { _bookEvaluator = bookEvaluator; _workbook = workbook; _sheetIndex = sheetIndex; _rowIndex = srcRowNum; _columnIndex = srcColNum; _tracker = tracker; + _isSingleValue = isSingleValue; } public EvaluationWorkbook getWorkbook() { @@ -411,6 +418,14 @@ public final class OperationEvaluationContext { return _sheetIndex; } + /** + * default true + * @return flag indicating whether evaluation should "unwrap" the result to a single value based on the context row/column + */ + public boolean isSingleValue() { + return _isSingleValue; + } + private ValueEval getExternalNameXEval(ExternalName externName, String workbookName) { try { // Fetch the workbook this refers to, and the name as defined with that diff --git a/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java b/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java index 7b870dc33c..dfad1873d2 100644 --- a/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java +++ b/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java @@ -44,11 +44,11 @@ import org.apache.poi.util.Internal; import org.apache.poi.util.POILogFactory; import org.apache.poi.util.POILogger; /** - * Evaluates formula cells.

+ * Evaluates formula cells.

* * For performance reasons, this class keeps a cache of all previously calculated intermediate * cell values. Be sure to call {@link #clearAllCachedResultValues()} if any workbook cells are changed between - * calls to evaluate~ methods on this class.
+ * calls to evaluate~ methods on this class.
* * For POI internal use only * @@ -529,7 +529,15 @@ public final class WorkbookEvaluator { if (!stack.isEmpty()) { throw new IllegalStateException("evaluation stack not empty"); } - ValueEval result = dereferenceResult(value, ec.getRowIndex(), ec.getColumnIndex()); + + // "unwrap" result to just the value relevant for the source cell if needed + ValueEval result; + if (ec.isSingleValue()) { + result = dereferenceResult(value, ec.getRowIndex(), ec.getColumnIndex()); + } else { + result = value; + } + if (dbgEvaluationOutputIndent > 0) { EVAL_LOG.log(POILogger.INFO, dbgIndentStr + "finshed eval of " + new CellReference(ec.getRowIndex(), ec.getColumnIndex()).formatAsString() @@ -595,7 +603,7 @@ public final class WorkbookEvaluator { /** * returns an appropriate Eval impl instance for the Ptg. The Ptg must be * one of: Area3DPtg, AreaPtg, ReferencePtg, Ref3DPtg, IntPtg, NumberPtg, - * StringPtg, BoolPtg
special Note: OperationPtg subtypes cannot be + * StringPtg, BoolPtg
special Note: OperationPtg subtypes cannot be * passed here! */ private ValueEval getEvalForPtg(Ptg ptg, OperationEvaluationContext ec) { @@ -721,16 +729,28 @@ public final class WorkbookEvaluator { * Evaluate a formula outside a cell value, e.g. conditional format rules or data validation expressions * * @param formula to evaluate - * @param ref defines the sheet and optionally row/column base for the formula, if it is relative + * @param ref defines the optional sheet and row/column base for the formula, if it is relative * @return value - * @throws IllegalArgumentException if ref does not define a sheet name to evaluate the formula on. */ public ValueEval evaluate(String formula, CellReference ref) { final String sheetName = ref == null ? null : ref.getSheetName(); - if (sheetName == null) throw new IllegalArgumentException("Sheet name is required"); - final int sheetIndex = getWorkbook().getSheetIndex(sheetName); - final OperationEvaluationContext ec = new OperationEvaluationContext(this, getWorkbook(), sheetIndex, ref.getRow(), ref.getCol(), new EvaluationTracker(_cache)); - Ptg[] ptgs = FormulaParser.parse(formula, (FormulaParsingWorkbook) getWorkbook(), FormulaType.CELL, sheetIndex, ref.getRow()); + int sheetIndex; + if (sheetName == null) { + sheetIndex = -1; // workbook scope only + } else { + sheetIndex = getWorkbook().getSheetIndex(sheetName); + } + int rowIndex = ref == null ? -1 : ref.getRow(); + short colIndex = ref == null ? -1 : ref.getCol(); + final OperationEvaluationContext ec = new OperationEvaluationContext( + this, + getWorkbook(), + sheetIndex, + rowIndex, + colIndex, + new EvaluationTracker(_cache) + ); + Ptg[] ptgs = FormulaParser.parse(formula, (FormulaParsingWorkbook) getWorkbook(), FormulaType.CELL, sheetIndex, rowIndex); return evaluateNameFormula(ptgs, ec); } @@ -739,6 +759,8 @@ public final class WorkbookEvaluator { * such as some data validation and conditional format expressions, when those constraints apply * to contiguous cells. When a relative formula is used, it must be evaluated by shifting by the target * offset position relative to the top left of the range. + *

+ * Returns a single value e.g. a cell formula result or boolean value for conditional formatting. * * @param formula * @param target cell context for the operation @@ -747,15 +769,37 @@ public final class WorkbookEvaluator { * @throws IllegalArgumentException if target does not define a sheet name to evaluate the formula on. */ public ValueEval evaluate(String formula, CellReference target, CellRangeAddressBase region) { + return evaluate(formula, target, region, FormulaType.CELL); + } + + /** + * Some expressions need to be evaluated in terms of an offset from the top left corner of a region, + * such as some data validation and conditional format expressions, when those constraints apply + * to contiguous cells. When a relative formula is used, it must be evaluated by shifting by the target + * offset position relative to the top left of the range. + *

+ * Returns a ValueEval that may be one or more values, such as the allowed values for a data validation constraint. + * + * @param formula + * @param target cell context for the operation + * @param region containing the cell + * @return ValueEval for one or more values + * @throws IllegalArgumentException if target does not define a sheet name to evaluate the formula on. + */ + public ValueEval evaluateList(String formula, CellReference target, CellRangeAddressBase region) { + return evaluate(formula, target, region, FormulaType.DATAVALIDATION_LIST); + } + + private ValueEval evaluate(String formula, CellReference target, CellRangeAddressBase region, FormulaType formulaType) { final String sheetName = target == null ? null : target.getSheetName(); if (sheetName == null) throw new IllegalArgumentException("Sheet name is required"); final int sheetIndex = getWorkbook().getSheetIndex(sheetName); - Ptg[] ptgs = FormulaParser.parse(formula, (FormulaParsingWorkbook) getWorkbook(), FormulaType.CELL, sheetIndex, target.getRow()); + Ptg[] ptgs = FormulaParser.parse(formula, (FormulaParsingWorkbook) getWorkbook(), formulaType, sheetIndex, target.getRow()); adjustRegionRelativeReference(ptgs, target, region); - final OperationEvaluationContext ec = new OperationEvaluationContext(this, getWorkbook(), sheetIndex, target.getRow(), target.getCol(), new EvaluationTracker(_cache)); + final OperationEvaluationContext ec = new OperationEvaluationContext(this, getWorkbook(), sheetIndex, target.getRow(), target.getCol(), new EvaluationTracker(_cache), formulaType.isSingleValue()); return evaluateNameFormula(ptgs, ec); } diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidation.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidation.java index e0da16ba29..ae234bda28 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidation.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidation.java @@ -22,6 +22,9 @@ import java.io.IOException; import java.math.BigDecimal; import java.util.List; +import org.apache.poi.ss.formula.DataValidationEvaluator; +import org.apache.poi.ss.formula.WorkbookEvaluator; +import org.apache.poi.ss.formula.eval.ValueEval; import org.apache.poi.ss.usermodel.BaseTestDataValidation; import org.apache.poi.ss.usermodel.Cell; import org.apache.poi.ss.usermodel.CellType; @@ -341,4 +344,13 @@ public class TestXSSFDataValidation extends BaseTestDataValidation { final XSSFDataValidation validation = (XSSFDataValidation) dataValidationHelper.createValidation(constraint, new CellRangeAddressList(0, 0, 0, 0)); return validation; } + + @Test + public void testTableBasedValidationList() { + XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("dataValidationTableRange.xlsx"); + XSSFFormulaEvaluator fEval = wb.getCreationHelper().createFormulaEvaluator(); + DataValidationEvaluator dve = new DataValidationEvaluator(wb, fEval); + List values = dve.getValidationValuesForCell(new CellReference("County Ranking", 8, 6, false, false)); + assertEquals("wrong # of valid values", 32, values.size()); + } } diff --git a/test-data/spreadsheet/dataValidationTableRange.xlsx b/test-data/spreadsheet/dataValidationTableRange.xlsx new file mode 100644 index 0000000000..76e24344e2 Binary files /dev/null and b/test-data/spreadsheet/dataValidationTableRange.xlsx differ