Skip to content

fix(csv): function parse option fieldsPerRecord when negative records may have less fields #7153

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
PengjuXu wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −2
Open

fix(csv): function parse option fieldsPerRecord when negative records may have less fields #7153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion csv/_io.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,16 +228,26 @@ export function convertRowToObject(
row: readonly string[],
headers: readonly string[],
zeroBasedLine: number,
/* allow less fields in a row than headers */
allowLessRowFields = false,
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

Two small nits:

  • Use // for one-line comments — that's the convention in this file (see e.g. parse.ts:229).
  • allowLessRowFields → allowFewerRowFields ("fewer" for countables).

Also, threading a boolean flag through an internal helper for one call-site is a bit awkward. Consider passing fieldsPerRecord (or { variable: boolean }) so the policy lives inside the helper rather than at every caller — this would also make it easier to fix CsvParseStream symmetrically.

) {
if (row.length !== headers.length) {
if (!allowLessRowFields && row.length !== headers.length) {
throw new Error(
`Syntax error on line ${
zeroBasedLine + 1
}: The record has ${row.length} fields, but the header has ${headers.length} fields`,
);
}
if (allowLessRowFields && row.length > headers.length) {
throw new Error(
`Syntax error on line ${
zeroBasedLine + 1
}: The record has ${row.length} fields, but the header allows maximum ${headers.length} fields`,
);
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

Two issues here:

  1. Message wording: the header doesn't "allow" anything. Please mirror the existing expected N fields but got M style used elsewhere (see parse_stream.ts:453): e.g. expected at most ${headers.length} fields but got ${row.length}.
  2. This throws plain Error, but the rest of the parser throws SyntaxError (see parse_stream.ts:450 and the doc example at parse.ts:486). The pre-existing throw above is also Error — worth fixing both while you're here for consistency.

}
const out: Record<string, unknown> = {};
for (const [index, header] of headers.entries()) {
if (index === row.length) break;
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

Minor: a mid-loop break reads a bit awkwardly. Could be expressed more directly as:

const limit = Math.min(row.length, headers.length);
for (let i = 0; i < limit; i++) {
  out[headers[i]] = row[i];
}

or headers.slice(0, row.length).forEach(...). Not blocking.

out[header] = row[index];
}
return out;
Expand Down
7 changes: 6 additions & 1 deletion csv/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,12 @@ export function parse<const T extends ParseOptions>(

const zeroBasedFirstLineIndex = options.skipFirstRow ? 1 : 0;
return r.map((row, i) => {
return convertRowToObject(row, headers, zeroBasedFirstLineIndex + i);
return convertRowToObject(
row,
headers,
zeroBasedFirstLineIndex + i,
(options?.fieldsPerRecord ?? 0) < 0,
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

If you adopt the suggestion to push the fieldsPerRecord < 0 check inside convertRowToObject, this call site becomes a plain convertRowToObject(row, headers, zeroBasedFirstLineIndex + i, options.fieldsPerRecord) and the same change can be applied verbatim in parse_stream.ts:460.

);
}) as ParseResult<ParseOptions, T>;
}
return r as ParseResult<ParseOptions, T>;
Expand Down
52 changes: 52 additions & 0 deletions csv/parse_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,29 @@ Deno.test({
);
},
});

await t.step({
name: "Allow less row fields than columns",
fn() {
const input = "a,b,c\nd,e";
const output = [{
foo: "a",
bar: "b",
baz: "c",
}, {
foo: "d",
bar: "e",
}];
assertEquals(
parse(input, {
skipFirstRow: false,
columns: ["foo", "bar", "baz"],
fieldsPerRecord: -1,
}),
output,
);
},
});
await t.step({
name: "NegativeFieldsPerRecord",
fn() {
Expand All @@ -320,6 +343,19 @@ Deno.test({
assertEquals(parse(input, { fieldsPerRecord: -1 }), output);
},
});
await t.step({
name: "LessFieldsPerRecordThanFirstLine",
fn() {
const input = `a,b,c\nd,e`;
const output = [
{ a: "d", "b": "e" },
];
assertEquals(
parse(input, { skipFirstRow: true, fieldsPerRecord: -1 }),
output,
);
},
});
await t.step({
name: "FieldCount",
fn() {
Expand Down Expand Up @@ -860,6 +896,22 @@ c"d,e`;
);
},
});
await t.step({
name: "mismatching number of headers and fields 3",
fn() {
const input = "a,b,c\nd,e,,g";
assertThrows(
() =>
parse(input, {
skipFirstRow: true,
columns: ["foo", "bar", "baz"],
fieldsPerRecord: -1,
}),
Error,
"Syntax error on line 2: The record has 4 fields, but the header allows maximum 3 fields",
);
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju May 26, 2026

Choose a reason for hiding this comment

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

Test name "mismatching number of headers and fields 3" is opaque — please name it after the case being asserted, e.g. "throws when row has more fields than columns with fieldsPerRecord: -1". Same suggestion for the new step at line ~313 — "Allow less row fields than columns" could be "allows rows with fewer fields than columns when fieldsPerRecord is negative".

},
});
await t.step({
name: "Strips leading byte-order mark with bare cell",
fn() {
Expand Down
Loading