Skip to content

fix: honor sheet-level autoTrim/autoStrip for CSV read and write - #1191

Open
LuciferYang wants to merge 3 commits into
apache:mainfrom
LuciferYang:fix/csv-builder-autotrim
Open

LuciferYang wants to merge 3 commits into
apache:mainfrom
LuciferYang:fix/csv-builder-autotrim

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Oct 7, 2026 •

Copy link
Copy Markdown

Purpose of the pull request

Closed: #1190

csv() returns a builder whose parameter object is the ReadSheet / WriteSheet, so autoTrim() and autoStrip() called after csv() are stored on the sheet. The CSV builders only read the workbook flags when they configure CSVFormat trimming, and the CSV read executor takes the flags from the workbook holder. So .csv().autoTrim(false) and .sheet().autoTrim(false) on a CSV file are ignored, on both read and write.

What's changed?

  • CsvReaderBuilder.buildExcelReader() and CsvWriterBuilder.buildExcelWriter() use the sheet flags when they are set and fall back to the workbook flags otherwise.
  • CsvExcelReadExecutor.dealRecord() reads autoTrim / autoStrip from currentReadHolder(), the sheet holder, as the xls and xlsx readers already do. This also makes .sheet().autoTrim(false) work for CSV, which never goes through CsvReaderBuilder.

Nothing changes for code that only sets the flags at workbook level, because the sheet flags default to null and inherit from the workbook. Code that already calls .csv().autoTrim(false) or .sheet().autoTrim(false) on CSV will now get untrimmed values, header cells included, which matches xlsx.

On the write side autoStrip(true) still maps to CSVFormat trim, so it removes ASCII whitespace only. That was already the case for the workbook-level flag.

Tests in CsvFormatTest:

  • testSheetAutoTrim: .csv().autoTrim(false) and .sheet().autoTrim(false) keep " a "; a sheet flag overrides the opposite workbook flag for both autoTrim and autoStrip.
  • testSheetAutoStrip: with autoTrim(false), autoStrip(true) set after csv() removes U+3000 and autoStrip(false) keeps it.
  • testWriteSheetAutoTrim: checks the exact written record for .csv().autoTrim(false) and for a sheet flag overriding the workbook flag.
  • testAutoTrim now also checks the written file and the value read back for the existing workbook-level case.

Each of the three source changes, reverted on its own, makes at least one of these fail.

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.

@BigDataDZ BigDataDZ 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.

LGTM - the sheet-over-workbook precedence matches the holder chain convention (sheet parameters override workbook ones), and moving dealRecord to currentReadHolder() finally aligns CSV with what the xls and xlsx readers already do. The trim expression semantics are preserved exactly (autoStrip still forces trimming).

One small test suggestion: the suite now covers sheet-level overrides (testSheetAutoTrim / testSheetAutoStrip / testWriteSheetAutoTrim) and the default, but not the explicit fallback branch - a case where autoTrim(false) is set only on the workbook parameter and the sheet flag stays null, asserting trimming is actually off in the output. That is the readSheet.getAutoTrim() != null ? ... : readWorkbook.getAutoTrim() fallback path, and locking it protects against a future "just read the sheet flag" simplification. One parameterized case each for read and write would do.

@LuciferYang

Copy link
Copy Markdown
Author

@BigDataDZ The workbook-only fallback is already tested on both sides. csv() creates a new ReadSheet / WriteSheet with null flags, so .autoTrim(false).csv() in testAutoTrim goes through that branch, for the written file and for the value read back; testSheetAutoTrim has the same read case (L195-203). To confirm, I changed each builder to read only the sheet flag: the reader change fails testAutoTrim and testSheetAutoTrim, the writer change fails testAutoTrim.

@BigDataDZ

Copy link
Copy Markdown
Contributor

You are right - .autoTrim(false).csv() sets the workbook flag and csv() then creates a fresh sheet with null flags, so testAutoTrim walks the fallback branch for both directions. Retracting the suggestion, and thanks for walking me through it.

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.

[Bug] autoTrim/autoStrip on the CSV sheet builders are ignored when reading and writing CSV

2 participants