Skip to content

Commit

Permalink
Correctig navigatgion expansion handling of owned navigations
Browse files Browse the repository at this point in the history 
Navigation expansion should not try expand owned navigations. We still add them to navigation tree for tracking purposes but we don't inject LeftJoin calls when processing them.
Collection navigations are converted directly to NavigationExpansionExpressions whose root is the navigation itself to avoid rewrite to a subquery.
  • Loading branch information
@maumar
maumar committed Jun 4, 2019
1 parent d61b9db commit 86bddc6
Show file tree
Hide file tree
Showing 6 changed files with 217 additions and 41 deletions.
13 changes: 11 additions & 2 deletions src/EFCore/ChangeTracking/Internal/StateManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,13 +288,22 @@ public virtual InternalEntityEntry CreateEntry(IDictionary<string, object> value
}

var valueBuffer = new ValueBuffer(valuesArray);

var entity = entityType.HasClrType()
? EntityMaterializerSource.GetMaterializer(entityType)(
new MaterializationContext(valueBuffer, Context))
: null;

var entry = _internalEntityEntryFactory.Create(this, entityType, entity, valueBuffer);
i = 0;
var shadowPropertyValuesArray = new object[entityType.ShadowPropertyCount()];
foreach (var shadowProperty in entityType.GetProperties().Where(p => p.IsShadowProperty()))
{
shadowPropertyValuesArray[i++] = values.TryGetValue(shadowProperty.Name, out var shadowValue)
? shadowValue
: shadowProperty.ClrType.GetDefaultValue();
}

var shadowPropertyValueBuffer = new ValueBuffer(shadowPropertyValuesArray);
var entry = _internalEntityEntryFactory.Create(this, entityType, entity, shadowPropertyValueBuffer);

UpdateReferenceMaps(entry, EntityState.Detached, null);

