Re: [PR] fix: fall back to default format index on malformed style index (#355) [fesod]

["Mikkey-f (via GitHub)" <gi...@apache.org>]

Mikkey-f commented on PR #1059:
URL: https://github.com/apache/fesod/pull/1059#issuecomment-5967917061

   @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 (fead1362) 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.
   


-- 
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: commits-unsubscribe@fesod.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org