Skip to content

Commit

Permalink
Query: Bring back detailed errors logging
Browse files Browse the repository at this point in the history 
Resolves #15751
  • Loading branch information
@smitpatel
smitpatel committed Mar 21, 2020
1 parent 9dfadbc commit ac46335
Show file tree
Hide file tree
Showing 7 changed files with 166 additions and 56 deletions.
102 changes: 72 additions & 30 deletions ...edQueryCompilingExpressionVisitor.RelationalProjectionBindingRemovingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<ParameterExpression, IDictionary<IProperty, int>> _materializationContextBindings
= new Dictionary<ParameterExpression, IDictionary<IProperty, int>>();
Expand All @@ -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];
Expand Down Expand Up @@ -108,7 +117,8 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
projectionIndex,
IsNullableProjection(projection),
property.GetRelationalTypeMapping(),
methodCallExpression.Type);
methodCallExpression.Type,
property);
}

return base.VisitMethodCall(methodCallExpression);
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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)
{
Expand All @@ -261,6 +265,44 @@ Expression valueExpression

return valueExpression;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static TValue ThrowReadValueException<TValue>(
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);
}
}
}
}
3 changes: 3 additions & 0 deletions src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public partial class RelationalShapedQueryCompilingExpressionVisitor : ShapedQue
private readonly Type _contextType;
private readonly IDiagnosticsLogger<DbLoggerCategory.Query> _logger;
private readonly ISet<string> _tags;
private readonly bool _detailedErrorsEnabled;
private readonly bool _useRelationalNulls;

public RelationalShapedQueryCompilingExpressionVisitor(
Expand All @@ -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;
}

Expand Down Expand Up @@ -66,6 +68,7 @@ protected override Expression VisitShapedQueryExpression(ShapedQueryExpression s
selectExpression,
dataReaderParameter,
isNonComposedFromSql ? indexMapParameter : null,
_detailedErrorsEnabled,
IsBuffering)
.Visit(shaper, out var projectionColumns);

Expand Down
35 changes: 30 additions & 5 deletions src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitorDependencies.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -69,6 +70,7 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies
SqlExpressionFactory = sqlExpressionFactory;
ParameterNameGeneratorFactory = parameterNameGeneratorFactory;
RelationalParameterBasedQueryTranslationPostprocessorFactory = relationalParameterBasedQueryTranslationPostprocessorFactory;
CoreSingletonOptions = coreSingletonOptions;
}

/// <summary>
Expand All @@ -91,6 +93,11 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies
/// </summary>
public IRelationalParameterBasedQueryTranslationPostprocessorFactory RelationalParameterBasedQueryTranslationPostprocessorFactory { get; }

/// <summary>
/// Core singleton options.
/// </summary>
public ICoreSingletonOptions CoreSingletonOptions { get; }

/// <summary>
/// Clones this dependency parameter object with one service replaced.
/// </summary>
Expand All @@ -102,7 +109,8 @@ public sealed class RelationalShapedQueryCompilingExpressionVisitorDependencies
querySqlGeneratorFactory,
SqlExpressionFactory,
ParameterNameGeneratorFactory,
RelationalParameterBasedQueryTranslationPostprocessorFactory);
RelationalParameterBasedQueryTranslationPostprocessorFactory,
CoreSingletonOptions);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -114,7 +122,8 @@ public RelationalShapedQueryCompilingExpressionVisitorDependencies With([NotNull
QuerySqlGeneratorFactory,
sqlExpressionFactory,
ParameterNameGeneratorFactory,
RelationalParameterBasedQueryTranslationPostprocessorFactory);
RelationalParameterBasedQueryTranslationPostprocessorFactory,
CoreSingletonOptions);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -127,7 +136,8 @@ public RelationalShapedQueryCompilingExpressionVisitorDependencies With([NotNull
QuerySqlGeneratorFactory,
SqlExpressionFactory,
parameterNameGeneratorFactory,
RelationalParameterBasedQueryTranslationPostprocessorFactory);
RelationalParameterBasedQueryTranslationPostprocessorFactory,
CoreSingletonOptions);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -140,6 +150,21 @@ public RelationalShapedQueryCompilingExpressionVisitorDependencies With([NotNull
QuerySqlGeneratorFactory,
SqlExpressionFactory,
ParameterNameGeneratorFactory,
relationalParameterBasedQueryTranslationPostprocessorFactory);
relationalParameterBasedQueryTranslationPostprocessorFactory,
CoreSingletonOptions);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
/// </summary>
/// <param name="coreSingletonOptions"> A replacement for the current dependency of this type. </param>
/// <returns> A new parameter object with the given service replaced. </returns>
public RelationalShapedQueryCompilingExpressionVisitorDependencies With(
[NotNull] ICoreSingletonOptions coreSingletonOptions)
=> new RelationalShapedQueryCompilingExpressionVisitorDependencies(
QuerySqlGeneratorFactory,
SqlExpressionFactory,
ParameterNameGeneratorFactory,
RelationalParameterBasedQueryTranslationPostprocessorFactory,
coreSingletonOptions);
}
}
23 changes: 9 additions & 14 deletions test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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<Product>().FromSqlRaw(
NormalizeDelimitersInRawString(
@"SELECT [ProductID], [ProductName], [SupplierID], [UnitPrice], [UnitsInStock], NULL AS [Discontinued]
FROM [Products]"))
.ToList();
Assert.Equal(
CoreStrings.ErrorMaterializingPropertyNullReference("Product", "Discontinued", typeof(bool)),
Assert.Throws<InvalidOperationException>(
Expand All @@ -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<InvalidOperationException>(
() =>
context.Set<Product>().FromSqlRaw(
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
Loading

0 comments on commit ac46335

Please sign in to comment.