Skip to content

Commit

Permalink
Query: Re-enable tests for Issue#16088
Browse files Browse the repository at this point in the history 
Fixed in new nav expansion

Close #16088
  • Loading branch information
@smitpatel
smitpatel committed Aug 8, 2019
1 parent af721db commit b84ba0c
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 153 deletions.
29 changes: 22 additions & 7 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ protected EntityReference UnwrapEntityReference(Expression expression)
case OwnedNavigationReference ownedNavigationReference:
return ownedNavigationReference.EntityReference;

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

default:
return null;
}
Expand All @@ -66,20 +69,17 @@ protected EntityReference UnwrapEntityReference(Expression expression)
protected override Expression VisitMember(MemberExpression memberExpression)
{
var innerExpression = Visit(memberExpression.Expression);
var expansion = TryExpandNavigation(innerExpression, MemberIdentity.Create(memberExpression.Member));
return expansion ?? memberExpression.Update(innerExpression);
return TryExpandNavigation(innerExpression, MemberIdentity.Create(memberExpression.Member))
?? memberExpression.Update(innerExpression);
}

protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
{
if (methodCallExpression.TryGetEFPropertyArguments(out var source, out var navigationName))
{
source = Visit(source);
var expansion = TryExpandNavigation(source, MemberIdentity.Create(navigationName));
if (expansion != null)
{
return expansion;
}
return TryExpandNavigation(source, MemberIdentity.Create(navigationName))
?? methodCallExpression.Update(null, new[] { source, methodCallExpression.Arguments[1] });
}

return base.VisitMethodCall(methodCallExpression);
Expand Down Expand Up @@ -602,5 +602,20 @@ public override Expression Visit(Expression expression)
}
}
}

private class EntityReferenceOptionalMarkingExpressionVisitor : ExpressionVisitor
{
public override Expression Visit(Expression expression)
{
if (expression is EntityReference entityReference)
{
entityReference.MarkAsOptional();

return entityReference;
}

return base.Visit(expression);
}
}
}
}
7 changes: 6 additions & 1 deletion src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public partial class NavigationExpandingExpressionVisitor : ExpressionVisitor
private readonly PendingSelectorExpandingExpressionVisitor _pendingSelectorExpandingExpressionVisitor;
private readonly SubqueryMemberPushdownExpressionVisitor _subqueryMemberPushdownExpressionVisitor;
private readonly ReducingExpressionVisitor _reducingExpressionVisitor;
private readonly EntityReferenceOptionalMarkingExpressionVisitor _entityReferenceOptionalMarkingExpressionVisitor;
private readonly ISet<string> _parameterNames = new HashSet<string>();
private static readonly MethodInfo _enumerableToListMethodInfo = typeof(Enumerable).GetTypeInfo()
.GetDeclaredMethods(nameof(Enumerable.ToList))
Expand All @@ -39,6 +40,7 @@ public NavigationExpandingExpressionVisitor(QueryCompilationContext queryCompila
_pendingSelectorExpandingExpressionVisitor = new PendingSelectorExpandingExpressionVisitor(this);
_subqueryMemberPushdownExpressionVisitor = new SubqueryMemberPushdownExpressionVisitor();
_reducingExpressionVisitor = new ReducingExpressionVisitor();
_entityReferenceOptionalMarkingExpressionVisitor = new EntityReferenceOptionalMarkingExpressionVisitor();
}

private string GetParameterName(string prefix)
Expand Down Expand Up @@ -898,12 +900,15 @@ private Expression ProcessLeftJoin(
Expression.Quote(innerKeySelector),
Expression.Quote(newResultSelector));

var innerPendingSelector = innerSource.PendingSelector;
innerPendingSelector = _entityReferenceOptionalMarkingExpressionVisitor.Visit(innerPendingSelector);

var currentTree = new NavigationTreeNode(outerSource.CurrentTree, innerSource.CurrentTree);
var pendingSelector = new ReplacingExpressionVisitor(
new Dictionary<Expression, Expression>
{
{ resultSelector.Parameters[0], outerSource.PendingSelector },
{ resultSelector.Parameters[1], innerSource.PendingSelector }
{ resultSelector.Parameters[1], innerPendingSelector }
}).Visit(resultSelector.Body);
var parameterName = GetParameterName("ti");

