Detach join entity when removing added relationship

Fixes #26779

Also fix related issue where fixup to the join entity was not happening when it was synthesized from navigation change.

src/EFCore/ChangeTracking/Internal/INavigationFixer.cs

@@ -23,7 +23,7 @@ public interface INavigationFixer
     ///     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 BeginAttachGraph();
+    bool BeginAttachGraph();
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

src/EFCore/ChangeTracking/Internal/NavigationFixer.cs

@@ -40,10 +40,17 @@ public NavigationFixer(
     ///     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 BeginAttachGraph()
+    public virtual bool BeginAttachGraph()
     {
+        if (_inAttachGraph)
+        {
+            return false;
+        }
+
         _danglingJoinEntities?.Clear();
         _inAttachGraph = true;
+
+        return true;
     }
 
     /// <summary>
@@ -120,6 +127,7 @@ public virtual void NavigationReferenceChanged(
             newTargetEntry = null;
         }
 
+        var delayingFixup = BeginAttachGraph();
         try
         {
             _inFixup = true;
@@ -227,6 +235,11 @@ public virtual void NavigationReferenceChanged(
         finally
         {
             _inFixup = false;
+
+            if (delayingFixup)
+            {
+                CompleteAttachGraph();
+            }
         }
 
         if (newValue != null
@@ -273,13 +286,18 @@ public virtual void NavigationCollectionChanged(
             if (oldTargetEntry != null
                 && oldTargetEntry.EntityState != EntityState.Detached)
             {
+                var delayingFixup = BeginAttachGraph();
                 try
                 {
                     _inFixup = true;
 
                     if (navigationBase is ISkipNavigation skipNavigation)
                     {
-                        FindJoinEntry(entry, oldTargetEntry, skipNavigation)?.SetEntityState(EntityState.Deleted);
+                        var joinEntry = FindJoinEntry(entry, oldTargetEntry, skipNavigation);
+                        joinEntry?.SetEntityState(
+                            joinEntry.EntityState == EntityState.Added
+                                ? EntityState.Detached
+                                : EntityState.Deleted);
 
                         Check.DebugAssert(
                             skipNavigation.Inverse.IsCollection,
@@ -310,6 +328,11 @@ public virtual void NavigationCollectionChanged(
                 finally
                 {
                     _inFixup = false;
+
+                    if (delayingFixup)
+                    {
+                        CompleteAttachGraph();
+                    }
                 }
             }
         }
@@ -319,6 +342,7 @@ public virtual void NavigationCollectionChanged(
             var newTargetEntry = stateManager.GetOrCreateEntry(newValue, targetEntityType);
             if (newTargetEntry.EntityState != EntityState.Detached)
             {
+                var delayingFixup = BeginAttachGraph();
                 try
                 {
                     _inFixup = true;
@@ -358,6 +382,11 @@ public virtual void NavigationCollectionChanged(
                 finally
                 {
                     _inFixup = false;
+
+                    if (delayingFixup)
+                    {
+                        CompleteAttachGraph();
+                    }
                 }
             }
             else
@@ -394,6 +423,7 @@ public virtual void KeyPropertyChanged(
             return;
         }
 
+        var delayingFixup = BeginAttachGraph();
         try
         {
             _inFixup = true;
@@ -539,6 +569,11 @@ var targetDependentEntry
         finally
         {
             _inFixup = false;
+
+            if (delayingFixup)
+            {
+                CompleteAttachGraph();
+            }
         }
     }
 

test/EFCore.Specification.Tests/ManyToManyTrackingTestBase.cs

@@ -4856,6 +4856,84 @@ static void ValidateFixup(DbContext context, IList<EntityTwo> leftEntities, ILis
         }
     }
 
+    [ConditionalTheory]
+    [InlineData(false, false)]
+    [InlineData(false, true)]
+    [InlineData(true, false)]
+    [InlineData(true, true)]
+    public virtual void Can_add_and_remove_a_new_relationship(bool modifyLeft, bool modifyRight)
+    {
+        int leftId = -1;
+        int rightId = -1;
+
+        ExecuteWithStrategyInTransaction(
+            context =>
+            {
+                var left = context.Set<EntityOne>().Where(e => !e.TwoSkip.Any()).OrderBy(e => e.Id).First();
+                var right = context.Set<EntityTwo>().OrderBy(e => e.Id).First();
+
+                if (modifyLeft)
+                {
+                    context.Entry(left).State = EntityState.Modified;
+                }
+
+                if (modifyRight)
+                {
+                    context.Entry(right).State = EntityState.Modified;
+                }
+
+                leftId = left.Id;
+                rightId = right.Id;
+
+                if (left.TwoSkip == null)
+                {
+                    left.TwoSkip = CreateCollection<EntityTwo>();
+                }
+                left.TwoSkip.Add(right);
+
+                if (RequiresDetectChanges)
+                {
+                    context.ChangeTracker.DetectChanges();
+                }
+
+                Assert.Same(right, left.TwoSkip.Single());
+                Assert.Same(left, right.OneSkip.Single());
+
+                var joinEntry = context.ChangeTracker.Entries<JoinOneToTwo>().Single();
+                Assert.Equal(EntityState.Added, joinEntry.State);
+                Assert.Same(left, joinEntry.Entity.One);
+                Assert.Same(right, joinEntry.Entity.Two);
+                Assert.Equal(left.Id, joinEntry.Entity.OneId);
+                Assert.Equal(right.Id, joinEntry.Entity.TwoId);
+
+                right.OneSkip.Remove(left);
+
+                if (RequiresDetectChanges)
+                {
+                    context.ChangeTracker.DetectChanges();
+                }
+
+                Assert.Empty(left.TwoSkip);
+                Assert.Empty(right.OneSkip);
+
+                Assert.Equal(EntityState.Detached, joinEntry.State);
+                Assert.Same(left, joinEntry.Entity.One);
+                Assert.Same(right, joinEntry.Entity.Two);
+                Assert.Equal(leftId, joinEntry.Entity.OneId);
+                Assert.Equal(rightId, joinEntry.Entity.TwoId);
+
+                context.SaveChanges();
+            },
+            context =>
+            {
+                var left = context.Set<EntityOne>().Where(e => !e.TwoSkip.Any()).OrderBy(e => e.Id).First();
+                var right = context.Set<EntityTwo>().OrderBy(e => e.Id).First();
+
+                Assert.Equal(leftId, left.Id);
+                Assert.Equal(rightId, right.Id);
+            });
+    }
+
     protected static void VerifyRelationshipSnapshots(DbContext context, IEnumerable<object> entities)
     {
         var detectChanges = context.ChangeTracker.AutoDetectChangesEnabled;

test/EFCore.Tests/ChangeTracking/Internal/InternalEntryEntrySubscriberTest.cs

@@ -504,9 +504,8 @@ private class TestNavigationListener : INavigationFixer
         public List<Tuple<InternalEntityEntry, INavigationBase, IEnumerable<object>, IEnumerable<object>>> CollectionChanged { get; }
             = new();
 
-        public void BeginAttachGraph()
-        {
-        }
+        public bool BeginAttachGraph()
+            => false;
 
         public void CompleteAttachGraph()
         {

test/EFCore.Tests/ChangeTracking/Internal/StateManagerTest.cs

@@ -627,9 +627,8 @@ private class TestListener : INavigationFixer
         public EntityState ChangingState;
         public EntityState ChangedState;
 
-        public void BeginAttachGraph()
-        {
-        }
+        public bool BeginAttachGraph()
+            => false;
 
         public void CompleteAttachGraph()
         {

test/EFCore.Tests/DbContextServicesTest.cs

@@ -386,9 +386,8 @@ public void StateChanging(InternalEntityEntry entry, EntityState newState)
             public void StateChanged(InternalEntityEntry entry, EntityState oldState, bool fromQuery)
                 => throw new NotImplementedException();
 
-            public void BeginAttachGraph()
-            {
-            }
+            public bool BeginAttachGraph()
+                => false;
 
             public void CompleteAttachGraph()
             {