Fix trim fields - #18
Conversation
📝 WalkthroughWalkthroughThe ChangesConfig-driven trimming behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant csv_parse_line_inplace
participant add_field
participant csv_utils_trim_whitespace
csv_parse_line_inplace->>add_field: add_field(field, len, config)
add_field->>add_field: check config->trimFields
alt trimFields true
add_field->>csv_utils_trim_whitespace: trim_whitespace(field, len)
csv_utils_trim_whitespace-->>add_field: trimmed result / status
else trimFields false
add_field->>add_field: keep field as-is
end
add_field-->>csv_parse_line_inplace: field appended
Related issues: Suggested labels: bug, parser Suggested reviewers: none identified 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: 2
🧹 Nitpick comments (1)
tests/test_csv_parser.c (1)
86-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale/copy-pasted comments.
All three assertions say
"// Leading spaces preserved"but actually assert the values are fully trimmed ("field1","field2","field3") — copied from thetrimFields = falseblock above without updating.✏️ Suggested fix
- assert(strcmp(result2.fields.fields[0], "field1") == 0); // Leading spaces preserved - assert(strcmp(result2.fields.fields[1], "field2") == 0); // Leading spaces preserved - assert(strcmp(result2.fields.fields[2], "field3") == 0); // Leading spaces preserved + assert(strcmp(result2.fields.fields[0], "field1") == 0); // Leading and trailing spaces trimmed + assert(strcmp(result2.fields.fields[1], "field2") == 0); // Leading and trailing spaces trimmed + assert(strcmp(result2.fields.fields[2], "field3") == 0); // Leading and trailing spaces trimmed🤖 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 86 - 88, Update the copied assertions in the CSV parser test so the inline comments match what the checks actually verify. In the test block around result2.fields.fields, either change the comments on the three strcmp assertions to describe trimmed fields or adjust the expected values if the intent was to preserve leading spaces; keep the naming consistent with the surrounding trimFields test cases.
🤖 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 52-59: The trimming path in add_field is passing the field string
length as the max_len argument to csv_utils_trim_whitespace, which makes clean
fields and empty fields fail incorrectly. Update the call in add_field to pass
the actual allocated buffer capacity for field (len + 1), matching the
arena_alloc size and the csv_utils_trim_whitespace contract. Keep the fix
localized to the trimFields branch so csv_parse_line_inplace and other callers
continue to behave correctly.
In `@tests/test_csv_parser.c`:
- Around line 72-89: Add test coverage in test_csv_parser.c around
csv_parse_line_inplace for trimFields=true with already-clean fields and empty
fields, since the current cases only exercise whitespace-trimmed inputs. Extend
the existing trimFields test block to include inputs like "a,b,c" and "a,,c" and
assert success plus expected field values/count. Reference
csv_parse_line_inplace, CSVParseResult, and the trimFields path so the add_field
capacity issue is exercised and caught.
---
Nitpick comments:
In `@tests/test_csv_parser.c`:
- Around line 86-88: Update the copied assertions in the CSV parser test so the
inline comments match what the checks actually verify. In the test block around
result2.fields.fields, either change the comments on the three strcmp assertions
to describe trimmed fields or adjust the expected values if the intent was to
preserve leading spaces; keep the naming consistent with the surrounding
trimFields test cases.
🪄 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: 9fb61ce2-4ead-4475-8a0b-3df1e946ad25
📒 Files selected for processing (2)
csv_parser.ctests/test_csv_parser.c
|
|
||
| if (config->trimFields) { | ||
| CSVUtilsResult utilsResult = csv_utils_trim_whitespace(field, len); | ||
| if (utilsResult != CSV_UTILS_OK) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Off-by-one: wrong max_len causes spurious failures for clean/empty fields.
csv_utils_trim_whitespace(field, len) passes the string length as the buffer capacity, but the actual buffer allocated for field is len + 1 bytes (Line 45: arena_alloc(arena, len + 1, &ptr)). Per csv_utils_trim_whitespace's contract (from csv_utils.c): it returns CSV_UTILS_ERROR_BUFFER_OVERFLOW when trimmed_len >= max_len, and CSV_UTILS_ERROR_INVALID_INPUT immediately when max_len == 0.
Since trimming can never increase length, trimmed_len == len whenever a field has no leading/trailing whitespace — which then satisfies trimmed_len >= max_len (both equal len) and wrongly returns BUFFER_OVERFLOW. Worse, for empty fields (e.g. the call at Line 130 with len == 0, reachable via consecutive delimiters like "a,,c"), max_len == 0 triggers CSV_UTILS_ERROR_INVALID_INPUT unconditionally.
In both cases add_field returns false, and csv_parse_line_inplace aborts the whole line with "Memory allocation failed" — so with trimFields = true, any already-clean field or any empty field breaks parsing entirely. The tests added in this PR only exercise fields that actually have whitespace to trim, so this doesn't surface there.
🐛 Proposed fix: pass buffer capacity, not string length
if (config->trimFields) {
- CSVUtilsResult utilsResult = csv_utils_trim_whitespace(field, len);
+ CSVUtilsResult utilsResult = csv_utils_trim_whitespace(field, len + 1);
if (utilsResult != CSV_UTILS_OK) {
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (config->trimFields) { | |
| CSVUtilsResult utilsResult = csv_utils_trim_whitespace(field, len); | |
| if (utilsResult != CSV_UTILS_OK) { | |
| return false; | |
| } | |
| } | |
| if (config->trimFields) { | |
| CSVUtilsResult utilsResult = csv_utils_trim_whitespace(field, len + 1); | |
| if (utilsResult != CSV_UTILS_OK) { | |
| return false; | |
| } | |
| } | |
🤖 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 52 - 59, The trimming path in add_field is passing
the field string length as the max_len argument to csv_utils_trim_whitespace,
which makes clean fields and empty fields fail incorrectly. Update the call in
add_field to pass the actual allocated buffer capacity for field (len + 1),
matching the arena_alloc size and the csv_utils_trim_whitespace contract. Keep
the fix localized to the trimFields branch so csv_parse_line_inplace and other
callers continue to behave correctly.
| // Test with trimFields = false | ||
| config->trimFields = false; | ||
| CSVParseResult result1 = csv_parse_line_inplace(" field1 , field2 , field3 ", &arena, config, 1); | ||
| assert(result1.success == true); | ||
| assert(result1.fields.count == 3); | ||
| assert(strcmp(result1.fields.fields[0], " field1") == 0); // Leading spaces preserved | ||
| assert(strcmp(result1.fields.fields[1], " field2") == 0); // Leading spaces preserved | ||
| assert(strcmp(result1.fields.fields[2], " field3") == 0); // Leading spaces preserved | ||
| assert(strcmp(result1.fields.fields[0], " field1 ") == 0); // Leading spaces preserved | ||
| assert(strcmp(result1.fields.fields[1], " field2 ") == 0); // Leading spaces preserved | ||
| assert(strcmp(result1.fields.fields[2], " field3 ") == 0); // Leading spaces preserved | ||
|
|
||
| // Test with quoted fields (should not trim inside quotes) | ||
| CSVParseResult result2 = csv_parse_line_inplace("\" field1 \", field2 ", &arena, config, 2); | ||
| // Test with trimFields = true | ||
| config->trimFields = true; | ||
| CSVParseResult result2 = csv_parse_line_inplace(" field1 , field2 , field3 ", &arena, config, 1); | ||
| assert(result2.success == true); | ||
| assert(result2.fields.count == 2); | ||
| assert(strcmp(result2.fields.fields[0], " field1 ") == 0); | ||
| assert(strcmp(result2.fields.fields[1], " field2") == 0); | ||
|
|
||
| // Test pure trailing whitespace trimming | ||
| CSVParseResult result3 = csv_parse_line_inplace("field1 ,field2\t\t,field3 ", &arena, config, 3); | ||
| assert(result2.fields.count == 3); | ||
| assert(strcmp(result2.fields.fields[0], "field1") == 0); // Leading spaces preserved | ||
| assert(strcmp(result2.fields.fields[1], "field2") == 0); // Leading spaces preserved | ||
| assert(strcmp(result2.fields.fields[2], "field3") == 0); // Leading spaces preserved | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing coverage for clean/empty fields under trimFields = true.
These new cases only cover fields with existing leading/trailing whitespace. Adding a case with an already-clean field (e.g. "a,b,c") or an empty field (e.g. "a,,c") under trimFields = true would have caught the buffer-capacity bug in add_field (see csv_parser.c Lines 52-59), where such fields currently cause the parse to fail.
🤖 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 72 - 89, Add test coverage in
test_csv_parser.c around csv_parse_line_inplace for trimFields=true with
already-clean fields and empty fields, since the current cases only exercise
whitespace-trimmed inputs. Extend the existing trimFields test block to include
inputs like "a,b,c" and "a,,c" and assert success plus expected field
values/count. Reference csv_parse_line_inplace, CSVParseResult, and the
trimFields path so the add_field capacity issue is exercised and caught.
1 participant
trimFields in config now controls whether the parser trims whitespace or not, by using the
csv_utils_trim_whitespacefunction fromcsv_utils.hSummary by CodeRabbit