Many-to-many lazy (and explicit) loading erroneously sets inverse collection as loaded

[smooij-hhs]

Using these model classes

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

namespace EFGetStarted
{
    public class BloggingContext : DbContext
    {
        public DbSet<Student> Students { get; set; }
        public DbSet<Course> Courses { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder options)
            => options.UseLazyLoadingProxies().UseSqlite("Data Source=students.db");
    }

    public class Student
    {
        public int Id { get; set; }
        public string Name { get; set; }

        public virtual ICollection<Course> Courses { get; set; }
    }

    public class Course
    {
        public int CourseId { get; set; }
        public string CourseName { get; set; }
        public virtual ICollection<Student> Students { get; set; }
    }
}

and this program

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

namespace EFGetStarted
{
    class Program
    {
        static void Main()
        {
            Write();
            Read();
        }

        static void Write()
        {
            using (var db = new BloggingContext())
            {
                Student s1 = new Student { Id = 1, Name = "Steven", Courses = new List<Course>() };
                Student s2 = new Student { Id = 2, Name = "Pascal", Courses = new List<Course>() };
                Course c1 = new Course { CourseId = 1, CourseName = "Wiskunde" };
                Course c2 = new Course { CourseId = 2, CourseName = "Geschiedenis" };
                s1.Courses.Add(c1);
                s1.Courses.Add(c2);
                s2.Courses.Add(c1);
                s2.Courses.Add(c2);
                db.Add(c1);
                db.Add(c2);
                db.Add(s1);
                db.Add(s2);
                db.SaveChanges();

            }
        }

        static void Read()
        {
            using (var db = new BloggingContext())
            {
                Student s1 = db.Students.Where(s => s.Id == 1).Single();
                // db.Entry(s1)
                // .Collection(s1 => s1.Courses)
                // .Load();
                // foreach (Course item in s1.Courses)
                // {
                //     Console.WriteLine(item.CourseName);
                // }
                Course c1 = db.Courses.Where(s => s.CourseId == 1).Single();
                // db.Entry(c1)
                // .Collection(c1 => c1.Students)
                // .Load();
                foreach (Student item in c1.Students)
                {
                    Console.WriteLine(item.Name);
                }
            }
        }
    }
}

the output is

Steven
Pascal

as it should be. If the 'foreach' loop in 'Read' is uncommented (and the 'Write' is commented out), the output is

Wiskunde
Geschiedenis
Steven

This is unexpected, the output should include 'Pascal' as well. Using expicit loading results in similar unexpected behaviour.

Version:
Entity Framework Core .NET Command-line Tools
5.0.0

[ajcvickers]

Note for triage: I am able to reproduce this; the issue is that the first Load also flags the inverse navigation as loaded. This isn't a valid thing to assume for many-to-many where one side and be fully loaded while the inverse is not.

[ajcvickers]

Underlying issue here is that the query used to do many-to-many loading uses a filtered Include:

IQueryable<EntityTwo> loaded
     = context.Set<EntityOne>()
        .AsTracking()
        .Where(e => e.Id == left.Id)
        .SelectMany(e => e.TwoSkip)
        .Include(e => e.OneSkip.Where(e => e.Id == left.Id));

Filtered Include, by-design, sets the navigation as loaded, but in this case we don't want that behavior.

[ajcvickers]

@smitpatel Proposal for the patch; a low-risk fix that doesn't introduce new surface. We should do something better in 6.0, but does this sound reasonable for the patch?

New internal method:

        // A version of Include that doesn't set the navigation as loaded
        internal static IIncludableQueryable<TEntity, TProperty> NotQuiteInclude<TEntity, TProperty>(
            [NotNull] this IQueryable<TEntity> source,
            [NotNull] Expression<Func<TEntity, TProperty>> navigationPropertyPath)
            where TEntity : class
        {
            return new IncludableQueryable<TEntity, TProperty>(
                source.Provider is EntityQueryProvider
                    ? source.Provider.CreateQuery<TEntity>(
                        Expression.Call(
                            instance: null,
                            method: NotQuiteIncludeMethodInfo.MakeGenericMethod(typeof(TEntity), typeof(TProperty)),
                            arguments: new[] { source.Expression, Expression.Quote(navigationPropertyPath) }))
                    : source);
        }

Then flow this through translation such that we have a flag when setting IsLoaded. (Note that I don't currently know how to flow this through. :-D) For example:

            private static void InitializeIncludeCollection<TParent, TNavigationEntity>(
                int collectionId,
                QueryContext queryContext,
                DbDataReader dbDataReader,
                SingleQueryResultCoordinator resultCoordinator,
                TParent entity,
                Func<QueryContext, DbDataReader, object[]> parentIdentifier,
                Func<QueryContext, DbDataReader, object[]> outerIdentifier,
                INavigationBase navigation,
                IClrCollectionAccessor clrCollectionAccessor,
                bool trackingQuery,
                bool setLoaded)
                where TParent : class
                where TNavigationEntity : class, TParent
            {
                object collection = null;
                if (entity is TNavigationEntity)
                {
                    if (setLoaded)
                    {
                        if (trackingQuery)
                        {
                            queryContext.SetNavigationIsLoaded(entity, navigation);
                        }
                        else
                        {
                            navigation.SetIsLoadedWhenNoTracking(entity);
                        }
                    }

                    collection = clrCollectionAccessor.GetOrCreate(entity, forMaterialization: true);
                }

                var parentKey = parentIdentifier(queryContext, dbDataReader);
                var outerKey = outerIdentifier(queryContext, dbDataReader);

                var collectionMaterializationContext = new SingleQueryCollectionContext(entity, collection, parentKey, outerKey);

                resultCoordinator.SetSingleQueryCollectionContext(collectionId, collectionMaterializationContext);
            }

[smitpatel]

Flowing that flag would require some breaks in IncludeExpression. If we use another kind of expression to represent it, then it becomes higher risk in our codebase due to churn in code + anyone outside of EF Core (provider/plugins/libraries), if they are not expecting it and don't handle would break.

[ajcvickers]

@smitpatel What are the breaks required in IncludeExpression?

[smitpatel]

A flag to flow the information to the shaper that don't mark this navigation as loaded