Skip to content

Fix trim fields #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Kwanddwo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix trim fields #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c45f763
fix: parser now trims fields based on config
Kwanddwo Jul 2, 2026
c587643
updated trimming tests
Kwanddwo Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions csv_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "csv_parser.h"
#include "arena.h"
#include "csv_utils.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -33,17 +34,13 @@ static bool grow_field_array(FieldArray *arr, Arena *arena) {
return true;
}

static bool add_field(FieldArray *arr, const char *start, size_t len, Arena *arena) {
static bool add_field(FieldArray *arr, const char *start, size_t len, Arena *arena, const CSVConfig *config) {
if (arr->count >= arr->capacity) {
if (!grow_field_array(arr, arena)) {
return false;
}
}

while (len > 0 && (start[len-1] == ' ' || start[len-1] == '\t')) {
len--;
}


void *ptr;
ArenaResult result = arena_alloc(arena, len + 1, &ptr);
if (result != ARENA_OK) {
Expand All @@ -52,6 +49,14 @@ static bool add_field(FieldArray *arr, const char *start, size_t len, Arena *are
char *field = (char*)ptr;
memcpy(field, start, len);
field[len] = '\0';

if (config->trimFields) {
CSVUtilsResult utilsResult = csv_utils_trim_whitespace(field, len);
if (utilsResult != CSV_UTILS_OK) {
return false;
}
}

Comment on lines +52 to +59

@coderabbitai coderabbitai Bot Jul 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

arr->fields[arr->count++] = field;
return true;
}
Expand Down Expand Up @@ -122,7 +127,7 @@ CSVParseResult csv_parse_line_inplace(const char *line, Arena *arena, const CSVC
field_start = &line[pos + 1];
field_len = 0;
} else if (c == config->delimiter) {
if (!add_field(&result.fields, "", 0, arena)) {
if (!add_field(&result.fields, "", 0, arena, config)) {
result.success = false;
result.error = "Memory allocation failed";
result.error_column = pos;
Expand All @@ -139,7 +144,7 @@ CSVParseResult csv_parse_line_inplace(const char *line, Arena *arena, const CSVC

case UNQUOTED_FIELD:
if (c == config->delimiter) {
if (!add_field(&result.fields, field_start, field_len, arena)) {
if (!add_field(&result.fields, field_start, field_len, arena, config)) {
result.success = false;
result.error = "Memory allocation failed";
result.error_column = pos;
Expand Down Expand Up @@ -209,7 +214,7 @@ CSVParseResult csv_parse_line_inplace(const char *line, Arena *arena, const CSVC
return result;
}
} else {
if (!add_field(&result.fields, field_start, field_len, arena)) {
if (!add_field(&result.fields, field_start, field_len, arena, config)) {
result.success = false;
result.error = "Memory allocation failed";
return result;
Expand Down
42 changes: 26 additions & 16 deletions tests/test_csv_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,28 +69,38 @@ void test_csv_parser_whitespace_trimming() {
assert(arena_create(&arena, 4096) == ARENA_OK);
CSVConfig *config = csv_config_create(&arena);

// Test trailing whitespace trimming (parser only trims trailing, not leading)
// 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

Comment on lines +72 to +89

@coderabbitai coderabbitai Bot Jul 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

// Test with quoted fields (should not trim inside quotes)
CSVParseResult result3 = csv_parse_line_inplace("\" field1 \", field2 ", &arena, config, 2);
assert(result3.success == true);
assert(result3.fields.count == 3);
assert(strcmp(result3.fields.fields[0], "field1") == 0); // Trailing spaces trimmed
assert(strcmp(result3.fields.fields[1], "field2") == 0); // Trailing tabs trimmed
assert(strcmp(result3.fields.fields[2], "field3") == 0); // Trailing space trimmed
assert(result3.fields.count == 2);
assert(strcmp(result3.fields.fields[0], " field1 ") == 0);
assert(strcmp(result3.fields.fields[1], "field2") == 0);

// Test pure trailing whitespace trimming with trimFields = true
CSVParseResult result4 = csv_parse_line_inplace("field1 ,field2\t\t,field3 ", &arena, config, 3);
assert(result4.success == true);
assert(result4.fields.count == 3);
assert(strcmp(result4.fields.fields[0], "field1") == 0); // Trailing spaces trimmed
assert(strcmp(result4.fields.fields[1], "field2") == 0); // Trailing tabs trimmed
assert(strcmp(result4.fields.fields[2], "field3") == 0); // Trailing space trimmed

arena_destroy(&arena);
printf("✓ CSV parser whitespace trimming test passed\n");
Expand Down