-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix to #32911 - Incorrect apply projection for complex properties
Problem was that when we lift structural type projection during pushdown, if that projection contains complex types with the same column names (e.g. cross join of same entities - it's perfectly fine to do in a vacuum, we just alias the columns whose names are repeated) we would lift the projection incorrectly.
What we do is go through all the properties, apply the corresponding columns to the selectExpression if needed and generate StructuralTypeProjection object if the projection needs to be applied one level up. For complex types we would generate a shaper expression and then run it through the same process, BUT the nested complex properties would be added to a flat structure along with the primitive properties, rather than in separate cache dedicated for complex property shapers.
This was wrong and not what we expected to see, when processing this structure one level up (i.e. when applying projection to the outer select)
SELECT [applying_this_projection_was_wrong]
FROM
(
SELECT c.Id, c.Name, c.ComplexProp, o.ComplexProp as ComplexProp0
FROM Customers as c
JOIN Orders as o ON ...
) as s
i.e. applying projection once worked fine, but doing it second time did not. The reason why is that we expected to see information about the complex type shape in the complex property shaper cache, rather than flat structure for primitives, but it wasn't there. So we assumed this is the first time we the projection is being applied, so we conjure up the complex type shaper based on table alias and IColumn metadata. This results in a situation, where complex property that was aliased is never picked. So we end up with:
SELECT s.Id, s.Name, s.ComplexProp -- we would also try to add s.ComplexProp again, instead of s.ComplexProp0 but of course we don't add same thing twice
FROM
(
SELECT c.Id, c.Name, c.ComplexProp, o.ComplexProp as ComplexProp0
FROM Customers as c
JOIN Orders as o ON ...
) as s
This leads to bad data - two different objects with distinct data in them are mapped to the same column in the database.
Fix is to property build a complex type shaper structure when applying projection instead, so the structure we generate matches expectations. Also modified VisitChildren and MakeNullable methods on StructuralTypeProjectionExpression to process/preserve complex type cache information, which was previously gobbled up/ignored.
Fixes #32911- Loading branch information
Showing
10 changed files
with
1,427 additions
and
59 deletions.
There are no files selected for viewing
237 changes: 180 additions & 57 deletions
237
src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 changes: 10 additions & 0 deletions
10
test/EFCore.InMemory.FunctionalTests/Query/AdHocAdvancedMappingsQueryInMemoryTest.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query; | ||
|
|
||
| public class AdHocAdvancedMappingsQueryInMemoryTest : AdHocAdvancedMappingsQueryTestBase | ||
| { | ||
| protected override ITestStoreFactory TestStoreFactory | ||
| => InMemoryTestStoreFactory.Instance; | ||
| } |
230 changes: 230 additions & 0 deletions
230
...Core.Relational.Specification.Tests/Query/AdHocAdvancedMappingsQueryRelationalTestBase.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,230 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query; | ||
|
|
||
| public abstract class AdHocAdvancedMappingsQueryRelationalTestBase : AdHocAdvancedMappingsQueryTestBase | ||
| { | ||
| protected TestSqlLoggerFactory TestSqlLoggerFactory | ||
| => (TestSqlLoggerFactory)ListLoggerFactory; | ||
|
|
||
| protected void ClearLog() | ||
| => TestSqlLoggerFactory.Clear(); | ||
|
|
||
| protected void AssertSql(params string[] expected) | ||
| => TestSqlLoggerFactory.AssertBaseline(expected); | ||
|
|
||
| #region 32911 | ||
|
|
||
| [ConditionalFact] | ||
| public virtual async Task Two_similar_complex_properties_projected_with_split_query1() | ||
| { | ||
| var contextFactory = await InitializeAsync<Context32911>(seed: c => c.Seed()); | ||
|
|
||
| using var context = contextFactory.CreateContext(); | ||
| var query = context.Offers | ||
| .Include(e => e.Variations) | ||
| .ThenInclude(v => v.Nested) | ||
| .AsSplitQuery() | ||
| .ToList(); | ||
|
|
||
| var resultElement = query.Single(); | ||
| foreach (var variation in resultElement.Variations) | ||
| { | ||
| Assert.NotEqual(variation.Payment.Brutto, variation.Nested.Payment.Brutto); | ||
| Assert.NotEqual(variation.Payment.Netto, variation.Nested.Payment.Netto); | ||
| } | ||
| } | ||
|
|
||
| [ConditionalFact] | ||
| public virtual async Task Two_similar_complex_properties_projected_with_split_query2() | ||
| { | ||
| var contextFactory = await InitializeAsync<Context32911>(seed: c => c.Seed()); | ||
|
|
||
| using var context = contextFactory.CreateContext(); | ||
| var query = context.Offers | ||
| .Include(e => e.Variations) | ||
| .ThenInclude(v => v.Nested) | ||
| .AsSplitQuery() | ||
| .Single(x => x.Id == 1); | ||
|
|
||
| foreach (var variation in query.Variations) | ||
| { | ||
| Assert.NotEqual(variation.Payment.Brutto, variation.Nested.Payment.Brutto); | ||
| Assert.NotEqual(variation.Payment.Netto, variation.Nested.Payment.Netto); | ||
| } | ||
| } | ||
|
|
||
| [ConditionalFact] | ||
| public virtual async Task Projecting_one_of_two_similar_complex_types_picks_the_correct_one() | ||
| { | ||
| var contextFactory = await InitializeAsync<Context32911_2>(seed: c => c.Seed()); | ||
|
|
||
| using var context = contextFactory.CreateContext(); | ||
|
|
||
| var query = context.Cs | ||
| .Where(x => x.B.AId.Value == 1) | ||
| .OrderBy(x => x.Id) | ||
| .Take(10) | ||
| .Select(x => new | ||
| { | ||
| x.B.A.Id, | ||
| x.B.Info.Created, | ||
| }).ToList(); | ||
|
|
||
| Assert.Equal(new DateTime(2000, 1, 1), query[0].Created); | ||
| } | ||
|
|
||
| protected class Context32911(DbContextOptions options) : DbContext(options) | ||
| { | ||
| public DbSet<Offer> Offers { get; set; } | ||
|
|
||
| protected override void OnModelCreating(ModelBuilder modelBuilder) | ||
| { | ||
| modelBuilder.Entity<Offer>().Property(x => x.Id).ValueGeneratedNever(); | ||
| modelBuilder.Entity<Variation>().Property(x => x.Id).ValueGeneratedNever(); | ||
| modelBuilder.Entity<Variation>().ComplexProperty(x => x.Payment, cpb => | ||
| { | ||
| cpb.IsRequired(); | ||
| cpb.Property(p => p.Netto).HasColumnName("payment_netto"); | ||
| cpb.Property(p => p.Brutto).HasColumnName("payment_brutto"); | ||
| }); | ||
| modelBuilder.Entity<NestedEntity>().Property(x => x.Id).ValueGeneratedNever(); | ||
| modelBuilder.Entity<NestedEntity>().ComplexProperty(x => x.Payment, cpb => | ||
| { | ||
| cpb.IsRequired(); | ||
| cpb.Property(p => p.Netto).HasColumnName("payment_netto"); | ||
| cpb.Property(p => p.Brutto).HasColumnName("payment_brutto"); | ||
| }); | ||
| } | ||
|
|
||
| public void Seed() | ||
| { | ||
| var v1 = new Variation | ||
| { | ||
| Id = 1, | ||
| Payment = new Payment(1, 10), | ||
| Nested = new NestedEntity | ||
| { | ||
| Id = 1, | ||
| Payment = new Payment(10, 100) | ||
| } | ||
| }; | ||
|
|
||
| var v2 = new Variation | ||
| { | ||
| Id = 2, | ||
| Payment = new Payment(2, 20), | ||
| Nested = new NestedEntity | ||
| { | ||
| Id = 2, | ||
| Payment = new Payment(20, 200) | ||
| } | ||
| }; | ||
|
|
||
| var v3 = new Variation | ||
| { | ||
| Id = 3, | ||
| Payment = new Payment(3, 30), | ||
| Nested = new NestedEntity | ||
| { | ||
| Id = 3, | ||
| Payment = new Payment(30, 300) | ||
| } | ||
| }; | ||
|
|
||
| Offers.Add(new Offer { Id = 1, Variations = new List<Variation> { v1, v2, v3 } }); | ||
|
|
||
| SaveChanges(); | ||
| } | ||
|
|
||
| public abstract class EntityBase | ||
| { | ||
| public int Id { get; set; } | ||
| } | ||
|
|
||
| public class Offer : EntityBase | ||
| { | ||
| public ICollection<Variation> Variations { get; set; } | ||
| } | ||
|
|
||
| public class Variation : EntityBase | ||
| { | ||
| public Payment Payment { get; set; } = new Payment(0, 0); | ||
|
|
||
| public NestedEntity Nested { get; set; } | ||
| } | ||
|
|
||
| public class NestedEntity : EntityBase | ||
| { | ||
| public Payment Payment { get; set; } = new Payment(0, 0); | ||
| } | ||
|
|
||
| public record Payment(decimal Netto, decimal Brutto); | ||
| } | ||
|
|
||
| protected class Context32911_2(DbContextOptions options) : DbContext(options) | ||
| { | ||
| public DbSet<A> As { get; set; } | ||
| public DbSet<B> Bs { get; set; } | ||
| public DbSet<C> Cs { get; set; } | ||
|
|
||
| protected override void OnModelCreating(ModelBuilder modelBuilder) | ||
| { | ||
| modelBuilder.Entity<A>().Property(x => x.Id).ValueGeneratedNever(); | ||
| modelBuilder.Entity<B>().Property(x => x.Id).ValueGeneratedNever(); | ||
| modelBuilder.Entity<C>().Property(x => x.Id).ValueGeneratedNever(); | ||
|
|
||
| modelBuilder.Entity<B>(x => x.ComplexProperty(b => b.Info).IsRequired()); | ||
| modelBuilder.Entity<C>(x => x.ComplexProperty(c => c.Info).IsRequired()); | ||
| } | ||
|
|
||
| public void Seed() | ||
| { | ||
| var c = new C | ||
| { | ||
| Id = 100, | ||
| Info = new Metadata { Created = new DateTime(2020, 10, 10) }, | ||
| B = new B | ||
| { | ||
| Id = 10, | ||
| Info = new Metadata { Created = new DateTime(2000, 1, 1) }, | ||
| A = new A { Id = 1 } | ||
| } | ||
| }; | ||
|
|
||
| Cs.Add(c); | ||
| SaveChanges(); | ||
| } | ||
|
|
||
| public class Metadata | ||
| { | ||
| public DateTime Created { get; set; } | ||
| } | ||
|
|
||
| public class A | ||
| { | ||
| public int Id { get; set; } | ||
| } | ||
|
|
||
| public class B | ||
| { | ||
| public int Id { get; set; } | ||
| public Metadata Info { get; set; } | ||
| public int? AId { get; set; } | ||
|
|
||
| public A A { get; set; } | ||
| } | ||
|
|
||
| public class C | ||
| { | ||
| public int Id { get; set; } | ||
| public Metadata Info { get; set; } | ||
| public int BId { get; set; } | ||
|
|
||
| public B B { get; set; } | ||
| } | ||
| } | ||
|
|
||
| #endregion | ||
| } |
Oops, something went wrong.