Bug 61431 - Conditional formatting evaluation ignores undefined cells

now evaluating based on cell references instead, and watching out for undefined cells in rules that require a cell.

Added unit test based on previously failing file.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1805245 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/poi/ss/formula/ConditionalFormattingEvaluator.java

@@ -150,57 +150,16 @@ public class ConditionalFormattingEvaluator {
      *         or null if none apply
      */
     public List<EvaluationConditionalFormatRule> getConditionalFormattingForCell(final CellReference cellRef) {
-        String sheetName = cellRef.getSheetName();
-        Sheet sheet = null;
-        if (sheetName == null) {
-            sheet = workbook.getSheetAt(workbook.getActiveSheetIndex());
-        } else {
-            sheet = workbook.getSheet(sheetName);
-        }
-        
-        final Cell cell = SheetUtil.getCell(sheet, cellRef.getRow(), cellRef.getCol());
-        
-        if (cell == null) {
-            return Collections.emptyList();
-        }
-        
-        return getConditionalFormattingForCell(cell, cellRef);
-    }
-    
-    /**
-     * This checks all applicable {@link ConditionalFormattingRule}s for the cell's sheet, 
-     * in defined "priority" order, returning the matches if any.  This is a property currently
-     * not exposed from <code>CTCfRule</code> in <code>XSSFConditionalFormattingRule</code>.  
-     * <p>
-     * Most cells will have zero or one applied rule, but it is possible to define multiple rules
-     * that apply at the same time to the same cell, thus the List result.
-     * <p>
-     * Note that to properly apply conditional rules, care must be taken to offset the base 
-     * formula by the relative position of the current cell, or the wrong value is checked.
-     * This is handled by {@link WorkbookEvaluator#evaluate(String, CellReference, CellRangeAddressBase)}.
-     * 
-     * @param cell
-     * @return Unmodifiable List of {@link EvaluationConditionalFormatRule}s that apply to the current cell value,
-     *         in priority order, as evaluated by Excel (smallest priority # for XSSF, definition order for HSSF), 
-     *         or null if none apply
-     */
-    public List<EvaluationConditionalFormatRule> getConditionalFormattingForCell(Cell cell) {
-        return getConditionalFormattingForCell(cell, getRef(cell));
-    }
-    
-    /**
-     * We need both, and can derive one from the other, but this is to avoid duplicate work
-     * 
-     * @param cell
-     * @param ref
-     * @return unmodifiable list of matching rules
-     */
-    private List<EvaluationConditionalFormatRule> getConditionalFormattingForCell(Cell cell, CellReference ref) {
-        List<EvaluationConditionalFormatRule> rules = values.get(ref);
+        List<EvaluationConditionalFormatRule> rules = values.get(cellRef);
         
         if (rules == null) {
             // compute and cache them
             rules = new ArrayList<EvaluationConditionalFormatRule>();
+            
+            Sheet sheet = null;
+            if (cellRef.getSheetName() != null) sheet = workbook.getSheet(cellRef.getSheetName());
+            else sheet = workbook.getSheetAt(workbook.getActiveSheetIndex());
+            
             /*
              * Per Excel help:
              * https://support.office.com/en-us/article/Manage-conditional-formatting-rule-precedence-e09711a3-48df-4bcb-b82c-9d8b8b22463d#__toc269129417
@@ -208,24 +167,45 @@ public class ConditionalFormattingEvaluator {
              * thus the explicit ordering of the rule lists in #getFormattingRulesForSheet(Sheet)
              */
             boolean stopIfTrue = false;
-            for (EvaluationConditionalFormatRule rule : getRules(cell.getSheet())) {
+            for (EvaluationConditionalFormatRule rule : getRules(sheet)) {
                 
                 if (stopIfTrue) {
                     continue; // a previous rule matched and wants no more evaluations
                 }
 
-                if (rule.matches(cell)) {
+                if (rule.matches(cellRef)) {
                     rules.add(rule);
                     stopIfTrue = rule.getRule().getStopIfTrue();
                 }
             }
             Collections.sort(rules);
-            values.put(ref, rules);
+            values.put(cellRef, rules);
         }
         
         return Collections.unmodifiableList(rules);
     }
     
+    /**
+     * This checks all applicable {@link ConditionalFormattingRule}s for the cell's sheet, 
+     * in defined "priority" order, returning the matches if any.  This is a property currently
+     * not exposed from <code>CTCfRule</code> in <code>XSSFConditionalFormattingRule</code>.  
+     * <p>
+     * Most cells will have zero or one applied rule, but it is possible to define multiple rules
+     * that apply at the same time to the same cell, thus the List result.
+     * <p>
+     * Note that to properly apply conditional rules, care must be taken to offset the base 
+     * formula by the relative position of the current cell, or the wrong value is checked.
+     * This is handled by {@link WorkbookEvaluator#evaluate(String, CellReference, CellRangeAddressBase)}.
+     * 
+     * @param cell
+     * @return Unmodifiable List of {@link EvaluationConditionalFormatRule}s that apply to the current cell value,
+     *         in priority order, as evaluated by Excel (smallest priority # for XSSF, definition order for HSSF), 
+     *         or null if none apply
+     */
+    public List<EvaluationConditionalFormatRule> getConditionalFormattingForCell(Cell cell) {
+        return getConditionalFormattingForCell(getRef(cell));
+    }
+    
     public static CellReference getRef(Cell cell) {
         return new CellReference(cell.getSheet().getSheetName(), cell.getRowIndex(), cell.getColumnIndex(), false, false);
     }

src/java/org/apache/poi/ss/formula/EvaluationConditionalFormatRule.java

@@ -45,6 +45,7 @@ import org.apache.poi.ss.usermodel.ExcelNumberFormat;
 import org.apache.poi.ss.usermodel.Row;
 import org.apache.poi.ss.usermodel.Sheet;
 import org.apache.poi.ss.util.CellRangeAddress;
+import org.apache.poi.ss.util.CellReference;
 
 /**
  * Abstracted and cached version of a Conditional Format rule for use with a
@@ -262,14 +263,14 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
     }
     
     /**
-     * @param cell
+     * @param ref
      * @return true if this rule evaluates to true for the given cell
      */
-    /* package */ boolean matches(Cell cell) {
+    /* package */ boolean matches(CellReference ref) {
         // first check that it is in one of the regions defined for this format
         CellRangeAddress region = null;
         for (CellRangeAddress r : regions) {
-            if (r.isInRange(cell)) {
+            if (r.isInRange(ref)) {
                 region = r;
                 break;
             }
@@ -290,14 +291,22 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
            return true; 
         }
         
+        Cell cell = null;
+        final Row row = sheet.getRow(ref.getRow());
+        if (row != null) {
+            cell = row.getCell(ref.getCol());
+        }
+        
         if (ruleType.equals(ConditionType.CELL_VALUE_IS)) {
+            // undefined cells never match a VALUE_IS condition
+            if (cell == null) return false;
             return checkValue(cell, region);
         }
         if (ruleType.equals(ConditionType.FORMULA)) {
-            return checkFormula(cell, region);
+            return checkFormula(ref, region);
         }
         if (ruleType.equals(ConditionType.FILTER)) {
-            return checkFilter(cell, region);
+            return checkFilter(cell, ref, region);
         }
         
         // TODO: anything else, we don't handle yet, such as top 10
@@ -361,12 +370,12 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
         return comp;
     }
     /**
-     * @param cell needed for offsets from region anchor
+     * @param cell needed for offsets from region anchor - may be null!
      * @param region for adjusting relative formulas
      * @return true/false using the same rules as Data Validation evaluations
      */
-    private boolean checkFormula(Cell cell, CellRangeAddress region) {
-        ValueEval comp = unwrapEval(workbookEvaluator.evaluate(rule.getFormula1(), ConditionalFormattingEvaluator.getRef(cell), region));
+    private boolean checkFormula(CellReference ref, CellRangeAddress region) {
+        ValueEval comp = unwrapEval(workbookEvaluator.evaluate(rule.getFormula1(), ref, region));
         
         // Copied for now from DataValidationEvaluator.ValidationEnum.FORMULA#isValidValue()
         if (comp instanceof BlankEval) {
@@ -386,11 +395,13 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
         return false; // anything else is false, such as text
     }
     
-    private boolean checkFilter(Cell cell, CellRangeAddress region) {
+    private boolean checkFilter(Cell cell, CellReference ref, CellRangeAddress region) {
         final ConditionFilterType filterType = rule.getConditionFilterType();
         if (filterType == null) {
             return false;
         }
+
+        final ValueAndFormat cv = getCellValue(cell);
         
         // TODO: this could/should be delegated to the Enum type, but that's in the usermodel package,
         // we may not want evaluation code there.  Of course, maybe the enum should go here in formula,
@@ -403,8 +414,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
             // from testing, Excel only operates on numbers and dates (which are stored as numbers) in the range.
             // numbers stored as text are ignored, but numbers formatted as text are treated as numbers.
             
-            final ValueAndFormat cv10 = getCellValue(cell);
-            if (! cv10.isNumber()) {
+            if (! cv.isNumber()) {
                 return false;
             }
             
@@ -430,7 +440,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
 
                     return new HashSet<ValueAndFormat>(allValues.subList(0, limit));
                 }
-            }).contains(cv10);
+            }).contains(cv);
         case UNIQUE_VALUES:
             // Per Excel help, "duplicate" means matching value AND format
             // https://support.office.com/en-us/article/Filter-for-unique-values-or-remove-duplicate-values-ccf664b0-81d6-449b-bbe1-8daaec1e83c2
@@ -455,7 +465,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
                     
                     return unique;
                 }
-            }).contains(getCellValue(cell));
+            }).contains(cv);
         case DUPLICATE_VALUES:
             // Per Excel help, "duplicate" means matching value AND format
             // https://support.office.com/en-us/article/Filter-for-unique-values-or-remove-duplicate-values-ccf664b0-81d6-449b-bbe1-8daaec1e83c2
@@ -478,7 +488,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
                     }
                     return dup;
                 }
-            }).contains(getCellValue(cell));
+            }).contains(cv);
         case ABOVE_AVERAGE:
             // from testing, Excel only operates on numbers and dates (which are stored as numbers) in the range.
             // numbers stored as text are ignored, but numbers formatted as text are treated as numbers.
@@ -507,7 +517,6 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
                 }
             }));
             
-            final ValueAndFormat cv = getCellValue(cell);
             Double val = cv.isNumber() ? cv.getValue() : null;
             if (val == null) {
                 return false;
@@ -541,19 +550,19 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
             return op != null && op.isValid(val, comp, null);
         case CONTAINS_TEXT:
             // implemented both by a cfRule "text" attribute and a formula.  Use the formula.
-            return checkFormula(cell, region);
+            return checkFormula(ref, region);
         case NOT_CONTAINS_TEXT:
             // implemented both by a cfRule "text" attribute and a formula.  Use the formula.
-            return checkFormula(cell, region);
+            return checkFormula(ref, region);
         case BEGINS_WITH:
             // implemented both by a cfRule "text" attribute and a formula.  Use the formula.
-            return checkFormula(cell, region);
+            return checkFormula(ref, region);
         case ENDS_WITH:
             // implemented both by a cfRule "text" attribute and a formula.  Use the formula.
-            return checkFormula(cell, region);
+            return checkFormula(ref, region);
         case CONTAINS_BLANKS:
             try {
-                String v = cell.getStringCellValue();
+                String v = cv.getString();
                 // see TextFunction.TRIM for implementation
                 return v == null || v.trim().length() == 0;
             } catch (Exception e) {
@@ -562,7 +571,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
             }
         case NOT_CONTAINS_BLANKS:
             try {
-                String v = cell.getStringCellValue();
+                String v = cv.getString();
                 // see TextFunction.TRIM for implementation
                 return v != null && v.trim().length() > 0;
             } catch (Exception e) {
@@ -575,7 +584,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
             return cell == null || ! DataValidationEvaluator.isType(cell, CellType.ERROR);
         case TIME_PERIOD:
             // implemented both by a cfRule "text" attribute and a formula.  Use the formula.
-            return checkFormula(cell, region);
+            return checkFormula(ref, region);
         default:
             return false;
         }
@@ -627,7 +636,7 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
                 return new ValueAndFormat(cell.getStringCellValue(), cell.getCellStyle().getDataFormatString());
             }
         }
