From 41fe3621065a789deb24724e1871d0c5ef102209 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Mon, 18 Sep 2017 18:41:27 -0700 Subject: [PATCH] [2.0.1] Query: Fix for Access to context bound variable can be optimized out Issue: We adopted short-circuiting logic in logical expression due to possible exceptions (which compiler would never throw in memory). This works correctly for general cases because the expression tree would have current value of closure variable hence evaluating to correct thing. When it comes to QueryFilter, the filter has closure member access on context instance which was used during OnModelCreating which may have stale value of context closure variables. We parametrize such closure variables and insert their values through parameters when running query. But if such closure variable is used in complex expression which can short-circuit then we would be using wrong value for short-circuit (by this point we haven't inserted correct value from current context instance) hence generating incorrect query model. The other cause for it is QueryCache has only 1 entry for all the possible values of context bound variables in filter. Solution: Since evaluating filter with context bound variables with different value may not always generate a re-usable query model for all values of variables, whenever we get an expression to evaluate during funcletizing query filter, we replace context, with context parameter (which would be injected later) and return this expression without evaluating anything in-memory. Any exception to be thrown from evaluation of context bound variables will be thrown when actual parameter value will be calculated while running query. Resolves #9825 --- .../Query/QueryTestBase.cs | 19 ++ .../ParameterExtractingExpressionVisitor.cs | 92 ++++-- .../Query/FiltersSqlServerTest.cs | 2 +- .../Query/NullSemanticsQuerySqlServerTest.cs | 3 +- .../Query/QueryBugsTest.cs | 298 ++++++++++++++++++ .../Query/QuerySqlServerTest.cs | 2 +- 6 files changed, 395 insertions(+), 21 deletions(-) diff --git a/src/EFCore.Specification.Tests/Query/QueryTestBase.cs b/src/EFCore.Specification.Tests/Query/QueryTestBase.cs index 3d8b5a3663e..93f9796dd1c 100644 --- a/src/EFCore.Specification.Tests/Query/QueryTestBase.cs +++ b/src/EFCore.Specification.Tests/Query/QueryTestBase.cs @@ -3126,6 +3126,25 @@ public virtual void Parameter_extraction_can_throw_exception_from_user_code() } } + [ConditionalFact] + public virtual void Parameter_extraction_can_throw_exception_from_user_code_2() + { + using (var context = CreateContext()) + { + DateTime? dateFilter = null; + + Assert.Throws( + () => + context.Orders + .Where( + o => (o.OrderID < 10400) + && ((o.OrderDate.HasValue + && o.OrderDate.Value.Month == dateFilter.Value.Month + && o.OrderDate.Value.Year == dateFilter.Value.Year))) + .ToList()); + } + } + [ConditionalFact] public virtual void Subquery_member_pushdown_does_not_change_original_subquery_model() { diff --git a/src/EFCore/Query/ExpressionVisitors/Internal/ParameterExtractingExpressionVisitor.cs b/src/EFCore/Query/ExpressionVisitors/Internal/ParameterExtractingExpressionVisitor.cs index 7186a1bf475..1dd2bc666c4 100644 --- a/src/EFCore/Query/ExpressionVisitors/Internal/ParameterExtractingExpressionVisitor.cs +++ b/src/EFCore/Query/ExpressionVisitors/Internal/ParameterExtractingExpressionVisitor.cs @@ -32,6 +32,8 @@ public class ParameterExtractingExpressionVisitor : ExpressionVisitor private bool _inLambda; + private static bool QuirkEnabled => AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue9825", out var enabled) && enabled; + /// /// This API supports the Entity Framework Core infrastructure and is not intended to be used /// directly from your code. This API may change or be removed in future releases. @@ -352,30 +354,48 @@ private Expression TryExtractParameter(Expression expression) { var parameterValue = Evaluate(expression, out var parameterName); - if (parameterValue is Expression valueExpression) + if (QuirkEnabled) { - return ExtractParameters(valueExpression); - } + if (parameterValue is Expression valueExpression) + { + return ExtractParameters(valueExpression); + } - if (!_parameterize) - { - if (_generateContextAccessors - && expression is MemberExpression memberExpression - && memberExpression.Expression != null - && typeof(DbContext).GetTypeInfo() - .IsAssignableFrom(memberExpression.Expression.Type.GetTypeInfo())) + if (!_parameterize) { - var contextParameterExpression - = Expression.Parameter(memberExpression.Expression.Type, "context"); + if (_generateContextAccessors + && expression is MemberExpression memberExpression + && memberExpression.Expression != null + && typeof(DbContext).GetTypeInfo() + .IsAssignableFrom(memberExpression.Expression.Type.GetTypeInfo())) + { + var contextParameterExpression + = Expression.Parameter(memberExpression.Expression.Type, "context"); - parameterValue - = Expression.Lambda( - memberExpression.Update(contextParameterExpression), - contextParameterExpression); + parameterValue + = Expression.Lambda( + memberExpression.Update(contextParameterExpression), + contextParameterExpression); + } + else + { + return Expression.Constant(parameterValue); + } } - else + } + else + { + if (!_generateContextAccessors) { - return Expression.Constant(parameterValue); + if (parameterValue is Expression valueExpression) + { + return ExtractParameters(valueExpression); + } + + if (!_parameterize) + { + return Expression.Constant(parameterValue); + } } } @@ -403,6 +423,22 @@ var compilerPrefixIndex return Expression.Parameter(expression.Type, parameterName); } + private class ContextParameterReplacingExpressionVisitor : ExpressionVisitor + { + public ParameterExpression ContextParameterExpression; + + protected override Expression VisitConstant(ConstantExpression node) + { + if (typeof(DbContext).GetTypeInfo() + .IsAssignableFrom(node.Type.GetTypeInfo())) + { + return ContextParameterExpression ?? (ContextParameterExpression = Expression.Parameter(node.Type, "context")); + } + + return base.VisitConstant(node); + } + } + /// /// This API supports the Entity Framework Core infrastructure and is not intended to be used /// directly from your code. This API may change or be removed in future releases. @@ -416,6 +452,26 @@ public virtual object Evaluate([CanBeNull] Expression expression, [CanBeNull] ou return null; } + if (!QuirkEnabled) + { + if (_generateContextAccessors) + { + var contextParameterReplacingExpressionVisitor = new ContextParameterReplacingExpressionVisitor(); + var updatedExpression = contextParameterReplacingExpressionVisitor.Visit(expression); + + if (contextParameterReplacingExpressionVisitor.ContextParameterExpression != null) + { + parameterName = updatedExpression is MemberExpression memberExpression + ? memberExpression.Member.Name + : "_queryFilter"; + + return Expression.Lambda( + updatedExpression, + contextParameterReplacingExpressionVisitor.ContextParameterExpression); + } + } + } + // ReSharper disable once SwitchStatementMissingSomeCases switch (expression.NodeType) { diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/FiltersSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/FiltersSqlServerTest.cs index e50491e001d..fac0c86331e 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/FiltersSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/FiltersSqlServerTest.cs @@ -217,7 +217,7 @@ WHERE [o].[Quantity] > @___quantity_1 [ConditionalFact] public void FromSql_is_composed() { - using (var context = Fixture.CreateContext()) + using (var context = Fixture.CreateContext(enableFilters: true)) { var results = context.Customers.FromSql("select * from Customers").ToList(); diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs index a07f1e56268..8abe45ee51c 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs @@ -12,7 +12,8 @@ public class NullSemanticsQuerySqlServerTest : NullSemanticsQueryTestBase(() => context.Blogs.ToList()); + } + + using (var context = new MyContext9791(_options)) + { + context.TenantIds = new List(); + var query = context.Blogs.ToList(); + + Assert.Empty(query); + } + + using (var context = new MyContext9791(_options)) + { + context.TenantIds = new List { 1 }; + var query = context.Blogs.ToList(); + + Assert.Single(query); + } + + using (var context = new MyContext9791(_options)) + { + context.TenantIds = new List { 1, 2 }; + var query = context.Blogs.ToList(); + + Assert.Equal(2, query.Count); + } + + AssertSql( + @"SELECT [b].[Id], [b].[IsDeleted], [b].[TenantId] +FROM [Blogs] AS [b] +WHERE [b].[IsDeleted] = 0", + // + @"SELECT [b].[Id], [b].[IsDeleted], [b].[TenantId] +FROM [Blogs] AS [b] +WHERE [b].[IsDeleted] = 0", + // + @"SELECT [b].[Id], [b].[IsDeleted], [b].[TenantId] +FROM [Blogs] AS [b] +WHERE [b].[IsDeleted] = 0"); + } + } + + private SqlServerTestStore CreateDatabase9791() + => CreateTestStore( + () => new MyContext9791(_options), + context => + { + context.AddRange( + new Blog9791 { IsDeleted = false, TenantId = 1 }, + new Blog9791 { IsDeleted = false, TenantId = 2 }, + new Blog9791 { IsDeleted = false, TenantId = 3 }, + new Blog9791 { IsDeleted = true, TenantId = 1 }, + new Blog9791 { IsDeleted = true, TenantId = 2 }, + new Blog9791 { IsDeleted = true, TenantId = 3 } + ); + context.SaveChanges(); + + ClearLog(); + }); + + public class MyContext9791 : DbContext + { + public MyContext9791(DbContextOptions options) + : base(options) + { + } + + public List TenantIds { get; set; } + + public DbSet Blogs { get; set; } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + base.OnModelCreating(modelBuilder); + + modelBuilder.Entity() + .HasQueryFilter(e => !e.IsDeleted && TenantIds.Contains(e.TenantId)); + } + } + + public class Blog9791 + { + public int Id { get; set; } + public bool IsDeleted { get; set; } + public int TenantId { get; set; } + } + + #endregion + + #region Bug9825 + + [Fact] + public virtual void Context_bound_variable_works_correctly_in_short_circuit_optimization_9825() + { + using (CreateDatabase9825()) + { + using (var context = new MyContext9825(_options)) + { + context.IsModerated = true; + var query = context.Users.ToList(); + + Assert.Single(query); + } + + using (var context = new MyContext9825(_options)) + { + context.IsModerated = false; + var query = context.Users.ToList(); + + Assert.Single(query); + } + + using (var context = new MyContext9825(_options)) + { + context.IsModerated = null; + var query = context.Users.ToList(); + + Assert.Equal(2, query.Count); + } + + AssertSql( + @"@__IsModerated_0='True' (Nullable = true) +@__IsModerated_1='True' (Nullable = true) + +SELECT [e].[Id], [e].[IsDeleted], [e].[IsModerated] +FROM [Users] AS [e] +WHERE ([e].[IsDeleted] = 0) AND (@__IsModerated_0 IS NULL OR (@__IsModerated_1 = [e].[IsModerated]))", + // + @"@__IsModerated_0='False' (Nullable = true) +@__IsModerated_1='False' (Nullable = true) + +SELECT [e].[Id], [e].[IsDeleted], [e].[IsModerated] +FROM [Users] AS [e] +WHERE ([e].[IsDeleted] = 0) AND (@__IsModerated_0 IS NULL OR (@__IsModerated_1 = [e].[IsModerated]))", + // + @"@__IsModerated_0='' (DbType = String) + +SELECT [e].[Id], [e].[IsDeleted], [e].[IsModerated] +FROM [Users] AS [e] +WHERE ([e].[IsDeleted] = 0) AND (@__IsModerated_0 IS NULL OR [e].[IsModerated] IS NULL)"); + } + } + + [Fact] + public virtual void Context_bound_variable_with_member_chain_works_correctly_9825() + { + using (CreateDatabase9825()) + { + using (var context = new MyContext9825(_options)) + { + context.IndirectionFlag = new Indirection { Enabled = true }; + var query = context.Chains.ToList(); + + Assert.Equal(2, query.Count); + } + + using (var context = new MyContext9825(_options)) + { + context.IndirectionFlag = new Indirection { Enabled = false }; + var query = context.Chains.ToList(); + + Assert.Equal(2, query.Count); + } + + using (var context = new MyContext9825(_options)) + { + context.IndirectionFlag = null; + var exception = Assert.Throws(() => context.Chains.ToList()); + Assert.Equal("Object reference not set to an instance of an object.", exception.Message); + Assert.StartsWith( + @" at lambda_method(Closure , QueryContext )", exception.StackTrace); + + } + + AssertSql( + @"@__Enabled_0='True' + +SELECT [e].[Id], [e].[IsDeleted], [e].[IsModerated] +FROM [Chains] AS [e] +WHERE @__Enabled_0 = [e].[IsDeleted]", + // + @"@__Enabled_0='False' + +SELECT [e].[Id], [e].[IsDeleted], [e].[IsModerated] +FROM [Chains] AS [e] +WHERE @__Enabled_0 = [e].[IsDeleted]"); + } + } + + [Fact] + public virtual void Local_variable_in_query_filter_throws_if_cannot_be_evaluated_9825() + { + using (CreateDatabase9825()) + { + using (var context = new MyContext9825(_options)) + { + context.IsModerated = true; + var exception = Assert.Throws(() => context.Locals.ToList()); + Assert.Equal(CoreStrings.ExpressionParameterizationExceptionSensitive("value(Microsoft.EntityFrameworkCore.Query.QueryBugsTest+MyContext9825+<>c__DisplayClass21_0).local.Enabled"), exception.Message); + } + } + } + + private SqlServerTestStore CreateDatabase9825() + => CreateTestStore( + () => new MyContext9825(_options), + context => + { + context.AddRange( + new EntityWithContextBoundComplexExpression9825 { IsDeleted = false, IsModerated = false }, + new EntityWithContextBoundComplexExpression9825 { IsDeleted = true, IsModerated = false }, + new EntityWithContextBoundComplexExpression9825 { IsDeleted = false, IsModerated = true }, + new EntityWithContextBoundComplexExpression9825 { IsDeleted = true, IsModerated = true }, + + new EntityWithContextBoundMemberChain9825 { IsDeleted = false, IsModerated = false }, + new EntityWithContextBoundMemberChain9825 { IsDeleted = true, IsModerated = false }, + new EntityWithContextBoundMemberChain9825 { IsDeleted = false, IsModerated = true }, + new EntityWithContextBoundMemberChain9825 { IsDeleted = true, IsModerated = true }, + + new EntityWithLocalVariableAccessInFilter9825 { IsDeleted = false, IsModerated = false }, + new EntityWithLocalVariableAccessInFilter9825 { IsDeleted = true, IsModerated = false }, + new EntityWithLocalVariableAccessInFilter9825 { IsDeleted = false, IsModerated = true }, + new EntityWithLocalVariableAccessInFilter9825 { IsDeleted = true, IsModerated = true } + ); + context.SaveChanges(); + + ClearLog(); + }); + + public class MyContext9825 : DbContext + { + public MyContext9825(DbContextOptions options) + : base(options) + { + } + + public bool? IsModerated { get; set; } + public Indirection IndirectionFlag { get; set; } + + public DbSet Users { get; set; } + public DbSet Chains { get; set; } + public DbSet Locals { get; set; } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + base.OnModelCreating(modelBuilder); + + modelBuilder.Entity() + .HasQueryFilter(x => !x.IsDeleted && (IsModerated == null || IsModerated == x.IsModerated)); + + modelBuilder.Entity() + .HasQueryFilter(x => IndirectionFlag.Enabled == x.IsDeleted); + + var local = new Indirection(); + local = null; + modelBuilder.Entity() + .HasQueryFilter(x => local.Enabled == x.IsDeleted); + } + } + + public class Indirection + { + public bool Enabled { get; set; } + } + + public class EntityWithContextBoundComplexExpression9825 + { + public int Id { get; set; } + public bool IsDeleted { get; set; } + public bool IsModerated { get; set; } + } + + public class EntityWithContextBoundMemberChain9825 + { + public int Id { get; set; } + public bool IsDeleted { get; set; } + public bool IsModerated { get; set; } + } + + public class EntityWithLocalVariableAccessInFilter9825 + { + public int Id { get; set; } + public bool IsDeleted { get; set; } + public bool IsModerated { get; set; } + } + + #endregion + private DbContextOptions _options; private SqlServerTestStore CreateTestStore( diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QuerySqlServerTest.cs index 3f14c5f66e8..7ae53374ff4 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/QuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/QuerySqlServerTest.cs @@ -30,7 +30,7 @@ FROM [Customers] AS [e] // @"SELECT COUNT(*) FROM [Customers] AS [e] -WHERE [e].[CustomerID] = N'ALFKI'"); +WHERE [e].[CustomerID] = N'ALFKI'"); } public override void Lifting_when_subquery_nested_order_by_anonymous()