Repository navigation
fix(sheet): honor a sheet-level head class when counting head rows - #1225
Open
Sean-Walker0 wants to merge 1 commit into
Open
Sean-Walker0 wants to merge 1 commit into
Sean-Walker0 wants to merge 1 commit into
Conversation
When the read parameters did not set headRowNumber explicitly, the sheet holder always inherited the workbook holder's value, even when the sheet builder declared its own head class. For a model with a multi-row head read via read(file).sheet().head(ComplexHead.class), the sheet head row count was ignored: the workbook holder (without any head) had answered 1, so the remaining head rows were delivered as data rows and name-bound fields read as garbage. Sheet-declared heads (clazz or list set on the sheet parameters) now define the head row count for their sheet; heads inherited from the workbook keep inheriting its headRowNumber, which preserves the explicit headRowNumber(0)/headRowNumber(3) workbook overrides covered by NoHeadDataTest and TemplateDataTest. Verified with SheetLevelHeadClassTest (the sheet-level case leaks head rows as data before the change and passes after; the workbook-level case pins the working form on both sides), the existing head and template suites, and the full fesod-sheet suite (1049 tests) plus spotless:check.
skytin1004
approved these changes
Oct 16, 2026
skytin1004
left a comment
Contributor
There was a problem hiding this comment.
@Sean-Walker0 Thanks for the fix! I verified it locally, and everything looks good.
LGTM.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the pull request
When
headRowNumberis not set explicitly, the sheet holder always inherits the workbook holder's value — even when the sheet builder declares its own head class. For a multi-row-head model read viaread(file).sheet().head(ComplexHead.class), the workbook holder (which saw no head) answered1, so the model's remaining head rows were delivered as data rows: a 3-row head file returned data rows containing head text, and name-bound fields read as garbage.The working form (
read(file).head(ComplexHead.class).sheet()) is pinned byComplexHeadDataTest; the sheet-level form is untested and broken.No issue exists for this yet; found by field-by-field audit of the read holder initialization.
What's changed?
In
AbstractReadHolder, when the parameters carry no explicitheadRowNumberand the sheet parameters declare their own head (clazzorheadlist set on the sheet), the sheet's own head row count now defines the head rows for that sheet. Heads inherited from the workbook (sheet parameters carry neither) keep inheriting the workbook value, so explicit workbook overrides likeheadRowNumber(0)(NoHeadDataTest) andheadRowNumber(3)(TemplateDataTest) behave exactly as before.Checklist
Verifying this change
SheetLevelHeadClassTest(both forms of the same written file):sheetLevelHeadClassParsesMultiRowHead— red before (the multi-row head must not leak into the data rows ==> expected: <2> but was: <4>), green after;workbookLevelHeadClassParsesMultiRowHead— pins the working form, green before and after.Existing suites covering every inheritance combination:
NoHeadDataTest(explicit workbookheadRowNumber(0)),TemplateDataTest(explicit workbookheadRowNumber(3)),ComplexHeadDataTest,MaxHeadSizeTest,ImmutableListHeadDataTest— all pass../mvnw clean package -B -Dmaven.test.skip=false -pl fesod-common,fesod-shaded,fesod-sheet: 1049 tests, 0 failures, 0 errors../mvnw -pl fesod-sheet spotless:check: clean.Notes
headRowNumber: those now parse the declared head correctly instead of leaking head rows as data.clazz/headset on the sheet) is what keeps inheritance intact. Disclosed since reviewers may wonder why the condition is nothasHead().AbstractReadHolderonly in the import block and converter-resolution hunks below this change; their hunks merge cleanly.