Don't break transactions across batches

dotnet · Oct 10, 2024 · efa76b3 · efa76b3
1 parent 9d2ce49
commit efa76b3
Show file tree

Hide file tree

Showing 9 changed files with 496 additions and 663 deletions.
diff --git a/src/EFCore.Relational/Migrations/HistoryRepository.cs b/src/EFCore.Relational/Migrations/HistoryRepository.cs
@@ -233,7 +233,7 @@ private IReadOnlyList<MigrationCommand> GetCreateIfNotExistsCommands()
     ///     Gets an exclusive lock on the database.
     /// </summary>
     /// <param name="cancellationToken">A <see cref="CancellationToken" /> to observe while waiting for the task to complete.</param>
-    /// 
+    ///
     /// <returns>An object that can be disposed to release the lock.</returns>
     /// <exception cref="OperationCanceledException">If the <see cref="CancellationToken" /> is canceled.</exception>
     public abstract Task<IMigrationsDatabaseLock> AcquireDatabaseLockAsync(CancellationToken cancellationToken = default);

diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs b/src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs
@@ -156,7 +156,7 @@ public virtual Task ExecuteNonQueryAsync(
         CancellationToken cancellationToken = default)
         => ExecuteNonQueryAsync(
             migrationCommands.ToList(), connection, new MigrationExecutionState(), commitTransaction: true, System.Data.IsolationLevel.Unspecified, cancellationToken);
-    
+
     /// <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

diff --git a/src/EFCore.Relational/Migrations/Internal/Migrator.cs b/src/EFCore.Relational/Migrations/Internal/Migrator.cs
@@ -607,7 +607,7 @@ public virtual string GenerateScript(
                 builder, _sqlGenerationHelper, ref transactionStarted, noTransactions, idempotencyCondition, idempotencyEnd);
         }
 
-        if (!noTransactions && transactionStarted)
+        if (transactionStarted)
         {
             builder
                 .AppendLine(_sqlGenerationHelper.CommitTransactionStatement)
@@ -633,8 +633,7 @@ private static void GenerateSqlScript(
                 if (!transactionStarted && !command.TransactionSuppressed)
                 {
                     builder
-                        .AppendLine(sqlGenerationHelper.StartTransactionStatement)
-                        .Append(sqlGenerationHelper.BatchTerminator);
+                        .AppendLine(sqlGenerationHelper.StartTransactionStatement);
                     transactionStarted = true;
                 }
 
@@ -663,7 +662,14 @@ private static void GenerateSqlScript(
                 builder.Append(command.CommandText);
             }
 
-            builder.Append(sqlGenerationHelper.BatchTerminator);
+            if (!transactionStarted)
+            {
+                builder.Append(sqlGenerationHelper.BatchTerminator);
+            }
+            else
+            {
+                builder .Append(Environment.NewLine);
+            }
         }
     }
 
@@ -680,10 +686,20 @@ protected virtual IReadOnlyList<MigrationCommand> GenerateUpSql(
         var insertCommand = _rawSqlCommandBuilder.Build(
             _historyRepository.GetInsertScript(new HistoryRow(migration.GetId(), ProductInfo.GetVersion())));
 
-        return _migrationsSqlGenerator
-            .Generate(migration.UpOperations, FinalizeModel(migration.TargetModel), options)
-            .Concat([new MigrationCommand(insertCommand, _currentContext.Context, _commandLogger)])
-            .ToList();
+        var operations = _migrationsSqlGenerator
+            .Generate(
+                migration.UpOperations,
+                FinalizeModel(migration.TargetModel),
+                options);
+
+        return
+        [
+            .. operations,
+            new MigrationCommand(insertCommand, _currentContext.Context, _commandLogger,
+                transactionSuppressed: operations.Any(o => o.TransactionSuppressed)),
+            // If any command was transaction-suppressed then the migrations history table is also updated without a transaction
+            // to decrease the risk that a non-recoverable exception happens during execution and the database is left in a broken state.
+        ];
     }
 
     /// <summary>
@@ -700,11 +716,19 @@ protected virtual IReadOnlyList<MigrationCommand> GenerateDownSql(
         var deleteCommand = _rawSqlCommandBuilder.Build(
             _historyRepository.GetDeleteScript(migration.GetId()));
 
-        return _migrationsSqlGenerator
+        var operations = _migrationsSqlGenerator
             .Generate(
-                migration.DownOperations, previousMigration == null ? null : FinalizeModel(previousMigration.TargetModel), options)
-            .Concat([new MigrationCommand(deleteCommand, _currentContext.Context, _commandLogger)])
-            .ToList();
+                migration.DownOperations,
+                previousMigration == null ? null : FinalizeModel(previousMigration.TargetModel),
+                options);
+
+        return [
+            .. operations,
+            new MigrationCommand(deleteCommand, _currentContext.Context, _commandLogger,
+                transactionSuppressed: operations.Any(o => o.TransactionSuppressed))
+            // If any command was transaction-suppressed then the migrations history table is also updated without a transaction
+            // to decrease the risk that a non-recoverable exception happens during execution and the database is left in a broken state.
+            ];
     }
 
     private IModel? FinalizeModel(IModel? model)