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,21 +150,39 @@ public class ConditionalFormattingEvaluator {
      *         or null if none apply
      */
     public List<EvaluationConditionalFormatRule> getConditionalFormattingForCell(final CellReference cellRef) {
-        String sheetName = cellRef.getSheetName();
+        List<EvaluationConditionalFormatRule> rules = values.get(cellRef);
+        
+        if (rules == null) {
+            // compute and cache them
+            rules = new ArrayList<EvaluationConditionalFormatRule>();
+            
             Sheet sheet = null;
-        if (sheetName == null) {
+            if (cellRef.getSheetName() != null) sheet = workbook.getSheet(cellRef.getSheetName());
-            sheet = workbook.getSheetAt(workbook.getActiveSheetIndex());
+            else sheet = workbook.getSheetAt(workbook.getActiveSheetIndex());
-        } else {
+            
-            sheet = workbook.getSheet(sheetName);
+            /*
+             * Per Excel help:
+             * https://support.office.com/en-us/article/Manage-conditional-formatting-rule-precedence-e09711a3-48df-4bcb-b82c-9d8b8b22463d#__toc269129417
+             * stopIfTrue is true for all rules from HSSF files, and an explicit value for XSSF files.
+             * thus the explicit ordering of the rule lists in #getFormattingRulesForSheet(Sheet)
+             */
+            boolean stopIfTrue = false;
+            for (EvaluationConditionalFormatRule rule : getRules(sheet)) {
+                
+                if (stopIfTrue) {
+                    continue; // a previous rule matched and wants no more evaluations
                 }
 
-        final Cell cell = SheetUtil.getCell(sheet, cellRef.getRow(), cellRef.getCol());
+                if (rule.matches(cellRef)) {
-        
+                    rules.add(rule);
-        if (cell == null) {
+                    stopIfTrue = rule.getRule().getStopIfTrue();
-            return Collections.emptyList();
+                }
+            }
+            Collections.sort(rules);
+            values.put(cellRef, rules);
         }
         
-        return getConditionalFormattingForCell(cell, cellRef);
+        return Collections.unmodifiableList(rules);
     }
     
     /**
@@ -185,45 +203,7 @@ public class ConditionalFormattingEvaluator {
      *         or null if none apply
      */
     public List<EvaluationConditionalFormatRule> getConditionalFormattingForCell(Cell cell) {
-        return getConditionalFormattingForCell(cell, getRef(cell));
+        return getConditionalFormattingForCell(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);
-        
-        if (rules == null) {
-            // compute and cache them
-            rules = new ArrayList<EvaluationConditionalFormatRule>();
-            /*
-             * Per Excel help:
-             * https://support.office.com/en-us/article/Manage-conditional-formatting-rule-precedence-e09711a3-48df-4bcb-b82c-9d8b8b22463d#__toc269129417
-             * stopIfTrue is true for all rules from HSSF files, and an explicit value for XSSF files.
-             * thus the explicit ordering of the rule lists in #getFormattingRulesForSheet(Sheet)
-             */
-            boolean stopIfTrue = false;
-            for (EvaluationConditionalFormatRule rule : getRules(cell.getSheet())) {
-                
-                if (stopIfTrue) {
-                    continue; // a previous rule matched and wants no more evaluations
-                }
-
-                if (rule.matches(cell)) {
-                    rules.add(rule);
-                    stopIfTrue = rule.getRule().getStopIfTrue();
-                }
-            }
-            Collections.sort(rules);
-            values.put(ref, rules);
-        }
-        
-        return Collections.unmodifiableList(rules);
     }
     
     public static CellReference getRef(Cell cell) {

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) {
+    private boolean checkFormula(CellReference ref, CellRangeAddress region) {
-        ValueEval comp = unwrapEval(workbookEvaluator.evaluate(rule.getFormula1(), ConditionalFormattingEvaluator.getRef(cell), region));
+        ValueEval comp = unwrapEval(workbookEvaluator.evaluate(rule.getFormula1(), ref, region));
         
         // Copied for now from DataValidationEvaluator.ValidationEnum.FORMULA#isValidValue()
         if (comp instanceof BlankEval) {
@@ -386,12 +395,14 @@ 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,
         // and not be returned by the SS model, but then we need the XSSF rule to expose the raw OOXML
@@ -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 (! cv.isNumber()) {
-            if (! cv10.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

@@ -118,6 +118,18 @@ public class ConditionalFormattingEvalTest {
         
     }
 
+    @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);
         return rules = cfe.getConditionalFormattingForCell(ref);