Skip to content

fix(csv-stringify): do not alter the source record when columns is set - #504

Open
youdie006 wants to merge 1 commit into
adaltas:masterfrom
youdie006:fix-stringify-source-record-mutation
Open

fix(csv-stringify): do not alter the source record when columns is set#504
youdie006 wants to merge 1 commit into
adaltas:masterfrom
youdie006:fix-stringify-source-record-mutation

Conversation

@youdie006

Copy link
Copy Markdown

When a record is an array and columns is shorter than that record, stringify truncates the caller's array in place:

https://github.com/adaltas/node-csv/blob/745f045/packages/csv-stringify/lib/api/index.js#L113-L120

chunk here is the record the user passed in, not a copy, so splice deletes their data. The output is correct; the caller's input is destroyed as a side effect.

Reproduction

import { stringify } from "csv-stringify";

const rows = [
  ["id-1", "alice", "eng"],
  ["id-2", "bob", "sales"],
];

stringify(rows, { columns: ["id", "name"] }, (_, summary) => {
  stringify(rows, { columns: ["id", "name", "team"] }, (_, full) => {
    console.log(JSON.stringify(summary));
    console.log(JSON.stringify(full));
  });
});
before  "id-1,alice\nid-2,bob\n"
        "id-1,alice\nid-2,bob\n"      <- the "team" column is gone
after   "id-1,alice\nid-2,bob\n"
        "id-1,alice,eng\nid-2,bob,sales\n"

Writing a narrow summary and then a full export from the same rows gives the wrong answer on the second call. Nothing warns the caller, and the loss is permanent.

Why this looks like an oversight rather than a contract

The object branch of the same function already avoids this: it reads through get(chunk, columns[i].key) into a fresh local record and never writes back. Of the two record branches, the array branch was the only one that wrote into the caller's value, and a scan of .splice(/.sort(/.reverse(/Object.assign( across all five packages' lib/ finds no other in-place write to caller data in the serialization path.

columns was already made readonly in #358 on the stated grounds that "this array has no reasons to be mutated by stringify". The same reasoning applies to the record itself, which lib/index.d.ts still types as the mutable Input = unknown[].

Change

Bound the cast loop by Math.min(chunk.length, columns.length) instead of shortening chunk. Emitted output is unchanged in every case.

Verification

  • New test in test/option.columns.ts next to its sibling is an array, should be the same length. It asserts both the output and that the input array is untouched. On current master it fails at the input assertion with A has 2 and B has 3; with the change it passes.
  • Mutating the fix in both directions fails disjoint assertions: reverting to splice fails only the input-immutability assertion, while dropping the column bound fails only the output assertions (the new one and the existing is an array, should be the same length). So neither side is unpinned.
  • npm test in packages/csv-stringify (tsc --noEmit + mocha): 210 passing, 1 pending. eslint and prettier --check clean on both changed files.

Assisted-by: Claude Code:claude-opus-5

When a record is an array and the `columns` option is shorter than that
array, the stringifier called `chunk.splice(columns.length)`, which
truncates the array owned by the caller instead of only limiting what is
written. Bound the cast loop instead, leaving the input untouched. The
object branch already reads through `get()` into a fresh local record.

Assisted-by: Claude Code:claude-opus-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant