From 4b4d434a7fc2b5ee4fb60e8525e56ee279c92b7d Mon Sep 17 00:00:00 2001 From: Maurycy Markowski Date: Fri, 31 May 2019 18:22:56 -0700 Subject: [PATCH] Correctig navigatgion expansion handling of owned navigations 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. --- .../ChangeTracking/Internal/StateManager.cs | 13 +- .../NavigationExpansionHelpers.cs | 10 +- .../NavigationExpansion/NavigationTreeNode.cs | 35 +++-- .../CollectionNavigationRewritingVisitor.cs | 6 +- .../Query/OwnedQueryCosmosTest.cs | 4 +- .../Query/OwnedQueryTestBase.cs | 134 +++++++++++++++--- .../Query/OwnedQuerySqlServerTest.cs | 60 +++++++- 7 files changed, 219 insertions(+), 43 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/StateManager.cs b/src/EFCore/ChangeTracking/Internal/StateManager.cs index eb3180a7eb9..023c0bdc3d9 100644 --- a/src/EFCore/ChangeTracking/Internal/StateManager.cs +++ b/src/EFCore/ChangeTracking/Internal/StateManager.cs @@ -288,13 +288,22 @@ public virtual InternalEntityEntry CreateEntry(IDictionary 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); diff --git a/src/EFCore/Query/NavigationExpansion/NavigationExpansionHelpers.cs b/src/EFCore/Query/NavigationExpansion/NavigationExpansionHelpers.cs index a391adf8aa4..67e21288a7a 100644 --- a/src/EFCore/Query/NavigationExpansion/NavigationExpansionHelpers.cs +++ b/src/EFCore/Query/NavigationExpansion/NavigationExpansionHelpers.cs @@ -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; @@ -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) { diff --git a/src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs b/src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs index 8a578aa5df8..e1e266d7d3b 100644 --- a/src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs +++ b/src/EFCore/Query/NavigationExpansion/NavigationTreeNode.cs @@ -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(); @@ -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; diff --git a/src/EFCore/Query/NavigationExpansion/Visitors/CollectionNavigationRewritingVisitor.cs b/src/EFCore/Query/NavigationExpansion/Visitors/CollectionNavigationRewritingVisitor.cs index 5ec9f689bd2..0810077ebf2 100644 --- a/src/EFCore/Query/NavigationExpansion/Visitors/CollectionNavigationRewritingVisitor.cs +++ b/src/EFCore/Query/NavigationExpansion/Visitors/CollectionNavigationRewritingVisitor.cs @@ -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 { diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs index f760f430e25..bdd8c897196 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs @@ -29,9 +29,9 @@ FROM root c WHERE ((c[""Discriminator""] = ""LeafB"") OR ((c[""Discriminator""] = ""LeafA"") OR ((c[""Discriminator""] = ""Branch"") OR (c[""Discriminator""] = ""OwnedPerson""))))"); } - public override void Navigation_rewrite_on_owned_reference() + public override void Navigation_rewrite_on_owned_reference_projecting_entity() { - base.Navigation_rewrite_on_owned_reference(); + base.Navigation_rewrite_on_owned_reference_projecting_entity(); AssertSql( @"SELECT c diff --git a/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs index 5b9358e33ee..bb697cec6f3 100644 --- a/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/OwnedQueryTestBase.cs @@ -38,7 +38,7 @@ from b in context.Set() } } - [Fact(Skip = "issue #15285")] + [Fact] public virtual void Query_with_owned_entity_equality_method() { using (var context = CreateContext()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -166,13 +166,15 @@ 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().Where(p => p.PersonAddress.Country.Name == "USA") + var query = ctx.Set() + .Where(p => p.PersonAddress.Country.Name == "USA") .Select(p => p.PersonAddress.Country.Name); + var result = query.ToList(); Assert.Equal(4, result.Count); @@ -180,6 +182,18 @@ public virtual void Navigation_rewrite_on_owned_reference() } } + [Fact] + public virtual void Navigation_rewrite_on_owned_reference_projecting_entity() + { + using (var ctx = CreateContext()) + { + var query = ctx.Set().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() { @@ -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().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().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()) @@ -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()) @@ -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()) @@ -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().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().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().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().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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs index fa9add50683..3b84865e679 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs @@ -547,9 +547,9 @@ ORDER BY [t8].[Id] ORDER BY [t18].[Id]"); } - public override void Navigation_rewrite_on_owned_reference() + public override void Navigation_rewrite_on_owned_reference_projecting_scalar() { - base.Navigation_rewrite_on_owned_reference(); + base.Navigation_rewrite_on_owned_reference_projecting_scalar(); AssertSql( @"SELECT [t0].[PersonAddress_Country_Name] @@ -567,6 +567,14 @@ WHERE [p.PersonAddress.Country].[Discriminator] IN (N'LeafB', N'LeafA', N'Branch WHERE [p].[Discriminator] IN (N'LeafB', N'LeafA', N'Branch', N'OwnedPerson') AND ([t0].[PersonAddress_Country_Name] = N'USA')"); } + public override void Navigation_rewrite_on_owned_reference_projecting_entity() + { + base.Navigation_rewrite_on_owned_reference_projecting_entity(); + + AssertSql( + @""); + } + public override void Navigation_rewrite_on_owned_collection() { base.Navigation_rewrite_on_owned_collection(); @@ -595,6 +603,22 @@ FROM [Order] AS [o0] ORDER BY [t].[Id]"); } + public override void Navigation_rewrite_on_owned_collection_with_composition() + { + base.Navigation_rewrite_on_owned_collection_with_composition(); + + AssertSql( + @""); + } + + public override void Navigation_rewrite_on_owned_collection_with_composition_complex() + { + base.Navigation_rewrite_on_owned_collection_with_composition_complex(); + + AssertSql( + @""); + } + public override void Select_many_on_owned_collection() { base.Select_many_on_owned_collection(); @@ -627,6 +651,38 @@ WHERE [p.PersonAddress.Country].[Discriminator] IN (N'LeafB', N'LeafA', N'Branch WHERE [p].[Discriminator] IN (N'LeafB', N'LeafA', N'Branch', N'OwnedPerson')"); } + public override void Filter_owned_entity_chained_with_regular_entity_followed_by_projecting_owned_collection() + { + base.Filter_owned_entity_chained_with_regular_entity_followed_by_projecting_owned_collection(); + + AssertSql( + @""); + } + + public override void Project_multiple_owned_navigations() + { + base.Project_multiple_owned_navigations(); + + AssertSql( + @""); + } + + public override void Project_multiple_owned_navigations_with_expansion_on_owned_collections() + { + base.Project_multiple_owned_navigations_with_expansion_on_owned_collections(); + + AssertSql( + @""); + } + + public override void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_filter() + { + base.Navigation_rewrite_on_owned_reference_followed_by_regular_entity_filter(); + + AssertSql( + @""); + } + public override void Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_property() { base.Navigation_rewrite_on_owned_reference_followed_by_regular_entity_and_property();