Skip to content

Commit 3371bf2

Browse files
NickCraverNickCraver
authored
Normalize deserialization errors for bad data (#1453)
This normalizes all the cases of trying to set null to non-null things and giving a better exception when it happens. Today when a user does something like this: ```cs conn.QueryFirstOrDefault<int>("Select null;"); ``` ...we'll get a null ref in the `(T)val;` portion of the deserialization pass. It's not very intuitive as to what's gone wrong. The same is true for value tuples and all `Query*<T>()` methods. However, we handle this in a much friendlier way in the generated type deserializers (already, in master) with a message like: > Error parsing column 1 (Foo=bar - String) This gives the type we _actually_ got for the column and the position it was in. This PR improves the case for anything from `Query<int>` to `QueryFirst<int>`, etc. We could be more intuitive _if we passed the expected type in_ (which we don't today via the IL Path), or if we can come up with a generic error message along the lines of "... - use a nullable type (e.g. `int?` instead of `int`) if null is expected.". Or something like that - thoughts? We're using `DataException` because that's what we're already doing on type deserialization in the default paths. That's the generic exception type that'll always work (it's not always a cast or parsing exception...it could be either)...and since it's already in use, sticking with it here. Overall changes: - DRYs up `QueryRowImpl<T>` and `QueryRowAsync<T>` (single row paths) - into `ReadRow<T>` - DRYs up value setting code between `Query<T>`, `QueryAsync<T>`, `QueryRowImpl<T>`, `QueryRowAsync<T>` - into `GetValue<T>` - When trying to set a null to a non-nullable type, it'll throw a much better exception. - Adds tests to ensure these error messages are consistent. Theoretical breaking change: - Someone's doing this and catching null ref exceptions or string format exceptions. If that's the case, we'll change their exception expectations here - I think in net we should still do this. Related to: - #567 - #1421 - #1440 ### TODO - [ ] ValueTuple support - these are handled in yet another path and still throw a different casting error. Note: they won't throw a null error because that's a column and we won't set it - unless `.ApplyNullValues` is set in settings. ### Benchmarks: ```ini BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.208 (2004/?/20H1) Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores .NET Core SDK=3.1.201 [Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT ShortRun : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT ``` #### Before (3 runs) | ORM | Method | Return | Mean | StdDev | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |------- |------------------------------ |-------- |---------:|--------:|--------:|-------:|-------:|-------:|----------:| | Dapper | QueryFirstOrDefault<dynamic> | dynamic | 117.4 us | 1.62 us | 2.46 us | 3.6250 | - | - | 11.39 KB | | Dapper | 'Query<T> (buffered)' | Post | 119.4 us | 0.58 us | 0.88 us | 1.8750 | 0.8750 | - | 11.65 KB | | Dapper | QueryFirstOrDefault<T> | Post | 120.7 us | 0.78 us | 1.30 us | 1.8750 | 0.8750 | - | 11.35 KB | | Dapper | 'Query<dynamic> (buffered)' | dynamic | 125.0 us | 2.62 us | 4.41 us | 2.5000 | 1.0000 | 0.2500 | 11.73 KB | | Dapper | 'Contrib Get<T>' | Post | 127.8 us | 3.19 us | 4.82 us | 2.5000 | 1.0000 | 0.2500 | 12.29 KB | | Dapper | 'Query<T> (unbuffered)' | Post | 160.8 us | 1.19 us | 2.28 us | 2.0000 | 1.0000 | - | 11.77 KB | | Dapper | 'Query<dynamic> (unbuffered)' | dynamic | 170.6 us | 1.71 us | 2.58 us | 2.0000 | 1.0000 | - | 11.81 KB | | ORM | Method | Return | Mean | StdDev | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |------- |------------------------------ |-------- |---------:|--------:|--------:|-------:|-------:|-------:|----------:| | Dapper | QueryFirstOrDefault<dynamic> | dynamic | 114.5 us | 1.81 us | 2.73 us | 3.6250 | - | - | 11.39 KB | | Dapper | QueryFirstOrDefault<T> | Post | 117.9 us | 0.93 us | 1.56 us | 1.8750 | 0.8750 | - | 11.35 KB | | Dapper | 'Query<dynamic> (buffered)' | dynamic | 121.3 us | 1.52 us | 2.55 us | 2.5000 | 1.0000 | 0.2500 | 11.73 KB | | Dapper | 'Query<T> (buffered)' | Post | 122.7 us | 1.09 us | 2.08 us | 2.0000 | 1.0000 | - | 11.65 KB | | Dapper | 'Contrib Get<T>' | Post | 127.9 us | 1.40 us | 2.12 us | 2.5000 | 0.7500 | 0.2500 | 12.29 KB | | Dapper | 'Query<T> (unbuffered)' | Post | 166.9 us | 1.02 us | 1.71 us | 2.0000 | 1.0000 | - | 11.77 KB | | Dapper | 'Query<dynamic> (unbuffered)' | dynamic | 170.1 us | 2.19 us | 3.31 us | 2.0000 | 1.0000 | - | 11.81 KB | | ORM | Method | Return | Mean | StdDev | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |------- |------------------------------ |-------- |---------:|--------:|--------:|-------:|-------:|-------:|----------:| | Dapper | QueryFirstOrDefault<dynamic> | dynamic | 115.5 us | 3.14 us | 4.75 us | 3.6250 | - | - | 11.39 KB | | Dapper | QueryFirstOrDefault<T> | Post | 121.5 us | 4.29 us | 7.21 us | 1.8750 | 0.8750 | - | 11.35 KB | | Dapper | 'Query<T> (buffered)' | Post | 123.9 us | 2.16 us | 3.27 us | 2.0000 | 1.0000 | - | 11.65 KB | | Dapper | 'Query<dynamic> (buffered)' | dynamic | 127.0 us | 3.65 us | 5.52 us | 2.3750 | 0.8750 | 0.3750 | 11.73 KB | | Dapper | 'Contrib Get<T>' | Post | 130.0 us | 3.18 us | 4.80 us | 2.5000 | 0.7500 | 0.2500 | 12.29 KB | | Dapper | 'Query<dynamic> (unbuffered)' | dynamic | 166.7 us | 1.39 us | 2.33 us | 2.7500 | 1.0000 | 0.2500 | 11.81 KB | | Dapper | 'Query<T> (unbuffered)' | Post | 170.6 us | 2.18 us | 3.29 us | 2.2500 | 1.0000 | 0.2500 | 11.77 KB | #### After (3 runs): | ORM | Method | Return | Mean | StdDev | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |------- |------------------------------ |-------- |---------:|--------:|--------:|-------:|-------:|-------:|----------:| | Dapper | QueryFirstOrDefault<dynamic> | dynamic | 113.2 us | 0.45 us | 0.75 us | 3.6250 | - | - | 11.39 KB | | Dapper | 'Query<dynamic> (buffered)' | dynamic | 118.7 us | 1.97 us | 2.98 us | 1.8750 | 0.8750 | - | 11.72 KB | | Dapper | QueryFirstOrDefault<T> | Post | 119.6 us | 1.12 us | 1.88 us | 1.8750 | 0.8750 | - | 11.35 KB | | Dapper | 'Query<T> (buffered)' | Post | 121.8 us | 4.16 us | 6.28 us | 2.5000 | 1.0000 | 0.2500 | 11.64 KB | | Dapper | 'Contrib Get<T>' | Post | 124.0 us | 1.21 us | 2.04 us | 2.0000 | 1.0000 | - | 12.28 KB | | Dapper | 'Query<T> (unbuffered)' | Post | 162.4 us | 1.95 us | 3.27 us | 2.2500 | 1.0000 | 0.2500 | 11.76 KB | | Dapper | 'Query<dynamic> (unbuffered)' | dynamic | 173.0 us | 2.68 us | 4.50 us | 2.5000 | 1.0000 | 0.2500 | 11.8 KB | | ORM | Method | Return | Mean | StdDev | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |------- |------------------------------ |-------- |---------:|--------:|--------:|-------:|-------:|-------:|----------:| | Dapper | QueryFirstOrDefault<dynamic> | dynamic | 114.1 us | 1.42 us | 2.15 us | 3.6250 | - | - | 11.39 KB | | Dapper | QueryFirstOrDefault<T> | Post | 120.4 us | 1.23 us | 1.87 us | 1.8750 | 0.8750 | - | 11.35 KB | | Dapper | 'Query<dynamic> (buffered)' | dynamic | 121.4 us | 3.90 us | 5.90 us | 1.8750 | 0.8750 | - | 11.72 KB | | Dapper | 'Query<T> (buffered)' | Post | 125.7 us | 1.65 us | 2.50 us | 2.0000 | 1.0000 | - | 11.64 KB | | Dapper | 'Contrib Get<T>' | Post | 132.1 us | 3.01 us | 4.55 us | 2.2500 | 0.7500 | - | 12.28 KB | | Dapper | 'Query<dynamic> (unbuffered)' | dynamic | 162.1 us | 2.29 us | 3.85 us | 2.7500 | 1.0000 | 0.2500 | 11.8 KB | | Dapper | 'Query<T> (unbuffered)' | Post | 166.9 us | 2.19 us | 3.31 us | 2.2500 | 1.0000 | 0.2500 | 11.76 KB | | ORM | Method | Return | Mean | StdDev | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |------- |------------------------------ |-------- |---------:|--------:|--------:|-------:|-------:|-------:|----------:| | Dapper | QueryFirstOrDefault<T> | Post | 114.1 us | 2.80 us | 4.23 us | 1.8750 | 0.8750 | - | 11.35 KB | | Dapper | 'Query<T> (buffered)' | Post | 115.5 us | 1.30 us | 1.97 us | 2.6250 | 1.0000 | 0.3750 | 11.64 KB | | Dapper | QueryFirstOrDefault<dynamic> | dynamic | 120.8 us | 2.99 us | 4.53 us | 3.6250 | - | - | 11.39 KB | | Dapper | 'Query<dynamic> (buffered)' | dynamic | 123.7 us | 3.61 us | 5.46 us | 2.0000 | 1.0000 | - | 11.72 KB | | Dapper | 'Contrib Get<T>' | Post | 129.2 us | 2.79 us | 4.22 us | 2.0000 | 1.0000 | - | 12.28 KB | | Dapper | 'Query<T> (unbuffered)' | Post | 163.0 us | 3.67 us | 6.17 us | 2.2500 | 1.0000 | 0.2500 | 11.76 KB | | Dapper | 'Query<dynamic> (unbuffered)' | dynamic | 167.2 us | 0.73 us | 1.10 us | 2.2500 | 1.0000 | - | 11.8 KB | ...in short: it's within the jitter on this laptop and no difference is discernable. It's worth looking at for performance because these are hot paths. I want to de-dupe the repeated code here, but given the hot path nature whether it inlines is important.
1 parent f486848 commit 3371bf2

