Skip to content

Commit

Permalink
Query: Correctly compute ordering to be copied from outer in collecti…
Browse files Browse the repository at this point in the history
…on join (#23273)

Resolves #23211
Add test for #23276
  • Loading branch information
smitpatel authored Nov 17, 2020
1 parent 34239e7 commit 46cfc53
Show file tree
Hide file tree
Showing 2 changed files with 179 additions and 12 deletions.
40 changes: 28 additions & 12 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1387,18 +1387,34 @@ public Expression ApplyCollectionJoin(

var identifierFromParent = _identifier;
if (innerSelectExpression.Tables.LastOrDefault(e => e is InnerJoinExpression) is InnerJoinExpression
collectionInnerJoinExpression
&& collectionInnerJoinExpression.Table is SelectExpression collectionInnerSelectExpression)
{
// This computes true parent identifier count for correlation.
// The last inner joined table in innerSelectExpression brings collection data.
// Further tables load additional data on the collection (hence uses outer pattern)
// So identifier not coming from there (which would be at the start only) are for correlation with parent.
// Parent can have additional identifier if a owned reference was expanded.
var actualParentIdentifierCount = innerSelectExpression._identifier
.TakeWhile(e => !ReferenceEquals(e.Column.Table, collectionInnerSelectExpression))
.Count();
identifierFromParent = _identifier.Take(actualParentIdentifierCount).ToList();
collectionInnerJoinExpression)
{
TableExpressionBase tableToMatch = null;
var useOldBehavior = AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23111", out var isEnabled)
&& isEnabled;
if (useOldBehavior
&& collectionInnerJoinExpression.Table is SelectExpression collectionInnerSelectExpression)
{
tableToMatch = collectionInnerSelectExpression;
}
else if (!useOldBehavior
&& collectionInnerJoinExpression.Table is TableExpressionBase collectionTableExpressionBase)
{
tableToMatch = collectionTableExpressionBase;
}

if (tableToMatch != null)
{
// This computes true parent identifier count for correlation.
// The last inner joined table in innerSelectExpression brings collection data.
// Further tables load additional data on the collection (hence uses outer pattern)
// So identifier not coming from there (which would be at the start only) are for correlation with parent.
// Parent can have additional identifier if a owned reference was expanded.
var actualParentIdentifierCount = innerSelectExpression._identifier
.TakeWhile(e => !ReferenceEquals(e.Column.Table, tableToMatch))
.Count();
identifierFromParent = _identifier.Take(actualParentIdentifierCount).ToList();
}
}

var parentIdentifier = GetIdentifierAccessor(identifierFromParent).Item1;
Expand Down
151 changes: 151 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9139,6 +9139,157 @@ private SqlServerTestStore CreateDatabase22568()

#endregion

#region Issue23211

[ConditionalFact]
public virtual void Collection_include_on_owner_with_two_owned_types_mapped_to_different_table()
{
using (CreateDatabase23211())
{
using var context = new MyContext23211(_options);

var owner = context.Set<Owner23211>().Include(e => e.Dependents).AsSplitQuery().OrderBy(e => e.Id).Single();
Assert.NotNull(owner.Dependents);
Assert.Equal(2, owner.Dependents.Count);
Assert.NotNull(owner.Owned1);
Assert.Equal("A", owner.Owned1.Value);
Assert.NotNull(owner.Owned2);
Assert.Equal("B", owner.Owned2.Value);

AssertSql(
@"SELECT [t].[Id], [t].[Owner23211Id], [t].[Value], [t].[Owner23211Id0], [t].[Value0]
FROM (
SELECT TOP(2) [o].[Id], [o0].[Owner23211Id], [o0].[Value], [o1].[Owner23211Id] AS [Owner23211Id0], [o1].[Value] AS [Value0]
FROM [Owner23211] AS [o]
LEFT JOIN [Owned123211] AS [o0] ON [o].[Id] = [o0].[Owner23211Id]
LEFT JOIN [Owned223211] AS [o1] ON [o].[Id] = [o1].[Owner23211Id]
ORDER BY [o].[Id]
) AS [t]
ORDER BY [t].[Id]",
//
@"SELECT [d].[Id], [d].[Owner23211Id], [t].[Id]
FROM (
SELECT TOP(1) [o].[Id]
FROM [Owner23211] AS [o]
ORDER BY [o].[Id]
) AS [t]
INNER JOIN [Dependent23211] AS [d] ON [t].[Id] = [d].[Owner23211Id]
ORDER BY [t].[Id]");
}
}

[ConditionalFact]
public virtual void Collection_include_on_owner_with_owned_type_mapped_to_different_table()
{
using (CreateDatabase23211())
{
using var context = new MyContext23211(_options);

var owner = context.Set<SecondOwner23211>().Include(e => e.Dependents).AsSplitQuery().OrderBy(e => e.Id).Single();
Assert.NotNull(owner.Dependents);
Assert.Equal(2, owner.Dependents.Count);
Assert.NotNull(owner.Owned);
Assert.Equal("A", owner.Owned.Value);

AssertSql(
@"SELECT [t].[Id], [t].[SecondOwner23211Id], [t].[Value]
FROM (
SELECT TOP(2) [s].[Id], [o].[SecondOwner23211Id], [o].[Value]
FROM [SecondOwner23211] AS [s]
LEFT JOIN [Owned23211] AS [o] ON [s].[Id] = [o].[SecondOwner23211Id]
ORDER BY [s].[Id]
) AS [t]
ORDER BY [t].[Id]",
//
@"SELECT [s0].[Id], [s0].[SecondOwner23211Id], [t].[Id]
FROM (
SELECT TOP(1) [s].[Id]
FROM [SecondOwner23211] AS [s]
ORDER BY [s].[Id]
) AS [t]
INNER JOIN [SecondDependent23211] AS [s0] ON [t].[Id] = [s0].[SecondOwner23211Id]
ORDER BY [t].[Id]");
}
}

private class Owner23211
{
public int Id { get; set; }
public List<Dependent23211> Dependents { get; set; }
public OwnedType23211 Owned1 { get; set; }
public OwnedType23211 Owned2 { get; set; }
}

private class OwnedType23211
{
public string Value { get; set; }
}

private class Dependent23211
{
public int Id { get; set; }
}

private class SecondOwner23211
{
public int Id { get; set; }
public List<SecondDependent23211> Dependents { get; set; }
public OwnedType23211 Owned { get; set; }
}

private class SecondDependent23211
{
public int Id { get; set; }
}

private class MyContext23211 : DbContext
{
public MyContext23211(DbContextOptions options)
: base(options)
{
}

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Owner23211>().OwnsOne(e => e.Owned1, b => b.ToTable("Owned123211"));
modelBuilder.Entity<Owner23211>().OwnsOne(e => e.Owned2, b => b.ToTable("Owned223211"));
modelBuilder.Entity<SecondOwner23211>().OwnsOne(e => e.Owned, b => b.ToTable("Owned23211"));
}
}

private SqlServerTestStore CreateDatabase23211()
=> CreateTestStore(
() => new MyContext23211(_options),
context =>
{
context.Add(new Owner23211
{
Dependents = new List<Dependent23211>
{
new Dependent23211(),
new Dependent23211()
},
Owned1 = new OwnedType23211 { Value = "A" },
Owned2 = new OwnedType23211 { Value = "B" }
});
context.Add(new SecondOwner23211
{
Dependents = new List<SecondDependent23211>
{
new SecondDependent23211(),
new SecondDependent23211()
},
Owned = new OwnedType23211 { Value = "A" }
});
context.SaveChanges();
ClearLog();
});

#endregion

private DbContextOptions _options;

private SqlServerTestStore CreateTestStore<TContext>(
Expand Down

0 comments on commit 46cfc53

Please sign in to comment.