Efcore: Projecting entity type removes auto-includes added for ownership

[ajcvickers]

Originally filed as https://github.com/aspnet/EntityFrameworkCore/issues/9210#issuecomment-427736113 as comment by @Oblomoff

The bug strikes back in Microsoft.EntityFrameworkCore.SqlServer 2.1.4 (maybe in earlier versions too)! See the attached solution: EfBug.zip

```C#
using System;
using System.Linq;

using Microsoft.EntityFrameworkCore;

namespace EfBug
{
internal class Bar
{
public int Id { get; set; }

    public Owned Owned { get; set; }

    public string Simple { get; set; }
}

internal class Foo
{
    public Bar Bar { get; set; }

    public int Id { get; set; }
}

internal class MyContext : DbContext
{        
    public MyContext() : base(new DbContextOptionsBuilder()
         .UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=MyContext;Trusted_Connection=True;MultipleActiveResultSets=true").Options)
    {
    }

    public DbSet<Bar> Bars { get; private set; }

    public DbSet<Foo> Foos { get; private set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Bar>().OwnsOne(e => e.Owned);
    }
}

internal class Owned
{
    public string Property { get; set; }
}

internal class Program
{
    private static void Main(string[] args)
    {
        using (var context = new MyContext())
        {
            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();
            context.Add(new Foo
            {
                Bar = new Bar
                {
                    Simple = "Simple property is OK!",
                    Owned = new Owned
                    {
                        Property = "Owned property is OK!"
                    }
                }
            });

            context.SaveChanges();
        }


        // This works fine
        using (var context = new MyContext())
        {
            var bar = context.Bars.Single();

            Dump(bar);
        }

        // Here is the bug
        using (var context = new MyContext())
        {
            var bar = context.Foos.Select(e => e.Bar).Single();

            Dump(bar);
        }

        // ThenInclude workarond does not work!
        using (var context = new MyContext())
        {
            var bar = context.Foos
                .Include(f => f.Bar)
                .ThenInclude(b => b.Owned)
                .ThenInclude(o => o.Property).Select(e => e.Bar).Single();

            Dump(bar);
        }

        Console.ReadKey();

        void Dump(Bar bar)
        {
            Console.WriteLine("Simple property: {0}", bar.Simple);
            Console.WriteLine("Owned property : {0}", bar.Owned?.Property); // <= The bug is here
        }
    }
}

}
```

[ajcvickers]

@ajcvickers commented:

Note for triage: this is a different issue than the original issue reported since in the new variation the query root is for Foo which then makes a projection to Bar. Based on how Include currently works, this is by-design. Based on owned types as aggregates the behavior is wrong. I would rather this be tracked by a new issue, rather than re-opening this one, but let's discuss in triage.

[breyed]

This bug occurs even if inclusion is inferred. I encountered this in production (bad regression - results show up as null out of the blue!) and haven't tried a simplified repro, but I expect it would look like this:

c# string prop = (from f in context.Foos select Foo.Bar.Owned.Property).Single();

[smitpatel]

@breyed - You are not projecting out any EntityType in your results, so the issue you are seeing is not related to this issue. Please file a new issue if you think there is a bug in EF Core.

[breyed]

You're right. It's a different issue. The problem I ran into occurs when there are multiple paths to an entity and OnModelCreating doesn't specify one of the paths. Unfortunately, it's very subtle. I can get a unit test on the full production build to pass/fail based on keeping/removing the line definition in OnModelCreating, but I couldn't narrow the problem down in a repro. It may be due to using SSDT to create the database in production vs EF to create the database in the repro.

[breyed]

I was able to reproduce the issue in #14329. My initial difficulty with reproing appears to have been due to EF Core caching of the DbContext configuration.

[thj-dk]

I'm seing this issue as well. When projecting entities, the owned types are excluded in the result.

[IT-CASADO]

@breyed @thj-dk
Do you see any relation with my reported issue #14302? It was marked as a duplicate, but I don't see this...

[jonathanantoine]

Any workaround existing for this bug :) ?

[dstj]

@jonathanantoine, if that can help, the way I got around it was to do a full-fledged Join()

_i.e._, instead of:

```c#
var bar = context.Foos
.Include(f => f.Bar)
.Select(e => e.Bar)
.Single();

do

```c#
context.Foos
   .Join(
      inner: context.Bar,
      innerKeySelector: b => b.Id,
      outerKeySelector: f => f.BarId,
      resultSelector: ((f, b) => b)
   )
   .Single();

There may be another, simpler way, but that's the one I found.

[jonathanantoine]

Thanks @dstj that will do the job for now :)

[space-alien]

I feel really sad about the state of Owned Types in 2(.2.2) and seriously regret trying to use them in my project. I think I have actually accumulated days of lost development time trying to understand the issues caused by some of the unresolved bugs. Figuring out #12865 was a thoroughly unpleasant experience. #9005 requires horrible workarounds... and many others. This particular issue is the final, closing chapter of my personal Owned Types odyssey!

[alexschrod]

Note that in #15150 I mention that I discovered that this is a problem that is seemingly unrelated to the provider used, whereas this issue explicitly mentions Microsoft.EntityFrameworkCore.SqlServer being the culprit.

[ajcvickers]

@alexschrod I don't think there is anything here that means we think this is only an issue on SQL Server. It's just that SQL Server was the provider being used by the person who reported the issue.

[MikaelEliasson]

I got across this problem today. The simplest way to fix it I found to be to do something like this.

using Microsoft.VisualStudio.TestTools.UnitTesting;
using Shouldly;
using System.Linq;