File tree

3 files changed

+144
-55
lines changed

3 files changed

+144
-55
lines changed

‎Dapper.Tests/MiscTests.cs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,91 @@ public void Test_Single_First_Default()
167167
Assert.Equal("Sequence contains more than one element", ex.Message);
168168
}
169169

170+
/// <summary>
171+
/// This test is ensuring our "single row" methods also behave like a type being deserialized
172+
/// and give a useful error message when the types don't match.
173+
/// </summary>
174+
[Fact]
175+
public async Task TestConversionExceptionMessages()
176+
{
177+
const string sql = "Select Null;";
178+
179+
// Nullable is expected to work if we get a null in all cases
180+
// List paths
181+
var list = connection.Query<int?>(sql);
182+
Assert.Null(Assert.Single(list));
183+
list = await connection.QueryAsync<int?>(sql);
184+
Assert.Null(Assert.Single(list));
185+
186+
// Single row paths
187+
Assert.Null(connection.QueryFirst<int?>(sql));
188+
Assert.Null(connection.QueryFirstOrDefault<int?>(sql));
189+
Assert.Null(connection.QuerySingle<int?>(sql));
190+
Assert.Null(connection.QuerySingleOrDefault<int?>(sql));
191+
192+
Assert.Null(await connection.QueryFirstAsync<int?>(sql));
193+
Assert.Null(await connection.QueryFirstOrDefaultAsync<int?>(sql));
194+
Assert.Null(await connection.QuerySingleAsync<int?>(sql));
195+
Assert.Null(await connection.QuerySingleOrDefaultAsync<int?>(sql));
196+
197+
static async Task TestExceptionsAsync<T>(DbConnection connection, string sql, string exception)
198+
{
199+
var ex = Assert.Throws<DataException>(() => connection.Query<T>(sql));
200+
Assert.Equal(exception, ex.Message);
201+
ex = Assert.Throws<DataException>(() => connection.QueryFirst<T>(sql));
202+
Assert.Equal(exception, ex.Message);
203+
ex = Assert.Throws<DataException>(() => connection.QueryFirstOrDefault<T>(sql));
204+
Assert.Equal(exception, ex.Message);
205+
ex = Assert.Throws<DataException>(() => connection.QuerySingle<T>(sql));
206+
Assert.Equal(exception, ex.Message);
207+
ex = Assert.Throws<DataException>(() => connection.QuerySingleOrDefault<T>(sql));
208+
Assert.Equal(exception, ex.Message);
209+
210+
ex = await Assert.ThrowsAsync<DataException>(() => connection.QueryAsync<T>(sql));
211+
Assert.Equal(exception, ex.Message);
212+
ex = await Assert.ThrowsAsync<DataException>(() => connection.QueryFirstAsync<T>(sql));
213+
Assert.Equal(exception, ex.Message);
214+
ex = await Assert.ThrowsAsync<DataException>(() => connection.QueryFirstOrDefaultAsync<T>(sql));
215+
Assert.Equal(exception, ex.Message);
216+
ex = await Assert.ThrowsAsync<DataException>(() => connection.QuerySingleAsync<T>(sql));
217+
Assert.Equal(exception, ex.Message);
218+
ex = await Assert.ThrowsAsync<DataException>(() => connection.QuerySingleOrDefaultAsync<T>(sql));
219+
Assert.Equal(exception, ex.Message);
220+
}
221+
222+
// Null value throws
223+
await TestExceptionsAsync<int>(
224+
connection,
225+
"Select null as Foo",
226+
"Error parsing column 0 (Foo=<null>)");
227+
// Incompatible value throws (testing unnamed column bits here too)
228+
await TestExceptionsAsync<int>(
229+
connection,
230+
"Select 'bar'",
231+
"Error parsing column 0 ((Unnamed Column)=bar - String)");
232+
// Null with a full type (testing position too)
233+
await TestExceptionsAsync<NullTestType>(
234+
connection,
235+
"Select 1 Id, 'bar' Foo",
236+
"Error parsing column 1 (Foo=bar - String)");
237+
238+
// And a ValueTuple! (testing position too)
239+
// Still needs love, because we handle ValueTuple differently today
240+
// It'll yield a raw: typeof(System.FormatException): Input string was not in a correct format.
241+
// Note: not checking the "Select 1 Id, null Foo" case here, because we won't attempt to set the column
242+
// ...and there will no error in that case.
243+
//await TestExceptionsAsync<(int Id, int Foo)>(
244+
// connection,
245+
// "Select 1 Id, 'bar' Foo",
246+
// "Error parsing column 1 (Foo=bar - String)");
247+
}
248+
249+
private class NullTestType
250+
{
251+
public int Id { get; }
252+
public int Foo { get; }
253+
}
254+
170255
[Fact]
171256
public void TestStrings()
172257
{

‎Dapper/SqlMapper.Async.cs

Lines changed: 3 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -437,14 +437,7 @@ private static async Task<IEnumerable<T>> QueryAsync<T>(this IDbConnection cnn,
437437
while (await reader.ReadAsync(cancel).ConfigureAwait(false))
438438
{
439439
object val = func(reader);
440-
if (val == null || val is T)
441-
{
442-
buffer.Add((T)val);
443-
}
444-
else
445-
{
446-
buffer.Add((T)Convert.ChangeType(val, convertToType, CultureInfo.InvariantCulture));
447-
}
440+
buffer.Add(GetValue<T>(reader, effectiveType, val));
448441
}
449442
while (await reader.NextResultAsync(cancel).ConfigureAwait(false)) { /* ignore subsequent result sets */ }
450443
command.OnCompleted();
@@ -484,29 +477,11 @@ private static async Task<T> QueryRowAsync<T>(this IDbConnection cnn, Row row, T
484477
? CommandBehavior.SequentialAccess | CommandBehavior.SingleResult // need to allow multiple rows, to check fail condition
485478
: CommandBehavior.SequentialAccess | CommandBehavior.SingleResult | CommandBehavior.SingleRow, cancel).ConfigureAwait(false);
486479

487-
T result = default(T);
480+
T result = default;
488481
if (await reader.ReadAsync(cancel).ConfigureAwait(false) && reader.FieldCount != 0)
489482
{
490-
var tuple = info.Deserializer;
491-
int hash = GetColumnHash(reader);
492-
if (tuple.Func == null || tuple.Hash != hash)
493-
{
494-
tuple = info.Deserializer = new DeserializerState(hash, GetDeserializer(effectiveType, reader, 0, -1, false));
495-
if (command.AddToCache) SetQueryCache(identity, info);
496-
}
497-
498-
var func = tuple.Func;
483+
result = ReadRow<T>(info, identity, ref command, effectiveType, reader);
499484

500-
object val = func(reader);
501-
if (val == null || val is T)
502-
{
503-
result = (T)val;
504-
}
505-
else
506-
{
507-
var convertToType = Nullable.GetUnderlyingType(effectiveType) ?? effectiveType;
508-
result = (T)Convert.ChangeType(val, convertToType, CultureInfo.InvariantCulture);
509-
}
510485
if ((row & Row.Single) != 0 && await reader.ReadAsync(cancel).ConfigureAwait(false)) ThrowMultipleRows(row);
511486
while (await reader.ReadAsync(cancel).ConfigureAwait(false)) { /* ignore rows after the first */ }
512487
}

‎Dapper/SqlMapper.cs

Lines changed: 56 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
using System.Linq;
1313
using System.Reflection;
1414
using System.Reflection.Emit;
15+
using System.Runtime.CompilerServices;
1516
using System.Text;
1617
using System.Text.RegularExpressions;
1718
using System.Threading;
@@ -1096,14 +1097,7 @@ private static IEnumerable<T> QueryImpl<T>(this IDbConnection cnn, CommandDefini
10961097
while (reader.Read())
10971098
{
10981099
object val = func(reader);
1099-
if (val == null || val is T)
1100-
{
1101-
yield return (T)val;
1102-
}
1103-
else
1104-
{
1105-
yield return (T)Convert.ChangeType(val, convertToType, CultureInfo.InvariantCulture);
1106-
}
1100+
yield return GetValue<T>(reader, effectiveType, val);
11071101
}
11081102
while (reader.NextResult()) { /* ignore subsequent result sets */ }
11091103
// happy path; close the reader cleanly - no
@@ -1179,31 +1173,14 @@ private static T QueryRowImpl<T>(IDbConnection cnn, Row row, ref CommandDefiniti
11791173
: CommandBehavior.SequentialAccess | CommandBehavior.SingleResult | CommandBehavior.SingleRow);
11801174
wasClosed = false; // *if* the connection was closed and we got this far, then we now have a reader
11811175

1182-
T result = default(T);
1176+
T result = default;
11831177
if (reader.Read() && reader.FieldCount != 0)
11841178
{
11851179
// with the CloseConnection flag, so the reader will deal with the connection; we
11861180
// still need something in the "finally" to ensure that broken SQL still results
11871181
// in the connection closing itself
1188-
var tuple = info.Deserializer;
1189-
int hash = GetColumnHash(reader);
1190-
if (tuple.Func == null || tuple.Hash != hash)
1191-
{
1192-
tuple = info.Deserializer = new DeserializerState(hash, GetDeserializer(effectiveType, reader, 0, -1, false));
1193-
if (command.AddToCache) SetQueryCache(identity, info);
1194-
}
1182+
result = ReadRow<T>(info, identity, ref command, effectiveType, reader);
11951183

1196-
var func = tuple.Func;
1197-
object val = func(reader);
1198-
if (val == null || val is T)
1199-
{
1200-
result = (T)val;
1201-
}
1202-
else
1203-
{
1204-
var convertToType = Nullable.GetUnderlyingType(effectiveType) ?? effectiveType;
1205-
result = (T)Convert.ChangeType(val, convertToType, CultureInfo.InvariantCulture);
1206-
}
12071184
if ((row & Row.Single) != 0 && reader.Read()) ThrowMultipleRows(row);
12081185
while (reader.Read()) { /* ignore subsequent rows */ }
12091186
}
@@ -1236,6 +1213,53 @@ private static T QueryRowImpl<T>(IDbConnection cnn, Row row, ref CommandDefiniti
12361213
}
12371214
}
12381215

1216+
/// <summary>
1217+
/// Shared value deserilization path for QueryRowImpl and QueryRowAsync
1218+
/// </summary>
1219+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
1220+
private static T ReadRow<T>(CacheInfo info, Identity identity, ref CommandDefinition command, Type effectiveType, IDataReader reader)
1221+
{
1222+
var tuple = info.Deserializer;
1223+
int hash = GetColumnHash(reader);
1224+
if (tuple.Func == null || tuple.Hash != hash)
1225+
{
1226+
tuple = info.Deserializer = new DeserializerState(hash, GetDeserializer(effectiveType, reader, 0, -1, false));
1227+
if (command.AddToCache) SetQueryCache(identity, info);
1228+
}
1229+
1230+
var func = tuple.Func;
1231+
object val = func(reader);
1232+
return GetValue<T>(reader, effectiveType, val);
1233+
}
1234+
1235+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
1236+
private static T GetValue<T>(IDataReader reader, Type effectiveType, object val)
1237+
{
1238+
if (val is T tVal)
1239+
{
1240+
return tVal;
1241+
}
1242+
else if (val == null && (!effectiveType.IsValueType || Nullable.GetUnderlyingType(effectiveType) != null))
1243+
{
1244+
return default;
1245+
}
1246+
else
1247+
{
1248+
try
1249+
{
1250+
var convertToType = Nullable.GetUnderlyingType(effectiveType) ?? effectiveType;
1251+
return (T)Convert.ChangeType(val, convertToType, CultureInfo.InvariantCulture);
1252+
}
1253+
catch (Exception ex)
1254+
{
1255+
#pragma warning disable CS0618 // Type or member is obsolete
1256+
ThrowDataException(ex, 0, reader, val);
1257+
#pragma warning restore CS0618 // Type or member is obsolete
1258+
return default; // For the compiler - we've already thrown
1259+
}
1260+
}
1261+
}
1262+
12391263
/// <summary>
12401264
/// Perform a multi-mapping query with 2 input types.
12411265
/// This returns a single type, combined from the raw types via <paramref name="map"/>.
@@ -3619,6 +3643,11 @@ public static void ThrowDataException(Exception ex, int index, IDataReader reade
36193643
if (reader != null && index >= 0 && index < reader.FieldCount)
36203644
{
36213645
name = reader.GetName(index);
3646+
if (name == string.Empty)
3647+
{
3648+
// Otherwise we throw (=value) below, which isn't intuitive
3649+
name = "(Unnamed Column)";
3650+
}
36223651
try
36233652
{
36243653
if (value == null || value is DBNull)

0 commit comments

Comments
 (0)