From 431b78c2a3384f2068e3aaba2e873d84428e7e02 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Thu, 31 Oct 2019 12:42:05 -0700 Subject: [PATCH] Query: Deprecate NullConditionalExpression We flow nullability information through joins. And InMemory works with nullable values from database differently. Hence we no longer need it. It creates unnecessary wrapper expression complicating things. --- .../CosmosSqlTranslatingExpressionVisitor.cs | 3 -- ...yExpressionTranslatingExpressionVisitor.cs | 8 ---- ...lationalSqlTranslatingExpressionVisitor.cs | 3 -- ...ntityEqualityRewritingExpressionVisitor.cs | 33 +++---------- ...ingExpressionVisitor.ExpressionVisitors.cs | 3 -- .../NullCheckRemovingExpressionVisitor.cs | 27 +---------- src/EFCore/Query/NullConditionalExpression.cs | 1 + .../Query/SimpleQueryCosmosTest.cs | 21 --------- .../Query/SimpleQueryTestBase.cs | 46 ------------------- .../Query/SimpleQuerySqlServerTest.cs | 20 -------- .../Query/ExpressionPrinterTest.cs | 13 +----- 11 files changed, 10 insertions(+), 168 deletions(-) diff --git a/src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs b/src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs index c27e6cebbc7..b99b76f476b 100644 --- a/src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs @@ -441,9 +441,6 @@ protected override Expression VisitExtension(Expression extensionExpression) .GetMappedProjection(projectionBindingExpression.ProjectionMember) : null; - case NullConditionalExpression nullConditionalExpression: - return Visit(nullConditionalExpression.AccessOperation); - default: return null; } diff --git a/src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs b/src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs index faabef15fa7..5161f7a3a6f 100644 --- a/src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs +++ b/src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs @@ -612,14 +612,6 @@ protected override Expression VisitExtension(Expression extensionExpression) .GetMappedProjection(projectionBindingExpression.ProjectionMember) : null; - case NullConditionalExpression nullConditionalExpression: - { - var translation = Visit(nullConditionalExpression.AccessOperation); - return translation.Type == nullConditionalExpression.Type - ? translation - : Expression.Convert(translation, nullConditionalExpression.Type); - } - default: return null; } diff --git a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs index d3d1b07e291..cffde2a6e6a 100644 --- a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs @@ -573,9 +573,6 @@ protected override Expression VisitExtension(Expression extensionExpression) case SqlExpression _: return extensionExpression; - case NullConditionalExpression nullConditionalExpression: - return Visit(nullConditionalExpression.AccessOperation); - case EntityShaperExpression entityShaperExpression: return Visit(entityShaperExpression.ValueBufferExpression); diff --git a/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs b/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs index 5dd386805da..b16a1252ce9 100644 --- a/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs +++ b/src/EFCore/Query/Internal/EntityEqualityRewritingExpressionVisitor.cs @@ -897,34 +897,13 @@ private Expression RewriteEntityEquality( CreateKeyAccessExpression(Unwrap(right), keyProperties)); } - protected override Expression VisitExtension(Expression expression) + protected override Expression VisitExtension(Expression extensionExpression) { - switch (expression) - { - case EntityReferenceExpression _: - // If the expression is an EntityReferenceExpression, simply returns it as all rewriting has already occurred. - // This is necessary when traversing wrapping expressions that have been injected into the lambda for parameters. - return expression; - - case NullConditionalExpression nullConditionalExpression: - return VisitNullConditional(nullConditionalExpression); - - default: - return base.VisitExtension(expression); - } - } - - private Expression VisitNullConditional(NullConditionalExpression expression) - { - var newCaller = Visit(expression.Caller); - var newAccessOperation = Visit(expression.AccessOperation); - var visitedExpression = expression.Update(Unwrap(newCaller), Unwrap(newAccessOperation)); - - // TODO: Can the access operation be anything else than a MemberExpression? - return newCaller is EntityReferenceExpression wrapper - && expression.AccessOperation is MemberExpression memberExpression - ? wrapper.TraverseProperty(memberExpression.Member.Name, visitedExpression) - : visitedExpression; + // If the expression is an EntityReferenceExpression, simply returns it as all rewriting has already occurred. + // This is necessary when traversing wrapping expressions that have been injected into the lambda for parameters. + return extensionExpression is EntityReferenceExpression + ? extensionExpression + : base.VisitExtension(extensionExpression); } private Expression CreateKeyAccessExpression( diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs index 0aee93fa1ab..1ec3c26900d 100644 --- a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs +++ b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs @@ -69,9 +69,6 @@ protected EntityReference UnwrapEntityReference(Expression expression) case OwnedNavigationReference ownedNavigationReference: return ownedNavigationReference.EntityReference; - case NullConditionalExpression nullConditionalExpression: - return UnwrapEntityReference(nullConditionalExpression.AccessOperation); - default: return null; } diff --git a/src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs b/src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs index b1bf53ea2b7..722d354febe 100644 --- a/src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs +++ b/src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs @@ -11,9 +11,6 @@ public class NullCheckRemovingExpressionVisitor : ExpressionVisitor private readonly NullSafeAccessVerifyingExpressionVisitor _nullSafeAccessVerifyingExpressionVisitor = new NullSafeAccessVerifyingExpressionVisitor(); - private readonly NullConditionalRemovingExpressionVisitor _nullConditionalRemovingExpressionVisitor - = new NullConditionalRemovingExpressionVisitor(); - protected override Expression VisitConditional(ConditionalExpression conditionalExpression) { var test = Visit(conditionalExpression.Test); @@ -39,37 +36,15 @@ protected override Expression VisitConditional(ConditionalExpression conditional ? conditionalExpression.IfFalse : conditionalExpression.IfTrue; - // Unwrap nested nullConditional - if (caller is NullConditionalExpression nullConditionalCaller) - { - accessOperation = ReplacingExpressionVisitor.Replace( - _nullConditionalRemovingExpressionVisitor.Visit(nullConditionalCaller.AccessOperation), - nullConditionalCaller, - accessOperation); - } - if (_nullSafeAccessVerifyingExpressionVisitor.Verify(caller, accessOperation)) { - return new NullConditionalExpression(caller, accessOperation); + return accessOperation; } } return base.VisitConditional(conditionalExpression); } - private class NullConditionalRemovingExpressionVisitor : ExpressionVisitor - { - public override Expression Visit(Expression expression) - { - if (expression is NullConditionalExpression nullConditionalExpression) - { - return Visit(nullConditionalExpression.AccessOperation); - } - - return base.Visit(expression); - } - } - private class NullSafeAccessVerifyingExpressionVisitor : ExpressionVisitor { private readonly ISet _nullSafeAccesses = new HashSet(ExpressionEqualityComparer.Instance); diff --git a/src/EFCore/Query/NullConditionalExpression.cs b/src/EFCore/Query/NullConditionalExpression.cs index 0ac0b273afb..a85663a3314 100644 --- a/src/EFCore/Query/NullConditionalExpression.cs +++ b/src/EFCore/Query/NullConditionalExpression.cs @@ -15,6 +15,7 @@ namespace Microsoft.EntityFrameworkCore.Query /// Expression representing null-conditional access. /// Logic in this file is based on https://github.com/bartdesmet/ExpressionFutures /// + [Obsolete("Use ConditionalExpression with null check instead")] public class NullConditionalExpression : Expression, IPrintableExpression { /// diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs index a2c33d163f8..8d53560bfc7 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs @@ -742,27 +742,6 @@ FROM root c WHERE ((c[""Discriminator""] = ""Order"") AND (c[""CustomerID""] = ""FRANK""))"); } - public override async Task Null_conditional_simple(bool isAsync) - { - await base.Null_conditional_simple(isAsync); - - AssertSql( - @"SELECT c -FROM root c -WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] = ""ALFKI""))"); - } - - [ConditionalTheory(Skip = "Issue #17246")] - public override async Task Null_conditional_deep(bool isAsync) - { - await base.Null_conditional_deep(isAsync); - - AssertSql( - @"SELECT c -FROM root c -WHERE (c[""Discriminator""] = ""Customer"")"); - } - public override async Task Queryable_simple(bool isAsync) { await base.Queryable_simple(isAsync); diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs index 9e9f014c707..8cff0b3965c 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs @@ -518,52 +518,6 @@ public virtual Task Entity_equality_orderby_descending_composite_key(bool isAsyn assertOrder: true); } - [ConditionalTheory] - [MemberData(nameof(IsAsyncData))] - public virtual Task Null_conditional_simple(bool isAsync) - { - var c = Expression.Parameter(typeof(Customer)); - - var predicate - = Expression.Lambda>( - Expression.Equal( - new NullConditionalExpression(c, Expression.Property(c, "CustomerID")), - Expression.Constant("ALFKI")), - c); - - return AssertQuery( - isAsync, - ss => ss.Set().Where(predicate), - entryCount: 1); - } - - [ConditionalTheory] - [MemberData(nameof(IsAsyncData))] - public virtual Task Null_conditional_deep(bool isAsync) - { - var c = Expression.Parameter(typeof(Customer)); - - var nullConditionalExpression - = new NullConditionalExpression(c, Expression.Property(c, "CustomerID")); - - nullConditionalExpression - = new NullConditionalExpression( - nullConditionalExpression, - Expression.Property(nullConditionalExpression, "Length")); - - var predicate - = Expression.Lambda>( - Expression.Equal( - nullConditionalExpression, - Expression.Constant(5, typeof(int?))), - c); - - return AssertQuery( - isAsync, - ss => ss.Set().Where(predicate), - entryCount: 91); - } - [ConditionalTheory] [MemberData(nameof(IsAsyncData))] public virtual Task Queryable_simple(bool isAsync) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs index bed1b8ae00b..e37e559c72d 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/SimpleQuerySqlServerTest.cs @@ -1234,26 +1234,6 @@ FROM [Orders] AS [o] ) AS [t0]"); } - public override async Task Null_conditional_simple(bool isAsync) - { - await base.Null_conditional_simple(isAsync); - - AssertSql( - @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] -FROM [Customers] AS [c] -WHERE [c].[CustomerID] = N'ALFKI'"); - } - - public override async Task Null_conditional_deep(bool isAsync) - { - await base.Null_conditional_deep(isAsync); - - AssertSql( - @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] -FROM [Customers] AS [c] -WHERE CAST(LEN([c].[CustomerID]) AS int) = 5"); - } - public override async Task Queryable_simple(bool isAsync) { await base.Queryable_simple(isAsync); diff --git a/test/EFCore.Tests/Query/ExpressionPrinterTest.cs b/test/EFCore.Tests/Query/ExpressionPrinterTest.cs index 51257f86dad..719f0176266 100644 --- a/test/EFCore.Tests/Query/ExpressionPrinterTest.cs +++ b/test/EFCore.Tests/Query/ExpressionPrinterTest.cs @@ -165,7 +165,8 @@ public void Simple_MethodCall_printed_correctly() public void Complex_MethodCall_printed_correctly() { Assert.Equal( - @"""Foobar"".Substring( + "\"Foobar\"" ++ @".Substring( startIndex: 0, length: 4)", _expressionPrinter.Print( @@ -195,15 +196,5 @@ public void Linq_methods_printed_as_extensions() .Where(x => x.Length > 1)", _expressionPrinter.Print(expr.Body)); } - - [ConditionalFact] - public void Use_Print_method_when_printing_extension_expression() - { - var expr = new NullConditionalExpression( - Expression.Constant("caller"), - Expression.Constant("accessOperation")); - - Assert.Equal(expr.Print(), _expressionPrinter.Print(expr)); - } } }