#63264 Conditional Format rule evaluation calculates relative references incorrectly

Fixing the offset calculation and passing the top-left-most region rather than the region matching the current cell fixed these cases.  I've augmented unit tests to check for them to avoid future regressions.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1855619 13f79535-47bb-0310-9956-ffa450edef68
This commit is contained in:
Greg Woolsey 2019-03-16 00:27:13 +00:00
parent 126606c838
commit 8225326b56
3 changed files with 43 additions and 13 deletions

View File

@ -69,6 +69,9 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
/* cached values */ /* cached values */
private final CellRangeAddress[] regions; private final CellRangeAddress[] regions;
private CellRangeAddress topLeftRegion;
/** /**
* Depending on the rule type, it may want to know about certain values in the region when evaluating {@link #matches(CellReference)}, * Depending on the rule type, it may want to know about certain values in the region when evaluating {@link #matches(CellReference)},
* such as top 10, unique, duplicate, average, etc. This collection stores those if needed so they are not repeatedly calculated * such as top 10, unique, duplicate, average, etc. This collection stores those if needed so they are not repeatedly calculated
@ -114,6 +117,14 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
this.priority = rule.getPriority(); this.priority = rule.getPriority();
this.regions = regions; this.regions = regions;
for (CellRangeAddress region : regions) {
if (topLeftRegion == null) topLeftRegion = region;
else if (region.getFirstColumn() < topLeftRegion.getFirstColumn()
|| region.getFirstRow() < topLeftRegion.getFirstRow()) {
topLeftRegion = region;
}
}
formula1 = rule.getFormula1(); formula1 = rule.getFormula1();
formula2 = rule.getFormula2(); formula2 = rule.getFormula2();
@ -316,13 +327,13 @@ public class EvaluationConditionalFormatRule implements Comparable<EvaluationCon
if (ruleType.equals(ConditionType.CELL_VALUE_IS)) { if (ruleType.equals(ConditionType.CELL_VALUE_IS)) {
// undefined cells never match a VALUE_IS condition // undefined cells never match a VALUE_IS condition
if (cell == null) return false; if (cell == null) return false;
return checkValue(cell, region); return checkValue(cell, topLeftRegion);
} }
if (ruleType.equals(ConditionType.FORMULA)) { if (ruleType.equals(ConditionType.FORMULA)) {
return checkFormula(ref, region); return checkFormula(ref, topLeftRegion);
} }
if (ruleType.equals(ConditionType.FILTER)) { if (ruleType.equals(ConditionType.FILTER)) {
return checkFilter(cell, ref, region); return checkFilter(cell, ref, topLeftRegion);
} }
// TODO: anything else, we don't handle yet, such as top 10 // TODO: anything else, we don't handle yet, such as top 10

View File

@ -880,20 +880,16 @@ public final class WorkbookEvaluator {
* <p><pre>formula ref + current cell ref</pre></p> * <p><pre>formula ref + current cell ref</pre></p>
* @param ptgs * @param ptgs
* @param target cell within the region to use. * @param target cell within the region to use.
* @param region containing the cell * @param region containing the cell, OR, for conditional format rules with multiple ranges, the region with the top-left-most cell
* @return true if any Ptg references were shifted * @return true if any Ptg references were shifted
* @throws IndexOutOfBoundsException if the resulting shifted row/column indexes are over the document format limits * @throws IndexOutOfBoundsException if the resulting shifted row/column indexes are over the document format limits
* @throws IllegalArgumentException if target is not within region. * @throws IllegalArgumentException if target is not within region.
*/ */
protected boolean adjustRegionRelativeReference(Ptg[] ptgs, CellReference target, CellRangeAddressBase region) { protected boolean adjustRegionRelativeReference(Ptg[] ptgs, CellReference target, CellRangeAddressBase region) {
if (! region.isInRange(target)) { // region may not be the one that contains the target, if a conditional formatting rule applies to multiple regions
throw new IllegalArgumentException(target + " is not within " + region);
}
//return adjustRegionRelativeReference(ptgs, target.getRow() - region.getFirstRow(), target.getCol() - region.getFirstColumn()); int deltaRow = target.getRow() - region.getFirstRow();
int deltaColumn = target.getCol() - region.getFirstColumn();
int deltaRow = target.getRow();
int deltaColumn = target.getCol();
boolean shifted = false; boolean shifted = false;
for (Ptg ptg : ptgs) { for (Ptg ptg : ptgs) {
@ -902,7 +898,7 @@ public final class WorkbookEvaluator {
RefPtgBase ref = (RefPtgBase) ptg; RefPtgBase ref = (RefPtgBase) ptg;
// re-calculate cell references // re-calculate cell references
final SpreadsheetVersion version = _workbook.getSpreadsheetVersion(); final SpreadsheetVersion version = _workbook.getSpreadsheetVersion();
if (ref.isRowRelative()) { if (ref.isRowRelative() && deltaRow > 0) {
final int rowIndex = ref.getRow() + deltaRow; final int rowIndex = ref.getRow() + deltaRow;
if (rowIndex > version.getMaxRows()) { if (rowIndex > version.getMaxRows()) {
throw new IndexOutOfBoundsException(version.name() + " files can only have " + version.getMaxRows() + " rows, but row " + rowIndex + " was requested."); throw new IndexOutOfBoundsException(version.name() + " files can only have " + version.getMaxRows() + " rows, but row " + rowIndex + " was requested.");
@ -910,7 +906,7 @@ public final class WorkbookEvaluator {
ref.setRow(rowIndex); ref.setRow(rowIndex);
shifted = true; shifted = true;
} }
if (ref.isColRelative()) { if (ref.isColRelative() && deltaColumn > 0) {
final int colIndex = ref.getColumn() + deltaColumn; final int colIndex = ref.getColumn() + deltaColumn;
if (colIndex > version.getMaxColumns()) { if (colIndex > version.getMaxColumns()) {
throw new IndexOutOfBoundsException(version.name() + " files can only have " + version.getMaxColumns() + " columns, but column " + colIndex + " was requested."); throw new IndexOutOfBoundsException(version.name() + " files can only have " + version.getMaxColumns() + " columns, but column " + colIndex + " was requested.");

View File

@ -19,6 +19,7 @@ package org.apache.poi.ss.usermodel;
import java.io.IOException; import java.io.IOException;
import java.util.Date;
import java.util.List; import java.util.List;
import org.apache.poi.ss.formula.ConditionalFormattingEvaluator; import org.apache.poi.ss.formula.ConditionalFormattingEvaluator;
@ -117,6 +118,28 @@ public class ConditionalFormattingEvalTest {
getRulesFor(8,2); getRulesFor(8,2);
assertEquals("wrong # of rules for " + ref, 1, rules.size()); assertEquals("wrong # of rules for " + ref, 1, rules.size());
sheet = wb.getSheet("Compare to totals");
getRulesFor(3, 2);
assertEquals("wrong # of rules for " + ref, 1, rules.size());
assertEquals("wrong fg color for " + ref, "FFFF0000", getColor(rules.get(0).getRule().getFontFormatting().getFontColor()));
getRulesFor(3, 3);
assertEquals("wrong # of rules for " + ref, 0, rules.size());
getRulesFor(15, 4);
assertEquals("wrong # of rules for " + ref, 0, rules.size());
getRulesFor(16, 1);
assertEquals("wrong # of rules for " + ref, 1, rules.size());
assertEquals("wrong fg color for " + ref, "FFFF0000", getColor(rules.get(0).getRule().getFontFormatting().getFontColor()));
sheet = wb.getSheet("Products3");
sheet.getRow(8).getCell(0).setCellValue(new Date());
getRulesFor(8, 0);
assertEquals("wrong # of rules for " + ref, 1, rules.size());
getRulesFor(8, 3);
assertEquals("wrong # of rules for " + ref, 1, rules.size());
sheet = wb.getSheet("Customers2");
getRulesFor(3, 0);
assertEquals("wrong # of rules for " + ref, 0, rules.size());
} }
@Test @Test