Skip to content

fix(sheet): honor a sheet-level head class when counting head rows - #1225

Open
Sean-Walker0 wants to merge 1 commit into
apache:mainfrom
Sean-Walker0:fix/sheet-level-head-row-number
Open

Sean-Walker0 wants to merge 1 commit into
apache:mainfrom
Sean-Walker0:fix/sheet-level-head-row-number

Conversation

@Sean-Walker0

Copy link
Copy Markdown
Contributor

Purpose of the pull request

When headRowNumber is 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 via read(file).sheet().head(ComplexHead.class), the workbook holder (which saw no head) answered 1, 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 by ComplexHeadDataTest; 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 explicit headRowNumber and the sheet parameters declare their own head (clazz or head list 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 like headRowNumber(0) (NoHeadDataTest) and headRowNumber(3) (TemplateDataTest) behave exactly as before.

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

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 workbook headRowNumber(0)), TemplateDataTest (explicit workbook headRowNumber(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

  • Behavior change is scoped to reads whose sheet parameters declare their own head while the workbook does not set headRowNumber: those now parse the declared head correctly instead of leaking head rows as data.
  • An intermediate version of this fix keyed on the holder's head property alone and broke the explicit-override tests above; the parameters-provenance check (clazz/head set on the sheet) is what keeps inheritance intact. Disclosed since reviewers may wonder why the condition is not hasHead().
  • The same logic exists verbatim in the EasyExcel lineage; this is a parity fix, not a fesod regression.
  • Orthogonal to open PRs: refactor: decouple read/write responsibilities and introduce ColumnBinding #1128 and feat: honor wildcard custom converters when reading #1086 touch AbstractReadHolder only in the import block and converter-resolution hunks below this change; their hunks merge cleanly.

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 skytin1004 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sean-Walker0 Thanks for the fix! I verified it locally, and everything looks good.
LGTM.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants