Avoid double-enumerating IEnumerable<T> in AsTableValuedParameter#2217
Open
hexonal wants to merge 3 commits into
Open
Avoid double-enumerating IEnumerable<T> in AsTableValuedParameter#2217hexonal wants to merge 3 commits into
hexonal wants to merge 3 commits into
Conversation
SqlDataRecordListTVPParameter<T>.Set used data.Any() to swap an empty sequence for null, which forced an extra enumeration of the source before the caller ever executed the command. For single-pass sources (open readers, streaming iterators, etc.) the real enumeration during command execution then either restarted from scratch or failed outright. Only short-circuit to null when the source is a known IReadOnlyCollection<T> and we can check Count for free; anything else now flows straight through untouched, so it's enumerated exactly once by the provider. Fixes DapperLib#2064
The two new offline tests hardcoded Microsoft.Data.SqlClient.SqlParameter even when running under the SystemSqlClientParameterTests variant, unlike the rest of this file which uses Provider.CreateRawParameter to stay provider-agnostic across the two test subclasses.
TestSqlDataRecordListParametersWithAsTableValuedParameterSinglePassSource was failing against real SQL Server with: InvalidCastException: Failed to convert parameter value from a SingleEnumerationEnumerable`1 to a IEnumerable`1. Object must implement IConvertible. SqlParameter.CoerceValue recognizes a Structured/TVP value only when it is a DataTable, a DbDataReader, or satisfies "value is IEnumerable<SqlDataRecord>" - a hard runtime interface check, not an assignment-compatibility check. Generic interface implementations are invariant on their concrete type parameter even though IEnumerable<T> itself is declared covariant, so a wrapper class instantiated as SingleEnumerationEnumerable<IDataRecord> never satisfies that check, no matter what concrete objects it yields. Everything else falls through to CoerceValue's scalar Convert.ChangeType path, which is exactly the IConvertible error above. The test was wrapping the shared, provider-agnostic IEnumerable<IDataRecord> helper, so AsTableValuedParameter<T> always inferred T as IDataRecord and the wrapper only ever implemented IEnumerable<IDataRecord>. This has nothing to do with the double-enumeration fix itself; SqlDataRecordListTVPParameter<T>.Set still only enumerates its source once either way. Wrap the provider-specific SqlDataRecord list directly instead, matching the branching already used in TestSqlDataRecordListParametersWithTypeHandlers, so the wrapper's concrete type parameter is the real SqlDataRecord type SqlClient is checking for.
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
2 participants
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.
SqlDataRecordListTVPParameter<T>.Setuseddata.Any()to swap an empty sequence fornull, which forced an extra enumeration of the source before the caller ever executed the command. For single-pass sources (open readers, streaming iterators, etc.) that meant the real enumeration during command execution either restarted from scratch or failed outright with something like "There is already an open DataReader...".This now only short-circuits to
nullwhen the source is a knownIReadOnlyCollection<T>, checked viaCountas @mgravell suggested in the issue thread. Everything else flows straight through untouched, so it's enumerated exactly once, by whoever actually executes the command.Added three tests: one mirrors the original repro end-to-end (needs SQL Server to run), and two run offline against
SqlDataRecordListTVPParameter<T>.Setdirectly, checking that a non-collection source is never touched and a known-empty collection still nulls out.Fixes #2064