Implement preserve quotes - #19
Conversation
📝 WalkthroughWalkthroughThe CSV parser's quote-handling logic in ChangesPreserve Quotes Feature
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)Not applicable. Related issues: None specified. Related PRs: None specified. Suggested labels: enhancement, tests Suggested reviewers: None specified. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_csv_parser.c (1)
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case coverage for
preserveQuoteswith empty/adjacent quotes.The new test only covers a quoted field with no internal or boundary-adjacent quotes. Add cases like
""(empty quoted field) and"""quoted"""(escaped quote touching the boundary) withpreserveQuotes = true— these currently produce incorrect output due to the boundary-quote collapsing bug incsv_parser.c(see comment there).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_csv_parser.c` around lines 61 - 68, Extend the preserveQuotes coverage in the CSV parser tests to include edge cases for empty and boundary-adjacent quotes, using csv_parse_line_inplace and the existing preserveQuotes setup in test_csv_parser.c. Add assertions for inputs like "" and """quoted""" so the behavior of CSVParseResult.fields is validated when quotes are empty or touch the field boundary. Keep the new checks alongside the current preserveQuotes=true test so the boundary-quote collapsing bug in csv_parse_line_inplace is exercised directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@csv_parser.c`:
- Around line 122-128: The preserveQuotes handling in the quoted-field path is
feeding boundary enclosure chars into add_quoted_field, which then mis-collapses
them as escaped quotes. Keep field_start/field_len pointed only at the inner
content in the FIELD_START/QUOTED_FIELD logic, and let add_quoted_field wrap the
enclosure chars itself outside its escape-collapsing loop. Update both call
sites that invoke add_quoted_field to pass preserveQuotes so it can decide
whether to re-add the boundary quotes without scanning them as input.
---
Nitpick comments:
In `@tests/test_csv_parser.c`:
- Around line 61-68: Extend the preserveQuotes coverage in the CSV parser tests
to include edge cases for empty and boundary-adjacent quotes, using
csv_parse_line_inplace and the existing preserveQuotes setup in
test_csv_parser.c. Add assertions for inputs like "" and """quoted""" so the
behavior of CSVParseResult.fields is validated when quotes are empty or touch
the field boundary. Keep the new checks alongside the current
preserveQuotes=true test so the boundary-quote collapsing bug in
csv_parse_line_inplace is exercised directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91aad80a-c0b4-468e-ad61-f14bc1215b2a
📒 Files selected for processing (2)
csv_parser.ctests/test_csv_parser.c
| if (!config->preserveQuotes) { | ||
| field_start = &line[pos + 1]; | ||
| field_len = 0; | ||
| } else { | ||
| field_start = &line[pos]; | ||
| field_len = 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Boundary quotes get mis-collapsed by add_quoted_field's escape logic when preserveQuotes is enabled.
Including the opening/closing enclosure chars directly in field_start/field_len means the span later passed to add_quoted_field (lines 179, 214) now contains the boundary quote characters. add_quoted_field's existing escape-collapsing loop (lines 75-82) treats any two consecutive enclosure characters as an escaped pair and collapses them to one — it has no way to distinguish "real" escaped quotes from the artificially-included boundary quotes.
Concrete repro: parsing "",normal with preserveQuotes = true:
field_start=&line[0],field_len= 2 (span ="").add_quoted_fieldseesstart[0]=='"'andstart[1]=='"'and collapses them into a single".- Result: the empty quoted field becomes
"instead of the expected"".
The same mis-collapse happens whenever content immediately adjacent to a boundary quote is itself an escaped quote (e.g. """quoted"""), producing output that is neither the verbatim source nor correctly re-quoted content.
Since add_quoted_field itself wasn't updated for this PR, this is a latent correctness bug not caught by the added test (which uses a quoted field with no adjacent/internal quotes).
🛠 Suggested fix — don't feed boundary quotes into the escape-scan; wrap after unescaping
Keep field_start/field_len scoped to content only (as before), and instead have add_quoted_field write the enclosure chars directly, outside the escape-scanning loop:
-static bool add_quoted_field(FieldArray *arr, const char *start, size_t len, Arena *arena, char enclosure) {
+static bool add_quoted_field(FieldArray *arr, const char *start, size_t len, Arena *arena, char enclosure, bool preserve_quotes) {
if (arr->count >= arr->capacity) {
if (!grow_field_array(arr, arena)) {
return false;
}
}
void *ptr;
- ArenaResult result = arena_alloc(arena, len + 1, &ptr);
+ ArenaResult result = arena_alloc(arena, len + 3, &ptr);
if (result != ARENA_OK) {
return false;
}
char *field = (char*)ptr;
size_t write_pos = 0;
+ if (preserve_quotes) field[write_pos++] = enclosure;
for (size_t i = 0; i < len; i++) {
if (start[i] == enclosure && i + 1 < len && start[i + 1] == enclosure) {
field[write_pos++] = enclosure;
i++;
} else {
field[write_pos++] = start[i];
}
}
-
+ if (preserve_quotes) field[write_pos++] = enclosure;
field[write_pos] = '\0';Then revert the state-machine changes to always exclude the boundary quotes from field_start/field_len:
case FIELD_START:
if (c == config->enclosure) {
state = QUOTED_FIELD;
field_start = &line[pos + 1];
field_len = 0;
}case QUOTED_FIELD:
if (c == config->enclosure) {
if (pos + 1 < len && line[pos + 1] == config->enclosure) {
field_len += 2;
pos++;
} else {
state = FIELD_END;
}
}And pass config->preserveQuotes at both call sites (lines 179 and 214):
add_quoted_field(&result.fields, field_start, field_len, arena, config->enclosure, config->preserveQuotes)Also applies to: 167-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@csv_parser.c` around lines 122 - 128, The preserveQuotes handling in the
quoted-field path is feeding boundary enclosure chars into add_quoted_field,
which then mis-collapses them as escaped quotes. Keep field_start/field_len
pointed only at the inner content in the FIELD_START/QUOTED_FIELD logic, and let
add_quoted_field wrap the enclosure chars itself outside its escape-collapsing
loop. Update both call sites that invoke add_quoted_field to pass preserveQuotes
so it can decide whether to re-add the boundary quotes without scanning them as
input.
1 participant
This pr implements the preserveQuotes feature, where setting
config->preserveQuotesto true will let you preserve quotes in quoted fields.test_csv_parser.cto test preserveQuotesSummary by CodeRabbit
Bug Fixes
Tests