Repository navigation
Fix TOML key and table header escaping - #1956
Conversation
PR Summary
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1956 +/- ##
============================================
+ Coverage 92.41% 92.43% +0.01%
- Complexity 3579 3583 +4
============================================
Files 348 348
Lines 7081 7084 +3
Branches 694 695 +1
============================================
+ Hits 6544 6548 +4
+ Misses 371 370 -1
Partials 166 166 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
asolntsev
left a comment
There was a problem hiding this comment.
Thanks! The key quoting looks correct: the bare-key check matches exactly the TOML 1.0 bare-key set, and the tests cover it well. Two small suggestions inline.
While reviewing, I noticed some older bugs in TomlTransformer. This PR doesn't introduce them, so they can go in separate issues or a follow-up:
- Fields after a nested table end up inside it.
Schema.of(field("a", () -> Schema.of(field("x", () -> 1))), field("b", () -> 2))produces[a]\nx = 1\nb = 2, which parses asa.b = 2. Plain fields need to be written before tables. nullvalues are written askey = null, which is not valid TOML.- A
nullfield name becomes the keynulland clashes with a field literally named"null". The duplicate check holds raw names, so it keeps both, and the output has a duplicate key. - Mixed arrays: array-of-tables detection only checks the first element.
List.of(Schema.of(...), "x")throwsClassCastException, andList.of("x", Schema.of(...))writes the schema'stoString()as a string. NaN/Infinityare written in Java form. TOML requiresnan/inf.- Nested lists come out as quoted strings:
List.of(List.of(1, 2))produces[ "[1, 2]" ]instead of[ [ 1, 2 ] ].
| private void writeValue(StringBuilder sb, @Nullable String key, String pathPrefix, @Nullable Object val) { | ||
| private String formatKey(@Nullable String key) { | ||
| String name = String.valueOf(key); | ||
| return name.matches("[A-Za-z0-9_-]+") ? name : value2String(name); |
There was a problem hiding this comment.
Two suggestions:
String.matchescompiles the regex again on every call, so it runs for every field of every record. A precompiled constant avoids that:
private static final Pattern BARE_KEY = Pattern.compile("[A-Za-z0-9_-]+");
...
return BARE_KEY.matcher(name).matches() ? name : value2String(name);- Quoting keys through
value2String, the general value serializer, ties key output to value formatting. A later change there, for examplenullhandling, would silently change keys too. Consider extracting a sharedescapeString(String)helper that bothformatKeyandvalue2Stringcall.
There was a problem hiding this comment.
Thanks for the review! I've addressed both suggestions in 89e4936:
- Precompiled the bare-key regex as a
static final Pattern. - Extracted
quoteAndEscapeString(String), shared byformatKeyandvalue2String, so key formatting no longer depends on the general value serializer.
All 335 TOML tests and the full suite of 21,811 tests pass locally.
I'll address the six pre-existing issues in separate follow-up PRs. Null-value handling is already tracked in #1955, where the intended behavior still needs to be agreed on.
There was a problem hiding this comment.
For null handling, I plan to throw an IllegalArgumentException for null field names, null field values, and null elements in arrays or collections, including nested containers. This avoids silently dropping data or changing its meaning. Literal "null" strings would remain valid as keys and values.
I'm happy to adjust this policy if you prefer a different approach.
There was a problem hiding this comment.
Thanks for the update, 89e4936 looks good.
About null handling: I agree with throwing for null field names and for null elements inside arrays/collections, because TOML can't represent either of them.
For null field values, though, I'd rather not throw. Datafaker schemas often have optional or nullable fields. If TOML throws on them, the same schema works with JSON/CSV but fails with TOML. TOML has no null, and the usual way to say "no value" is to leave the key out. So I suggest:
field("a", () -> null)→ thea = ...line isn't written at all- a nested
Schemaor array-of-tables that is null → its[a]/[[a]]header isn't written either nullfield name →IllegalArgumentExceptionnullelement in an array/collection (including nested ones) →IllegalArgumentException- the literal string
"null"stays a normal key/value, as you said
Let's continue this in #1955 and keep this PR as it is.
|
The JDK 21 / macOS job passed the Maven build, but failed while uploading test results with Could you please rerun the failed job? I don't have the rerun option available. Thanks! |
Summary
Fixes #1954.
Null-value handling and the separate test cleanup discussed in #1949 are not included.
Tests
Added parameterized regression tests for key names containing spaces, dots, quotes, backslashes, Unicode, and control characters, including nested tables and arrays of tables.
Verified locally with Java 17:
.\mvnw.cmd -B "-Dtest=TomlTest" test -PnoGpg— 335 tests passed..\mvnw.cmd -B test -PnoGpg— 21,811 tests passed.