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

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());

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. */
+    /** formula is expected to return a single value vs. multiple values */
-    private final int code;
+    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.
+    private FormulaType(boolean singleValue) {
-     * Remove hard-coded numbers from the enums above. */
+        this.isSingleValue = singleValue;
-    private FormulaType(int code) {
-        this.code = code;
     }
     
     /**
+     * @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 <code>int</code>s (possibly stored in the Excel file) to <code>FormulaType</code>s.
+     * @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 <code>int</code>s to <code>FormulaType</code>s.
      */
     public static FormulaType forInt(int code) {
-        for (FormulaType type : values()) {
+        if (code >= 0 && code < values().length) {
-            if (type.code == code) {
+            return values()[code];
-                return type;
-            }
         }
         throw new IllegalArgumentException("Invalid FormulaType code: " + code);
     }

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

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.<p>
+ * Evaluates formula cells.<p/>
  *
  * 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.<br>
+ * calls to evaluate~ methods on this class.<br/>
  *
  * 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 <br>special Note: OperationPtg subtypes cannot be
+     * StringPtg, BoolPtg <br/>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");
+        int sheetIndex;
-        final int sheetIndex = getWorkbook().getSheetIndex(sheetName);
+        if (sheetName == null) {
-        final OperationEvaluationContext ec = new OperationEvaluationContext(this, getWorkbook(), sheetIndex, ref.getRow(), ref.getCol(), new EvaluationTracker(_cache));
+            sheetIndex = -1; // workbook scope only
-        Ptg[] ptgs = FormulaParser.parse(formula, (FormulaParsingWorkbook) getWorkbook(), FormulaType.CELL, sheetIndex, ref.getRow());
+        } 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.
+     * <p>
+     * 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.
+     * <p>
+     * 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);
     }
     

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<ValueEval> values = dve.getValidationValuesForCell(new CellReference("County Ranking", 8, 6, false, false));
+        assertEquals("wrong # of valid values", 32, values.size());
+    }
 }