Fix WriteRecordsAsync writing a delimiter instead of a blank line for null records - #2384
Open
virzak wants to merge 2 commits into
Open
Fix WriteRecordsAsync writing a delimiter instead of a blank line for null records#2384virzak wants to merge 2 commits into
virzak wants to merge 2 commits into
Conversation
WriteRecords(IEnumerable) writes a blank line when a record is null, since every record in an untyped sequence could be a different type. The asynchronous overload lost that branch and falls through to the write delegate instead, so it emits a delimiter per member, and throws when the first record is null because there is no type to resolve a delegate from. The two implementations were last changed together in d0b6e3b, which updated them inconsistently. WriteNullTests only covered the synchronous path, so nothing noticed. Add the branch back, and give the null tests asynchronous counterparts.
Three comments in these files ask that the synchronous and asynchronous
implementations be kept identical by hand:
CsvReader.Read Don't forget about the async method below!
CsvWriter.WriteRecords Changes in this method require changes in
method WriteRecords<T>(IEnumerable<T>) also.
CsvWriter.WriteRecordsAsync These methods should all be the same
The previous commit fixes what happens when one of them is missed.
Delete the synchronous implementations and generate them from the
asynchronous ones instead, so that the two cannot drift apart again:
CsvWriter.WriteRecords(IEnumerable)
CsvWriter.WriteRecords<T>(IEnumerable<T>)
CsvWriter.NextRecord
CsvWriter.Flush
CsvReader.Read
CsvParser.Read
The generator runs at build time only and contributes nothing to the
shipped assembly.
Two pairs are left as they are. FlushBuffer documents itself as
asynchronous, and documentation is copied to the generated method as
written. GetRecords and EnumerateRecords name the method in an exception
message, and the asynchronous copies of those messages still describe the
synchronous overload.
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.
The bug
WriteRecords(IEnumerable)andWriteRecordsAsync(IEnumerable)disagree about null records.The synchronous version has an explicit branch - "Since every record could be a different type, just write a blank line" - which the asynchronous version lacks, so the asynchronous path falls through to the write delegate and emits a delimiter per member instead. When the first record is null it throws a
WriterException, because there is no type to resolve a delegate from.git blamepoints atd0b6e3b8("Added nullable", May 2024), which updated the two copies inconsistently. It has been latent for a little over two years, becauseWriteNullTestscovers the synchronous path only.Commit 1 - the fix
Restores the branch in
WriteRecordsAsync, and adds the missing asynchronous twins toWriteNullTests. Those tests fail before the change and pass after. This commit is self-contained - if you would rather not take the second one, it stands on its own.Commit 2 - removing the duplication
Three comments in these files are doing real work:
CsvReader.Read()- "Don't forget about the async method below!"CsvWriter.WriteRecords()- "Changes in this method require changes in methodWriteRecords<T>(IEnumerable<T>)also."WriteRecordsAsync()- "These methods should all be the same"The bug above is what happens when one of them is missed. This commit deletes the synchronous implementations and generates them from the asynchronous ones with Zomp.SyncMethodGenerator, so the two cannot drift apart again:
CsvWriter.WriteRecords(IEnumerable)CsvWriter.WriteRecords<T>(IEnumerable<T>)CsvWriter.NextRecord()CsvWriter.Flush()CsvReader.Read()CsvParser.Read()Around 200 lines of duplicated implementation removed. The generator is build time only (
PrivateAssets="all"), contributes nothing to the shipped assembly, and supports every framework this project targets.Disclosure: I maintain that generator. It is also used by FluentValidation and MiniExcel (mini-software/MiniExcel#799).
Deliberately left alone
FlushBuffer- its documentation says "Asynchronously flushes the buffer", and documentation is copied to the generated method as written, so the synchronous one would carry a wrong summary.NextRecordandFlushwere fine because they use<inheritdoc/>.GetRecords/EnumerateRecords- theirObjectDisposedExceptionmessages name the method, and the asynchronous copies still describe the synchronous overload ("GetRecords<T>() returns an IEnumerable<T>"). Generating would push that text into the synchronous side.Verification
Builds on all seven target frameworks.
net9.0: 1061 passed, 4 failed.net48: 1051 passed, 4 failed. The 4 are pre-existing failures onmaster(culture dependent date formatting) and unrelated to this change. Two more tests pass than onmaster, from the added asynchronous coverage.