From ac4633559b117b92156d75f07b7ef81dc920b4fd Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Fri, 20 Mar 2020 14:43:00 -0700 Subject: [PATCH] Query: Bring back detailed errors logging Resolves #15751 --- ...jectionBindingRemovingExpressionVisitor.cs | 102 ++++++++++++------ ...alShapedQueryCompilingExpressionVisitor.cs | 3 + ...yCompilingExpressionVisitorDependencies.cs | 35 +++++- .../Query/FromSqlQueryTestBase.cs | 23 ++-- .../Query/FromSqlQuerySqlServerTest.cs | 48 +++++++++ .../Query/QueryBugsTest.cs | 2 +- .../Query/BadDataSqliteTest.cs | 9 +- 7 files changed, 166 insertions(+), 56 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.RelationalProjectionBindingRemovingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.RelationalProjectionBindingRemovingExpressionVisitor.cs index d159e00482c..3c02c6739bd 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.RelationalProjectionBindingRemovingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.RelationalProjectionBindingRemovingExpressionVisitor.cs @@ -7,6 +7,8 @@ using System.Linq; using System.Linq.Expressions; using System.Reflection; +using System.Runtime.CompilerServices; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Metadata; using Microsoft.EntityFrameworkCore.Query.Internal; @@ -22,10 +24,15 @@ private sealed class RelationalProjectionBindingRemovingExpressionVisitor : Expr { private static readonly MethodInfo _isDbNullMethod = typeof(DbDataReader).GetRuntimeMethod(nameof(DbDataReader.IsDBNull), new[] { typeof(int) }); + private static readonly MethodInfo _getFieldValueMethod = + typeof(DbDataReader).GetRuntimeMethod(nameof(DbDataReader.GetFieldValue), new[] { typeof(int) }); + private static readonly MethodInfo _throwReadValueExceptionMethod = + typeof(RelationalProjectionBindingRemovingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(ThrowReadValueException)); private readonly SelectExpression _selectExpression; private readonly ParameterExpression _dbDataReaderParameter; private readonly ParameterExpression _indexMapParameter; + private readonly bool _detailedErrorsEnabled; private readonly IDictionary> _materializationContextBindings = new Dictionary>(); @@ -34,11 +41,13 @@ private sealed class RelationalProjectionBindingRemovingExpressionVisitor : Expr SelectExpression selectExpression, ParameterExpression dbDataReaderParameter, ParameterExpression indexMapParameter, + bool detailedErrorsEnabled, bool buffer) { _selectExpression = selectExpression; _dbDataReaderParameter = dbDataReaderParameter; _indexMapParameter = indexMapParameter; + _detailedErrorsEnabled = detailedErrorsEnabled; if (buffer) { ProjectionColumns = new ReaderColumn[selectExpression.Projection.Count]; @@ -108,7 +117,8 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp projectionIndex, IsNullableProjection(projection), property.GetRelationalTypeMapping(), - methodCallExpression.Type); + methodCallExpression.Type, + property); } return base.VisitMethodCall(methodCallExpression); @@ -149,7 +159,8 @@ private static bool IsNullableProjection(ProjectionExpression projection) int index, bool nullable, RelationalTypeMapping typeMapping, - Type clrType) + Type clrType, + IPropertyBase property = null) { var getMethod = typeMapping.GetDataReaderMethod(); @@ -221,34 +232,27 @@ Expression valueExpression valueExpression = Expression.Convert(valueExpression, clrType); } - //var exceptionParameter - // = Expression.Parameter(typeof(Exception), name: "e"); - - //var property = materializationInfo.Property; - - //if (detailedErrorsEnabled) - //{ - // var catchBlock - // = Expression - // .Catch( - // exceptionParameter, - // Expression.Call( - // _throwReadValueExceptionMethod - // .MakeGenericMethod(valueExpression.Type), - // exceptionParameter, - // Expression.Call( - // dataReaderExpression, - // _getFieldValueMethod.MakeGenericMethod(typeof(object)), - // indexExpression), - // Expression.Constant(property, typeof(IPropertyBase)))); - - // valueExpression = Expression.TryCatch(valueExpression, catchBlock); - //} - - //if (box && valueExpression.Type.GetTypeInfo().IsValueType) - //{ - // valueExpression = Expression.Convert(valueExpression, typeof(object)); - //} + var exceptionParameter + = Expression.Parameter(typeof(Exception), name: "e"); + + if (_detailedErrorsEnabled) + { + var catchBlock + = Expression + .Catch( + exceptionParameter, + Expression.Call( + _throwReadValueExceptionMethod + .MakeGenericMethod(valueExpression.Type), + exceptionParameter, + Expression.Call( + dbDataReader, + _getFieldValueMethod.MakeGenericMethod(typeof(object)), + indexExpression), + Expression.Constant(property, typeof(IPropertyBase)))); + + valueExpression = Expression.TryCatch(valueExpression, catchBlock); + } if (nullable) { @@ -261,6 +265,44 @@ Expression valueExpression return valueExpression; } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static TValue ThrowReadValueException( + Exception exception, object value, IPropertyBase property = null) + { + var expectedType = typeof(TValue); + var actualType = value?.GetType(); + + string message; + + if (property != null) + { + var entityType = property.DeclaringType.DisplayName(); + var propertyName = property.Name; + if (expectedType == typeof(object)) + { + expectedType = property.ClrType; + } + + message = exception is NullReferenceException + || Equals(value, DBNull.Value) + ? CoreStrings.ErrorMaterializingPropertyNullReference(entityType, propertyName, expectedType) + : exception is InvalidCastException + ? CoreStrings.ErrorMaterializingPropertyInvalidCast(entityType, propertyName, expectedType, actualType) + : CoreStrings.ErrorMaterializingProperty(entityType, propertyName); + } + else + { + message = exception is NullReferenceException + || Equals(value, DBNull.Value) + ? CoreStrings.ErrorMaterializingValueNullReference(expectedType) + : exception is InvalidCastException + ? CoreStrings.ErrorMaterializingValueInvalidCast(expectedType, actualType) + : CoreStrings.ErrorMaterializingValue; + } + + throw new InvalidOperationException(message, exception); + } } } } diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.cs index e695596cf26..5cbc05c5115 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.cs @@ -21,6 +21,7 @@ public partial class RelationalShapedQueryCompilingExpressionVisitor : ShapedQue private readonly Type _contextType; private readonly IDiagnosticsLogger _logger; private readonly ISet _tags; + private readonly bool _detailedErrorsEnabled; private readonly bool _useRelationalNulls; public RelationalShapedQueryCompilingExpressionVisitor( @@ -36,6 +37,7 @@ public partial class RelationalShapedQueryCompilingExpressionVisitor : ShapedQue _contextType = queryCompilationContext.ContextType; _logger = queryCompilationContext.Logger; _tags = queryCompilationContext.Tags; + _detailedErrorsEnabled = relationalDependencies.CoreSingletonOptions.AreDetailedErrorsEnabled; _useRelationalNulls = RelationalOptionsExtension.Extract(queryCompilationContext.ContextOptions).UseRelationalNulls; } @@ -66,6 +68,7 @@ protected override Expression VisitShapedQueryExpression(ShapedQueryExpression s selectExpression, dataReaderParameter, isNonComposedFromSql ? indexMapParameter : null, + _detailedErrorsEnabled, IsBuffering) .Visit(shaper, out var projectionColumns); diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitorDependencies.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitorDependencies.cs index 1ca17399f3a..f0a4191305f 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitorDependencies.cs +++ b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitorDependencies.cs @@ -58,7 +58,8 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies [NotNull] IQuerySqlGeneratorFactory querySqlGeneratorFactory, [NotNull] ISqlExpressionFactory sqlExpressionFactory, [NotNull] IParameterNameGeneratorFactory parameterNameGeneratorFactory, - [NotNull] IRelationalParameterBasedQueryTranslationPostprocessorFactory relationalParameterBasedQueryTranslationPostprocessorFactory) + [NotNull] IRelationalParameterBasedQueryTranslationPostprocessorFactory relationalParameterBasedQueryTranslationPostprocessorFactory, + [NotNull] ICoreSingletonOptions coreSingletonOptions) { Check.NotNull(querySqlGeneratorFactory, nameof(querySqlGeneratorFactory)); Check.NotNull(sqlExpressionFactory, nameof(sqlExpressionFactory)); @@ -69,6 +70,7 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies SqlExpressionFactory = sqlExpressionFactory; ParameterNameGeneratorFactory = parameterNameGeneratorFactory; RelationalParameterBasedQueryTranslationPostprocessorFactory = relationalParameterBasedQueryTranslationPostprocessorFactory; + CoreSingletonOptions = coreSingletonOptions; } /// @@ -91,6 +93,11 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies /// public IRelationalParameterBasedQueryTranslationPostprocessorFactory RelationalParameterBasedQueryTranslationPostprocessorFactory { get; } + /// + /// Core singleton options. + /// + public ICoreSingletonOptions CoreSingletonOptions { get; } + /// /// Clones this dependency parameter object with one service replaced. /// @@ -102,7 +109,8 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies querySqlGeneratorFactory, SqlExpressionFactory, ParameterNameGeneratorFactory, - RelationalParameterBasedQueryTranslationPostprocessorFactory); + RelationalParameterBasedQueryTranslationPostprocessorFactory, + CoreSingletonOptions); /// /// Clones this dependency parameter object with one service replaced. @@ -114,7 +122,8 @@ public RelationalShapedQueryCompilingExpressionVisitorDependencies With([NotNull QuerySqlGeneratorFactory, sqlExpressionFactory, ParameterNameGeneratorFactory, - RelationalParameterBasedQueryTranslationPostprocessorFactory); + RelationalParameterBasedQueryTranslationPostprocessorFactory, + CoreSingletonOptions); /// /// Clones this dependency parameter object with one service replaced. @@ -127,7 +136,8 @@ public RelationalShapedQueryCompilingExpressionVisitorDependencies With([NotNull QuerySqlGeneratorFactory, SqlExpressionFactory, parameterNameGeneratorFactory, - RelationalParameterBasedQueryTranslationPostprocessorFactory); + RelationalParameterBasedQueryTranslationPostprocessorFactory, + CoreSingletonOptions); /// /// Clones this dependency parameter object with one service replaced. @@ -140,6 +150,21 @@ public RelationalShapedQueryCompilingExpressionVisitorDependencies With([NotNull QuerySqlGeneratorFactory, SqlExpressionFactory, ParameterNameGeneratorFactory, - relationalParameterBasedQueryTranslationPostprocessorFactory); + relationalParameterBasedQueryTranslationPostprocessorFactory, + CoreSingletonOptions); + + /// + /// Clones this dependency parameter object with one service replaced. + /// + /// A replacement for the current dependency of this type. + /// A new parameter object with the given service replaced. + public RelationalShapedQueryCompilingExpressionVisitorDependencies With( + [NotNull] ICoreSingletonOptions coreSingletonOptions) + => new RelationalShapedQueryCompilingExpressionVisitorDependencies( + QuerySqlGeneratorFactory, + SqlExpressionFactory, + ParameterNameGeneratorFactory, + RelationalParameterBasedQueryTranslationPostprocessorFactory, + coreSingletonOptions); } } diff --git a/test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs index 85c76e27324..6ce0743894b 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs @@ -32,7 +32,7 @@ protected FromSqlQueryTestBase(TFixture fixture) protected TFixture Fixture { get; } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_invalid_cast_key() { using var context = CreateContext(); @@ -47,7 +47,7 @@ public virtual void Bad_data_error_handling_invalid_cast_key() .ToList()).Message); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_invalid_cast() { using var context = CreateContext(); @@ -62,7 +62,7 @@ public virtual void Bad_data_error_handling_invalid_cast() .ToList()).Message); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_invalid_cast_projection() { using var context = CreateContext(); @@ -78,7 +78,7 @@ public virtual void Bad_data_error_handling_invalid_cast_projection() .ToList()).Message); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_invalid_cast_no_tracking() { using var context = CreateContext(); @@ -94,15 +94,10 @@ public virtual void Bad_data_error_handling_invalid_cast_no_tracking() .ToList()).Message); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_null() { using var context = CreateContext(); - context.Set().FromSqlRaw( - NormalizeDelimitersInRawString( - @"SELECT [ProductID], [ProductName], [SupplierID], [UnitPrice], [UnitsInStock], NULL AS [Discontinued] - FROM [Products]")) - .ToList(); Assert.Equal( CoreStrings.ErrorMaterializingPropertyNullReference("Product", "Discontinued", typeof(bool)), Assert.Throws( @@ -114,12 +109,12 @@ public virtual void Bad_data_error_handling_null() .ToList()).Message); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_null_projection() { using var context = CreateContext(); Assert.Equal( - CoreStrings.ErrorMaterializingPropertyNullReference("Product", "Discontinued", typeof(bool)), + CoreStrings.ErrorMaterializingValueNullReference(typeof(bool)), Assert.Throws( () => context.Set().FromSqlRaw( @@ -130,7 +125,7 @@ public virtual void Bad_data_error_handling_null_projection() .ToList()).Message); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void Bad_data_error_handling_null_no_tracking() { using var context = CreateContext(); @@ -184,7 +179,7 @@ public virtual void FromSqlRaw_queryable_simple_columns_out_of_order_and_extra_c Assert.Equal(91, context.ChangeTracker.Entries().Count()); } - [ConditionalFact(Skip = "#15751")] + [ConditionalFact] public virtual void FromSqlRaw_queryable_simple_columns_out_of_order_and_not_enough_columns_throws() { using var context = CreateContext(); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/FromSqlQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/FromSqlQuerySqlServerTest.cs index 0a7840a3ec2..4c25fa22175 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/FromSqlQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/FromSqlQuerySqlServerTest.cs @@ -785,6 +785,54 @@ UNION ALL ) AS [c0]"); } + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_invalid_cast() + { + base.Bad_data_error_handling_invalid_cast(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_invalid_cast_key() + { + base.Bad_data_error_handling_invalid_cast_key(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_invalid_cast_no_tracking() + { + base.Bad_data_error_handling_invalid_cast_no_tracking(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_invalid_cast_projection() + { + base.Bad_data_error_handling_invalid_cast_projection(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_null() + { + base.Bad_data_error_handling_null(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_null_no_tracking() + { + base.Bad_data_error_handling_null_no_tracking(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void Bad_data_error_handling_null_projection() + { + base.Bad_data_error_handling_null_projection(); + } + + [ConditionalFact(Skip = "Issue#20364")] + public override void FromSqlRaw_queryable_simple_columns_out_of_order_and_not_enough_columns_throws() + { + base.FromSqlRaw_queryable_simple_columns_out_of_order_and_not_enough_columns_throws(); + } + protected override DbParameter CreateDbParameter(string name, object value) => new SqlParameter { ParameterName = name, Value = value }; diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs index 8bb83b5b233..bd9249a09c8 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs @@ -442,7 +442,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) #endregion - [ConditionalFact(Skip = "Issue#15751")] + [ConditionalFact(Skip = "Issue#20364")] public void Query_when_null_key_in_database_should_throw() { using var testStore = SqlServerTestStore.CreateInitialized("QueryBugsTest"); diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/BadDataSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/BadDataSqliteTest.cs index 259ddbadfe1..4891d8b4702 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/BadDataSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/BadDataSqliteTest.cs @@ -20,10 +20,7 @@ // ReSharper disable InconsistentNaming namespace Microsoft.EntityFrameworkCore.Query { - // Issue #15751 -#pragma warning disable xUnit1000 // Test classes must be public - internal class BadDataSqliteTest : IClassFixture -#pragma warning restore xUnit1000 // Test classes must be public + public class BadDataSqliteTest : IClassFixture { public BadDataSqliteTest(BadDataSqliteFixture fixture) => Fixture = fixture; @@ -67,7 +64,7 @@ public void Bad_data_error_handling_invalid_cast_projection() { using var context = CreateContext(1); Assert.Equal( - CoreStrings.ErrorMaterializingPropertyInvalidCast("Product", "ProductName", typeof(string), typeof(int)), + CoreStrings.ErrorMaterializingValueInvalidCast(typeof(string), typeof(int)), Assert.Throws( () => context.Set().Where(p => p.ProductID != 4) @@ -105,7 +102,7 @@ public void Bad_data_error_handling_null_projection() { using var context = CreateContext(new object[] { null }); Assert.Equal( - CoreStrings.ErrorMaterializingPropertyNullReference("Product", "Discontinued", typeof(bool)), + CoreStrings.ErrorMaterializingValueNullReference(typeof(bool)), Assert.Throws( () => context.Set()