Avoid double-enumerating IEnumerable<T> in AsTableValuedParameter - #2217
Avoid double-enumerating IEnumerable<T> in AsTableValuedParameter#2217hexonal 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.
|
@mgravell — a gentle bump on this one; it has had no eyes on it since it was opened a month ago, and it is small. Short version: Three tests: one end-to-end repro (needs SQL Server), and two offline ones against Happy to change the shape if you would rather it were done differently. |
1 participant
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