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.

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

[Regression] ThenInclude back to parent entity isn't working #23674

Closed
Neme12 opened this issue Dec 13, 2020 · 8 comments
Closed

[Regression] ThenInclude back to parent entity isn't working #23674

Neme12 opened this issue Dec 13, 2020 · 8 comments

Comments

@Neme12
Copy link

@Neme12 Neme12 commented Dec 13, 2020

Steps to reproduce

  1. Start with this project file and a C# file:
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="5.0.1" />
  </ItemGroup>

</Project>
using Microsoft.EntityFrameworkCore;
using System;
using System.Collections.Generic;

namespace EfCore5Test
{
    public sealed class Lesson
    {
        public Guid Id { get; set; }

        public IList<Question> Questions { get; set; }
    }

    public sealed class Question
    {
        public Guid Id { get; set; }

        public Guid LessonId { get; set; }
        public Lesson Lesson { get; set; }

        public IList<QuestionLocalization> Localizations { get; set; }

        public IList<Answer> Answers { get; set; }
    }

    public sealed class QuestionLocalization
    {
        public Guid QuestionId { get; set; }
        public Question Question { get; set; }

        public string LanguageCode { get; set; }
    }

    public sealed class Answer
    {
        public Guid Id { get; set; }

        public Guid QuestionId { get; set; }
        public Question Question { get; set; }
    }

    public sealed class ApplicationDbContext : DbContext
    {
        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            base.OnConfiguring(optionsBuilder);
            optionsBuilder.UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=aspnet-WebApplication-6E483A89-BEE6-4A76-96CC-CEB276E5E112;Trusted_Connection=True;MultipleActiveResultSets=true");
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            modelBuilder.Entity<Question>(builder =>
            {
                builder.HasOne(x => x.Lesson).WithMany(x => x.Questions).HasForeignKey(x => x.LessonId).OnDelete(DeleteBehavior.Cascade);
            });

            modelBuilder.Entity<QuestionLocalization>(builder =>
            {
                builder.HasKey(x => new { x.QuestionId, x.LanguageCode });

                builder.HasOne(x => x.Question).WithMany(x => x.Localizations).HasForeignKey(x => x.QuestionId).OnDelete(DeleteBehavior.Cascade);
            });

            modelBuilder.Entity<Answer>(builder =>
            {
                builder.HasOne(x => x.Question).WithMany(x => x.Answers).HasForeignKey(x => x.QuestionId).OnDelete(DeleteBehavior.Cascade);
            });
        }

        public DbSet<Lesson> Lessons { get; set; }
        public DbSet<Question> Questions { get; set; }
        public DbSet<QuestionLocalization> QuestionLocalizations { get; set; }
        public DbSet<Answer> Answers { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {
            using (var context = new ApplicationDbContext())
            {
                context.Database.EnsureDeleted();
                context.Database.EnsureCreated();

                var query = context.Lessons
                    .Include(x => x.Questions)
                    .ThenInclude(x => x.Localizations)
                    .ThenInclude(x => x.Question.Answers);

                Console.WriteLine(query.ToQueryString());
            }
        }
    }
}
  1. As you can see in the output, the generated query looks like this:
SELECT [l].[Id], [t].[Id], [t].[LessonId], [t].[QuestionId], [t].[LanguageCode]
FROM [Lessons] AS [l]
LEFT JOIN (
    SELECT [q].[Id], [q].[LessonId], [q0].[QuestionId], [q0].[LanguageCode]
    FROM [Questions] AS [q]
    LEFT JOIN [QuestionLocalizations] AS [q0] ON [q].[Id] = [q0].[QuestionId]
) AS [t] ON [l].[Id] = [t].[LessonId]
ORDER BY [l].[Id], [t].[Id], [t].[QuestionId], [t].[LanguageCode]

Notice that there is no join with the Answers table, so when the query is executed, Question.Answers is not included in the result (you can see that if you put some data into the tables first and then execute the query) with no warning or error generated.

In EF Core 3.1, this worked correctly and Answers was indeed included. I agree that the 3.1 generated query looks a bit complicated and is probably suboptimal and that this way to include "backwards" to the parent and then to another entity seems odd to me but I found it in our code base and noticed it stopped working (I'm guessing the author did this to avoid repeating .Include(x => x.Questions) - in the real code, there's actually one more Include before that).

I know that the proper way would have been to do this all along:

var query = context.Lessons
    .Include(x => x.Questions).ThenInclude(x => x.Localizations)
    .Include(x => x.Questions).ThenInclude(x => x.Answers);

which still works in EF Core 5.0 and it generates better SQL, so I'm going to rewrite the code to this. But the original piece of code just silently stopped working correctly. If it was intentional to make this a breaking change, it should have been documented and also it should throw an error or a warning instead of just running an incomplete query and returning incomplete data 😕

Include provider and version information

EF Core version: 5.0.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 5.0
Operating system: Windows 10 Version 1909
IDE: Visual Studio 16.9.0 Preview 2.0

