Strip a byte order mark at the start of the input - #2387
Open
dngr2 wants to merge 1 commit into
Open
Conversation
A StreamReader consumes the BOM while detecting the encoding, so this only
shows up when the text arrives already decoded: a StringReader over an API
response, a database column, a decompressed buffer.
Left in place the mark joins the first field. Unquoted it is silent, and
the first header becomes "Id", so mapping by name misses that column.
Quoted it throws BadDataException, because the field then starts with a
character before its opening quote:
new StringReader("\"username\";\"email\"")
-> CsvHelper.BadDataException
The mark is now dropped from the buffer on the first fill rather than
skipped over, so positions, counts and RawRecord all read as though it was
never sent, matching what a StreamReader would have produced. Only the very
start of the input qualifies: a U+FEFF anywhere else is data and is kept.
Tests cover quoted and unquoted first fields, RawRecord, a mark inside the
document, a document with no mark, a buffer small enough to refill mid
document, and the async path. Five of the seven fail without the change.
Fields are asserted one at a time rather than comparing the whole record:
Assert.Equal on two string[] does not catch a leading U+FEFF under xUnit
2.4.1, so an array comparison passes with or without the fix.
Fixes JoshClose#2311
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2311.
The problem
A
StreamReaderconsumes the BOM while detecting the encoding, so this never shows up when reading a file. It shows up when the text arrives already decoded — aStringReaderover an API response, a database column, a decompressed buffer.Left in place, the mark joins the first field:
"\"Id\",\"Name\""(no BOM)["Id","Name"]BadDataException["Id","Name"]["Id","Name"]["Id","Name"]StreamReader["Id","Name"]The quoted case throws because the field starts with a character before its opening quote. The unquoted case is quieter and arguably worse: the first header becomes
Id, so mapping by name silently misses that column.The change
The mark is dropped from the buffer on the first fill rather than skipped over, so
RawRecord,CharCountand the field positions all read as though it was never sent — the same result aStreamReaderwould have produced.Only the very start of the input qualifies. A
U+FEFFanywhere else is data and is kept, which has its own test.Both
FillBufferandFillBufferAsyncare covered, guarded by a flag so a mid-document refill cannot strip a second time.Tests
tests/CsvHelper.Tests/Parsing/ByteOrderMarkTests.cs— quoted and unquoted first fields,RawRecord, a mark inside the document, a document with no mark, aBufferSize = 4document that refills mid-parse, and the async path. Five of the seven fail without the change.One thing worth flagging for anyone writing parser tests here:
The collection comparer does not catch a leading
U+FEFF. My first version of these tests comparedparser.Recordas an array and four of them passed with and without the fix. They now assert field by field, where the comparison is strict. Same trap would hide any test about invisible characters in this suite.Suite
1066 passed, 4 failedonnet8.0. The 4 are pre-existing and environmental — the Linux newline/culture failures reported in #2375 — and I verified the failing set is byte-identical with and without this change, so nothing here masks or causes them..NET SDK 10.0.400, Debian 12.