Skip to content

Commit

Permalink
Query: Deprecate NullConditionalExpression
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
smitpatel committed Oct 31, 2019
1 parent 036f31e commit 431b78c
Show file tree
Hide file tree
Showing 11 changed files with 10 additions and 168 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -441,9 +441,6 @@ protected override Expression VisitExtension(Expression extensionExpression)
.GetMappedProjection(projectionBindingExpression.ProjectionMember)
: null;

case NullConditionalExpression nullConditionalExpression:
return Visit(nullConditionalExpression.AccessOperation);

default:
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
27 changes: 1 addition & 26 deletions src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<Expression> _nullSafeAccesses = new HashSet<Expression>(ExpressionEqualityComparer.Instance);
Expand Down
1 change: 1 addition & 0 deletions src/EFCore/Query/NullConditionalExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// </summary>
[Obsolete("Use ConditionalExpression with null check instead")]
public class NullConditionalExpression : Expression, IPrintableExpression
{
/// <summary>
Expand Down
21 changes: 0 additions & 21 deletions test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
46 changes: 0 additions & 46 deletions test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Func<Customer, bool>>(
Expression.Equal(
new NullConditionalExpression(c, Expression.Property(c, "CustomerID")),
Expression.Constant("ALFKI")),
c);

return AssertQuery(
isAsync,
ss => ss.Set<Customer>().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<Func<Customer, bool>>(
Expression.Equal(
nullConditionalExpression,
Expression.Constant(5, typeof(int?))),
c);

return AssertQuery(
isAsync,
ss => ss.Set<Customer>().Where(predicate),
entryCount: 91);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Queryable_simple(bool isAsync)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 2 additions & 11 deletions test/EFCore.Tests/Query/ExpressionPrinterTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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));
}
}
}

0 comments on commit 431b78c

Please sign in to comment.