@Neme12
Copy link
Author

@Neme12 Neme12 commented Dec 13, 2020

Simplified repro:

using Microsoft.EntityFrameworkCore;
using System;
using System.Collections.Generic;

namespace EfCore5Test
{
    public sealed class Question
    {
        public Guid Id { get; set; }

        public IList<QuestionLocalization> Localizations { get; set; }

        public IList<Answer> Answers { get; set; }
    }

    public sealed class QuestionLocalization
    {
        public Guid Id { get; set; }

        public Guid QuestionId { get; set; }
        public Question Question { get; set; }
    }

    public sealed class Answer
    {
        public Guid Id { get; set; }

        public Guid QuestionId { get; set; }
        public Question Question { get; set; }
    }

    public sealed class ApplicationDbContext : DbContext
    {
        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            base.OnConfiguring(optionsBuilder);
            optionsBuilder.UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=aspnet-WebApplication-6E483A89-BEE6-4A76-96CC-CEB276E5E112;Trusted_Connection=True;MultipleActiveResultSets=true");
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            modelBuilder.Entity<QuestionLocalization>(builder =>
            {
                builder.HasOne(x => x.Question).WithMany(x => x.Localizations).HasForeignKey(x => x.QuestionId).OnDelete(DeleteBehavior.Cascade);
            });

            modelBuilder.Entity<Answer>(builder =>
            {
                builder.HasOne(x => x.Question).WithMany(x => x.Answers).HasForeignKey(x => x.QuestionId).OnDelete(DeleteBehavior.Cascade);
            });
        }

        public DbSet<Question> Questions { get; set; }
        public DbSet<QuestionLocalization> QuestionLocalizations { get; set; }
        public DbSet<Answer> Answers { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {
            using (var context = new ApplicationDbContext())
            {
                context.Database.EnsureDeleted();
                context.Database.EnsureCreated();

                var query = context.Questions
                    .Include(x => x.Localizations)
                    .ThenInclude(x => x.Question.Answers);

                Console.WriteLine(query.ToQueryString());
            }
        }
    }
}

@maumar
Copy link
Contributor

@maumar maumar commented Dec 14, 2020

probable cause: 6ad2c8a (fix for #22568)

maumar added a commit that referenced this issue Dec 15, 2020
maumar added a commit that referenced this issue Dec 15, 2020
maumar added a commit that referenced this issue Dec 15, 2020
@ajcvickers ajcvickers added this to the 5.0.3 milestone Dec 15, 2020
@smitpatel
Copy link
Member

@smitpatel smitpatel commented Jan 5, 2021

There are 2 sub-issues involved in this

  • As decision made in #22568 (comment), the key point is we started skipping loading inverse navigation, if fixup will do it anyway. We did that so that pair of navigations in a relationship, both of which are auto-included still works (since it is not infinite space graph). That decision changed behavior for includes which are manual too. While it did block this scenario, I believe it is right to block the scenario. The implementation of Include (or any navigation in query) involves following the chain of navigations used. Above way of writing include will causes Question table to be loaded again and materialized twice (tracking would find the same instance and non-tracking will materialize twice and replace the instance, potentially incorrect graph). So even if we make this work, it will do a lot more work in terms of database query and materialization.
  • If user tries to write manual include which involves a cycle which would be removed due to first point (skip inverse nav if fixup will do it already), then we should throw an error rather than silently ignoring and not giving included data.

@smitpatel smitpatel removed this from the 5.0.3 milestone Jan 5, 2021
@smitpatel
Copy link
Member

@smitpatel smitpatel commented Jan 6, 2021

Design meeting decision

  • We are going to block this scenario - walking back up an include tree to include more navigations
  • In 5.0 patch, we will add a warning with error message adding that it would become an error in future versions.
  • In 6.0 we will bump the warning to error by default.

@ajcvickers ajcvickers added this to the 5.0.3 milestone Jan 8, 2021
@ajcvickers ajcvickers removed this from the 5.0.3 milestone Jan 8, 2021
@AndriySvyryd AndriySvyryd added this to the 5.0.3 milestone Jan 8, 2021
@ajcvickers ajcvickers removed this from the 5.0.3 milestone Jan 17, 2021
@ajcvickers ajcvickers added this to the 5.0.4 milestone Jan 17, 2021
@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Jan 25, 2021

@smitpatel Don't forget about this one. :-)

@smitpatel
Copy link
Member

@smitpatel smitpatel commented Jan 25, 2021

First thing on my list to fix once I get my home office setup back.

@SimonCropp
Copy link
Contributor

@SimonCropp SimonCropp commented Mar 9, 2021

given 5.0.4 is released , should this be closed?

@smitpatel smitpatel removed this from the 5.0.4 milestone Mar 9, 2021
@smitpatel smitpatel added this to the 5.0.5 milestone Mar 9, 2021
@smitpatel
Copy link
Member

@smitpatel smitpatel commented Mar 24, 2021

This has been merged to respective branches according to resolution above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants