Skip to content

fix(sheet): escape hex sequences in in-memory XLSX writes too - #1223

Open
Sean-Walker0 wants to merge 1 commit into
apache:mainfrom
Sean-Walker0:fix/inmemory-xlsx-hex-escape
Open

Sean-Walker0 wants to merge 1 commit into
apache:mainfrom
Sean-Walker0:fix/inmemory-xlsx-hex-escape

Conversation

@Sean-Walker0

Copy link
Copy Markdown
Contributor

Purpose of the pull request

EscapeHexCellWriteHandler is registered by default for XLSX writes to keep literal _xHHHH_ sequences intact across a round trip. Its guard required cell instanceof SXSSFCell, so in-memory writes (inMemory(true), whose cells are XSSFCell) were skipped entirely and a literal sequence was stored unescaped: POI's reader decodes _xHHHH_ to the character it names, so "_xB9f0_ and _x1234_" round-tripped as "맰 and ሴ" — silent data corruption on exactly the mode users pick when they need POI features that streaming does not support.

No issue exists for this yet; found while reviewing the default handler set.

What's changed?

The guard now accepts both OOXML cell kinds, SXSSFCell (streaming) and XSSFCell (in-memory). XLS (HSSFCell) and CSV (CsvCell) keep being skipped on purpose: they have no _xHHHH_ decoding, so escaping there would corrupt literals — which is exactly what the existing across-formats round-trip test pins, and those tests pass untouched.

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

EscapeHexInMemoryWriteTest (default handler registration, no explicit handler):

  • inMemoryWriteRoundTripsLiteralEscapeSequences — red before (expected: <_xB9f0_ and _x1234_> but was: <맰 and ሴ>), green after;

  • streamingWriteRoundTripsLiteralEscapeSequences — pins the default-handler streaming path, green before and after.

  • Existing escape tests (EscapeHexCellWriteHandlerTest 19 cases incl. the HSSFCell skip pin, EscapeHexCellWriteHandlerRoundTripTest across XLSX/XLS/CSV): all pass unmodified.

  • ./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: in-memory XLSX writes now escape _xHHHH_ literals like streaming writes do. Writers whose literal files intentionally contain decodable _xHHHH_ (relying on the previous in-memory leak) would see them preserved instead of decoded — that preservation is the handler's documented purpose.
  • Remaining coverage gap (not touched here, disclosed for completeness): header cells and multi-variable fill cells are written via setCellValue outside the afterCellDataConverted hook, so they are never escaped in any mode. Fixing that spans the head and fill write paths and seemed worth its own discussion rather than folding into this fix.
  • Orthogonal to open PRs: no open PR touches EscapeHexCellWriteHandler; the two open PRs touching DefaultWriteHandlerLoader (refactor: Improve the basic javadoc content #1181 javadoc, feat: add support for ODS file format with read/write capabilities #745 ODS case) do not overlap this file.

EscapeHexCellWriteHandler, registered by default for XLSX writes,
guarded on cell instanceof SXSSFCell and therefore skipped in-memory
(XSSFCell) writes entirely: with inMemory(true) a literal _xHHHH_
sequence was stored unescaped and POI's reader decoded it to the
character it names, so "_xB9f0_ and _x1234_" round-tripped as
"\uB9f0 and \u1234" instead of the literal the caller wrote.

Accept both OOXML cell kinds, SXSSFCell and XSSFCell. XLS (HSSFCell)
and CSV (CsvCell) keep being skipped: they have no _xHHHH_ decoding,
so escaping there would corrupt literals - which is exactly what the
existing across-formats round-trip test pins.

Verified with EscapeHexInMemoryWriteTest (the in-memory case corrupts
before the change and passes after; the streaming case pins the
default-handler path on both sides) plus the untouched existing
escape tests, the full fesod-sheet suite (1049 tests) and
spotless:check.

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.

1 participant