You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "Sean-Walker0 (via GitHub)" <gi...@apache.org> on 2026/10/09 02:18:57 UTC
[PR] fix(sheet): skip the trailing endSheet when no XLS sheet matched [fesod]
Sean-Walker0 opened a new pull request, #1201:
URL: https://github.com/apache/fesod/pull/1201
## Purpose of the pull request
Reading an `.xls` while requesting a sheet the file does not contain (by number or by name) fails with a raw `NullPointerException` instead of returning an empty read. The same request against `.xlsx` (or CSV) returns an empty list.
Found by code audit comparing the three readers' unmatched-sheet handling.
## What's changed?
`XlsSaxAnalyser.execute()` ends with an unconditional `endSheet` call that covers sheet substreams lacking the `EOF` terminator. When no requested sheet ever matched, no sheet was started and `xlsReadContext.readSheetHolder()` is still `null`, so `DefaultAnalysisEventProcessor.endSheet` throws NPE on `readSheetHolder.getEnded()` and the whole read fails.
The xlsx sibling (`XlsxSaxAnalyser.execute()`) and the CSV executor both guard the equivalent call (`if (readSheet != null)` / `if (readSheet == null) continue;`). This change adds the same null guard to the XLS trailing call: an unmatched sheet now reads as empty, while files without an `EOF` terminator keep their final `endSheet`.
## Checklist
- [x] I have read the [Contributor Guide](https://fesod.apache.org/community/contribution/).
- [x] I have written the necessary doc or comment.
- [x] I have added the necessary unit tests and all cases have passed.
## Verifying this change
`NoMatchSheetReadTest` (3 cases):
- `readXlsWithNonExistingSheetNumberReturnsEmpty` and `readXlsWithNonExistingSheetNameReturnsEmpty` — red before (both throw `NullPointerException: ... ReadSheetHolder.getEnded() because "readSheetHolder" is null`), green after;
- `readXlsxWithNonExistingSheetNumberReturnsEmpty` — pins the expected empty-read behavior of the xlsx sibling, green before and after.
- `./mvnw clean package -B -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet`: 951 tests, 0 failures, 0 errors.
- `./mvnw -pl fesod-sheet spotless:check`: clean.
## Notes
- Orthogonal to open PRs: #1195 touches `XlsSaxAnalyser` only in the import block and the `FormatTrackingHSSFListener` construction inside `execute()` — no overlap with the trailing `endSheet` statement changed here, and the hunks merge cleanly.
- No behavior change for any read that matches at least one sheet, including files without an `EOF` terminator (the guard keeps the existing final `endSheet`).
--
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
Re: [PR] fix(sheet): skip the trailing endSheet when no XLS sheet matched [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1201:
URL: https://github.com/apache/fesod/pull/1201#discussion_r4226351812
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v03/XlsSaxAnalyser.java:
##########
@@ -216,7 +216,11 @@ public void execute() {
}
// There are some special xls that do not have the terminator "[EOF]", so an additional
- xlsReadContext.analysisEventProcessor().endSheet(xlsReadContext);
+ // endSheet is required. When no requested sheet matched, no sheet was started and there is
+ // nothing to end - the xlsx reader returns an empty read in that case.
Review Comment:
The rationale about aligning with the xlsx behavior is great context for the PR, but for in-code comments, the code `if (... != null)` is already self-explanatory. Keeping the comment lean and focused on the original [EOF] workaround will read much better long-term.
--
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
Re: [PR] fix(sheet): skip the trailing endSheet when no XLS sheet matched [fesod]
Posted by "Sean-Walker0 (via GitHub)" <gi...@apache.org>.
Sean-Walker0 commented on PR #1201:
URL: https://github.com/apache/fesod/pull/1201#issuecomment-6097415080
Rebased onto the latest main (bf9bf92) and force-pushed as 2e87334. No conflicts; NoMatchSheetReadTest still 3/3 green with the new checkstyle gate active.
--
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
Re: [PR] fix(sheet): skip the trailing endSheet when no XLS sheet matched [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on PR #1201:
URL: https://github.com/apache/fesod/pull/1201#issuecomment-6097355712
LGTM.
However, the branch is not up to date with the target branch. Could you please rebase it onto the latest main branch before merging? Otherwise, I can use GitHub's "Update branch" button.
--
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
Re: [PR] fix(sheet): skip the trailing endSheet when no XLS sheet matched [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng merged PR #1201:
URL: https://github.com/apache/fesod/pull/1201
--
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
Re: [PR] fix(sheet): skip the trailing endSheet when no XLS sheet matched [fesod]
Posted by "Sean-Walker0 (via GitHub)" <gi...@apache.org>.
Sean-Walker0 commented on PR #1201:
URL: https://github.com/apache/fesod/pull/1201#issuecomment-6091292874
Trimmed the comment back to the original [EOF] workaround line — the null guard now stands on its own. Force-pushed as 7f84a23 (comment-only change; NoMatchSheetReadTest still 3/3 green, spotless clean).
--
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