Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Query: Improve detection of non-composed FromSql #20366

Merged
merged 2 commits into from
Mar 21, 2020
Merged

Query: Improve detection of non-composed FromSql #20366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47608b3
Query: Improve detection of non-composed FromSql
smitpatel Mar 20, 2020
994760b
Query: Bring back detailed errors logging
smitpatel Mar 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 72 additions & 30 deletions ...edQueryCompilingExpressionVisitor.RelationalProjectionBindingRemovingExpressionVisitor.cs
View file
Open in desktop
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 @@ public RelationalProjectionBindingRemovingExpressionVisitor(
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 Expression CreateGetValueExpression(
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
Open in desktop
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 RelationalShapedQueryCompilingExpressionVisitor(
_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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public 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 RelationalShapedQueryCompilingExpressionVisitorDependencies(
SqlExpressionFactory = sqlExpressionFactory;
ParameterNameGeneratorFactory = parameterNameGeneratorFactory;
RelationalParameterBasedQueryTranslationPostprocessorFactory = relationalParameterBasedQueryTranslationPostprocessorFactory;
CoreSingletonOptions = coreSingletonOptions;
}

/// <summary>
Expand All @@ -91,6 +93,11 @@ public 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 RelationalShapedQueryCompilingExpressionVisitorDependencies With(
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(
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(
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);
}
}
4 changes: 3 additions & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ public bool IsNonComposedFromSql()
&& Tables[0] is FromSqlExpression fromSql
&& Projection.All(
pe => pe.Expression is ColumnExpression column
&& string.Equals(fromSql.Alias, column.Table.Alias, StringComparison.OrdinalIgnoreCase));
&& string.Equals(fromSql.Alias, column.Table.Alias, StringComparison.OrdinalIgnoreCase))
&& _projectionMapping.TryGetValue(new ProjectionMember(), out var mapping)
&& mapping.Type == typeof(Dictionary<IProperty, int>);

public void ApplyProjection()
{
Expand Down
23 changes: 9 additions & 14 deletions test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs
View file
Open in desktop
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
11 changes: 5 additions & 6 deletions test/EFCore.Relational.Specification.Tests/Query/FromSqlSprocQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,11 @@ public virtual async Task From_sql_queryable_stored_procedure_projection(bool as
.FromSqlRaw(TenMostExpensiveProductsSproc, GetTenMostExpensiveProductsParameters())
.Select(mep => mep.TenMostExpensiveProducts);

var actual = async
? await query.ToArrayAsync()
: query.ToArray();

Assert.Equal(10, actual.Length);
Assert.Contains(actual, r => r == "Côte de Blaye");
Assert.Equal(
RelationalStrings.FromSqlNonComposable,
(async
? await Assert.ThrowsAsync<InvalidOperationException>(() => query.ToArrayAsync())
: Assert.Throws<InvalidOperationException>(() => query.ToArray())).Message);
}

[ConditionalTheory]
Expand Down
Loading