fix: fall back to default format index on malformed style index (#355)

[Mikkey-f]

Fixes #355

What changes were proposed in this pull request?

CellTagHandler parses a cell's s attribute (style index) with a bare Integer.parseInt. A non-numeric value — as produced by some third-party tools — threw NumberFormatException and aborted the whole file read at the first corrupt cell (in the report: reading stopped at row 3218 of 3000+ rows).

The parse now falls back to the default format index (0) on NumberFormatException: a corrupt style index only loses that cell's style info, while the cell value and all subsequent rows are still read. This follows the reporter's suggestion, and is deliberately more lenient than POI — POI's native streaming read throws NumberFormatException on the same input when styles.xml is present, and only reads such a file completely when styles.xml is absent.

How was this patch tested?

• CellTagHandlerTest (2 new unit tests): non-numeric s falls back to dataFormatData(0) without throwing; numeric s keeps the exact lookup.
• CorruptStyleIndexReadTest (new end-to-end test, two variants): a hand-built xlsx whose middle row carries s="abc" is read completely — both the minimal variant (no styles.xml) and the structurally complete variant (styles.xml + one cellXf present).
• Red/green check: on the pre-fix code both e2e variants fail with the exact exception from the report (NumberFormatException: For input string: "abc"); with the fix they pass.
• Full fesod-sheet suite: 898/898 green. spotless:check and javadoc pass.

Notes

• Zero impact on valid files: the numeric parse path is untouched.
• No API change, no new dependencies.

[skytin1004]

Reviewed and verified locally.

The focused tests pass locally (4 tests). I also temporarily reverted the handler change and confirmed that the end-to-end test reproduces the original NumberFormatException in CellTagHandler.

The fix is narrowly scoped, and the regression coverage verifies both the internal handler and the public read path. I also confirmed that valid numeric style indexes are still handled through the existing lookup path, so the change does not affect the normal style lookup behavior.

LGTM.

[bengbengbalabalabeng]

POI doesn't actually provide fault tolerance for this scenario during streaming reads:

Looking at the source code of XSSFSheetXMLHandler, reading the test XLSX file with POI’s native streaming API only succeeds because the test file lacks a styles.xml. Consequently, stylesTable is null, which naturally bypasses the style resolution logic. In real-world scenarios, however, as long as styles.xml is present, POI will still throw a NumberFormatException and abort the read when encountering s="abc".

https://github.com/apache/poi/blob/dca7cda1dc8a64197f7e7e8e7892ba9a0d79c860/poi-ooxml/src/main/java/org/apache/poi/xssf/eventusermodel/XSSFSheetXMLHandler.java#L265-L300

Therefore, mentioning "matching POI's lenient behavior" in the comment might not be entirely accurate. In addition, while I agree that we should be lenient with non-standard files missing styles.xml (ensuring unstyled sheets can still be processed), handling malformed data like s="abc" in a structurally complete file is a different story. Whether the framework should catch this exception—and whether it should fall back to DEFAULT_FORMAT_INDEX = 0 or simply skip it (leave it as null)—is probably worth further discussion.

[Mikkey-f]

@bengbengbalabalabeng Thanks for the source-level pointer — you're right on both points, and I verified your reading locally before replying.

On the wording. "Matches POI's lenient behavior" was inaccurate, and the test file masked it. I reproduced the scenario against POI 5.5.1 with a structurally complete xlsx (styles.xml present, one cellXf, middle cell s="abc"):

• POI with styles.xml present: native streaming read (XSSFReader + XSSFSheetXMLHandler) throws NumberFormatException: For input string: "abc" and aborts — even the rows after the corrupt cell are lost.
• POI without styles.xml: reads the file completely (3/3 rows) — which is exactly why the e2e test in this PR passed: it was built minimal and deliberately has no styles.xml, so format resolution is bypassed.

The accurate statement is: fesod deliberately goes further than POI here — a malformed style index never aborts the read, both in structurally complete files and in files missing styles.xml. I've updated the PR description accordingly.

On the test gap. Agreed the test only covered the no-styles.xml case, which is not the scenario the report came from. The latest commit (fead136) adds a variant with a structurally complete file (styles.xml + one cellXf inside the zip): all 3 rows are read and the corrupt cell's value 2 comes back correctly. Red/green: on pre-fix code the same file reproduces the report's NumberFormatException; with the fix it passes.

On 0 vs null. Agreed this deserves settling explicitly. My reasoning in favor of DEFAULT_FORMAT_INDEX:

1. It keeps the malformed-s case on the exact path the code already takes when s is absent (StringUtils.isEmpty(dateFormatIndex) → DEFAULT_FORMAT_INDEX). A malformed index is semantically "no usable style index", so treating it like a missing one is the most consistent reading.
2. Downstream is null-safe either way (StringNumberConverter guards on null; files without styles.xml already yield null from dataFormatData(...)), so both are workable — but null would introduce a third state (malformed ≠ missing) and bypass the existing default-format path.
3. The difference is confined to the cell's format metadata (index 0, typically "General"); the value is read identically.

That said, if you or the maintainers prefer the stricter reading — malformed data in a structurally complete file should lose format info entirely — I'm glad to switch. Happy to follow your call.

[skytin1004]

I agree with using 0 here. A malformed s value means there is no usable style index, and a missing s already follows the same fallback path.

[skytin1004]

I noticed that this comment still refers to POI's lenient behavior, although the updated PR description now explains that Fesod is intentionally more lenient here. How about updating it to this?

Suggested change

	// whole file read; fall back to the default format, matching POI's lenient behavior.
	dateFormatIndexInteger = DEFAULT_FORMAT_INDEX;
	// A malformed style index (e.g. produced by third-party tools) must not abort the
	// whole file read. Treat it like a missing style index and use the default format.