Delay synthesis of join entities until entire graph is attached

Fixes #23339

This stops the change tracker creating a join entity instance for the same join entity that is later discovered in the graph.

src/EFCore/ChangeTracking/Internal/EntityGraphAttacher.cs

@@ -47,27 +47,32 @@ public virtual void AttachGraph(
             EntityState targetState,
             EntityState storeGeneratedWithKeySetTargetState,
             bool forceStateWhenUnknownKey)
-            => _graphIterator.TraverseGraph(
+        {
+            _graphIterator.TraverseGraph(
                 new EntityEntryGraphNode<(EntityState TargetState, EntityState StoreGenTargetState, bool Force)>(
                     rootEntry,
                     (targetState, storeGeneratedWithKeySetTargetState, forceStateWhenUnknownKey),
                     null,
                     null),
                 PaintAction);
+
+            rootEntry.StateManager.CompleteAttachGraph();
+        }
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
         ///     any release. You should only use it directly in your code with extreme caution and knowing that
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
-        public virtual Task AttachGraphAsync(
+        public virtual async Task AttachGraphAsync(
             InternalEntityEntry rootEntry,
             EntityState targetState,
             EntityState storeGeneratedWithKeySetTargetState,
             bool forceStateWhenUnknownKey,
             CancellationToken cancellationToken = default)
-            => _graphIterator.TraverseGraphAsync(
+        {
+            await _graphIterator.TraverseGraphAsync(
                 new EntityEntryGraphNode<(EntityState TargetState, EntityState StoreGenTargetState, bool Force)>(
                     rootEntry,
                     (targetState, storeGeneratedWithKeySetTargetState, forceStateWhenUnknownKey),
@@ -76,6 +81,9 @@ public virtual Task AttachGraphAsync(
                 PaintActionAsync,
                 cancellationToken);
 
+            rootEntry.StateManager.CompleteAttachGraph();
+        }
+
         private static bool PaintAction(
             EntityEntryGraphNode<(EntityState TargetState, EntityState StoreGenTargetState, bool Force)> node)
         {

src/EFCore/ChangeTracking/Internal/INavigationFixer.cs

@@ -23,6 +23,14 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
     /// </summary>
     public interface INavigationFixer
     {
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        void CompleteAttachGraph();
+
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in

src/EFCore/ChangeTracking/Internal/IStateManager.cs

@@ -271,6 +271,14 @@ void RecordReferencedUntrackedEntity(
         /// </summary>
         IEnumerable<Tuple<INavigationBase, InternalEntityEntry>> GetRecordedReferrers(object referencedEntity, bool clear);
 
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        void CompleteAttachGraph();
+
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in

src/EFCore/ChangeTracking/Internal/NavigationFixer.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
@@ -28,6 +29,7 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
     /// </summary>
     public class NavigationFixer : INavigationFixer
     {
+        private IList<Action>? _danglingJoinEntities;
         private readonly IChangeDetector _changeDetector;
         private readonly IEntityGraphAttacher _attacher;
         private bool _inFixup;
@@ -46,6 +48,25 @@ public NavigationFixer(
             _attacher = attacher;
         }
 
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        public virtual void CompleteAttachGraph()
+        {
+            if (_danglingJoinEntities != null)
+            {
+                foreach (var synthesize in _danglingJoinEntities)
+                {
+                    synthesize();
+                }
+
+                _danglingJoinEntities = null;
+            }
+        }
+
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -287,7 +308,8 @@ public virtual void NavigationCollectionChanged(
 
                         if (navigationBase is ISkipNavigation skipNavigation)
                         {
-                            FindOrCreateJoinEntry(entry, newTargetEntry, skipNavigation, fromQuery: false, setModified: true);
+                            FindOrCreateJoinEntry(
+                                entry, newTargetEntry, skipNavigation, fromQuery: false, setModified: true, synthesize: true);
 
                             Check.DebugAssert(
                                 skipNavigation.Inverse.IsCollection,
@@ -903,47 +925,45 @@ private void FixupSkipNavigations(InternalEntityEntry entry, IForeignKey foreign
             }
         }
 
-        private InternalEntityEntry FindOrCreateJoinEntry(
+        private void FindOrCreateJoinEntry(
             InternalEntityEntry entry,
             InternalEntityEntry otherEntry,
             ISkipNavigation skipNavigation,
             bool fromQuery,
-            bool setModified)
+            bool setModified,
+            bool synthesize = false)
         {
             var joinEntry = FindJoinEntry(entry, otherEntry, skipNavigation);
-
-            if (joinEntry == null)
+            if (joinEntry != null)
+            {
+                SetForeignKeyProperties(joinEntry, entry, skipNavigation.ForeignKey, setModified, fromQuery);
+                SetForeignKeyProperties(joinEntry, otherEntry, skipNavigation.Inverse.ForeignKey, setModified, fromQuery);
+            }
+            else if (synthesize)
             {
                 var joinEntityType = skipNavigation.JoinEntityType;
                 var joinEntity = joinEntityType.GetInstanceFactory()(
                     new MaterializationContext(ValueBuffer.Empty, entry.StateManager.Context));
 
                 joinEntry = entry.StateManager.GetOrCreateEntry(joinEntity, joinEntityType);
-            }
 
-            SetForeignKeyProperties(joinEntry, entry, skipNavigation.ForeignKey, setModified, fromQuery);
-            SetForeignKeyProperties(joinEntry, otherEntry, skipNavigation.Inverse.ForeignKey, setModified, fromQuery);
+                SetForeignKeyProperties(joinEntry, entry, skipNavigation.ForeignKey, setModified, fromQuery);
+                SetForeignKeyProperties(joinEntry, otherEntry, skipNavigation.Inverse.ForeignKey, setModified, fromQuery);
 
-            if (joinEntry.EntityState == EntityState.Detached)
+                joinEntry.SetEntityState(
+                    setModified
+                    || entry.EntityState == EntityState.Added
+                    || otherEntry.EntityState == EntityState.Added
+                        ? EntityState.Added
+                        : EntityState.Unchanged);
+            }
+            else
             {
-                try
-                {
-                    _inFixup = false;
-
-                    joinEntry.SetEntityState(
-                        setModified
-                        || entry.EntityState == EntityState.Added
-                        || otherEntry.EntityState == EntityState.Added
-                            ? EntityState.Added
-                            : EntityState.Unchanged);
-                }
-                finally
-                {
-                    _inFixup = true;
-                }
+                _danglingJoinEntities ??= new List<Action>();
+
+                _danglingJoinEntities.Add(
+                    () => FindOrCreateJoinEntry(entry, otherEntry, skipNavigation, fromQuery, setModified, synthesize: true));
             }
-
-            return joinEntry;
         }
 
         private static InternalEntityEntry? FindJoinEntry(

src/EFCore/ChangeTracking/Internal/StateManager.cs

@@ -86,7 +86,6 @@ public StateManager(StateManagerDependencies dependencies)
             _changeTrackingLogger = dependencies.ChangeTrackingLogger;
             _changeDetectorInitialized = false;
         }
-
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -683,6 +682,15 @@ public virtual Task ResetStateAsync(CancellationToken cancellationToken = defaul
             return Task.CompletedTask;
         }
 
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        public virtual void CompleteAttachGraph()
+            => Dependencies.NavigationFixer.CompleteAttachGraph();
+
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
         ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -694,11 +702,8 @@ public virtual void RecordReferencedUntrackedEntity(
             INavigationBase navigation,
             InternalEntityEntry referencedFromEntry)
         {
-            if (_referencedUntrackedEntities == null)
-            {
-                _referencedUntrackedEntities
-                    = new Dictionary<object, IList<Tuple<INavigationBase, InternalEntityEntry>>>(LegacyReferenceEqualityComparer.Instance);
-            }
+            _referencedUntrackedEntities ??=
+                new Dictionary<object, IList<Tuple<INavigationBase, InternalEntityEntry>>>(LegacyReferenceEqualityComparer.Instance);
 
             if (!_referencedUntrackedEntities.TryGetValue(referencedEntity, out var danglers))
             {

src/EFCore/ChangeTracking/Internal/StateManagerDependencies.cs

@@ -78,7 +78,8 @@ public StateManagerDependencies(
             ICoreSingletonOptions coreSingletonOptions,
             ILoggingOptions loggingOptions,
             IDiagnosticsLogger<DbLoggerCategory.Update> updateLogger,
-            IDiagnosticsLogger<DbLoggerCategory.ChangeTracking> changeTrackingLogger)
+            IDiagnosticsLogger<DbLoggerCategory.ChangeTracking> changeTrackingLogger,
+            INavigationFixer navigationFixer)
         {
             InternalEntityEntryFactory = internalEntityEntryFactory;
             InternalEntityEntrySubscriber = internalEntityEntrySubscriber;
@@ -96,6 +97,7 @@ public StateManagerDependencies(
             LoggingOptions = loggingOptions;
             UpdateLogger = updateLogger;
             ChangeTrackingLogger = changeTrackingLogger;
+            NavigationFixer = navigationFixer;
         }
 
         /// <summary>
@@ -227,5 +229,13 @@ public StateManagerDependencies(
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
         public IDiagnosticsLogger<DbLoggerCategory.ChangeTracking> ChangeTrackingLogger { get; init; }
+
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        public INavigationFixer NavigationFixer { get; init; }
     }
 }