Mark join entities as Added when either side is Added (#22669)

Fixes #22647

**Description**

If the entity on either end of a many-to-many relationship is Added, then any join entity instance must be new since it references a new entity via a required relationship. This change improves the many-to-many experience by detecting this case.

**Customer Impact**

This is an improvement to the experience for the new many-to-many feature. Doing this now instead of in the future avoids a breaking change after 5.0. Therefore, doing this now will help customers have a seamless update to 6.0.

**How found**

Customer reported on RC1.

**Test coverage**

Added additional test coverage around this case.

**Regression?**

No; new feature.

**Risk**

Low; small change in behavior.

src/EFCore/ChangeTracking/Internal/NavigationFixer.cs

@@ -292,21 +292,7 @@ public virtual void NavigationCollectionChanged(
 
                         if (navigationBase is ISkipNavigation skipNavigation)
                         {
-                            var joinEntry = FindOrCreateJoinEntry(
-                                entry, newTargetEntry, skipNavigation, fromQuery: false, setModified: false);
-
-                            if (joinEntry.EntityState == EntityState.Detached)
-                            {
-                                try
-                                {
-                                    _inFixup = false;
-                                    joinEntry.SetEntityState(EntityState.Added);
-                                }
-                                finally
-                                {
-                                    _inFixup = true;
-                                }
-                            }
+                            FindOrCreateJoinEntry(entry, newTargetEntry, skipNavigation, fromQuery: false, setModified: true);
 
                             Check.DebugAssert(
                                 skipNavigation.Inverse.IsCollection,
@@ -845,21 +831,7 @@ private void InitialFixup(
                             }
                             else
                             {
-                                var joinEntry = FindOrCreateJoinEntry(
-                                    entry, otherEntry, skipNavigation, fromQuery, setModified);
-
-                                if (joinEntry.EntityState == EntityState.Detached)
-                                {
-                                    try
-                                    {
-                                        _inFixup = false;
-                                        joinEntry.SetEntityState(setModified ? EntityState.Added : EntityState.Unchanged);
-                                    }
-                                    finally
-                                    {
-                                        _inFixup = true;
-                                    }
-                                }
+                                FindOrCreateJoinEntry(entry, otherEntry, skipNavigation, fromQuery, setModified);
 
                                 Check.DebugAssert(
                                     skipNavigation.Inverse.IsCollection,
@@ -893,21 +865,11 @@ private void DelayedFixup(
                 var setModified = referencedEntry.EntityState != EntityState.Unchanged;
                 if (navigationBase is ISkipNavigation skipNavigation)
                 {
-                    var joinEntry = FindOrCreateJoinEntry(
-                        entry, referencedEntry, skipNavigation, fromQuery, setModified);
+                    FindOrCreateJoinEntry(entry, referencedEntry, skipNavigation, fromQuery, setModified);
 
-                    if (joinEntry.EntityState == EntityState.Detached)
-                    {
-                        try
-                        {
-                            _inFixup = false;
-                            joinEntry.SetEntityState(setModified ? EntityState.Added : EntityState.Unchanged);
-                        }
-                        finally
-                        {
-                            _inFixup = true;
-                        }
-                    }
+                    Check.DebugAssert(
+                        skipNavigation.Inverse.IsCollection,
+                        "Issue #21673. Non-collection skip navigations not supported.");
 
                     AddToCollection(referencedEntry, skipNavigation.Inverse, entry, fromQuery);
                 }
@@ -937,7 +899,7 @@ private void DelayedFixup(
             }
         }
 
-        private static InternalEntityEntry FindOrCreateJoinEntry(
+        private InternalEntityEntry FindOrCreateJoinEntry(
             InternalEntityEntry entry,
             InternalEntityEntry otherEntry,
             ISkipNavigation skipNavigation,
@@ -958,6 +920,25 @@ private static InternalEntityEntry FindOrCreateJoinEntry(
             SetForeignKeyProperties(joinEntry, entry, skipNavigation.ForeignKey, setModified, fromQuery);
             SetForeignKeyProperties(joinEntry, otherEntry, skipNavigation.Inverse.ForeignKey, setModified, fromQuery);
 
+            if (joinEntry.EntityState == EntityState.Detached)
+            {
+                try
+                {
+                    _inFixup = false;
+
+                    joinEntry.SetEntityState(
+                        setModified
+                        || entry.EntityState == EntityState.Added
+                        || otherEntry.EntityState == EntityState.Added
+                            ? EntityState.Added
+                            : EntityState.Unchanged);
+                }
+                finally
+                {
+                    _inFixup = true;
+                }
+            }
+
             return joinEntry;
         }
 

test/EFCore.Specification.Tests/ManyToManyTrackingTestBase.cs

@@ -2920,6 +2920,216 @@ static void ValidateFixup(DbContext context, IList<ImplicitManyToManyA> leftEnti
             }
         }
 
+        [ConditionalFact]
+        public virtual void Can_insert_many_to_many_fully_by_convention_generated_keys()
+        {
+            ExecuteWithStrategyInTransaction(
+                context =>
+                {
+                    var leftEntities = new[]
+                    {
+                        context.Set<GeneratedKeysLeft>().CreateInstance(),
+                        context.Set<GeneratedKeysLeft>().CreateInstance(),
+                        context.Set<GeneratedKeysLeft>().CreateInstance()
+                    };
+                    var rightEntities = new[]
+                    {
+                        context.Set<GeneratedKeysRight>().CreateInstance(),
+                        context.Set<GeneratedKeysRight>().CreateInstance(),
+                        context.Set<GeneratedKeysRight>().CreateInstance()
+                    };
+
+                    leftEntities[0].Rights.Add(rightEntities[0]); // 11 - 21
+                    leftEntities[0].Rights.Add(rightEntities[1]); // 11 - 22
+                    leftEntities[0].Rights.Add(rightEntities[2]); // 11 - 23
+
+                    rightEntities[0].Lefts.Add(leftEntities[0]); // 21 - 11 (Dupe)
+                    rightEntities[0].Lefts.Add(leftEntities[1]); // 21 - 12
+                    rightEntities[0].Lefts.Add(leftEntities[2]); // 21 - 13
+
+                    context.AddRange(leftEntities[0], leftEntities[1], leftEntities[2]);
+
+                    ValidateFixup(context, leftEntities, rightEntities);
+
+                    context.SaveChanges();
+
+                    ValidateFixup(context, leftEntities, rightEntities);
+                },
+                context =>
+                {
+                    var results = context.Set<GeneratedKeysLeft>().Include(e => e.Rights).ToList();
+                    Assert.Equal(3, results.Count);
+
+                    Assert.Equal(11, context.ChangeTracker.Entries().Count());
+                    Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysLeft>().Count());
+                    Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysRight>().Count());
+                    Assert.Equal(5, context.ChangeTracker.Entries<Dictionary<string, object>>().Count());
+
+                    var leftEntities = context.ChangeTracker.Entries<GeneratedKeysLeft>().Select(e => e.Entity).OrderBy(e => e.Id)
+                        .ToList();
+                    var rightEntities = context.ChangeTracker.Entries<GeneratedKeysRight>().Select(e => e.Entity).OrderBy(e => e.Id)
+                        .ToList();
+
+                    Assert.Equal(3, leftEntities[0].Rights.Count);
+                    Assert.Single(leftEntities[1].Rights);
+                    Assert.Single(leftEntities[2].Rights);
+
+                    Assert.Equal(3, rightEntities[0].Lefts.Count);
+                    Assert.Single(rightEntities[1].Lefts);
+                    Assert.Single(rightEntities[2].Lefts);
+                });
+
+            static void ValidateFixup(DbContext context, IList<GeneratedKeysLeft> leftEntities, IList<GeneratedKeysRight> rightEntities)
+            {
+                Assert.Equal(11, context.ChangeTracker.Entries().Count());
+                Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysLeft>().Count());
+                Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysRight>().Count());
+                Assert.Equal(5, context.ChangeTracker.Entries<Dictionary<string, object>>().Count());
+
+                Assert.Equal(3, leftEntities[0].Rights.Count);
+                Assert.Single(leftEntities[1].Rights);
+                Assert.Single(leftEntities[2].Rights);
+
+                Assert.Equal(3, rightEntities[0].Lefts.Count);
+                Assert.Single(rightEntities[1].Lefts);
+                Assert.Single(rightEntities[2].Lefts);
+            }
+        }
+
+        [ConditionalTheory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public virtual void Can_Attach_or_Update_a_many_to_many_with_mixed_set_and_unset_keys(bool useUpdate)
+        {
+            var existingLeftId = -1;
+            var existingRightId = -1;
+            ExecuteWithStrategyInTransaction(
+                context =>
+                {
+                    var left = context.Set<GeneratedKeysLeft>().CreateInstance();
+                    var right = context.Set<GeneratedKeysRight>().CreateInstance();
+
+                    if (!useUpdate)
+                    {
+                        left.Rights.Add(right);
+                    }
+
+                    context.AddRange(left, right);
+                    context.SaveChanges();
+
+                    existingLeftId = left.Id;
+                    existingRightId = right.Id;
+                },
+                context =>
+                {
+                    var leftEntities = new[]
+                    {
+                        context.Set<GeneratedKeysLeft>().CreateInstance((e, p) => e.Id = existingLeftId),
+                        context.Set<GeneratedKeysLeft>().CreateInstance(),
+                        context.Set<GeneratedKeysLeft>().CreateInstance()
+                    };
+                    var rightEntities = new[]
+                    {
+                        context.Set<GeneratedKeysRight>().CreateInstance((e, p) => e.Id = existingRightId),
+                        context.Set<GeneratedKeysRight>().CreateInstance(),
+                        context.Set<GeneratedKeysRight>().CreateInstance()
+                    };
+
+                    leftEntities[0].Rights.Add(rightEntities[0]); // 11 - 21
+                    leftEntities[0].Rights.Add(rightEntities[1]); // 11 - 22
+                    leftEntities[0].Rights.Add(rightEntities[2]); // 11 - 23
+
+                    rightEntities[0].Lefts.Add(leftEntities[0]); // 21 - 11 (Dupe)
+                    rightEntities[0].Lefts.Add(leftEntities[1]); // 21 - 12
+                    rightEntities[0].Lefts.Add(leftEntities[2]); // 21 - 13
+
+                    if (useUpdate)
+                    {
+                        context.Update(leftEntities[0]);
+                    }
+                    else
+                    {
+                        context.Attach(leftEntities[0]);
+                    }
+
+                    ValidateFixup(context, leftEntities, rightEntities);
+
+                    var entityEntries = context.ChangeTracker.Entries<Dictionary<string, object>>().ToList();
+                    foreach (var joinEntry in entityEntries)
+                    {
+                        Assert.Equal(
+                            !useUpdate
+                            && joinEntry.Property<int>("RightsId").CurrentValue == existingRightId
+                            && joinEntry.Property<int>("LeftsId").CurrentValue == existingLeftId
+                                ? EntityState.Unchanged
+                                : EntityState.Added, joinEntry.State);
+                    }
+
+                    foreach (var leftEntry in context.ChangeTracker.Entries<GeneratedKeysLeft>())
+                    {
+                        Assert.Equal(
+                            leftEntry.Entity.Id == existingLeftId
+                                ? useUpdate
+                                    ? EntityState.Modified
+                                    : EntityState.Unchanged
+                                : EntityState.Added, leftEntry.State);
+                    }
+
+                    foreach (var rightEntry in context.ChangeTracker.Entries<GeneratedKeysRight>())
+                    {
+                        Assert.Equal(
+                            rightEntry.Entity.Id == existingRightId
+                                ? useUpdate
+                                    ? EntityState.Modified
+                                    : EntityState.Unchanged
+                                : EntityState.Added, rightEntry.State);
+                    }
+
+                    context.SaveChanges();
+
+                    ValidateFixup(context, leftEntities, rightEntities);
+                },
+                context =>
+                {
+                    var results = context.Set<GeneratedKeysLeft>().Include(e => e.Rights).ToList();
+                    Assert.Equal(3, results.Count);
+
+                    Assert.Equal(11, context.ChangeTracker.Entries().Count());
+                    Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysLeft>().Count());
+                    Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysRight>().Count());
+                    Assert.Equal(5, context.ChangeTracker.Entries<Dictionary<string, object>>().Count());
+
+                    var leftEntities = context.ChangeTracker.Entries<GeneratedKeysLeft>().Select(e => e.Entity).OrderBy(e => e.Id)
+                        .ToList();
+                    var rightEntities = context.ChangeTracker.Entries<GeneratedKeysRight>().Select(e => e.Entity).OrderBy(e => e.Id)
+                        .ToList();
+
+                    Assert.Equal(3, leftEntities[0].Rights.Count);
+                    Assert.Single(leftEntities[1].Rights);
+                    Assert.Single(leftEntities[2].Rights);
+
+                    Assert.Equal(3, rightEntities[0].Lefts.Count);
+                    Assert.Single(rightEntities[1].Lefts);
+                    Assert.Single(rightEntities[2].Lefts);
+                });
+
+            static void ValidateFixup(DbContext context, IList<GeneratedKeysLeft> leftEntities, IList<GeneratedKeysRight> rightEntities)
+            {
+                Assert.Equal(11, context.ChangeTracker.Entries().Count());
+                Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysLeft>().Count());
+                Assert.Equal(3, context.ChangeTracker.Entries<GeneratedKeysRight>().Count());
+                Assert.Equal(5, context.ChangeTracker.Entries<Dictionary<string, object>>().Count());
+
+                Assert.Equal(3, leftEntities[0].Rights.Count);
+                Assert.Single(leftEntities[1].Rights);
+                Assert.Single(leftEntities[2].Rights);
+
+                Assert.Equal(3, rightEntities[0].Lefts.Count);
+                Assert.Single(rightEntities[1].Lefts);
+                Assert.Single(rightEntities[2].Lefts);
+            }
+        }
+
         [ConditionalFact]
         public virtual void Initial_tracking_uses_skip_navigations()
         {

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/GeneratedKeysLeft.cs

@@ -0,0 +1,16 @@
+// 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.Collections.Generic;
+using System.Collections.ObjectModel;
+
+namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel
+{
+    public class GeneratedKeysLeft
+    {
+        public virtual int Id { get; set; }
+        public virtual string Name { get; set; }
+
+        public virtual ICollection<GeneratedKeysRight> Rights { get; } = new ObservableCollection<GeneratedKeysRight>();
+    }
+}

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/GeneratedKeysRight.cs

@@ -0,0 +1,16 @@
+// 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.Collections.Generic;
+using System.Collections.ObjectModel;
+
+namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel
+{
+    public class GeneratedKeysRight
+    {
+        public virtual int Id { get; set; }
+        public virtual string Name { get; set; }
+
+        public virtual ICollection<GeneratedKeysLeft> Lefts { get; } = new ObservableCollection<GeneratedKeysLeft>();
+    }
+}

test/EFCore.Specification.Tests/TestModels/ManyToManyModel/ManyToManyContext.cs

@@ -22,21 +22,23 @@ public ManyToManyContext(DbContextOptions options)
         public DbSet<EntityRoot> EntityRoots { get; set; }
         public DbSet<ImplicitManyToManyA> ImplicitManyToManyAs { get; set; }
         public DbSet<ImplicitManyToManyB> ImplicitManyToManyBs { get; set; }
+        public DbSet<GeneratedKeysLeft> GeneratedKeysLefts { get; set; }
+        public DbSet<GeneratedKeysRight> GeneratedKeysRights { get; set; }
 
         public static void Seed(ManyToManyContext context)
             => ManyToManyData.Seed(context);
     }
 
     public static class ManyToManyContextExtensions
     {
-        public static TEntity CreateInstance<TEntity>(this DbSet<TEntity> set, Action<TEntity, bool> configureEntity)
+        public static TEntity CreateInstance<TEntity>(this DbSet<TEntity> set, Action<TEntity, bool> configureEntity = null)
             where TEntity : class, new()
         {
             var isProxy = set.GetService<IDbContextOptions>().FindExtension<ProxiesOptionsExtension>()?.UseChangeTrackingProxies == true;
 
             var entity = isProxy ? set.CreateProxy() : new TEntity();
 
-            configureEntity(entity, isProxy);
+            configureEntity?.Invoke(entity, isProxy);
 
             return entity;
         }