From ee74a60d509c54afd661b1cdc6d8204f251efa65 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Tue, 16 May 2023 19:59:27 +0100 Subject: [PATCH] Stop skipping shadow skip navigation slots when creating the shadow values array (#30902) --- .../Internal/ShadowValuesFactoryFactory.cs | 3 +- .../Internal/SnapshotFactoryFactory.cs | 4 +- .../Internal/TemporaryValuesFactoryFactory.cs | 2 +- .../Infrastructure/ExpressionExtensions.cs | 12 +-- .../ShapedQueryCompilingExpressionVisitor.cs | 7 +- .../GraphUpdates/GraphUpdatesTestBase.cs | 73 +++++++++++++++++++ .../GraphUpdatesTestBaseMiscellaneous.cs | 15 ++++ .../GraphUpdatesSqlServerOwnedTest.cs | 4 + 8 files changed, 109 insertions(+), 11 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/ShadowValuesFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/ShadowValuesFactoryFactory.cs index 2a866d432ee..cf6b4ee81d9 100644 --- a/src/EFCore/ChangeTracking/Internal/ShadowValuesFactoryFactory.cs +++ b/src/EFCore/ChangeTracking/Internal/ShadowValuesFactoryFactory.cs @@ -20,8 +20,7 @@ public class ShadowValuesFactoryFactory : SnapshotFactoryFactory /// doing so can result in application failures when updating to a new Entity Framework Core release. /// protected override int GetPropertyIndex(IPropertyBase propertyBase) - // Navigations are not included in the supplied value buffer - => (propertyBase as IProperty)?.GetShadowIndex() ?? -1; + => propertyBase.GetShadowIndex(); /// /// 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 index 65d37aa9b5e..04499ea5858 100644 --- 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 propertyBases) + IList propertyBases) { var count = types.Length; diff --git a/src/EFCore/ChangeTracking/Internal/TemporaryValuesFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/TemporaryValuesFactoryFactory.cs index 125597bb2a7..0ecc939920d 100644 --- 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 propertyBases) + IList propertyBases) { var constructorExpression = Expression.Convert( Expression.New( diff --git a/src/EFCore/Infrastructure/ExpressionExtensions.cs b/src/EFCore/Infrastructure/ExpressionExtensions.cs index fb4266ef148..e2764778894 100644 --- 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))); /// /// diff --git a/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs b/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs index ea140564a1d..b717cc5bc09 100644 --- 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(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 index 0b199e212d0..3c63ce6b52c 100644 --- 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().HasData( + new { Id = 1, Key = "root-1", Name = "Root One" }); + + modelBuilder.Entity().HasData( + new { Id = 4, Key = "root-1/leaf-1", Name = "Leaf One-One", RootId = 1 }); + + modelBuilder.Entity() + .HasMany(entity => entity.Entities) + .WithMany() + .UsingEntity(); } 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 _entities = new ObservableHashSet(); + + public Guid Id + { + get => _id; + set => SetWithNotify(value, ref _id); + } + + public ICollection Entities + { + get => _entities; + set => SetWithNotify(value, ref _entities); + } + } + + protected class Beetroot2 : Parsnip2 + { + } + protected class NotifyingEntity : INotifyPropertyChanging, INotifyPropertyChanged { protected void SetWithNotify(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 index 2e5f0564dd4..555d4f6deca 100644 --- 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().ToListAsync() + : context.Set().ToList(); + + Assert.Equal(1, entities.Count); + Assert.Equal(nameof(Lettuce2), context.Entry(entities[0]).Property("Discriminator").CurrentValue); + }); } diff --git a/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs b/test/EFCore.SqlServer.FunctionalTests/GraphUpdates/GraphUpdatesSqlServerOwnedTest.cs index 60390b6a4cd..1b195ce764c 100644 --- 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,