Expand Down
31 changes: 5 additions & 26 deletions src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@ private class NullSafeAccessVerifyingExpressionVisitor : ExpressionVisitor
{
private readonly ISet<Expression> _nullSafeAccesses = new HashSet<Expression>(ExpressionEqualityComparer.Instance);

public NullSafeAccessVerifyingExpressionVisitor()
{
}

public virtual bool Verify(Expression caller, Expression result)
{
_nullSafeAccesses.Clear();
Expand All @@ -87,19 +83,9 @@ public virtual bool Verify(Expression caller, Expression result)
}

public override Expression Visit(Expression expression)
{
if (expression == null)
{
return expression;
}

if (_nullSafeAccesses.Contains(expression))
{
return expression;
}

return base.Visit(expression);
}
=> expression == null || _nullSafeAccesses.Contains(expression)
? expression
: base.Visit(expression);

protected override Expression VisitMember(MemberExpression memberExpression)
{
Expand Down Expand Up @@ -127,14 +113,7 @@ protected override Expression VisitUnary(UnaryExpression unaryExpression)
}

private bool IsNullConstant(Expression expression)
{
if (expression is ConstantExpression constantExpression
&& constantExpression.Value == null)
{
return true;
}

return false;
}
=> expression is ConstantExpression constantExpression
&& constantExpression.Value == null;
}
}
29 changes: 13 additions & 16 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,9 @@ public virtual Task Key_equality_two_conditions_on_same_navigation(bool isAsync)
isAsync,
l1s => l1s.Where(
l => l.OneToOne_Required_FK1 == new Level2
{
Id = 1
}
{
Id = 1
}
|| l.OneToOne_Required_FK1 == new Level2
{
Id = 2
Expand All @@ -192,9 +192,9 @@ public virtual Task Key_equality_two_conditions_on_same_navigation2(bool isAsync
isAsync,
l2s => l2s.Where(
l => l.OneToOne_Required_FK_Inverse2 == new Level1
{
Id = 1
}
{
Id = 1
}
|| l.OneToOne_Required_FK_Inverse2 == new Level1
{
Id = 2
Expand Down Expand Up @@ -3365,7 +3365,8 @@ from l3 in grouping.DefaultIfEmpty()
orderby l2.OneToOne_Optional_FK2.Id
select new
{
Entity = l4, Collection = l2.OneToMany_Optional_Self2.Where(e => e.Id != 42).ToList(),
Entity = l4,
Collection = l2.OneToMany_Optional_Self2.Where(e => e.Id != 42).ToList(),
Property = l3.OneToOne_Optional_FK_Inverse3.OneToOne_Required_FK2.Name
},
(l1s, l4s)
Expand Down Expand Up @@ -3578,7 +3579,7 @@ public virtual Task Required_navigation_on_a_subquery_with_First_in_predicate(bo
.Where(l1 => l2s.OrderBy(l2i => l2i.Id).First().OneToOne_Required_FK_Inverse2.Name == "L1 02"));
}

[ConditionalTheory(Skip = "Issue#16088")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Manually_created_left_join_propagates_nullability_to_navigations(bool isAsync)
{
Expand Down Expand Up @@ -6227,22 +6228,20 @@ public virtual Task Include_multiple_collections_on_same_level(bool isAsync)
assertOrder: true);
}

[ConditionalTheory(Skip = "Issue#16088")]
[ConditionalTheory(Skip = "Issue#17020")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Null_check_removal_applied_recursively(bool isAsync)
{
return AssertQuery<Level1>(
isAsync,
l1s => l1s.Where(l1 =>
((((l1.OneToOne_Optional_FK1 == null
(((l1.OneToOne_Optional_FK1 == null
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2) == null
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3) == null
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3) == null
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3.Name) == "L4 01"));
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3.Name) == "L4 01"));
}

[ConditionalTheory]
Expand All @@ -6258,9 +6257,7 @@ public virtual Task Null_check_different_structure_does_not_remove_null_checks(b
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3 == null
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3 == null
? null
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3.Name) == "L4 01"));
: l1.OneToOne_Optional_FK1.OneToOne_Optional_FK2.OneToOne_Optional_FK3.Name) == "L4 01"));
}
}
}
10 changes: 5 additions & 5 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5104,7 +5104,7 @@ public virtual Task Correlated_collections_with_FirstOrDefault(bool isAsync)
elementAsserter: (e, a) => CollectionAsserter<Gear>(elementAsserter: (ee, aa) => Assert.Equal(ee.Nickname, aa.Nickname)));
}

[ConditionalTheory(Skip = "Issue#16088")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_on_left_join_with_predicate(bool isAsync)
{
Expand All @@ -5124,7 +5124,7 @@ from g in grouping.DefaultIfEmpty()
from t in ts
join g in gs on t.GearNickName equals g.Nickname into grouping
from g in grouping.DefaultIfEmpty()
where !MaybeScalar<bool>(g, () => g.HasSoulPatch) == true || g == null
where !MaybeScalar<bool>(g, () => g.HasSoulPatch) == true
select new
{
Nickname = Maybe(g, () => g.Nickname),
Expand All @@ -5138,7 +5138,7 @@ from g in grouping.DefaultIfEmpty()
});
}

[ConditionalTheory(Skip = "Issue#16088")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_on_left_join_with_null_value(bool isAsync)
{
Expand All @@ -5160,7 +5160,7 @@ orderby t.Note
elementAsserter: (e, a) => CollectionAsserter<string>(ee => ee));
}

[ConditionalTheory(Skip = "Issue#16088")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_left_join_with_self_reference(bool isAsync)
{
Expand Down Expand Up @@ -5192,7 +5192,7 @@ from o in grouping.DefaultIfEmpty()
});
}

[ConditionalTheory(Skip = "Issue#16088")]
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Correlated_collections_deeply_nested_left_join(bool isAsync)
{
Expand Down
Loading

0 comments on commit b84ba0c

Please sign in to comment.