From 38b6225618e9bbd82d98415a1f2162e283702753 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Wed, 24 Jun 2020 00:33:15 -0700 Subject: [PATCH] Query: Fix a minor bug in adding dependent conditions --- .../Query/SqlExpressionFactory.cs | 149 +++++++----------- .../Query/TPTInheritanceQueryFixture.cs | 14 +- .../Query/InheritanceQueryFixtureBase.cs | 9 +- .../TableSplittingSqlServerTest.cs | 12 +- 4 files changed, 81 insertions(+), 103 deletions(-) diff --git a/src/EFCore.Relational/Query/SqlExpressionFactory.cs b/src/EFCore.Relational/Query/SqlExpressionFactory.cs index 2bed788fafd..0a2aaa7875e 100644 --- a/src/EFCore.Relational/Query/SqlExpressionFactory.cs +++ b/src/EFCore.Relational/Query/SqlExpressionFactory.cs @@ -791,91 +791,69 @@ public virtual SelectExpression Select(IEntityType entityType, string sql, Expre return selectExpression; } - private void AddConditions( - SelectExpression selectExpression, - IEntityType entityType, - ITableBase table = null, - bool skipJoins = false) + private void AddSelfConditions(SelectExpression selectExpression, IEntityType entityType, ITableBase table = null) { + // Add conditions if TPH + var discriminatorAdded = AddDiscriminatorCondition(selectExpression, entityType); if (entityType.FindPrimaryKey() == null) { - AddDiscriminatorCondition(selectExpression, entityType); + return; } - else - { - var tableMappings = entityType.GetViewOrTableMappings(); - if (!tableMappings.Any()) - { - return; - } - table ??= tableMappings.Single().Table; - var discriminatorAdded = AddDiscriminatorCondition(selectExpression, entityType); + // Add conditions if dependent sharing table with principal + table ??= entityType.GetViewOrTableMappings().SingleOrDefault()?.Table; + if (table != null + && table.GetRowInternalForeignKeys(entityType).Any() + && !discriminatorAdded) + { + AddOptionalDependentConditions(selectExpression, entityType, table); + } + } + private void AddConditions(SelectExpression selectExpression, IEntityType entityType, ITableBase table = null) + { + AddSelfConditions(selectExpression, entityType, table); + // Add inner join to principal if table sharing + table ??= entityType.GetViewOrTableMappings().SingleOrDefault()?.Table; + if (table != null) + { var linkingFks = table.GetRowInternalForeignKeys(entityType); - if (linkingFks.Any()) + var first = true; + foreach (var foreignKey in linkingFks) { - if (!discriminatorAdded) + if (first) { - AddOptionalDependentConditions(selectExpression, entityType, table); + AddInnerJoin(selectExpression, foreignKey, table); + first = false; } - - if (!skipJoins) + else { - var first = true; - - foreach (var foreignKey in linkingFks) - { - if (!(entityType.FindOwnership() == foreignKey - && foreignKey.PrincipalEntityType.BaseType == null)) - { - var otherSelectExpression = first - ? selectExpression - : new SelectExpression(entityType); - - AddInnerJoin(otherSelectExpression, foreignKey, table, skipInnerJoins: false); - - if (first) - { - first = false; - } - else - { - selectExpression.ApplyUnion(otherSelectExpression, distinct: true); - } - } - } + var dependentSelectExpression = new SelectExpression(entityType); + AddSelfConditions(dependentSelectExpression, entityType, table); + AddInnerJoin(dependentSelectExpression, foreignKey, table); + selectExpression.ApplyUnion(dependentSelectExpression, distinct: true); } } } } - private void AddInnerJoin( - SelectExpression selectExpression, IForeignKey foreignKey, ITableBase table, bool skipInnerJoins) - { - var joinPredicate = GenerateJoinPredicate(selectExpression, foreignKey, table, skipInnerJoins, out var innerSelect); - - selectExpression.AddInnerJoin(innerSelect, joinPredicate); - } - - private SqlExpression GenerateJoinPredicate( - SelectExpression selectExpression, - IForeignKey foreignKey, - ITableBase table, - bool skipInnerJoins, - out SelectExpression innerSelect) + private void AddInnerJoin(SelectExpression selectExpression, IForeignKey foreignKey, ITableBase table) { var outerEntityProjection = GetMappedEntityProjectionExpression(selectExpression); var outerIsPrincipal = foreignKey.PrincipalEntityType.IsAssignableFrom(outerEntityProjection.EntityType); - innerSelect = outerIsPrincipal + var innerSelect = outerIsPrincipal ? new SelectExpression(foreignKey.DeclaringEntityType) : new SelectExpression(foreignKey.PrincipalEntityType); - AddConditions( - innerSelect, - outerIsPrincipal ? foreignKey.DeclaringEntityType : foreignKey.PrincipalEntityType, - table, - skipInnerJoins); + + if (outerIsPrincipal) + { + AddSelfConditions(innerSelect, foreignKey.DeclaringEntityType, table); + } + else + { + AddConditions(innerSelect, foreignKey.PrincipalEntityType, table); + } var innerEntityProjection = GetMappedEntityProjectionExpression(innerSelect); @@ -884,42 +862,27 @@ private SqlExpression GenerateJoinPredicate( var innerKey = (outerIsPrincipal ? foreignKey.Properties : foreignKey.PrincipalKey.Properties) .Select(p => innerEntityProjection.BindProperty(p)); - return outerKey.Zip(innerKey, Equal) - .Aggregate(AndAlso); + var joinPredicate = outerKey.Zip(innerKey, Equal).Aggregate(AndAlso); + + selectExpression.AddInnerJoin(innerSelect, joinPredicate); } private bool AddDiscriminatorCondition(SelectExpression selectExpression, IEntityType entityType) { - if (entityType.GetRootType().GetIsDiscriminatorMappingComplete() - && entityType.GetAllBaseTypesInclusiveAscending() - .All(e => (e == entityType || e.IsAbstract()) && !HasSiblings(e))) + var discriminatorProperty = entityType.GetDiscriminatorProperty(); + if (discriminatorProperty == null + || (entityType.GetRootType().GetIsDiscriminatorMappingComplete() + && entityType.GetAllBaseTypesInclusiveAscending() + .All(e => (e == entityType || e.IsAbstract()) && !HasSiblings(e)))) { return false; } - SqlExpression predicate; + var discriminatorColumn = GetMappedEntityProjectionExpression(selectExpression).BindProperty(discriminatorProperty); var concreteEntityTypes = entityType.GetConcreteDerivedTypesInclusive().ToList(); - if (concreteEntityTypes.Count == 1) - { - var concreteEntityType = concreteEntityTypes[0]; - if (concreteEntityType.BaseType == null) - { - return false; - } - - var discriminatorColumn = GetMappedEntityProjectionExpression(selectExpression) - .BindProperty(concreteEntityType.GetDiscriminatorProperty()); - - predicate = Equal(discriminatorColumn, Constant(concreteEntityType.GetDiscriminatorValue())); - } - else - { - var discriminatorColumn = GetMappedEntityProjectionExpression(selectExpression) - .BindProperty(concreteEntityTypes[0].GetDiscriminatorProperty()); - - predicate = In( - discriminatorColumn, Constant(concreteEntityTypes.Select(et => et.GetDiscriminatorValue()).ToList()), negated: false); - } + var predicate = concreteEntityTypes.Count == 1 + ? (SqlExpression)Equal(discriminatorColumn, Constant(concreteEntityTypes[0].GetDiscriminatorValue())) + : In(discriminatorColumn, Constant(concreteEntityTypes.Select(et => et.GetDiscriminatorValue()).ToList()), negated: false); selectExpression.ApplyPredicate(predicate); @@ -977,6 +940,8 @@ private void AddOptionalDependentConditions( selectExpression.ApplyPredicate(predicate); + // If there is no non-nullable property then we also need to add optional dependents which are acting as principal for + // other dependents. foreach (var referencingFk in entityType.GetReferencingForeignKeys()) { var otherSelectExpression = new SelectExpression(entityType); @@ -984,8 +949,8 @@ private void AddOptionalDependentConditions( var sameTable = table.GetRowInternalForeignKeys(referencingFk.DeclaringEntityType).Any(); AddInnerJoin( otherSelectExpression, referencingFk, - sameTable ? table : null, - skipInnerJoins: sameTable); + sameTable ? table : null); + selectExpression.ApplyUnion(otherSelectExpression, distinct: true); } } diff --git a/test/EFCore.Relational.Specification.Tests/Query/TPTInheritanceQueryFixture.cs b/test/EFCore.Relational.Specification.Tests/Query/TPTInheritanceQueryFixture.cs index 33c04e19d58..bf1a61295d9 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/TPTInheritanceQueryFixture.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/TPTInheritanceQueryFixture.cs @@ -8,8 +8,12 @@ namespace Microsoft.EntityFrameworkCore.Query { public abstract class TPTInheritanceQueryFixture : InheritanceQueryFixtureBase { + protected override string StoreName => "TPTInheritanceTest"; + public TestSqlLoggerFactory TestSqlLoggerFactory => (TestSqlLoggerFactory)ListLoggerFactory; + protected override bool HasDiscriminator => false; + protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context) { base.OnModelCreating(modelBuilder, context); @@ -20,10 +24,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con modelBuilder.Entity().ToTable("Daisies"); modelBuilder.Entity().Property(e => e.Id).ValueGeneratedNever(); - modelBuilder.Entity().ToTable("Plants"); - modelBuilder.Entity().ToTable("Roses"); - modelBuilder.Entity().ToTable("Daisies"); - modelBuilder.Entity().ToTable("Daisies"); + modelBuilder.Entity().ToTable("Animals"); + modelBuilder.Entity().ToTable("Birds"); + modelBuilder.Entity().ToTable("Eagle"); + modelBuilder.Entity().ToTable("Kiwi"); modelBuilder.Entity().Property(e => e.Species).HasMaxLength(100); modelBuilder.Entity().HasMany(e => e.Prey).WithOne().HasForeignKey(e => e.EagleId).IsRequired(false); @@ -41,7 +45,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con // Keyless entities are mapped to TPH modelBuilder.Entity().HasNoKey().ToQuery( - () => context.Set().FromSqlRaw("SELECT * FROM Animal")); + () => context.Set().FromSqlRaw("SELECT * FROM Animals")); modelBuilder.Entity().HasDiscriminator().HasValue("Kiwi"); modelBuilder.Entity().HasDiscriminator().HasValue("Eagle"); } diff --git a/test/EFCore.Specification.Tests/Query/InheritanceQueryFixtureBase.cs b/test/EFCore.Specification.Tests/Query/InheritanceQueryFixtureBase.cs index 85f098e01fe..d93d2349253 100644 --- a/test/EFCore.Specification.Tests/Query/InheritanceQueryFixtureBase.cs +++ b/test/EFCore.Specification.Tests/Query/InheritanceQueryFixtureBase.cs @@ -10,6 +10,7 @@ public abstract class InheritanceQueryFixtureBase : SharedStoreFixtureBase false; protected virtual bool IsDiscriminatorMappingComplete => true; + protected virtual bool HasDiscriminator => true; protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context) { @@ -27,9 +28,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con modelBuilder.Entity(); modelBuilder.Entity(); - modelBuilder.Entity().HasDiscriminator("Discriminator").IsComplete(IsDiscriminatorMappingComplete); + if (HasDiscriminator) + { + modelBuilder.Entity().HasDiscriminator("Discriminator").IsComplete(IsDiscriminatorMappingComplete); + modelBuilder.Entity().HasDiscriminator().IsComplete(IsDiscriminatorMappingComplete); + } + modelBuilder.Entity().HasDiscriminator().IsComplete(IsDiscriminatorMappingComplete); - modelBuilder.Entity().HasDiscriminator().IsComplete(IsDiscriminatorMappingComplete); if (EnableFilters) { diff --git a/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs index 094c4d332dc..b1948bf5948 100644 --- a/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs @@ -21,7 +21,7 @@ public override void Can_use_with_redundant_relationships() // TODO: [Name] shouldn't be selected multiple times and no joins are needed AssertSql( - @"SELECT [v].[Name], [v].[Discriminator], [v].[SeatingCapacity], [t].[Name], [t].[Operator_Discriminator], [t].[Operator_Name], [t].[LicenseType], [t1].[Name], [t1].[Type], [t3].[Name], [t3].[Description], [t3].[Engine_Discriminator], [t7].[Name], [t7].[Capacity], [t7].[FuelTank_Discriminator], [t7].[FuelType], [t7].[GrainGeometry] + @"SELECT [v].[Name], [v].[Discriminator], [v].[SeatingCapacity], [t].[Name], [t].[Operator_Discriminator], [t].[Operator_Name], [t].[LicenseType], [t1].[Name], [t1].[Type], [t3].[Name], [t3].[Description], [t3].[Engine_Discriminator], [t7].[Name], [t7].[Capacity], [t7].[FuelTank_Discriminator], [t7].[FuelType], [t7].[GrainGeometry] FROM [Vehicles] AS [v] LEFT JOIN ( SELECT [v0].[Name], [v0].[Operator_Discriminator], [v0].[Operator_Name], [v0].[LicenseType], [v1].[Name] AS [Name0] @@ -72,6 +72,7 @@ FROM [Vehicles] AS [v11] ) AS [t5] ON [v10].[Name] = [t5].[Name] WHERE [v10].[Engine_Discriminator] IN (N'ContinuousCombustionEngine', N'IntermittentCombustionEngine', N'SolidRocket') ) AS [t6] ON [v9].[Name] = [t6].[Name] + WHERE [v9].[FuelTank_Discriminator] IS NOT NULL ) AS [t7] ON [t3].[Name] = [t7].[Name] ORDER BY [v].[Name]"); } @@ -152,7 +153,8 @@ FROM [Vehicles] AS [v3] WHERE [v3].[Discriminator] = N'PoweredVehicle' ) AS [t0] ON [v2].[Name] = [t0].[Name] WHERE [v2].[Engine_Discriminator] IN (N'ContinuousCombustionEngine', N'IntermittentCombustionEngine', N'SolidRocket') -) AS [t1] ON [v1].[Name] = [t1].[Name]"); +) AS [t1] ON [v1].[Name] = [t1].[Name] +WHERE [v1].[FuelTank_Discriminator] IS NOT NULL"); } public override void Can_query_shared_derived_nonhierarchy() @@ -180,7 +182,8 @@ FROM [Vehicles] AS [v3] WHERE [v3].[Discriminator] = N'PoweredVehicle' ) AS [t0] ON [v2].[Name] = [t0].[Name] WHERE [v2].[Engine_Discriminator] IN (N'ContinuousCombustionEngine', N'IntermittentCombustionEngine', N'SolidRocket') -) AS [t1] ON [v1].[Name] = [t1].[Name]"); +) AS [t1] ON [v1].[Name] = [t1].[Name] +WHERE [v1].[FuelType] IS NOT NULL OR [v1].[Capacity] IS NOT NULL"); } public override void Can_query_shared_derived_nonhierarchy_all_required() @@ -208,7 +211,8 @@ FROM [Vehicles] AS [v3] WHERE [v3].[Discriminator] = N'PoweredVehicle' ) AS [t0] ON [v2].[Name] = [t0].[Name] WHERE [v2].[Engine_Discriminator] IN (N'ContinuousCombustionEngine', N'IntermittentCombustionEngine', N'SolidRocket') -) AS [t1] ON [v1].[Name] = [t1].[Name]"); +) AS [t1] ON [v1].[Name] = [t1].[Name] +WHERE [v1].[FuelType] IS NOT NULL AND [v1].[Capacity] IS NOT NULL"); } public override void Can_change_dependent_instance_non_derived()