-        return null;
+        return new ValueAndFormat("", "");
     }
     /**
      * instances evaluate the values for a region and return the positive matches for the function type.
@@ -754,6 +763,10 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
             return value;
         }
         
+        public String getString() {
+            return string;
+        }
+        
         @Override
         public boolean equals(Object obj) {
             if (!(obj instanceof ValueAndFormat)) {

src/ooxml/testcases/org/apache/poi/ss/usermodel/ConditionalFormattingEvalTest.java

@@ -117,6 +117,18 @@ public class ConditionalFormattingEvalTest {
         assertEquals("wrong # of rules for " + ref, 1, rules.size());
         
     }
+
+    @Test
+    public void testFormattingOnUndefinedCell() throws Exception {
+        wb = XSSFTestDataSamples.openSampleWorkbook("conditional_formatting_with_formula_on_second_sheet.xlsx");
+        formulaEval = new XSSFFormulaEvaluator(wb);
+        cfe = new ConditionalFormattingEvaluator(wb, formulaEval);
+
+        sheet = wb.getSheet("Sales Plan");
+        getRulesFor(9,2);
+        assertNotEquals("No rules for " + ref, 0, rules.size());
+        assertEquals("wrong bg color for " + ref, "FFFFFF00", getColor(rules.get(0).getRule().getPatternFormatting().getFillBackgroundColorColor()));
+    }
     
     private List<EvaluationConditionalFormatRule> getRulesFor(int row, int col) {
         ref = new CellReference(sheet.getSheetName(), row, col, false, false);