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: Re-enable tests for Issue#16088 #17034

Merged
merged 1 commit into from
Aug 8, 2019
Merged

Query: Re-enable tests for Issue#16088 #17034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 22 additions & 7 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
smitpatel marked this conversation as resolved.
Show resolved Hide resolved
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