Expand Down
10 changes: 8 additions & 2 deletions src/EFCore/Query/NavigationExpansion/NavigationExpansionHelpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Extensions.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
Expand Down Expand Up @@ -182,8 +183,13 @@ var transparentIdentifierCtorInfo
foreach (var mapping in state.SourceMappings)
{
var nodes = include
? mapping.NavigationTree.Flatten().Where(n => (n.Included == NavigationTreeNodeIncludeMode.ReferenceComplete || n.ExpansionMode == NavigationTreeNodeExpansionMode.ReferenceComplete) && n != navigationTree)
: mapping.NavigationTree.Flatten().Where(n => n.ExpansionMode == NavigationTreeNodeExpansionMode.ReferenceComplete && n != navigationTree);
? mapping.NavigationTree.Flatten().Where(n => (n.Included == NavigationTreeNodeIncludeMode.ReferenceComplete
|| n.ExpansionMode == NavigationTreeNodeExpansionMode.ReferenceComplete
|| n.Navigation.ForeignKey.IsOwnership)
&& n != navigationTree)
: mapping.NavigationTree.Flatten().Where(n => (n.ExpansionMode == NavigationTreeNodeExpansionMode.ReferenceComplete
|| n.Navigation.ForeignKey.IsOwnership)
&& n != navigationTree);

foreach (var navigationTreeNode in nodes)
{
Expand Down
35 changes: 25 additions & 10 deletions src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@ private NavigationTreeNode(
Included = NavigationTreeNodeIncludeMode.NotNeeded;
}

// for ownership don't mark for include or expansion
// just track the navigations in the tree in the original form
// they will be expanded/translated later in the pipeline
if (navigation.ForeignKey.IsOwnership)
{
ExpansionMode = NavigationTreeNodeExpansionMode.NotNeeded;
Included = NavigationTreeNodeIncludeMode.NotNeeded;

ToMapping = parent.ToMapping.ToList();
ToMapping.Add(navigation.Name);
}

foreach (var parentFromMapping in parent.FromMappings)
{
var newMapping = parentFromMapping.ToList();
Expand Down Expand Up @@ -92,17 +104,20 @@ public static NavigationTreeNode Create(
var existingChild = parent.Children.Where(c => c.Navigation == navigation).SingleOrDefault();
if (existingChild != null)
{
if (include && existingChild.Included == NavigationTreeNodeIncludeMode.NotNeeded)
{
existingChild.Included = navigation.IsCollection()
? NavigationTreeNodeIncludeMode.Collection
: NavigationTreeNodeIncludeMode.ReferencePending;
}
else if (!include && existingChild.ExpansionMode == NavigationTreeNodeExpansionMode.NotNeeded)
if (!navigation.ForeignKey.IsOwnership)
{
existingChild.ExpansionMode = navigation.IsCollection()
? NavigationTreeNodeExpansionMode.Collection
: NavigationTreeNodeExpansionMode.ReferencePending;
if (include && existingChild.Included == NavigationTreeNodeIncludeMode.NotNeeded)
{
existingChild.Included = navigation.IsCollection()
? NavigationTreeNodeIncludeMode.Collection
: NavigationTreeNodeIncludeMode.ReferencePending;
}
else if (!include && existingChild.ExpansionMode == NavigationTreeNodeExpansionMode.NotNeeded)
{
existingChild.ExpansionMode = navigation.IsCollection()
? NavigationTreeNodeExpansionMode.Collection
: NavigationTreeNodeExpansionMode.ReferencePending;
}
}

return existingChild;
Expand Down
6 changes: 3 additions & 3 deletions src/EFCore/Query/NavigationExpansion/Visitors/CollectionNavigationRewritingVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
&& navigationBindingExpression.NavigationTreeNode.Navigation is INavigation lastNavigation
&& lastNavigation.IsCollection())
{
var result = CreateCollectionNavigationExpression(navigationBindingExpression.NavigationTreeNode, navigationBindingExpression.RootParameter, navigationBindingExpression.SourceMapping);

return result;
return lastNavigation.ForeignKey.IsOwnership
? NavigationExpansionHelpers.CreateNavigationExpansionRoot(navigationBindingExpression, lastNavigation.GetTargetType(), lastNavigation)
: CreateCollectionNavigationExpression(navigationBindingExpression.NavigationTreeNode, navigationBindingExpression.RootParameter, navigationBindingExpression.SourceMapping);
}
else
{
Expand Down
134 changes: 112 additions & 22 deletions test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ from b in context.Set<LeafB>()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_with_owned_entity_equality_method()
{
using (var context = CreateContext())
Expand All @@ -53,7 +53,7 @@ where a.LeafAAddress.Equals(b.LeafBAddress)
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_with_owned_entity_equality_object_method()
{
using (var context = CreateContext())
Expand All @@ -68,7 +68,7 @@ where Equals(a.LeafAAddress, b.LeafBAddress)
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_for_base_type_loads_all_owned_navs()
{
using (var context = CreateContext())
Expand All @@ -85,7 +85,7 @@ public virtual void Query_for_base_type_loads_all_owned_navs()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void No_ignored_include_warning_when_implicit_load()
{
using (var context = CreateContext())
Expand All @@ -96,7 +96,7 @@ public virtual void No_ignored_include_warning_when_implicit_load()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_for_branch_type_loads_all_owned_navs()
{
using (var context = CreateContext())
Expand All @@ -112,7 +112,7 @@ public virtual void Query_for_branch_type_loads_all_owned_navs()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_for_leaf_type_loads_all_owned_navs()
{
using (var context = CreateContext())
Expand Down Expand Up @@ -141,7 +141,7 @@ public virtual void Query_when_group_by()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_when_subquery()
{
using (var context = CreateContext())
Expand All @@ -166,20 +166,34 @@ var people
}
}

[Fact(Skip = "issue #15285")]
public virtual void Navigation_rewrite_on_owned_reference()
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_projecting_scalar()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Where(p => p.PersonAddress.Country.Name == "USA")
var query = ctx.Set<OwnedPerson>()
.Where(p => p.PersonAddress.Country.Name == "USA")
.Select(p => p.PersonAddress.Country.Name);

var result = query.ToList();

Assert.Equal(4, result.Count);
Assert.True(result.All(r => r == "USA"));
}
}

[Fact]
public virtual void Navigation_rewrite_on_owned_reference_projecting_entity()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Where(p => p.PersonAddress.Country.Name == "USA");
var result = query.ToList();

Assert.Equal(4, result.Count);
}
}

[Fact(Skip = "issue #15043")]
public virtual void Navigation_rewrite_on_owned_collection()
{
Expand All @@ -193,7 +207,35 @@ public virtual void Navigation_rewrite_on_owned_collection()
}
}

[Fact(Skip = "issue #15285")]
[Fact(Skip = "issue #15043")]
public virtual void Navigation_rewrite_on_owned_collection_with_composition()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Select(p => p.Orders.Select(o => o.Id != 42).FirstOrDefault());
var result = query.ToList();

//Assert.Equal(4, result.Count);
//Assert.True(result.All(r => r.Count > 0));
}
}


[Fact(Skip = "issue #15043")]
public virtual void Navigation_rewrite_on_owned_collection_with_composition_complex()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Select(p => p.Orders.Select(o => o.Client.PersonAddress.Country.Name).FirstOrDefault());
var result = query.ToList();

//Assert.Equal(4, result.Count);
//Assert.True(result.All(r => r.Count > 0));
}
}


[Fact]
public virtual void Select_many_on_owned_collection()
{
using (var ctx = CreateContext())
Expand All @@ -205,7 +247,7 @@ public virtual void Select_many_on_owned_collection()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Set_throws_for_owned_type()
{
using (var ctx = CreateContext())
Expand All @@ -215,7 +257,7 @@ public virtual void Set_throws_for_owned_type()
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity()
{
using (var ctx = CreateContext())
Expand All @@ -227,7 +269,55 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Filter_owned_entity_chained_with_regular_entity_followed_by_projecting_owned_collection()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Where(p => p.PersonAddress.Country.Planet.Id != 42).Select(p => new { p.Orders });
var result = query.ToList();

Assert.Equal(4, result.Count);
}
}

[Fact]
public virtual void Project_multiple_owned_navigations()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Select(p => new { p.Orders, p.PersonAddress, p.PersonAddress.Country.Planet });
var result = query.ToList();

Assert.Equal(4, result.Count);
}
}

