Skip to content

fix(parser): return real error from csvIterator.NextRow instead of nil#2719

Open
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-csv-nextrow-swallow-err
Open

fix(parser): return real error from csvIterator.NextRow instead of nil#2719
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-csv-nextrow-swallow-err

Conversation

@nankingjing

Copy link
Copy Markdown

What

csvIterator.NextRow returns a nil error on genuine (non-EOF) CSV read failures, so parse errors are silently swallowed.

backend/infra/document/parser/impl/builtin/parse_csv.go:

func (c *csvIterator) NextRow() (row []string, end bool, err error) {
	row, e := c.reader.Read()
	if e != nil {
		if errors.Is(e, io.EOF) {
			return nil, true, nil
		}
		return nil, false, err   // <-- returns the named return `err`, which is never assigned (always nil)
	}
	return row, false, nil
}

The actual read error is captured in the local variable e. The named
return err is never assigned anywhere in the function, so it is always
nil. On the non-EOF error path the function therefore returns
(nil, false, nil) instead of surfacing e.

Why it matters

The caller parseByRowIterator (parse_iter.go) relies on this error to
abort parsing:

row, end, err := iter.NextRow()
if err != nil {
    return nil, err
}
if end {
    break
}

Because err is nil, a malformed CSV row (e.g. a quoting error or a
ErrFieldCount mismatch surfaced by encoding/csv) is not reported.
The loop treats the failed read as an empty row, drops it, and continues
— so CSV parsing silently loses data instead of failing loudly. EOF is
handled separately and correctly (return nil, true, nil); only the real
error path is broken.

Fix

Return the actual error e on the non-EOF path:

		return nil, false, e

One-line, focused change. The EOF and success paths are unchanged.

Verification

Verified by reading; not executed (no Go toolchain available in this
environment). The change is a direct language-semantics fix: the local
e holds the real error from csv.Reader.Read(), while the named return
err is provably nil at that point.

NextRow read the row error into a local variable e, but on the non-EOF
error path it returned the named return value err (never assigned, so
always nil). parseByRowIterator treats a nil error as success, so genuine
CSV parse failures were silently swallowed and the failing row dropped.
Return e so the real error propagates.
@nankingjing

Copy link
Copy Markdown
Author

Review: fix-csv-nextrow-swallow-err (#2719)

Verdict: LGTM

Classic Go named-return-value bug. The function signature is func (c *csvIterator) NextRow() (row []string, end bool, err error). When c.reader.Read() returns a non-EOF error e, the old code returned nil, false, err where err is the named return variable that defaults to nil. This effectively swallowed every CSV parse error, returning nil error when one actually occurred.

The fix changes the return to nil, false, e, returning the actual error from csv.Reader.Read(). This is the correct minimal fix.

No issues found. Merge-ready.

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