Stop skipping shadow skip navigation slots when creating the shadow v…

…alues array (dotnet#30902)
roji · May 16, 2023 · ee74a60 · ee74a60
1 parent 9626596
commit ee74a60
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 11 deletions.
diff --git a/src/EFCore/ChangeTracking/Internal/ShadowValuesFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/ShadowValuesFactoryFactory.cs
@@ -20,8 +20,7 @@ public class ShadowValuesFactoryFactory : SnapshotFactoryFactory<ValueBuffer>
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     protected override int GetPropertyIndex(IPropertyBase propertyBase)
-        // Navigations are not included in the supplied value buffer
-        => (propertyBase as IProperty)?.GetShadowIndex() ?? -1;
+        => propertyBase.GetShadowIndex();
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

diff --git a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs
@@ -41,7 +41,7 @@ protected virtual Expression CreateConstructorExpression(
         var count = GetPropertyCount(entityType);
 
         var types = new Type[count];
-        var propertyBases = new IPropertyBase[count];
+        var propertyBases = new IPropertyBase?[count];
 
         foreach (var propertyBase in entityType.GetPropertiesAndNavigations())
         {
@@ -93,7 +93,7 @@ protected virtual Expression CreateSnapshotExpression(
         Type? entityType,
         ParameterExpression? parameter,
         Type[] types,
-        IList<IPropertyBase> propertyBases)
+        IList<IPropertyBase?> propertyBases)
     {
         var count = types.Length;
 

diff --git a/src/EFCore/ChangeTracking/Internal/TemporaryValuesFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/TemporaryValuesFactoryFactory.cs
@@ -23,7 +23,7 @@ protected override Expression CreateSnapshotExpression(
         [DynamicallyAccessedMembers(IEntityType.DynamicallyAccessedMemberTypes)] Type? entityType,
         ParameterExpression? parameter,
         Type[] types,
-        IList<IPropertyBase> propertyBases)
+        IList<IPropertyBase?> propertyBases)
     {
         var constructorExpression = Expression.Convert(
             Expression.New(

diff --git a/src/EFCore/Infrastructure/ExpressionExtensions.cs b/src/EFCore/Infrastructure/ExpressionExtensions.cs
@@ -288,11 +288,13 @@ public static Expression CreateValueBufferReadValueExpression(
         Type type,
         int index,
         IPropertyBase? property)
-        => Expression.Call(
-            MakeValueBufferTryReadValueMethod(type),
-            valueBuffer,
-            Expression.Constant(index),
-            Expression.Constant(property, typeof(IPropertyBase)));
+        => property is INavigationBase
+            ? Expression.Constant(null, typeof(object))
+            : Expression.Call(
+                MakeValueBufferTryReadValueMethod(type),
+                valueBuffer,
+                Expression.Constant(index),
+                Expression.Constant(property, typeof(IPropertyBase)));
 
     /// <summary>
     ///     <para>

diff --git a/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs b/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs
@@ -588,7 +588,12 @@ private BlockExpression CreateFullMaterializeExpression(
             {
                 var valueBufferExpression = Expression.Call(
                     materializationContextVariable, MaterializationContext.GetValueBufferMethod);
-                var shadowProperties = concreteEntityType.GetProperties().Where(p => p.IsShadowProperty());
+
+                var shadowProperties = concreteEntityType.GetProperties()
+                    .Concat<IPropertyBase>(concreteEntityType.GetNavigations())
+                    .Concat(concreteEntityType.GetSkipNavigations())
+                    .Where(n => n.IsShadowProperty())
+                    .OrderBy(e => e.GetShadowIndex());
 
                 blockExpressions.Add(
                     Expression.Assign(

diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBase.cs
@@ -555,6 +555,17 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                     b.HasOne(x => x.Parent).WithMany(x => x.Children).OnDelete(DeleteBehavior.Restrict);
                     b.HasAlternateKey(x => new { x.Id, x.ParentId });
                 });
+
+            modelBuilder.Entity<Beetroot2>().HasData(
+                new { Id = 1, Key = "root-1", Name = "Root One" });
+
+            modelBuilder.Entity<Lettuce2>().HasData(
+                new { Id = 4, Key = "root-1/leaf-1", Name = "Leaf One-One", RootId = 1 });
+
+            modelBuilder.Entity<Radish2>()
+                .HasMany(entity => entity.Entities)
+                .WithMany()
+                .UsingEntity<RootStructure>();
         }
 
         protected virtual object CreateFullGraph()
@@ -4073,6 +4084,68 @@ public virtual NaiveParent Parent
         }
     }
 
+    protected abstract class Parsnip2 : NotifyingEntity
+    {
+        private int _id;
+
+        public int Id
+        {
+            get => _id;
+            set => SetWithNotify(value, ref _id);
+        }
+    }
+
+    protected class Lettuce2 : Parsnip2
+    {
+        private Beetroot2 _root;
+
+        public Beetroot2 Root
+        {
+            get => _root;
+            set => SetWithNotify(value, ref _root);
+        }
+    }
+
+    protected class RootStructure : NotifyingEntity
+    {
+        private Guid _radish2Id;
+        private int _parsnip2Id;
+
+        public Guid Radish2Id
+        {
+            get => _radish2Id;
+            set => SetWithNotify(value, ref _radish2Id);
+        }
+
+        public int Parsnip2Id
+        {
+            get => _parsnip2Id;
+            set => SetWithNotify(value, ref _parsnip2Id);
+        }
+    }
+
+    protected class Radish2 : NotifyingEntity
+    {
+        private Guid _id;
+        private ICollection<Parsnip2> _entities = new ObservableHashSet<Parsnip2>();
+
+        public Guid Id
+        {
+            get => _id;
+            set => SetWithNotify(value, ref _id);
+        }
+
+        public ICollection<Parsnip2> Entities
+        {
+            get => _entities;
+            set => SetWithNotify(value, ref _entities);
+        }
+    }
+
+    protected class Beetroot2 : Parsnip2
+    {
+    }
+
     protected class NotifyingEntity : INotifyPropertyChanging, INotifyPropertyChanged
     {
         protected void SetWithNotify<T>(T value, ref T field, [CallerMemberName] string propertyName = "")

diff --git a/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs b/test/EFCore.Specification.Tests/GraphUpdates/GraphUpdatesTestBaseMiscellaneous.cs
@@ -1983,4 +1983,19 @@ public virtual Task Alternate_key_over_foreign_key_doesnt_bypass_delete_behavior
                                 : context.SaveChanges();
                         })).Message);
             });
+
+    [ConditionalTheory] // Issue #30764
+    [InlineData(false)]
+    [InlineData(true)]
+    public virtual Task Shadow_skip_navigation_in_base_class_is_handled(bool async)
+        => ExecuteWithStrategyInTransactionAsync(
+            async context =>
+            {
+                var entities = async
+                    ? await context.Set<Lettuce2>().ToListAsync()
+                    : context.Set<Lettuce2>().ToList();
+
+                Assert.Equal(1, entities.Count);
+                Assert.Equal(nameof(Lettuce2), context.Entry(entities[0]).Property<string>("Discriminator").CurrentValue);
+            });
 }
diff --git a/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs b/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs
@@ -42,6 +42,10 @@ public override Task Sever_relationship_that_will_later_be_deleted(bool async)
     public override Task Alternate_key_over_foreign_key_doesnt_bypass_delete_behavior(bool async)
         => Task.CompletedTask;
 
+    // No owned types
+    public override Task Shadow_skip_navigation_in_base_class_is_handled(bool async)
+        => Task.CompletedTask;
+
     // Owned dependents are always loaded
     public override void Required_one_to_one_are_cascade_deleted_in_store(
         CascadeTiming? cascadeDeleteTiming,