namespace EFCorePlayground.Tests
{
    [TestClass]
    public class OwnedEntityReadTests
    {
        //Fails
        [TestMethod]
        public void Project_DoesIncludeNested_NoHack()
        {
            var db = new Context();
            db.Database.EnsureDeleted();
            db.Database.EnsureCreated();

            db.Person.Add(new Person { Name = "A", Approved = new AuditInfo(), Requested = new AuditInfo(), WorkDetails = new WorkDetails { Address = new Address { City = "Dummy" } } });
            db.SaveChanges();

            db = new Context();

            var projected = db.Person.Select(x => new ViewModel
                {
                    Name = x.Name,
                    WorkDetails = x.WorkDetails,
                }).First();

            projected.WorkDetails.Address.City.ShouldBe("Dummy");
        }

        //Passes
        [TestMethod]
        public void Project_DoesIncludeNested()
        {
            var db = new Context();
            db.Database.EnsureDeleted();
            db.Database.EnsureCreated();

            db.Person.Add(new Person { Name = "A", Approved = new AuditInfo(), Requested = new AuditInfo(), WorkDetails = new WorkDetails { Address = new Address { City = "Dummy" } } });
            db.SaveChanges();

            db = new Context();

            var projected = db.Person.Select(x => new
            {
                vm = new ViewModel
                {
                    Name = x.Name,
                    WorkDetails = x.WorkDetails,
                },
                x.WorkDetails.Address
            }).First();

            projected.vm.WorkDetails.Address.City.ShouldBe("Dummy");
        }

        public class ViewModel
        {
            public string Name { get; set; }
            public WorkDetails WorkDetails { get; set; }
        }
    }
}

Looking in SQL profiler this doesn't seem to pull down any extra data. It's just annoying to have to write that code.

SELECT TOP(1) [x].[Id], [x].[WorkDetails_WorkName], [x].[Id], [x].[WorkDetails_Address_City], [x].[WorkDetails_Address_PostalCode], [x].[WorkDetails_Address_Street1], [x].[Name]
FROM [Person] AS [x]

[MikaelEliasson]

I think this is a very important issue to get fixed as soon as possible.

While there are workarounds, for example the one above they have very negative effect on code quality. For example this is one method we now have in our code base.

c# private static CompanyViewModel GetCompanyViewModel(Guid org, Context db) { return db.Companies .Select(c => new { vm = new CompanyViewModel { Id = c.Id, SenderData = c.SenderData, Image = c.Image, CompanyColor = c.CompanyColor, AddressPosition = c.AddressPosition, Payday = c.Payday }, //EF core hack https://github.com/aspnet/EntityFrameworkCore/issues/13546?_pjax=%23js-repo-pjax-container#issuecomment-497477456 AddressHack = c.SenderData.Address, PMHack = c.SenderData.PaymentMethod, FPMHack = c.SenderData.ForeignPaymentMethod } ) .First(c => c.vm.Id == org).vm; }

While it's doable the problem is that this bug affects the code paths for retrieving data which are often much more common the saving data in a code base so there will be way more places where we have to apply workarounds than for example the problem with updating nested owned entities like in #14713.

It's also incredibly unexpected behaviour and hard to explain users, and even if you know about it it's easy to forget because it's not easy to see at a single glance which nested owned types an entity have. The workaround above also have the problem that if someone update SenderData with another nested owned entity. Every single place where we query for this type will have to be updated with this hack.

I think this deserves to be fixed in a patch instead of waiting for 3.0. Compared to many other issues I would say that this is a fairly standard case that should just work and the workaround will have to be applied on every projection application wide. The case against a patch would be that there are working hacks to get this to work.

[ajcvickers]

Removing milestone to consider patch request. However, my initial thoughts are that this is too risky to fix in a patch.

[ajcvickers]

@MikaelEliasson We discussed this again, but unfortunately the risk of regression in other queries to too high for us to fix this in a patch release.

[MikaelEliasson]

@ajcvickers I understand. Do you know where the bug is? I would like to explore if I can automate the hack above but would like to understand the actual reason it happens a bit better first.

[ajcvickers]

@maumar Could you find some pointers as to where @MikaelEliasson should start looking?

[ajcvickers]

@maumar Did you get a chance to look at this?

[maumar]

@MikaelEliasson sorry for the late reply. Here is some background:

The problem is that include pipeline relies on QuerySourceReferenceExpression to figure out what/where needs to be included. This leads to the issue that include only works when the QuerySourceReferenceExpression in directly accessible in the final result (top level projection).
In the IncludeLoadTree QuerySourceReferenceExpression would have to be changed/removed and the EntityType information would have to be stored instead. Then there is matter of finding the entity type in the final projection. Currently we just find QuerySourceReferenceExpressions using QuerySourceTracingExpressionVisitor.

The problem is that when starting from final projection we would need to guess the entity type based on clr type. That's not safe because multiple entity types can manifest as the same clr type. This I would say is the ultimate cause of the entire problem.
In new pipeline we start from the root DbSet (and we always know what those initial EntityTypes are) and track the entity type of an expression as it's being processed up to the final projection so we never need to guess anything.

Finally, once we have the correct include call created (it happens in IncludeCompiler.Compile) we need to inject them into the original expression in the right place. Currently we just do a simple replace of the QuerySourceReferenceExpression, but that would have to change to match based on EntityType instead, and also have a way to track which entity types had their includes added to avoid duplicates. Currently the injection of include expressions to the tree happens in IncludeReplacingExpressionVisitor.