pjfanning commented on code in PR #1473:
URL: https://github.com/apache/poi/pull/1473#discussion_r4368660777
##########
poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java:
##########
@@ -680,7 +682,13 @@ private void writeFraction(double value, StringBuffer
result,
} else {
// buildFractionMaxDenominator only gives denominators below the
maximum it is passed, so pass
// 10^digits, as FractionFormat does: e.g. "??/??" shows 0.99 as
98/99, where 99 would give 1/1
- SimpleFraction frac =
SimpleFraction.buildFractionMaxDenominator(fractional, maxDenominator + 1);
+ SimpleFraction frac;
+ try {
+ frac = SimpleFraction.buildFractionMaxDenominator(fractional,
maxDenominator + 1);
+ } catch (RuntimeException e) {
+ LOG.atError().withThrowable(e).log("error while fraction
evaluation");
+ frac = new
SimpleFraction(Math.toIntExact(Math.round(fractional)), 1);
+ }
Review Comment:
Following up on my earlier comment: after the `SimpleFraction` fix, this
`catch` only ever sees the case where the value is above `Integer.MAX_VALUE`,
and that is exactly when the fallback overflows as well. I suggest dropping it:
```suggestion
SimpleFraction frac =
SimpleFraction.buildFractionMaxDenominator(fractional, maxDenominator + 1);
```
and putting a clear check at the top of `writeFraction`, just after the
signature (that line isn't in the diff, so it can't be a suggestion). It also
covers the whole-number and fixed-denominator branches, which use
`Math.toIntExact` as well:
```java
// the numerator is an int, so an improper fraction of a huge value
cannot be shown
if (Math.abs(fractional) * Math.max(fixedDenominator, 1) >=
Integer.MAX_VALUE) {
throw new IllegalArgumentException("Value is too large to format
as a fraction: " + fractional);
}
```
`CellFormat.getInstance("?/?").apply(3e9 + 0.5)` then throws a clear
`IllegalArgumentException` instead of logging an error and throwing
`ArithmeticException`. Formats with an integer part (`"# ?/?"`) aren't
affected, because `fractional` is below 1 there. `DataFormatter` callers such
as Tika are covered by the `CellFormatWrapper` change in the other comment, so
the workbook doesn't fail.
##########
poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java:
##########
@@ -930,6 +930,24 @@ void testSimpleFractionFormat() throws IOException {
}
}
+ @Test
+ void testFractionDenominatorDigitsCapped() {
+ // more than 9 denominator digits overflowed an int and the format
threw when applied
+ String nine = CellFormat.getInstance("#
?????????/?????????").apply(Math.PI).text.trim();
+ for (int digits : new int[]{10, 20}) {
+ String q = "?".repeat(digits);
+ String text = CellFormat.getInstance("# " + q + "/" +
q).apply(Math.PI).text;
+ assertEquals(nine.replaceAll(" +", " "), text.trim().replaceAll("
+", " "));
+ }
+ }
Review Comment:
I checked this in Excel: `=TEXT(PI(),"# ?…?/?…?")` gives `3 244252/1725033`
for 9, 10 and 20 `?`s, and the padding grows with every `?`. So the 9-digit cap
matches Excel for the value, and keeping the width is right. Stripping
whitespace hides that, though, and PI is a poor choice here: POI's 9-digit
result for PI (`35580937/251290841`) already differs from Excel's. That
mismatch predates this PR, and I'll raise it separately. I suggest asserting
the exact padded text for a value where both agree, and adding a test for the
too-large case from the other comment:
```suggestion
void testFractionDenominatorDigitsCapped() {
// more than 9 denominator digits overflowed an int and the format
threw when applied;
// Excel gives the same fraction however many digits there are, but
pads to every '?'
for (int digits : new int[]{9, 10, 20}) {
String q = "?".repeat(digits);
String pad = " ".repeat(digits - 1);
assertEquals("3 " + pad + "1/4" + pad, CellFormat.getInstance("#
" + q + "/" + q).apply(3.25).text);
}
}
@Test
void testImproperFractionTooLarge() {
// the numerator is an int, so this cannot be shown as 6000000001/2
assertThrows(IllegalArgumentException.class, () ->
CellFormat.getInstance("?/?").apply(3e9 + 0.5));
assertThrows(IllegalArgumentException.class, () ->
CellFormat.getInstance("?/16").apply(2.5e8 + 0.5));
assertEquals("3000000000 1/2", CellFormat.getInstance("#
?/?").apply(3e9 + 0.5).text);
}
```
##########
poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java:
##########
@@ -555,7 +555,10 @@ protected Format createFormat(double cellValue, int
formatIndex, String sFormat)
if (plainFraction || fractionWithText) {
if (fractionWithText) {
try {
- return new
CellFormatWrapper(CellFormat.getInstance(locale, formatStr));
+ CellFormat cellFormat = CellFormat.getInstance(locale,
formatStr);
+ // a format CellFormat cannot make sense of fails here,
not when formatting
Review Comment:
Following up on my earlier comment: I suggest keeping the probe, so a format
that never works still falls back to `FractionFormat` here, but making the
comment accurate:
```suggestion
// a format CellFormat cannot make sense of fails here,
so the FractionFormat below is used;
// a format that only fails for some values falls back
in CellFormatWrapper
```
and catching per-value failures in `CellFormatWrapper.format` (outside the
diff, so not a suggestion). That also covers the existing
`hasLiteralDecimalFormatCannotShow` path, which uses the same probe:
```java
public StringBuffer format(Object obj, StringBuffer toAppendTo,
FieldPosition pos) {
String text;
try {
text = cellFormat.apply(obj).text;
} catch (RuntimeException e) {
// the format was checked against the first value it was
created for, but this value fails
LOG.atWarn().withThrowable(e).log("Formatting failed for
value {}, falling back to General", obj);
return generalNumberFormat.format(obj, toAppendTo, pos);
}
return toAppendTo.append(emulateCSV ? text : text.trim());
}
```
##########
poi/src/test/java/org/apache/poi/ss/usermodel/TestDataFormatter.java:
##########
@@ -464,6 +464,15 @@ void testFractionWithLongRun() {
}
}
+ @Test
+ void testFractionWithTextAndLongDenominator() {
+ DataFormatter dfUS = new DataFormatter(Locale.US);
+
+ // text in a fraction goes to CellFormat, where more than 9
denominator digits overflowed and the format threw
+ assertEquals("3: 35580937/251290841",
+ dfUS.formatRawCellContents(Math.PI, -1,
"#\\:??????????/??????????").replaceAll(" +", " "));
+ }
Review Comment:
`3: 35580937/251290841` is POI's value, not Excel's: Excel shows π with
these digits as `3: 244252/1725033` (see the `TestCellFormat` comment). I
suggest a value both agree on, an exact assertion, and a test for the
cached-format fallback:
```suggestion
// text in a fraction goes to CellFormat, where more than 9
denominator digits overflowed and the format threw
assertEquals("3: 1/4", dfUS.formatRawCellContents(3.25, -1,
"#\\:??????????/??????????"));
}
@Test
void testFractionWithTextFallsBackForValueTooLarge() {
DataFormatter dfUS = new DataFormatter(Locale.US);
// the format works for the first value and is cached, but an
improper fraction this large cannot be shown
assertEquals("13=/=4", dfUS.formatRawCellContents(3.25, -1,
"??\\=/=??"));
assertEquals("3000000001", dfUS.formatRawCellContents(3e9 + 0.5, -1,
"??\\=/=??"));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
