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/10 00:06:47 UTC

[PR] fix(sheet): keep worksheet substreams paired with their sheets [fesod]

Sean-Walker0 opened a new pull request, #1210:
URL: https://github.com/apache/fesod/pull/1210

   ## Purpose of the pull request
   
   An `.xls` whose workbook-global substream order puts a chart sheet, Excel 4 macro sheet or VB module **before** a worksheet is misread: the worksheet's rows are attributed to the skipped sheet's name/number, later sheets are silently dropped, and when the skipped substream comes first the whole read fails with a `NullPointerException` from the cell handlers (`readSheetHolder()` is null).
   
   No issue exists for this yet; found by code audit of the BIFF substream state machine.
   
   ## What's changed?
   
   Every substream that carries a `BOUNDSHEET` entry occupies a slot in the ordered sheet list — worksheets, charts, macro sheets and VB modules alike (`BoundSheetRecord.orderByBofPosition` orders all of them, and `initReadSheetDataList` builds one `ReadSheet` per entry). `BofRecordHandler` consumed a `readSheetIndex` slot only for `TYPE_WORKSHEET` BOFs and returned early for every other type, so any non-worksheet substream shifted the slot pairing for everything after it, and its own cell records were never marked ignored.
   
   For `TYPE_CHART`, `TYPE_EXCEL_4_MACRO` and `TYPE_VB_MODULE` BOFs the handler now consumes the sheet-list slot and sets `ignoreRecord` — the same treatment the unmatched-worksheet path already applies — so the following worksheets pair with their own `ReadSheet` and the skipped substream's records are filtered by the existing ignorable-handler gate.
   
   ## 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
   
   `XlsNonWorksheetSubstreamTest` builds a real two-sheet workbook with POI and rewrites the `Workbook` stream so the first sheet substream declares itself as a chart (BOF type `0x0020`) while keeping its cell records — the layout Excel writes when a chart sheet precedes a worksheet:
   
   - `readWorksheetFollowingAChartSubstream` — red before (NPE: `readSheetHolder()` is null in `determineTargetColumnIndex`), green after: `sheet(1)` returns its own data;
   - `readAllSkipsTheChartSubstreamAndKeepsSheetPairing` — red before (same NPE), green after: the worksheet's data lands under `sheet1`, the chart substream contributes nothing;
   - `readUnmodifiedWorkbookWorksAsBefore` — pins ordinary two-sheet reads, green before and after.
   
   - `./mvnw clean package -B -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet`: 961 tests, 0 failures, 0 errors.
   - `./mvnw -pl fesod-sheet spotless:check`: clean.
   
   ## Notes
   
   - Behavior notes: non-worksheet substreams no longer deliver stray rows into the previously-matched sheet, and a chart/macro/VB sheet now simply reads as empty instead of leaking another sheet's data or crashing. `TYPE_WORKSPACE_FILE` and unknown BOF types keep the old no-op behavior (they carry no `BOUNDSHEET` entry).
   - Orthogonal to open PRs: no open PR touches `BofRecordHandler` or the v03 substream state machine. Related-but-separate: #1201 (this author) guards the trailing `endSheet` for unmatched sheets — on unpatched main, reading only the chart-shifted sheet without a worksheet match would surface that NPE instead; the two fixes are independent and compose.
   - Incidental observation (not touched here): line 65 of `BofRecordHandler` uses `assert` for the sheet lookup, which is a no-op in production; with the slot accounting fixed the index can no longer run past the list, so the assert stays as documentation only.


-- 
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