Skip to content

fix: decode _xHHHH_ escapes when reading inline string cells - #991

Open
nkuprins wants to merge 11 commits into
apache:mainfrom
nkuprins:fix/decode-inline-string-utf-escapes
Open

fix: decode _xHHHH_ escapes when reading inline string cells#991
nkuprins wants to merge 11 commits into
apache:mainfrom
nkuprins:fix/decode-inline-string-utf-escapes

Conversation

@nkuprins

@nkuprins nkuprins commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Related: #696

Purpose of the pull request

Characters that XML 1.0 forbids are stored in a cell as _xHHHH_ escapes. Fesod undid them only for cells backed by sharedStrings.xml, so t="inlineStr" and t="str" reached the caller with the raw escape. A value written by Fesod did not survive being read back by Fesod. The clearest case is a value escaped with EscapeHexCellWriteHandler, which stores the literal _xB9f0_ as _x005F_xB9f0_ precisely so a decoding reader restores it:

reading a cell written as Product_x005F_xB9f0_Code result
POI Product_xB9f0_Code
Fesod, before / after Product_x005F_xB9f0_Code / Product_xB9f0_Code

Why this is a defect and not intended behaviour:

  • Decoding is already an asserted contract for the other storage form - CompatibilityTest#readXlsxWithEscapeSequence pins it for sharedStrings.xml.
  • POI decodes inline strings on both of its read paths - the DOM XSSFCell and the streaming XSSFSheetXMLHandler, each via XSSFRichTextString#getString().
  • No test asserts that a raw escape survives a read.

What's changed?

  • XlsxEscapeUtils (new) - utfDecode moved here from SharedStringsTableHandler unchanged, and both read paths now call it. The smaller change would have been to make the existing method public and call it from CellTagHandler, but that leaves a cell parser depending on the sharedStrings.xml parser for decoding that has nothing to do with shared strings, and the next caller inherits the same detour. The escape convention belongs to neither handler, so it moved to util/. Happy to switch to the two-line version if you prefer the smaller diff.
  • CellTagHandler - the DIRECT_STRING branch now decodes instead of falling through to the ERROR branch; it sets the same STRING type as before.
  • HexEscapeRoundTripTest (new) - one round trip: a cell written as Product_x0002_Code must read back as Product\u0002Code.

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.

Copilot AI 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.

Pull request overview

This PR fixes XLSX string decoding so OOXML _xHHHH_ escape sequences are consistently decoded for both shared-strings and inline/direct string cells (t="inlineStr" / t="str"), restoring correct read-after-write behavior (notably for values written with EscapeHexCellWriteHandler).

Changes:

  • Introduces XlsxEscapeUtils.utfDecode(...) and reuses it across both shared-string and inline/direct-string read paths.
  • Updates CellTagHandler to correctly handle DIRECT_STRING cells by decoding escapes (instead of effectively treating them as error strings).
  • Adds a regression test covering inline-string decoding and the literal-escape round-trip behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
fesod-sheet/src/test/java/org/apache/fesod/sheet/analysis/v07/handlers/InlineStringUtfDecodeTest.java Adds regression coverage for inline/direct string escape decoding and literal-escape round-trips.
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/XlsxEscapeUtils.java Centralizes OOXML _xHHHH_ decoding logic for reuse across read paths.
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/sax/SharedStringsTableHandler.java Switches shared-strings decoding to the new shared utility.
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/v07/handlers/CellTagHandler.java Decodes escapes for DIRECT_STRING cells and normalizes the resulting type to STRING.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@alaahong alaahong left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice cleanup — moving utfDecode to util/ so CellTagHandler doesn't depend on the shared-strings parser is the right call, and t="str" is covered since it maps to DIRECT_STRING. Comments inline. One more note: the escape convention is now implemented twice (XlsxEscapeUtils for reading, EscapeHexCellWriteHandler for writing); consider cross-referencing them or unifying in XlsxEscapeUtils to prevent silent drift.

tempCellData.setStringValue(stringValue);
break;
case DIRECT_STRING:
// Undo the '_xHHHH_' escapes of characters XML forbids

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The reader now decodes _xHHHH_, but the default writer (SXSSF inline strings) never escapes them. Verified locally: writing Product_x0002_Code with the default writer and reading it back now returns Product\u0002Code (STX control char) instead of the literal — before this change it returned the literal. For third-party files this is the correct fix for #696, but for fesod→fesod round trips it's a silent behaviour change. Suggest aligning the writer (escape by default, like EscapeHexCellWriteHandler) or documenting that literal _xHHHH_ text requires registering that handler.

@nkuprins nkuprins Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @alaahong,

I see, but if I take the first option (escape by default), then do you agree that we need an opt-out - escapeHexText(false) on the write? Without it, escapes that arrived from another producer get escaped again on the way in, with no way to write them through unchanged.

Also, probably escape by default isn't a small addition to this PR, so not sure if I should do it here.

Thank you for the review!

@nkuprins nkuprins Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I chose to add javadoc note instead, as suggested in the second option. This probably belongs on the website too, but I'm not sure which section - FAQ?

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.

4 participants