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: Fix a minor bug in adding dependent conditions #21400

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Query: Fix a minor bug in adding dependent conditions #21400

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
149 changes: 57 additions & 92 deletions src/EFCore.Relational/Query/SqlExpressionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -884,42 +862,27 @@ private SqlExpression GenerateJoinPredicate(
var innerKey = (outerIsPrincipal ? foreignKey.Properties : foreignKey.PrincipalKey.Properties)
.Select(p => innerEntityProjection.BindProperty(p));

return outerKey.Zip<SqlExpression, SqlExpression, SqlExpression>(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);

Expand Down Expand Up @@ -977,15 +940,17 @@ 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);

var sameTable = table.GetRowInternalForeignKeys(referencingFk.DeclaringEntityType).Any();
AddInnerJoin(
otherSelectExpression, referencingFk,
sameTable ? table : null,
skipInnerJoins: sameTable);
sameTable ? table : null);

selectExpression.ApplyUnion(otherSelectExpression, distinct: true);
}
}
Expand Down
14 changes: 9 additions & 5 deletions test/EFCore.Relational.Specification.Tests/Query/TPTInheritanceQueryFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -20,10 +24,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
modelBuilder.Entity<Daisy>().ToTable("Daisies");
modelBuilder.Entity<Country>().Property(e => e.Id).ValueGeneratedNever();

modelBuilder.Entity<Animal>().ToTable("Plants");
modelBuilder.Entity<Bird>().ToTable("Roses");
modelBuilder.Entity<Eagle>().ToTable("Daisies");
modelBuilder.Entity<Kiwi>().ToTable("Daisies");
modelBuilder.Entity<Animal>().ToTable("Animals");
modelBuilder.Entity<Bird>().ToTable("Birds");
modelBuilder.Entity<Eagle>().ToTable("Eagle");
modelBuilder.Entity<Kiwi>().ToTable("Kiwi");
modelBuilder.Entity<Animal>().Property(e => e.Species).HasMaxLength(100);
modelBuilder.Entity<Eagle>().HasMany(e => e.Prey).WithOne().HasForeignKey(e => e.EagleId).IsRequired(false);

Expand All @@ -41,7 +45,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con

// Keyless entities are mapped to TPH
modelBuilder.Entity<AnimalQuery>().HasNoKey().ToQuery(
() => context.Set<AnimalQuery>().FromSqlRaw("SELECT * FROM Animal"));
() => context.Set<AnimalQuery>().FromSqlRaw("SELECT * FROM Animals"));
modelBuilder.Entity<KiwiQuery>().HasDiscriminator().HasValue("Kiwi");
modelBuilder.Entity<EagleQuery>().HasDiscriminator().HasValue("Eagle");
}
Expand Down
9 changes: 7 additions & 2 deletions test/EFCore.Specification.Tests/Query/InheritanceQueryFixtureBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public abstract class InheritanceQueryFixtureBase : SharedStoreFixtureBase<Inher
protected override string StoreName { get; } = "InheritanceTest";
protected virtual bool EnableFilters => false;
protected virtual bool IsDiscriminatorMappingComplete => true;
protected virtual bool HasDiscriminator => true;

protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
{
Expand All @@ -27,9 +28,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
modelBuilder.Entity<Lilt>();
modelBuilder.Entity<Coke>();

modelBuilder.Entity<Bird>().HasDiscriminator<string>("Discriminator").IsComplete(IsDiscriminatorMappingComplete);
if (HasDiscriminator)
{
modelBuilder.Entity<Bird>().HasDiscriminator<string>("Discriminator").IsComplete(IsDiscriminatorMappingComplete);
modelBuilder.Entity<Drink>().HasDiscriminator().IsComplete(IsDiscriminatorMappingComplete);
}

modelBuilder.Entity<KiwiQuery>().HasDiscriminator().IsComplete(IsDiscriminatorMappingComplete);
modelBuilder.Entity<Drink>().HasDiscriminator().IsComplete(IsDiscriminatorMappingComplete);

if (EnableFilters)
{
Expand Down
12 changes: 8 additions & 4 deletions test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]");
}
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down