Skip to content

fix(sheet): escape hex sequences in head and multi-variable fill cells - #1226

Open
Sean-Walker0 wants to merge 1 commit into
apache:mainfrom
Sean-Walker0:fix/escape-hex-head-and-fill
Open

Sean-Walker0 wants to merge 1 commit into
apache:mainfrom
Sean-Walker0:fix/escape-hex-head-and-fill

Conversation

@Sean-Walker0

Copy link
Copy Markdown
Contributor

Purpose of the pull request

The default EscapeHexCellWriteHandler only sees cells that pass through the converter flow (afterCellDataConverted), so two XLSX write paths stored literal _xHHHH_ sequences unescaped, and POI's reader decoded them into the characters they name:

  • head cells: WriteContextImpl.addOneRowOfHeadDataToExcel wrote the head string via setCellValue directly — a head "_x0041_ col" round-tripped as "A col";
  • multi-variable fill cells: ExcelWriteFillExecutor converted each variable value but assembled the surrounding template text itself and stored the joined string directly — filling p {a} s with a = "_x0041_ x" round-tripped as "p A x s".

This closes the coverage gap disclosed in the Notes of #1223 (data cells, streaming and in-memory, are covered there).

What's changed?

  • The escape scanner moves from the handler into XlsxEscapeUtils.escape(String) — the write half of the existing utfDecode — and the handler delegates to it (single implementation for both directions, as the class javadoc already documents).
  • WriteContextImpl applies XlsxEscapeUtils.escape to head strings for XLSX writes.
  • ExcelWriteFillExecutor applies it to the joined multi-variable fill string for XLSX writes (the variable values there never pass through afterCellDataConverted, so nothing is pre-escaped and double-escaping cannot occur).
  • Both paths stay outside the handler event flow on purpose: firing afterCellDataConverted for heads breaks the documented CellWriteHandler call matrix that WriteHandlerTest pins (19/23 of its cases fail with the hook variant). Escaping stays a property of the default handler set for XLSX, now applied wherever text reaches a cell.

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

EscapeHexHeadAndFillWriteTest (default handler registration, both red before, green after):

  • headCellRoundTripsLiteralEscapeSequences — expected: <_x0041_ col> but was: <A col> before;

  • fillMultiVariableCellRoundTripsLiteralEscapeSequences — expected: <p _x0041_ x s> but was: <p A x s> before.

  • All existing escape suites pass unmodified (EscapeHexCellWriteHandlerTest 19, EscapeHexCellWriteHandlerRoundTripTest 4, EscapeHexInMemoryWriteTest 2) and WriteHandlerTest 23/23 confirms the handler call matrix is unchanged.

  • ./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

The default EscapeHexCellWriteHandler only sees cells that pass
through the converter flow (afterCellDataConverted), so two XLSX
write paths stored literal _xHHHH_ sequences unescaped and POI's
reader decoded them into the characters they name:

- head cells: WriteContextImpl wrote the head string via
  setCellValue directly;
- multi-variable fill cells: ExcelWriteFillExecutor converted each
  variable value but assembled the surrounding template text itself
  and stored the joined string directly.

A head "_x0041_ col" round-tripped as "A col" and a fill cell
"p {a} s" with a=_x0041_ x as "p A x s".

Share the escape scanner as XlsxEscapeUtils.escape (the write half of
the existing utfDecode), have the handler delegate to it, apply it to
the head string for XLSX writes, and apply it to the joined fill
string for XLSX writes. Both paths stay outside the handler event
flow, keeping the documented CellWriteHandler call matrix untouched.

Verified with EscapeHexHeadAndFillWriteTest (both cases decode and
corrupt before the change and pass after), the existing escape suites
and WriteHandlerTest unchanged, plus the full fesod-sheet suite
(1049 tests) and 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