Quote null value that starts a record in minimal quote mode - #629
Open
rootvector2 wants to merge 1 commit into
Open
Quote null value that starts a record in minimal quote mode#629rootvector2 wants to merge 1 commit into
rootvector2 wants to merge 1 commit into
Conversation
a null value bypasses `printWithQuotes`, so the empty-value encapsulation minimal mode does at the start of a record never runs and a single-column null record prints as a blank line that `ignoreEmptyLines` drops on read.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes CSV printing of a leading null value (rendering as empty) so it’s properly quoted in minimal-quoting scenarios, preventing the record from being emitted as an “empty line” that CSVFormat.DEFAULT parsers can drop via ignoreEmptyLines.
Changes:
- Add quoting behavior for leading
null/empty values when quote char is set and quote mode is MINIMAL or unset. - Add/adjust unit tests to cover the round-trip regression and updated output expectations for leading nulls.
- Update an existing format-related assertion to reflect the new quoting of a leading empty field.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/csv/CSVFormat.java | Adds special-case quoting for leading empty null values under minimal quote policy. |
| src/test/java/org/apache/commons/csv/CSVPrinterTest.java | Adds regression test for dropped-record scenario and updates expectations for object-array printing. |
| src/test/java/org/apache/commons/csv/CSVFormatTest.java | Updates expected output to account for leading empty-field encapsulation. |
Comments suppressed due to low confidence (1)
src/main/java/org/apache/commons/csv/CSVFormat.java:2276
- This fixes the dropped-record case for MINIMAL/unset, but the same dropped-record behavior can still occur for
QuoteMode.ALL(which conceptually should quote empty fields too). WithquoteMode == ALL, a leading null that renders empty will still be output as a blank line and can be dropped when parsing withignoreEmptyLines. Consider extending the condition to also encapsulate forQuoteMode.ALL(while still excludingALL_NON_NULL/NON_NUMERICas intended), or otherwise aligning null-handling with the quote-mode semantics so ‘quote all’ doesn’t emit an empty record as an empty line.
private void print(final Object object, final CharSequence value, final Appendable out, final boolean newRecord) throws IOException {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2284
to
+2294
| if (len == 0 && newRecord && isQuoteCharacterSet() && isMinimalQuoteMode()) { | ||
| // Encapsulate like printWithQuotes does for an empty value that starts a record: an | ||
| // unquoted one makes the whole line empty, and a parser with ignoreEmptyLines enabled | ||
| // then drops the record. ALL_NON_NULL and NON_NUMERIC are excluded because they encode | ||
| // null as the bare empty field. | ||
| final char quoteChar = quoteCharacter.charValue(); // Explicit unboxing is intentional | ||
| out.append(quoteChar); | ||
| out.append(quoteChar); | ||
| } else { | ||
| out.append(value); | ||
| } |
Comment on lines
+1732
to
+1733
| @Test | ||
| void testPrintNullValueStartingRecord() throws IOException { |
Comment on lines
+1747
to
+1752
| // An explicit MINIMAL quote mode encapsulates it too. | ||
| assertEquals("\"\"" + RECORD_SEPARATOR, printNullRecord(CSVFormat.DEFAULT.builder().setQuoteMode(QuoteMode.MINIMAL).get())); | ||
| // ALL_NON_NULL encodes null as the bare empty field, so it must stay unquoted. | ||
| assertEquals(RECORD_SEPARATOR, printNullRecord(CSVFormat.DEFAULT.builder().setQuoteMode(QuoteMode.ALL_NON_NULL).get())); | ||
| // Without a quote character there is nothing to encapsulate with. | ||
| assertEquals(RECORD_SEPARATOR, printNullRecord(CSVFormat.DEFAULT.builder().setQuote(null).get())); |
Comment on lines
+1888
to
+1889
| assertEquals(16, charArrayWriter.size()); | ||
| assertEquals("\"\"\n\"\"\n\"\"\n\n\"\"\n\"\"\n", charArrayWriter.toString()); |
garydgregory
requested changes
Jul 28, 2026
garydgregory
left a comment
garydgregory
left a comment
Member
There was a problem hiding this comment.
@rootvector2
Thank you for the PR.
Please review the copilot comments.
garydgregory
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
print(Object, CharSequence, Appendable, boolean)sends a null value straight toout.append(value)and never reachesprintWithQuotes, so the encapsulationMINIMALdoes for an empty value at the start of a record is skipped. That rule exists precisely for this case, as its own comment says an empty line has no tokens. The result is that a record whose first value is null, and whose null string renders empty, prints as a blank line.CSVFormat.DEFAULTenablesignoreEmptyLines, so the record is silently dropped when the output is read back: printing[a],[null],[b]givesa\r\n\r\nb\r\n, which parses as 2 records. The same loss hits anyDEFAULT-derived format, includingMONGODB_CSVandINFORMIX_UNLOAD, and any format usingsetNullString(""). Found round-tripping printer output with null values back through the parser.The fix encapsulates the empty null value when it starts a record, a quote character is set, and the quote mode is
MINIMALor unset, which is the same conditionprintWithQuotesalready applies to an emptyCharSequence.ALL_NON_NULLandNON_NUMERICare left alone since they use the bare empty field to encode null, and formats without a quote character have nothing to encapsulate with. Two existing assertions move because a leading null now prints like a leading"";testPrintRecordsWithObjectArraywas pinning six blank lines for six records, which is the bug itself.mvn; that'smvnon the command line by itself.