[release/7.0] Fix batching boundary with cycle breaking (#29388)

src/Shared/Multigraph.cs

@@ -218,21 +218,7 @@ public IReadOnlyList<List<TVertex>> BatchingTopologicalSort()
                         if (predecessorCounts[successor] == 0)
                         {
                             nextRootsQueue.Add(successor);
-
-                            // Detect batch boundary (if batching is enabled).
-                            // If the successor has any predecessor where the edge requires a batching boundary, and that predecessor is
-                            // already in the current batch, then the next batch will have to be executed in a separate batch.
-                            // TODO: Optimization: Instead of currentBatchSet, store a batch counter on each vertex, and check if later
-                            // vertexes have a boundary-requiring dependency on a vertex with the same batch counter.
-                            if (withBatching
-                                && _predecessorMap[successor].Any(
-                                    kv =>
-                                        (kv.Value is Edge { RequiresBatchingBoundary: true }
-                                            || kv.Value is IEnumerable<Edge> edges && edges.Any(e => e.RequiresBatchingBoundary))
-                                        && currentBatchSet.Contains(kv.Key)))
-                            {
-                                batchBoundaryRequired = true;
-                            }
+                            CheckBatchingBoundary(successor);
                         }
                     }
                 }
@@ -278,6 +264,7 @@ public IReadOnlyList<List<TVertex>> BatchingTopologicalSort()
                         if (predecessorCounts[candidateVertex] == 0)
                         {
                             currentRootsQueue.Add(candidateVertex);
+                            CheckBatchingBoundary(candidateVertex);
                             broken = true;
                         }
 
@@ -334,6 +321,24 @@ public IReadOnlyList<List<TVertex>> BatchingTopologicalSort()
         }
 
         return result;
+
+        // Detect batch boundary (if batching is enabled).
+        // If the successor has any predecessor where the edge requires a batching boundary, and that predecessor is
+        // already in the current batch, then the next batch will have to be executed in a separate batch.
+        // TODO: Optimization: Instead of currentBatchSet, store a batch counter on each vertex, and check if later
+        // vertexes have a boundary-requiring dependency on a vertex with the same batch counter.
+        void CheckBatchingBoundary(TVertex vertex)
+        {
+            if (withBatching
+                && _predecessorMap[vertex].Any(
+                    kv =>
+                        (kv.Value is Edge { RequiresBatchingBoundary: true }
+                         || kv.Value is IEnumerable<Edge> edges && edges.Any(e => e.RequiresBatchingBoundary))
+                        && currentBatchSet.Contains(kv.Key)))
+            {
+                batchBoundaryRequired = true;
+            }
+        }
     }
 
     private void ThrowCycle(

test/EFCore.Relational.Specification.Tests/Update/NonSharedModelUpdatesTestBase.cs

@@ -0,0 +1,137 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// ReSharper disable MethodHasAsyncOverload
+namespace Microsoft.EntityFrameworkCore.Update;
+
+#nullable enable
+
+public abstract class NonSharedModelUpdatesTestBase : NonSharedModelTestBase
+{
+    protected override string StoreName
+        => "NonSharedModelUpdatesTestBase";
+
+    [ConditionalTheory] // Issue #29356
+    [InlineData(false)]
+    [InlineData(true)]
+    public virtual async Task Principal_and_dependent_roundtrips_with_cycle_breaking(bool async)
+    {
+        var contextFactory = await InitializeAsync<DbContext>(
+            onModelCreating: mb =>
+            {
+                mb.Entity<Author>(
+                    b =>
+                    {
+                        b.HasOne(a => a.AuthorsClub)
+                            .WithMany()
+                            .HasForeignKey(a => a.AuthorsClubId);
+                    });
+
+                mb.Entity<Book>(
+                    b =>
+                    {
+                        b.HasOne(book => book.Author)
+                            .WithMany()
+                            .HasForeignKey(book => book.AuthorId);
+                    });
+            });
+
+        await ExecuteWithStrategyInTransactionAsync(
+            contextFactory,
+            async context =>
+            {
+                context.Add(
+                    new Book
+                    {
+                        Author = new Author
+                        {
+                            Name = "Alice",
+                            AuthorsClub = new AuthorsClub
+                            {
+                                Name = "AC South"
+                            }
+                        }
+                    });
+
+                await context.SaveChangesAsync();
+            },
+            async context =>
+            {
+                AuthorsClub authorsClubNorth = new()
+                {
+                    Name = "AC North"
+                };
+                Author authorOfTheYear2023 = new()
+                {
+                    Name = "Author of the year 2023",
+                    AuthorsClub = authorsClubNorth
+                };
+                context.Add(authorsClubNorth);
+                context.Add(authorOfTheYear2023);
+
+                var book = await context
+                    .Set<Book>()
+                    .Include(b => b.Author)
+                    .SingleAsync();
+                var authorOfTheYear2022 = book.Author!;
+                book.Author = authorOfTheYear2023;
+
+                context.Remove(authorOfTheYear2022);
+
+                if (async)
+                {
+                    await context.SaveChangesAsync();
+                }
+                else
+                {
+                    context.SaveChanges();
+                }
+            });
+    }
+
+    private class AuthorsClub
+    {
+        public int Id { get; set; }
+        public string? Name { get; set; }
+    }
+
+    private class Author
+    {
+        public int Id { get; set; }
+        public string? Name { get; set; }
+        public int AuthorsClubId { get; set; }
+        public AuthorsClub? AuthorsClub { get; set; }
+    }
+
+    private class Book
+    {
+        public int Id { get; set; }
+        public string? Title { get; set; }
+        public int AuthorId { get; set; }
+        public Author? Author { get; set; }
+    }
+
+    protected virtual void ExecuteWithStrategyInTransaction(
+        ContextFactory<DbContext> contextFactory,
+        Action<DbContext> testOperation,
+        Action<DbContext>? nestedTestOperation1 = null,
+        Action<DbContext>? nestedTestOperation2 = null,
+        Action<DbContext>? nestedTestOperation3 = null)
+        => TestHelpers.ExecuteWithStrategyInTransaction(
+            contextFactory.CreateContext, UseTransaction, testOperation, nestedTestOperation1, nestedTestOperation2, nestedTestOperation3);
+
+    protected virtual Task ExecuteWithStrategyInTransactionAsync(
+        ContextFactory<DbContext> contextFactory,
+        Func<DbContext, Task> testOperation,
+        Func<DbContext, Task>? nestedTestOperation1 = null,
+        Func<DbContext, Task>? nestedTestOperation2 = null,
+        Func<DbContext, Task>? nestedTestOperation3 = null)
+        => TestHelpers.ExecuteWithStrategyInTransactionAsync(
+            contextFactory.CreateContext, UseTransaction, testOperation, nestedTestOperation1, nestedTestOperation2, nestedTestOperation3);
+
+    public void UseTransaction(DatabaseFacade facade, IDbContextTransaction transaction)
+        => facade.UseTransaction(transaction.GetDbTransaction());
+
+    protected TestSqlLoggerFactory TestSqlLoggerFactory
+        => (TestSqlLoggerFactory)ListLoggerFactory;
+}

test/EFCore.SqlServer.FunctionalTests/Update/NonSharedModelUpdatesSqlServerTest.cs

@@ -0,0 +1,91 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Update;
+
+public class NonSharedModelUpdatesSqlServerTest : NonSharedModelUpdatesTestBase
+{
+    public override async Task Principal_and_dependent_roundtrips_with_cycle_breaking(bool async)
+    {
+        await base.Principal_and_dependent_roundtrips_with_cycle_breaking(async);
+
+        AssertSql(
+"""
+@p0='AC South' (Size = 4000)
+
+SET IMPLICIT_TRANSACTIONS OFF;
+SET NOCOUNT ON;
+INSERT INTO [AuthorsClub] ([Name])
+OUTPUT INSERTED.[Id]
+VALUES (@p0);
+""",
+        //
+"""
+@p1='1'
+@p2='Alice' (Size = 4000)
+
+SET IMPLICIT_TRANSACTIONS OFF;
+SET NOCOUNT ON;
+INSERT INTO [Author] ([AuthorsClubId], [Name])
+OUTPUT INSERTED.[Id]
+VALUES (@p1, @p2);
+""",
+        //
+"""
+@p3='1'
+@p4=NULL (Size = 4000)
+
+SET IMPLICIT_TRANSACTIONS OFF;
+SET NOCOUNT ON;
+INSERT INTO [Book] ([AuthorId], [Title])
+OUTPUT INSERTED.[Id]
+VALUES (@p3, @p4);
+""",
+        //
+"""
+SELECT TOP(2) [b].[Id], [b].[AuthorId], [b].[Title], [a].[Id], [a].[AuthorsClubId], [a].[Name]
+FROM [Book] AS [b]
+INNER JOIN [Author] AS [a] ON [b].[AuthorId] = [a].[Id]
+""",
+        //
+"""
+@p0='AC North' (Size = 4000)
+
+SET IMPLICIT_TRANSACTIONS OFF;
+SET NOCOUNT ON;
+INSERT INTO [AuthorsClub] ([Name])
+OUTPUT INSERTED.[Id]
+VALUES (@p0);
+""",
+        //
+"""
+@p1='2'
+@p2='Author of the year 2023' (Size = 4000)
+
+SET IMPLICIT_TRANSACTIONS OFF;
+SET NOCOUNT ON;
+INSERT INTO [Author] ([AuthorsClubId], [Name])
+OUTPUT INSERTED.[Id]
+VALUES (@p1, @p2);
+""",
+        //
+"""
+@p4='1'
+@p3='2'
+@p5='1'
+
+SET NOCOUNT ON;
+UPDATE [Book] SET [AuthorId] = @p3
+OUTPUT 1
+WHERE [Id] = @p4;
+DELETE FROM [Author]
+OUTPUT 1
+WHERE [Id] = @p5;
+""");
+    }
+
+    private void AssertSql(params string[] expected)
+        => TestSqlLoggerFactory.AssertBaseline(expected);
+
+    protected override ITestStoreFactory TestStoreFactory => SqlServerTestStoreFactory.Instance;
+}

test/EFCore.Sqlite.FunctionalTests/Update/NonSharedModelUpdatesSqliteTest.cs

@@ -0,0 +1,85 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Update;
+
+public class NonSharedModelUpdatesSqlServerTest : NonSharedModelUpdatesTestBase
+{
+    public override async Task Principal_and_dependent_roundtrips_with_cycle_breaking(bool async)
+    {
+        await base.Principal_and_dependent_roundtrips_with_cycle_breaking(async);
+
+        AssertSql(
+"""
+@p0='AC South' (Size = 8)
+
+INSERT INTO "AuthorsClub" ("Name")
+VALUES (@p0)
+RETURNING "Id";
+""",
+            //
+"""
+@p1='1'
+@p2='Alice' (Size = 5)
+
+INSERT INTO "Author" ("AuthorsClubId", "Name")
+VALUES (@p1, @p2)
+RETURNING "Id";
+""",
+            //
+"""
+@p3='1'
+@p4=NULL
+
+INSERT INTO "Book" ("AuthorId", "Title")
+VALUES (@p3, @p4)
+RETURNING "Id";
+""",
+            //
+"""
+SELECT "b"."Id", "b"."AuthorId", "b"."Title", "a"."Id", "a"."AuthorsClubId", "a"."Name"
+FROM "Book" AS "b"
+INNER JOIN "Author" AS "a" ON "b"."AuthorId" = "a"."Id"
+LIMIT 2
+""",
+            //
+"""
+@p0='AC North' (Size = 8)
+
+INSERT INTO "AuthorsClub" ("Name")
+VALUES (@p0)
+RETURNING "Id";
+""",
+            //
+"""
+@p1='2'
+@p2='Author of the year 2023' (Size = 23)
+
+INSERT INTO "Author" ("AuthorsClubId", "Name")
+VALUES (@p1, @p2)
+RETURNING "Id";
+""",
+            //
+"""
+@p4='1'
+@p3='2'
+
+UPDATE "Book" SET "AuthorId" = @p3
+WHERE "Id" = @p4
+RETURNING 1;
+""",
+            //
+"""
+@p0='1'
+
+DELETE FROM "Author"
+WHERE "Id" = @p0
+RETURNING 1;
+""");
+    }
+
+    private void AssertSql(params string[] expected)
+        => TestSqlLoggerFactory.AssertBaseline(expected);
+
+    protected override ITestStoreFactory TestStoreFactory => SqliteTestStoreFactory.Instance;
+}