[Fact]
public virtual void Project_multiple_owned_navigations_with_expansion_on_owned_collections()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Select(p => new { Count = p.Orders.Where(o => o.Client.PersonAddress.Country.Planet.Star.Id != 42).Count(), p.PersonAddress.Country.Planet });
var result = query.ToList();

Assert.Equal(4, result.Count);
}
}

[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_filter()
{
using (var ctx = CreateContext())
{
var query = ctx.Set<OwnedPerson>().Where(p => p.PersonAddress.Country.Planet.Id != 7).Select(p => new { p });
var result = query.ToList();

Assert.Equal(4, result.Count);
}
}

[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_property()
{
using (var ctx = CreateContext())
Expand All @@ -240,7 +330,7 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_collection()
{
using (var ctx = CreateContext())
Expand All @@ -253,7 +343,7 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void SelectMany_on_owned_reference_followed_by_regular_entity_and_collection()
{
using (var ctx = CreateContext())
Expand All @@ -266,7 +356,7 @@ public virtual void SelectMany_on_owned_reference_followed_by_regular_entity_and
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void SelectMany_on_owned_reference_with_entity_in_between_ending_in_owned_collection()
{
using (var ctx = CreateContext())
Expand All @@ -279,7 +369,7 @@ public virtual void SelectMany_on_owned_reference_with_entity_in_between_ending_
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_collection_count()
{
using (var ctx = CreateContext())
Expand All @@ -292,7 +382,7 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_another_reference()
{
using (var ctx = CreateContext())
Expand All @@ -305,7 +395,7 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_another_reference_and_scalar()
{
using (var ctx = CreateContext())
Expand All @@ -318,7 +408,7 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_another_reference_in_predicate_and_projection()
{
using (var ctx = CreateContext())
Expand All @@ -332,7 +422,7 @@ public virtual void Navigation_rewrite_on_owned_reference_followed_by_regular_en
}
}

[Fact(Skip = "issue #15285")]
[Fact]
public virtual void Query_with_OfType_eagerly_loads_correct_owned_navigations()
{
using (var ctx = CreateContext())
Expand Down
Loading

0 comments on commit 86bddc6

Please sign in to comment.