Skip to content

Pass normalized separator in CsvSchema.withArrayElementSeparator (avoid null state / NPE) - #710

Open
uttam12331 wants to merge 1 commit into
FasterXML:2.21from
uttam12331:fix/with-array-element-separator-null
Open

Pass normalized separator in CsvSchema.withArrayElementSeparator (avoid null state / NPE)#710
uttam12331 wants to merge 1 commit into
FasterXML:2.21from
uttam12331:fix/with-array-element-separator-null

Conversation

@uttam12331

Copy link
Copy Markdown

Summary

CsvSchema.withArrayElementSeparator(String separator) normalizes a null argument to "" in the local sep and uses sep for the equality check, but then passes the raw separator to the copy constructor:

public CsvSchema withArrayElementSeparator(String separator) {
    String sep = separator == null ? "" : separator;
    return (_arrayElementSeparator.equals(sep)) ? this : new CsvSchema(_columns, _features,
        _columnSeparator, _quoteChar, _escapeChar, _lineSeparator, separator, // <-- should be `sep`
        _nullValue, _columnsByName, _anyPropertyName);
}

The copy constructor stores the value directly, so withArrayElementSeparator(null) leaves _arrayElementSeparator == null.

Impact

This breaks the field's non-null invariant:

  • hasArrayElementSeparator() does return !_arrayElementSeparator.isEmpty();NullPointerException.
  • A subsequent withArrayElementSeparator(...) call NPEs on _arrayElementSeparator.equals(sep).

Every sibling builder stores exactly the value it compared against (e.g. withColumnSeparator compares _columnSeparator == sep and constructs with sep); withArrayElementSeparator is the only one that compares sep but constructs with separator.

Fix

Pass the already-normalized sep:

-            _columnSeparator, _quoteChar, _escapeChar, _lineSeparator, separator,
+            _columnSeparator, _quoteChar, _escapeChar, _lineSeparator, sep,

Tests

Added CsvSchemaTest.testWithArrayElementSeparatorNull, which sets a separator, clears it with withArrayElementSeparator(null), and asserts hasArrayElementSeparator() returns false (and does not throw) — this fails with an NPE on the current code and passes with the fix.

Happy to add a release-notes/CREDITS entry or sign the CLA if needed.

@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Aug 3, 2026
@cowtowncoder

Copy link
Copy Markdown
Member

Sounds good! CLA needed if not sent before -- adding release notes would be great too.

But one question/suggestion: might make sense to target 2.21 as backport (2.21 latest LTS version)?
I can then merge forward to 2.22 -> 2.x -> 3.1 et.

…state

`CsvSchema.withArrayElementSeparator(String)` normalizes a null argument to
"" in the local `sep` and uses it for the equality check, but then passes the
raw `separator` to the copy constructor, so `withArrayElementSeparator(null)`
leaves `_arrayElementSeparator` null. `hasArrayElementSeparator()` then NPEs on
`!_arrayElementSeparator.isEmpty()`, as does a later `withArrayElementSeparator`
on `_arrayElementSeparator.equals(sep)`.

Pass the already-normalized `sep`, add a regression test, and add release notes.
@uttam12331
uttam12331 force-pushed the fix/with-array-element-separator-null branch from e31a3b8 to 3a43684 Compare August 4, 2026 05:37
@uttam12331
uttam12331 changed the base branch from 2.x to 2.21 August 4, 2026 05:37
@uttam12331

Copy link
Copy Markdown
Author

Thanks @cowtowncoder! Retargeted this to 2.21 and added the VERSION-2.x and CREDITS-2.x entries under 2.21.6 so it can merge forward as you described. Happy to adjust the release-notes wording if you'd prefer it phrased differently. I'll get the CLA sent over.

@uttam12331

Copy link
Copy Markdown
Author

I've filled and signed the 2026 ICLA (CLA-jackson-2026.pdf). Since the newer CLA file doesn't state a submission address, what's the best way to get it to you — email it, or attach it here?

@cowtowncoder

Copy link
Copy Markdown
Member

@uttam12331 email to "cla" at fasterxml dot com.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-needed PR looks good (although may also require code review), but CLA needed from submitter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants