RC2: Regression with Parent/Child relationship

[rwg0]

Steps to reproduce

Use the following code to reproduce:

    class Program
    {
        static void Main(string[] args)
        {
            if (File.Exists("test.db"))
                File.Delete("test.db");

            using (var context = new WidgetContext())
            {
                context.Database.Migrate();
            }

            using (var context = new WidgetContext())
            {
                for (int i = 0; i < 10; i++)
                {
                    var w = new Widget();
                    context.Widgets.Add(w);
                }

                context.SaveChanges();
            }

            using (var context = new WidgetContext())
            {
                var widgets = context.Widgets.ToList();
                widgets[0].ParentWidget = widgets[1];
                context.SaveChanges();
            }

            using (var context = new WidgetContext())
            {
                var widgets = context.Widgets.ToList();
                try
                {
                    Console.WriteLine(widgets[1].ChildWidgets.Count);
                }
                catch (Exception e)
                {
                    Console.WriteLine("oops, didn't connect childwidgets collection.");
                    Console.WriteLine(e);
                }
                widgets[0].ParentWidget = widgets[2];
                try
                {
                    context.SaveChanges();
                    Console.WriteLine("Saved change of parent.");
                }
                catch (Exception e)
                {
                    Console.WriteLine("oops, couldn't alter the parent Id");
                    Console.WriteLine(e);
                }
            }
        }
    }

    class WidgetContext : DbContext
    {
        public DbSet<Widget> Widgets { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlite("Filename=test.db");
        }
    }

    public class Widget
    {
        public int Id { get; set; }

        public int? ParentWidgetId { get; set; }

        [InverseProperty(nameof(ChildWidgets))]
        [ForeignKey(nameof(ParentWidgetId))]
        public Widget ParentWidget { get; set; }


        [InverseProperty(nameof(ParentWidget))]
        public List<Widget> ChildWidgets { get; set; }
    }

Add nuget packages for appropriate version of EntityFramework.Sqlite/EntityFrameworkCore.Sqlite and EntityFramework.Commands/EntityFrameworkCore.Tools.

Create an initial migration.

The issue

On RC2, the ChildWidgets collection on each Widget when loading the entire table, so the line
Console.WriteLine(widgets[1].ChildWidgets.Count);
throws a NullReferenceException. This works correctly on RC1.

Additionally, changing the ParentWidget of a Widget that already has a parent set fails when the changes are saved to the database with the following exception.

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.RemoveFromCollectionSnapshot(IPropertyBase propertyBase, Object removedEntity)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.RemoveFromCollection(InternalEntityEntry entry, INavigation navigation, IClrCollectionAccessor collectionAccessor, Object value)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.KeyPropertyChanged(InternalEntityEntry entry, IProperty property, IReadOnlyList`1 containingPrincipalKeys, IReadOnlyList`1 containingForeignKeys, Object oldValue, Object newValue)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntryNotifier.KeyPropertyChanged(InternalEntityEntry entry, IProperty property, IReadOnlyList`1 keys, IReadOnlyList`1 foreignKeys, Object oldValue, Object newValue)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectKeyChange(InternalEntityEntry entry, IProperty property)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.PropertyChanged(InternalEntityEntry entry, IPropertyBase propertyBase, Boolean setModified)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntryNotifier.PropertyChanged(InternalEntityEntry entry, IPropertyBase property, Boolean setModified)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetProperty(IPropertyBase propertyBase, Object value, Boolean setModified)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.SetForeignKeyProperties(InternalEntityEntry dependentEntry, InternalEntityEntry principalEntry, IForeignKey foreignKey, Boolean setModified)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NavigationFixer.NavigationReferenceChanged(InternalEntityEntry entry, INavigation navigation, Object oldValue, Object newValue)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntryNotifier.NavigationReferenceChanged(InternalEntityEntry entry, INavigation navigation, Object oldValue, Object newValue)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectNavigationChange(InternalEntityEntry entry, INavigation navigation)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectNavigationChanges(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectRelationshipChanges(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectChanges(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.ChangeDetector.DetectChanges(IStateManager stateManager)
   at Microsoft.EntityFrameworkCore.DbContext.TryDetectChanges(IStateManager stateManager)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess)
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges()

Again, this works correctly on RC1. I suspect this may be a consequence of the ChildWidget list being left null - so a maybe just another symptom of the first problem.

Further technical details

EF Core version: 1.0.0-rc2-final
Operating system: Windows 10 Pro x64
Visual Studio version: VS2015 update 2

Other details about my project setup: Console App, Target : .NET 4.5.2

[ajcvickers]

Confirmed that this is a bug. When loading the Widgets the self-referencing relationship is not being fixed up. The workaround is to include the child collection in the query:

var widgets = context.Widgets.Include(e => e.ChildWidgets).ToList();

This is a situation where the state manager is expecting query to do the fixup since it is all self-contained within the query, which means query needs to be able to do it anyway to support no-tracking queries.

However, I suspect from the perspective of query it will never do this fixup, even for no-tracking queries, because the navigation property is not included in the query. In other words, if you do a no-tracking query for entities with self references, then those references will never be fixed up. If you want them to be fixed up, then you have do an Include.

If we decide that this is the correct behavior for query, then the state manager cannot use fast single query mode for tracked queries that contain self references.

@divega @anpete @rowanmiller

[rwg0]

Thanks for the suggestion of a workaround - I think that will be fine for our use case for now.

While continuing to experiment I found another anomaly in the same area - if you explicitly query for the child widgets without using .Include(), the results of the fixup are inconsistent - the ChildWidgets list only contains the items loaded into the context by the query and not any items already loaded by previous queries.

  static void Main(string[] args)
        {
            if (File.Exists("test.db"))
                File.Delete("test.db");

            using (var context = new WidgetContext())
            {
                context.Database.Migrate();
            }

            using (var context = new WidgetContext())
            {
                Widget first = new Widget();
                context.Widgets.Add(first);
                for (int i = 0; i < 10; i++)
                {
                    var w = new Widget {ParentWidget = first};
                    context.Widgets.Add(w);
                }

                context.SaveChanges();
            }

            using (var context = new WidgetContext())
            {
                var widget = context.Widgets.First();
                var children = context.Widgets.Where(x => x.ParentWidgetId == widget.Id).ToList();
                Console.WriteLine(widget.ChildWidgets.Count);  // 10, that's good
            }

            using (var context = new WidgetContext())
            {
                var widgets = context.Widgets.Take(5).ToList(); // load 5 widgets into the context 
                var widget = widgets.First();
                var children = context.Widgets.Where(x => x.ParentWidgetId == widget.Id).ToList();
                Console.WriteLine(widget.ChildWidgets.Count);  // 6, that's interesting!
            }

            using (var context = new WidgetContext())
            {
                var widgets = context.Widgets.ToList();  // load all the widgets into the context
                var widget = widgets.First();
                var children = context.Widgets.Where(x => x.ParentWidgetId == widget.Id).ToList();
                Console.WriteLine(widget.ChildWidgets.Count);  // NullReferenceException!
            }
        }

Not sure what the expected behaviour should be here, but I don't think it should depend on which objects have already been loaded previously.

[ajcvickers]

@rwg0 The bug is due to fixup not happening in query. The change In have out for this will therefore fix the issues in the second repro as well.

[mkontula]

Sorry for really stupid question, but since we're having this as well - what version should we be targeting to get the fix? 1.0.0 was released 16 days ago and this fix was committed at the time of writing 8 days ago. We have a parent/child relation like the one on this issue and null reference flying from same method in the internals.

[ajcvickers]

@mkontula Fix is in 1.0.0.

[mkontula]

@ajcvickers Thanks for the info. I actually found out a way to make it brake, when you setup the parent/child relation for parent AND child and then SaveChanges to context. I'll make reproable sample later.