Skip to content

Commit

Permalink
Query: Throw exception for cycle in include path when no tracking query
Browse files Browse the repository at this point in the history 
Resolves #16225
  • Loading branch information
@smitpatel
smitpatel committed Aug 2, 2019
1 parent 53f64ac commit f22487a
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 246 deletions.
56 changes: 45 additions & 11 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,15 @@ private static Expression CreateKeyAccessExpression(Expression target, IReadOnly

private class IncludeExpandingExpressionVisitor : ExpandingExpressionVisitor
{
private readonly bool _isTracking;

public IncludeExpandingExpressionVisitor(
NavigationExpandingExpressionVisitor navigationExpandingExpressionVisitor,
NavigationExpansionExpression source)
NavigationExpansionExpression source,
bool tracking)
: base(navigationExpandingExpressionVisitor, source)
{
_isTracking = tracking;
}

public override Expression Visit(Expression expression)
Expand All @@ -302,7 +306,7 @@ public override Expression Visit(Expression expression)
case NavigationTreeExpression navigationTreeExpression:
if (navigationTreeExpression.Value is EntityReference entityReference)
{
return ExpandIncludes(navigationTreeExpression, entityReference);
return ExpandInclude(navigationTreeExpression, entityReference);
}

if (navigationTreeExpression.Value is NewExpression newExpression)
Expand All @@ -315,7 +319,7 @@ public override Expression Visit(Expression expression)
break;

case OwnedNavigationReference ownedNavigationReference:
return ExpandIncludes(ownedNavigationReference, ownedNavigationReference.EntityReference);
return ExpandInclude(ownedNavigationReference, ownedNavigationReference.EntityReference);
}

return base.Visit(expression);
Expand Down Expand Up @@ -350,7 +354,7 @@ protected override Expression VisitNew(NewExpression newExpression)
{
var argument = newExpression.Arguments[i];
arguments[i] = argument is EntityReference entityReference
? ExpandIncludes(argument, entityReference)
? ExpandInclude(argument, entityReference)
: Visit(argument);
}

Expand All @@ -375,7 +379,7 @@ private bool ReconstructAnonymousType(Expression currentRoot, NewExpression newE
if (argument is EntityReference entityReference)
{
changed = true;
arguments[i] = ExpandIncludes(newRoot, entityReference);
arguments[i] = ExpandInclude(newRoot, entityReference);
}
else if (argument is NewExpression innerNewExpression)
{
Expand Down Expand Up @@ -403,7 +407,35 @@ private bool ReconstructAnonymousType(Expression currentRoot, NewExpression newE
return changed;
}

private Expression ExpandIncludes(Expression root, EntityReference entityReference)
private Expression ExpandInclude(Expression root, EntityReference entityReference)
{
if (!_isTracking)
{
VerifyNoCycles(entityReference.IncludePaths);
}

return ExpandIncludesHelper(root, entityReference);
}

private void VerifyNoCycles(IncludeTreeNode includeTreeNode)
{
foreach (var keyValuePair in includeTreeNode)
{
var navigation = keyValuePair.Key;
var referenceIncludeTreeNode = keyValuePair.Value;
var inverseNavigation = navigation.FindInverse();
if (inverseNavigation != null
&& referenceIncludeTreeNode.ContainsKey(inverseNavigation))
{
throw new InvalidOperationException("There is a cycle in include path in no tracking query" +
$"{navigation.Name}->{inverseNavigation.Name}");
}

VerifyNoCycles(referenceIncludeTreeNode);
}
}

private Expression ExpandIncludesHelper(Expression root, EntityReference entityReference)
{
var result = root;
var convertedRoot = root;
Expand All @@ -426,7 +458,7 @@ private Expression ExpandIncludes(Expression root, EntityReference entityReferen
{
var elementType = navigation.ClrType.TryGetSequenceType();
var collectionSelectorParameter = Expression.Parameter(elementType);
var nestedInclude = ExpandIncludes(collectionSelectorParameter, includedEntityReference);
var nestedInclude = ExpandIncludesHelper(collectionSelectorParameter, includedEntityReference);
if (nestedInclude is IncludeExpression)
{

Expand All @@ -443,7 +475,7 @@ private Expression ExpandIncludes(Expression root, EntityReference entityReferen
}
else
{
included = ExpandIncludes(included, includedEntityReference);
included = ExpandIncludesHelper(included, includedEntityReference);
}
}
else
Expand All @@ -453,7 +485,7 @@ private Expression ExpandIncludes(Expression root, EntityReference entityReferen
{
var navigationTreeExpression = (NavigationTreeExpression)included;
var innerEntityReference = (EntityReference)navigationTreeExpression.Value;
included = ExpandIncludes(navigationTreeExpression, innerEntityReference);
included = ExpandIncludesHelper(navigationTreeExpression, innerEntityReference);
}
}

Expand All @@ -467,17 +499,19 @@ private Expression ExpandIncludes(Expression root, EntityReference entityReferen
private class IncludeApplyingExpressionVisitor : ExpressionVisitor
{
private readonly NavigationExpandingExpressionVisitor _visitor;
private readonly bool _isTracking;

public IncludeApplyingExpressionVisitor(NavigationExpandingExpressionVisitor visitor)
public IncludeApplyingExpressionVisitor(NavigationExpandingExpressionVisitor visitor, bool tracking)
{
_visitor = visitor;
_isTracking = tracking;
}

public override Expression Visit(Expression expression)
{
if (expression is NavigationExpansionExpression navigationExpansionExpression)
{
var innerVisitor = new IncludeExpandingExpressionVisitor(_visitor, navigationExpansionExpression);
var innerVisitor = new IncludeExpandingExpressionVisitor(_visitor, navigationExpansionExpression, _isTracking);
var pendingSelector = innerVisitor.Visit(navigationExpansionExpression.PendingSelector);
pendingSelector = _visitor.Visit(pendingSelector);
pendingSelector = Visit(pendingSelector);
Expand Down
4 changes: 3 additions & 1 deletion src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ namespace Microsoft.EntityFrameworkCore.Query.Internal
public partial class NavigationExpandingExpressionVisitor : ExpressionVisitor
{
private readonly IModel _model;
private readonly bool _isTracking;
private readonly PendingSelectorExpandingExpressionVisitor _pendingSelectorExpandingExpressionVisitor;
private readonly SubqueryMemberPushdownExpressionVisitor _subqueryMemberPushdownExpressionVisitor;
private readonly ReducingExpressionVisitor _reducingExpressionVisitor;
Expand All @@ -34,6 +35,7 @@ public partial class NavigationExpandingExpressionVisitor : ExpressionVisitor
public NavigationExpandingExpressionVisitor(QueryCompilationContext queryCompilationContext)
{
_model = queryCompilationContext.Model;
_isTracking = queryCompilationContext.IsTracking;
_pendingSelectorExpandingExpressionVisitor = new PendingSelectorExpandingExpressionVisitor(this);
_subqueryMemberPushdownExpressionVisitor = new SubqueryMemberPushdownExpressionVisitor();
_reducingExpressionVisitor = new ReducingExpressionVisitor();
Expand All @@ -56,7 +58,7 @@ public virtual Expression Expand(Expression query)
{
var result = Visit(query);
result = _pendingSelectorExpandingExpressionVisitor.Visit(result);
result = new IncludeApplyingExpressionVisitor(this).Visit(result);
result = new IncludeApplyingExpressionVisitor(this, _isTracking).Visit(result);

return Reduce(result);
}
Expand Down
53 changes: 0 additions & 53 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,21 +206,6 @@ public virtual Task Key_equality_two_conditions_on_same_navigation2(bool isAsync
(e, a) => Assert.Equal(e.Id, a.Id));
}

[ConditionalFact]
public virtual void Data_reader_is_closed_correct_number_of_times_for_include_queries_on_optional_navigations()
{
using (var context = CreateContext())
{
// reader for the last include is not opened because there is no data one level below - we should only try to close connection as many times as we tried to open it
// if we close it too many times, consecutive query will not work
// see issue #1457 for more details
context.LevelOne.Include(e => e.OneToMany_Optional1).ThenInclude(e => e.OneToMany_Optional2)
.ThenInclude(e => e.OneToMany_Optional_Inverse3.OneToMany_Optional2).ToList();

context.LevelOne.ToList();
}
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Multi_level_include_one_to_many_optional_and_one_to_many_optional_produces_valid_sql(bool isAsync)
Expand Down Expand Up @@ -262,44 +247,6 @@ public virtual Task Multi_level_include_correct_PK_is_chosen_as_the_join_predica
elementSorter: e => e.Id);
}

[ConditionalFact]
public virtual void Multi_level_include_reads_key_values_from_data_reader_rather_than_incorrect_reader_deep_into_the_stack()
{
using (var context = CreateContext())
{
// #1433
context.LevelOne.Include(e => e.OneToMany_Optional1).ToList();
context.LevelOne.Include(e => e.OneToMany_Optional_Self1).ToList();

// #1478
context.LevelOne
.Include(e => e.OneToMany_Optional1)
.ThenInclude(e => e.OneToMany_Optional_Inverse2.OneToMany_Optional_Self_Inverse1.OneToOne_Optional_FK1).ToList();

context.LevelOne
.Include(e => e.OneToMany_Optional1)
.ThenInclude(e => e.OneToMany_Optional_Inverse2.OneToMany_Optional_Self_Inverse1.OneToOne_Optional_PK1).ToList();

// #1487
context.LevelOne
.Include(e => e.OneToMany_Optional1)
.ThenInclude(e => e.OneToMany_Optional_Inverse2.OneToOne_Optional_PK1.OneToOne_Optional_FK2).ToList();

context.LevelOne
.Include(e => e.OneToMany_Optional1)
.ThenInclude(e => e.OneToMany_Optional_Inverse2.OneToOne_Optional_PK1.OneToOne_Optional_FK_Inverse2).ToList();

// #1488
context.LevelOne
.Include(e => e.OneToMany_Optional1)
.ThenInclude(e => e.OneToMany_Optional_Inverse2.OneToOne_Optional_PK1.OneToOne_Required_FK2).ToList();

context.LevelOne
.Include(e => e.OneToMany_Optional1)
.ThenInclude(e => e.OneToMany_Optional_Inverse2.OneToOne_Optional_PK1.OneToOne_Required_FK_Inverse2).ToList();
}
}

[ConditionalFact]
public virtual void Multi_level_include_with_short_circuiting()
{
Expand Down
4 changes: 0 additions & 4 deletions test/EFCore.Specification.Tests/Query/ComplexNavigationsWeakQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@ public override Task Multiple_complex_includes_self_ref(bool isAsync)
return Task.CompletedTask;
}

public override void Multi_level_include_reads_key_values_from_data_reader_rather_than_incorrect_reader_deep_into_the_stack()
{
}

public override Task Join_condition_optimizations_applied_correctly_when_anonymous_type_with_multiple_properties(bool isAsync)
{
return Task.CompletedTask;
Expand Down
73 changes: 19 additions & 54 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,15 @@ public virtual Task ToString_guid_property_projection(bool isAsync)
});
}

[ConditionalTheory(Skip = "Issue#16225")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Include_multiple_one_to_one_and_one_to_many_self_reference(bool isAsync)
[ConditionalFact]
public virtual void Include_multiple_one_to_one_and_one_to_many_self_reference()
{
var expectedIncludes = new List<IExpectedInclude>
using (var context = CreateContext())
{
new ExpectedInclude<Weapon>(w => w.Owner, "Owner"),
new ExpectedInclude<Gear>(g => g.Weapons, "Weapons", "Owner"),
new ExpectedInclude<Officer>(o => o.Weapons, "Weapons", "Owner")
};
Assert.Throws<InvalidOperationException>(
() => context.Weapons.Include(w => w.Owner.Weapons).ToList());

return AssertIncludeQuery<Weapon>(
isAsync,
ws => ws.Include(w => w.Owner.Weapons),
expectedIncludes);
}
}

[ConditionalTheory]
Expand All @@ -116,22 +110,14 @@ public virtual Task Include_multiple_one_to_one_optional_and_one_to_one_required
expectedIncludes);
}

[ConditionalTheory(Skip = "Issue#16225")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Include_multiple_one_to_one_and_one_to_one_and_one_to_many(bool isAsync)
[ConditionalFact]
public virtual void Include_multiple_one_to_one_and_one_to_one_and_one_to_many()
{
var expectedIncludes = new List<IExpectedInclude>
using (var context = CreateContext())
{
new ExpectedInclude<CogTag>(t => t.Gear, "Gear"),
new ExpectedInclude<Gear>(g => g.Squad, "Squad", "Gear"),
new ExpectedInclude<Officer>(o => o.Squad, "Squad", "Gear"),
new ExpectedInclude<Squad>(s => s.Members, "Members", "Gear.Squad")
};

return AssertIncludeQuery<CogTag>(
isAsync,
ts => ts.Include(t => t.Gear.Squad.Members),
expectedIncludes);
Assert.Throws<InvalidOperationException>(
() => context.Tags.Include(t => t.Gear.Squad.Members).ToList());
}
}

[ConditionalTheory]
Expand Down Expand Up @@ -184,39 +170,18 @@ public virtual Task Include_using_alternate_key(bool isAsync)
expectedIncludes);
}

[ConditionalTheory(Skip = "Issue#16225")]
[MemberData(nameof(IsAsyncData))]
public virtual Task Include_multiple_include_then_include(bool isAsync)
[ConditionalFact]
public virtual void Include_multiple_include_then_include()
{
var expectedIncludes = new List<IExpectedInclude>
using (var context = CreateContext())
{
new ExpectedInclude<Gear>(g => g.AssignedCity, "AssignedCity"),
new ExpectedInclude<Officer>(o => o.AssignedCity, "AssignedCity"),
new ExpectedInclude<City>(c => c.BornGears, "BornGears", "AssignedCity"),
new ExpectedInclude<Gear>(g => g.Tag, "Tag", "AssignedCity.BornGears"),
new ExpectedInclude<Officer>(o => o.Tag, "Tag", "AssignedCity.BornGears"),
new ExpectedInclude<City>(c => c.StationedGears, "StationedGears", "AssignedCity"),
new ExpectedInclude<Gear>(g => g.Tag, "Tag", "AssignedCity.StationedGears"),
new ExpectedInclude<Officer>(o => o.Tag, "Tag", "AssignedCity.StationedGears"),
new ExpectedInclude<Gear>(g => g.CityOfBirth, "CityOfBirth"),
new ExpectedInclude<Officer>(o => o.CityOfBirth, "CityOfBirth"),
new ExpectedInclude<City>(c => c.BornGears, "BornGears", "CityOfBirth"),
new ExpectedInclude<Gear>(g => g.Tag, "Tag", "CityOfBirth.BornGears"),
new ExpectedInclude<Officer>(o => o.Tag, "Tag", "CityOfBirth.BornGears"),
new ExpectedInclude<City>(c => c.StationedGears, "StationedGears", "CityOfBirth"),
new ExpectedInclude<Gear>(g => g.Tag, "Tag", "CityOfBirth.StationedGears"),
new ExpectedInclude<Officer>(o => o.Tag, "Tag", "CityOfBirth.StationedGears")
};

return AssertIncludeQuery<Gear>(
isAsync,
gs => gs.Include(g => g.AssignedCity.BornGears).ThenInclude(g => g.Tag)
Assert.Throws<InvalidOperationException>(
() => context.Gears.Include(g => g.AssignedCity.BornGears).ThenInclude(g => g.Tag)
.Include(g => g.AssignedCity.StationedGears).ThenInclude(g => g.Tag)
.Include(g => g.CityOfBirth.BornGears).ThenInclude(g => g.Tag)
.Include(g => g.CityOfBirth.StationedGears).ThenInclude(g => g.Tag)
.OrderBy(g => g.Nickname),
expectedIncludes,
assertOrder: true);
.OrderBy(g => g.Nickname).ToList());
}
}

[ConditionalTheory]
Expand Down
Loading

0 comments on commit f22487a

Please sign in to comment.