From 499d4541a007eaf7f142942ddc8576dd7e5e3ff3 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sun, 20 Feb 2022 12:30:38 +0100 Subject: [PATCH] Stop wrapping single changes in transactions where possible Closes #27439 --- .../Properties/RelationalStrings.Designer.cs | 12 + .../Properties/RelationalStrings.resx | 6 + .../Update/IUpdateSqlGenerator.cs | 57 ++++- .../Update/Internal/BatchExecutor.cs | 46 +++- .../Update/Internal/CommandBatchPreparer.cs | 14 +- .../Update/ModificationCommandBatch.cs | 10 + .../Update/ReaderModificationCommandBatch.cs | 93 ++++++-- .../Update/UpdateSqlGenerator.cs | 30 ++- .../SqlServerMigrationsSqlGenerator.cs | 2 +- .../Internal/ISqlServerUpdateSqlGenerator.cs | 15 +- .../SqlServerModificationCommandBatch.cs | 52 +++-- .../Internal/SqlServerUpdateSqlGenerator.cs | 67 ++++-- .../SaveChangesScenariosContext.cs | 30 +++ .../WithDatabaseGenerated.cs | 11 + .../WithoutDatabaseGenerated.cs | 10 + .../Update/SaveChangesScenariosFixtureBase.cs | 31 +++ .../Update/SaveChangesScenariosTestBase.cs | 220 ++++++++++++++++++ .../DataAnnotationSqlServerTest.cs | 2 + .../OptimisticConcurrencySqlServerTest.cs | 1 + ...eteMappingInheritanceQuerySqlServerTest.cs | 3 + .../Query/InheritanceQuerySqlServerTest.cs | 3 + .../Query/QueryBugsTest.cs | 1 + .../Query/TPTInheritanceQuerySqlServerTest.cs | 8 + .../TPTTableSplittingSqlServerTest.cs | 3 + .../TableSplittingSqlServerTest.cs | 2 + ...veChangesScenariosIdentitySqlServerTest.cs | 43 ++++ ...veChangesScenariosSequenceSqlServerTest.cs | 42 ++++ .../UpdatesSqlServerTest.cs | 3 + .../Update/SaveChangesScenariosSqliteTest.cs | 26 +++ 29 files changed, 780 insertions(+), 63 deletions(-) create mode 100644 test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/SaveChangesScenariosContext.cs create mode 100644 test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithDatabaseGenerated.cs create mode 100644 test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithoutDatabaseGenerated.cs create mode 100644 test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosFixtureBase.cs create mode 100644 test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosTestBase.cs create mode 100644 test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosIdentitySqlServerTest.cs create mode 100644 test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosSequenceSqlServerTest.cs create mode 100644 test/EFCore.Sqlite.FunctionalTests/Update/SaveChangesScenariosSqliteTest.cs diff --git a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs index d891f6405b1..a96438e2c9b 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs +++ b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs @@ -843,6 +843,18 @@ public static string MissingParameterValue(object? parameter) GetString("MissingParameterValue", nameof(parameter)), parameter); + /// + /// Cannot execute an ModificationCommandBatch which hasn't been completed. + /// + public static string ModificationCommandBatchAlreadyComplete + => GetString("ModificationCommandBatchAlreadyComplete"); + + /// + /// Cannot execute an ModificationCommandBatch which hasn't been completed. + /// + public static string ModificationCommandBatchNotComplete + => GetString("ModificationCommandBatchNotCompleted"); + /// /// Cannot save changes for an entity of type '{entityType}' in state '{entityState}'. This may indicate a bug in Entity Framework, please open an issue at https://go.microsoft.com/fwlink/?linkid=2142044. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values of the entity. /// diff --git a/src/EFCore.Relational/Properties/RelationalStrings.resx b/src/EFCore.Relational/Properties/RelationalStrings.resx index 57b5e40ab57..9ba10958462 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.resx +++ b/src/EFCore.Relational/Properties/RelationalStrings.resx @@ -669,6 +669,12 @@ No value was provided for the required parameter '{parameter}'. + + Cannot add commands to a completed ModificationCommandBatch. + + + Cannot execute an ModificationCommandBatch which hasn't been completed. + Cannot save changes for an entity of type '{entityType}' in state '{entityState}'. This may indicate a bug in Entity Framework, please open an issue at https://go.microsoft.com/fwlink/?linkid=2142044. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the key values of the entity. diff --git a/src/EFCore.Relational/Update/IUpdateSqlGenerator.cs b/src/EFCore.Relational/Update/IUpdateSqlGenerator.cs index 3022c1c7e29..9fbaca6a97f 100644 --- a/src/EFCore.Relational/Update/IUpdateSqlGenerator.cs +++ b/src/EFCore.Relational/Update/IUpdateSqlGenerator.cs @@ -54,6 +54,26 @@ void AppendNextSequenceValueOperation( /// The builder to which the SQL fragment should be appended. void AppendBatchHeader(StringBuilder commandStringBuilder); + /// + /// Prepends a SQL command for turning on autocommit mode in the database, in case it is off. + /// + /// The builder to which the SQL should be prepended. + void PrependEnsureAutocommit(StringBuilder commandStringBuilder); + + /// + /// Appends a SQL command for deleting a row to the commands being built. + /// + /// The builder to which the SQL should be appended. + /// The command that represents the delete operation. + /// The ordinal of this command in the batch. + /// Returns whether the SQL appended must be executed in a transaction to work correctly. + /// The for the command. + ResultSetMapping AppendDeleteOperation( + StringBuilder commandStringBuilder, + IReadOnlyModificationCommand command, + int commandPosition, + out bool requiresTransaction); + /// /// Appends a SQL command for deleting a row to the commands being built. /// @@ -64,7 +84,22 @@ void AppendNextSequenceValueOperation( ResultSetMapping AppendDeleteOperation( StringBuilder commandStringBuilder, IReadOnlyModificationCommand command, - int commandPosition); + int commandPosition) + => AppendDeleteOperation(commandStringBuilder, command, commandPosition, out _); + + /// + /// Appends a SQL command for inserting a row to the commands being built. + /// + /// The builder to which the SQL should be appended. + /// The command that represents the delete operation. + /// The ordinal of this command in the batch. + /// Returns whether the SQL appended must be executed in a transaction to work correctly. + /// The for the command. + ResultSetMapping AppendInsertOperation( + StringBuilder commandStringBuilder, + IReadOnlyModificationCommand command, + int commandPosition, + out bool requiresTransaction); /// /// Appends a SQL command for inserting a row to the commands being built. @@ -76,7 +111,22 @@ ResultSetMapping AppendDeleteOperation( ResultSetMapping AppendInsertOperation( StringBuilder commandStringBuilder, IReadOnlyModificationCommand command, - int commandPosition); + int commandPosition) + => AppendInsertOperation(commandStringBuilder, command, commandPosition, out _); + + /// + /// Appends a SQL command for updating a row to the commands being built. + /// + /// The builder to which the SQL should be appended. + /// The command that represents the delete operation. + /// The ordinal of this command in the batch. + /// Returns whether the SQL appended must be executed in a transaction to work correctly. + /// The for the command. + ResultSetMapping AppendUpdateOperation( + StringBuilder commandStringBuilder, + IReadOnlyModificationCommand command, + int commandPosition, + out bool requiresTransaction); /// /// Appends a SQL command for updating a row to the commands being built. @@ -88,5 +138,6 @@ ResultSetMapping AppendInsertOperation( ResultSetMapping AppendUpdateOperation( StringBuilder commandStringBuilder, IReadOnlyModificationCommand command, - int commandPosition); + int commandPosition) + => AppendUpdateOperation(commandStringBuilder, command, commandPosition, out _); } diff --git a/src/EFCore.Relational/Update/Internal/BatchExecutor.cs b/src/EFCore.Relational/Update/Internal/BatchExecutor.cs index 0ac9a116e26..986b0271c94 100644 --- a/src/EFCore.Relational/Update/Internal/BatchExecutor.cs +++ b/src/EFCore.Relational/Update/Internal/BatchExecutor.cs @@ -52,6 +52,16 @@ public virtual int Execute( IEnumerable commandBatches, IRelationalConnection connection) { + using var batchEnumerator = commandBatches.GetEnumerator(); + + if (!batchEnumerator.MoveNext()) + { + return 0; + } + + var currentBatch = batchEnumerator.Current; + var nextBatch = batchEnumerator.MoveNext() ? batchEnumerator.Current : null; + var rowsAffected = 0; var transaction = connection.CurrentTransaction; var beganTransaction = false; @@ -62,7 +72,9 @@ public virtual int Execute( if (transaction == null && transactionEnlistManager?.EnlistedTransaction is null && transactionEnlistManager?.CurrentAmbientTransaction is null - && CurrentContext.Context.Database.AutoTransactionsEnabled) + && CurrentContext.Context.Database.AutoTransactionsEnabled + // Don't start a transaction if we have a single batch which doesn't require a transaction (single command), for perf. + && (nextBatch is not null || currentBatch.RequiresTransaction)) { transaction = connection.BeginTransaction(); beganTransaction = true; @@ -79,10 +91,13 @@ public virtual int Execute( } } - foreach (var batch in commandBatches) + while (currentBatch is not null) { - batch.Execute(connection); - rowsAffected += batch.ModificationCommands.Count; + currentBatch.Execute(connection); + rowsAffected += currentBatch.ModificationCommands.Count; + + currentBatch = nextBatch; + nextBatch = batchEnumerator.MoveNext() ? batchEnumerator.Current : null; } if (beganTransaction) @@ -147,6 +162,16 @@ public virtual async Task ExecuteAsync( IRelationalConnection connection, CancellationToken cancellationToken = default) { + using var batchEnumerator = commandBatches.GetEnumerator(); + + if (!batchEnumerator.MoveNext()) + { + return 0; + } + + var currentBatch = batchEnumerator.Current; + var nextBatch = batchEnumerator.MoveNext() ? batchEnumerator.Current : null; + var rowsAffected = 0; var transaction = connection.CurrentTransaction; var beganTransaction = false; @@ -157,7 +182,9 @@ public virtual async Task ExecuteAsync( if (transaction == null && transactionEnlistManager?.EnlistedTransaction is null && transactionEnlistManager?.CurrentAmbientTransaction is null - && CurrentContext.Context.Database.AutoTransactionsEnabled) + && CurrentContext.Context.Database.AutoTransactionsEnabled + // Don't start a transaction if we have a single batch which doesn't require a transaction (single command), for perf. + && (nextBatch is not null || currentBatch.RequiresTransaction)) { transaction = await connection.BeginTransactionAsync(cancellationToken).ConfigureAwait(false); beganTransaction = true; @@ -174,10 +201,13 @@ public virtual async Task ExecuteAsync( } } - foreach (var batch in commandBatches) + while (currentBatch is not null) { - await batch.ExecuteAsync(connection, cancellationToken).ConfigureAwait(false); - rowsAffected += batch.ModificationCommands.Count; + await currentBatch.ExecuteAsync(connection, cancellationToken).ConfigureAwait(false); + rowsAffected += currentBatch.ModificationCommands.Count; + + currentBatch = nextBatch; + nextBatch = batchEnumerator.MoveNext() ? batchEnumerator.Current : null; } if (beganTransaction) diff --git a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs index 099058647df..e463bcc41e6 100644 --- a/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs +++ b/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs @@ -83,6 +83,8 @@ public virtual IEnumerable BatchCommands( batch.ModificationCommands.SelectMany(c => c.Entries), batch.ModificationCommands.Count); } + batch.Complete(); + yield return batch; } else @@ -92,7 +94,10 @@ public virtual IEnumerable BatchCommands( foreach (var command in batch.ModificationCommands) { - yield return StartNewBatch(parameterNameGenerator, command); + batch = StartNewBatch(parameterNameGenerator, command); + batch.Complete(); + + yield return batch; } } @@ -109,6 +114,8 @@ public virtual IEnumerable BatchCommands( batch.ModificationCommands.SelectMany(c => c.Entries), batch.ModificationCommands.Count); } + batch.Complete(); + yield return batch; } else @@ -118,7 +125,10 @@ public virtual IEnumerable BatchCommands( foreach (var command in batch.ModificationCommands) { - yield return StartNewBatch(parameterNameGenerator, command); + batch = StartNewBatch(parameterNameGenerator, command); + batch.Complete(); + + yield return batch; } } } diff --git a/src/EFCore.Relational/Update/ModificationCommandBatch.cs b/src/EFCore.Relational/Update/ModificationCommandBatch.cs index b01c52628af..152e665be45 100644 --- a/src/EFCore.Relational/Update/ModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/ModificationCommandBatch.cs @@ -33,6 +33,16 @@ public abstract class ModificationCommandBatch /// public abstract bool AddCommand(IReadOnlyModificationCommand modificationCommand); + /// + /// Indicates that no more commands will be added to this batch, and prepares it for execution. + /// + public abstract void Complete(); + + /// + /// Indicates whether the batch requires a transaction in order to execute correctly. + /// + public abstract bool RequiresTransaction { get; } + /// /// Sends insert/update/delete commands to the database. /// diff --git a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs index b84e25a6b57..e612a388587 100644 --- a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs @@ -21,6 +21,8 @@ namespace Microsoft.EntityFrameworkCore.Update; public abstract class ReaderModificationCommandBatch : ModificationCommandBatch { private readonly List _modificationCommands = new(); + private string? _finalCommandText; + private bool _requiresTransaction = true; /// /// Creates a new instance. @@ -74,6 +76,11 @@ public override IReadOnlyList ModificationCommands /// public override bool AddCommand(IReadOnlyModificationCommand modificationCommand) { + if (_finalCommandText is not null) + { + throw new InvalidOperationException(RelationalStrings.ModificationCommandBatchAlreadyComplete); + } + if (ModificationCommands.Count == 0) { ResetCommandText(); @@ -103,15 +110,35 @@ public override bool AddCommand(IReadOnlyModificationCommand modificationCommand /// protected virtual void ResetCommandText() { - if (CachedCommandText.Length > 0) - { - CachedCommandText = new StringBuilder(); - } + CachedCommandText.Clear(); UpdateSqlGenerator.AppendBatchHeader(CachedCommandText); + _batchHeaderLength = CachedCommandText.Length; + + SetRequiresTransaction(true); + LastCachedCommandIndex = -1; } + private int _batchHeaderLength; + + /// + /// Whether any SQL has already been added to the batch command text. + /// + protected virtual bool IsCachedCommandTextEmpty + => CachedCommandText.Length == _batchHeaderLength; + + /// + public override bool RequiresTransaction + => _requiresTransaction; + + /// + /// Sets whether the batch requires a transaction in order to execute correctly. + /// + /// Whether the batch requires a transaction in order to execute correctly. + protected virtual void SetRequiresTransaction(bool requiresTransaction) + => _requiresTransaction = requiresTransaction; + /// /// Checks whether a new command can be added to the batch. /// @@ -126,18 +153,15 @@ protected virtual void ResetCommandText() protected abstract bool IsCommandTextValid(); /// - /// Gets the command text for all the commands in the current batch and also caches it - /// on . + /// Processes all unprocessed commands in the batch, making sure their corresponding SQL is populated in + /// . /// - /// The command text. - protected virtual string GetCommandText() + protected virtual void UpdateCachedCommandText() { for (var i = LastCachedCommandIndex + 1; i < ModificationCommands.Count; i++) { UpdateCachedCommandText(i); } - - return CachedCommandText.ToString(); } /// @@ -149,22 +173,35 @@ protected virtual void UpdateCachedCommandText(int commandPosition) { var newModificationCommand = ModificationCommands[commandPosition]; + bool requiresTransaction; + switch (newModificationCommand.EntityState) { case EntityState.Added: CommandResultSet[commandPosition] = - UpdateSqlGenerator.AppendInsertOperation(CachedCommandText, newModificationCommand, commandPosition); + UpdateSqlGenerator.AppendInsertOperation( + CachedCommandText, newModificationCommand, commandPosition, out requiresTransaction); break; case EntityState.Modified: CommandResultSet[commandPosition] = - UpdateSqlGenerator.AppendUpdateOperation(CachedCommandText, newModificationCommand, commandPosition); + UpdateSqlGenerator.AppendUpdateOperation( + CachedCommandText, newModificationCommand, commandPosition, out requiresTransaction); break; case EntityState.Deleted: CommandResultSet[commandPosition] = - UpdateSqlGenerator.AppendDeleteOperation(CachedCommandText, newModificationCommand, commandPosition); + UpdateSqlGenerator.AppendDeleteOperation( + CachedCommandText, newModificationCommand, commandPosition, out requiresTransaction); break; + + default: + throw new InvalidOperationException( + RelationalStrings.ModificationCommandInvalidEntityState( + newModificationCommand.Entries[0].EntityType, + newModificationCommand.EntityState)); } + _requiresTransaction = commandPosition > 0 || requiresTransaction; + LastCachedCommandIndex = commandPosition; } @@ -175,15 +212,33 @@ protected virtual void UpdateCachedCommandText(int commandPosition) protected virtual int GetParameterCount() => ModificationCommands.Sum(c => c.ColumnModifications.Count); + /// + public override void Complete() + { + UpdateCachedCommandText(); + + // Some database have a more where autocommit is off, and so executing a command outside of an explicit transaction implicitly + // creates a new transaction (which needs to be explicitly committed). + // The below is a hook for allowing providers to turn autocommit on, in case it's off. + if (!RequiresTransaction) + { + UpdateSqlGenerator.PrependEnsureAutocommit(CachedCommandText); + } + + _finalCommandText = CachedCommandText.ToString(); + } + /// /// Generates a for the batch. /// /// The command. protected virtual RawSqlCommand CreateStoreCommand() { + Check.DebugAssert(_finalCommandText is not null, "_finalCommandText is not null, checked in Execute"); + var commandBuilder = Dependencies.CommandBuilderFactory .Create() - .Append(GetCommandText()); + .Append(_finalCommandText); var parameterValues = new Dictionary(GetParameterCount()); @@ -229,6 +284,11 @@ protected virtual RawSqlCommand CreateStoreCommand() /// The connection to the database to update. public override void Execute(IRelationalConnection connection) { + if (_finalCommandText is null) + { + throw new InvalidOperationException(RelationalStrings.ModificationCommandBatchNotComplete); + } + var storeCommand = CreateStoreCommand(); try @@ -263,6 +323,11 @@ public override async Task ExecuteAsync( IRelationalConnection connection, CancellationToken cancellationToken = default) { + if (_finalCommandText is null) + { + throw new InvalidOperationException(RelationalStrings.ModificationCommandBatchNotComplete); + } + var storeCommand = CreateStoreCommand(); try diff --git a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs index f18c7f3fc3b..f1768588955 100644 --- a/src/EFCore.Relational/Update/UpdateSqlGenerator.cs +++ b/src/EFCore.Relational/Update/UpdateSqlGenerator.cs @@ -53,11 +53,13 @@ protected virtual ISqlGenerationHelper SqlGenerationHelper /// The builder to which the SQL should be appended. /// The command that represents the delete operation. /// The ordinal of this command in the batch. + /// Returns whether the SQL appended must be executed in a transaction to work correctly. /// The for the command. public virtual ResultSetMapping AppendInsertOperation( StringBuilder commandStringBuilder, IReadOnlyModificationCommand command, - int commandPosition) + int commandPosition, + out bool requiresTransaction) { var name = command.TableName; var schema = command.Schema; @@ -72,9 +74,13 @@ public virtual ResultSetMapping AppendInsertOperation( { var keyOperations = operations.Where(o => o.IsKey).ToList(); + requiresTransaction = true; + return AppendSelectAffectedCommand(commandStringBuilder, name, schema, readOperations, keyOperations, commandPosition); } + requiresTransaction = false; + return ResultSetMapping.NoResultSet; } @@ -84,11 +90,13 @@ public virtual ResultSetMapping AppendInsertOperation( /// The builder to which the SQL should be appended. /// The command that represents the delete operation. /// The ordinal of this command in the batch. + /// Returns whether the SQL appended must be executed in a transaction to work correctly. /// The for the command. public virtual ResultSetMapping AppendUpdateOperation( StringBuilder commandStringBuilder, IReadOnlyModificationCommand command, - int commandPosition) + int commandPosition, + out bool requiresTransaction) { var name = command.TableName; var schema = command.Schema; @@ -104,9 +112,13 @@ public virtual ResultSetMapping AppendUpdateOperation( { var keyOperations = operations.Where(o => o.IsKey).ToList(); + requiresTransaction = true; + return AppendSelectAffectedCommand(commandStringBuilder, name, schema, readOperations, keyOperations, commandPosition); } + requiresTransaction = false; + return AppendSelectAffectedCountCommand(commandStringBuilder, name, schema, commandPosition); } @@ -116,11 +128,13 @@ public virtual ResultSetMapping AppendUpdateOperation( /// The builder to which the SQL should be appended. /// The command that represents the delete operation. /// The ordinal of this command in the batch. + /// Returns whether the SQL appended must be executed in a transaction to work correctly. /// The for the command. public virtual ResultSetMapping AppendDeleteOperation( StringBuilder commandStringBuilder, IReadOnlyModificationCommand command, - int commandPosition) + int commandPosition, + out bool requiresTransaction) { var name = command.TableName; var schema = command.Schema; @@ -128,6 +142,8 @@ public virtual ResultSetMapping AppendDeleteOperation( AppendDeleteCommand(commandStringBuilder, name, schema, conditionOperations); + requiresTransaction = false; + return AppendSelectAffectedCountCommand(commandStringBuilder, name, schema, commandPosition); } @@ -530,6 +546,14 @@ public virtual void AppendBatchHeader(StringBuilder commandStringBuilder) { } + /// + /// Prepends a SQL command for turning on autocommit mode in the database, in case it is off. + /// + /// The builder to which the SQL should be prepended. + public virtual void PrependEnsureAutocommit(StringBuilder commandStringBuilder) + { + } + /// /// Generates SQL that will obtain the next value in the given sequence. /// diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 209e9261080..d63a68ce451 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1377,7 +1377,7 @@ protected override void Generate( GenerateIdentityInsert(builder, operation, on: true, model); var sqlBuilder = new StringBuilder(); - ((SqlServerUpdateSqlGenerator)Dependencies.UpdateSqlGenerator).AppendBulkInsertOperation( + ((ISqlServerUpdateSqlGenerator)Dependencies.UpdateSqlGenerator).AppendBulkInsertOperation( sqlBuilder, GenerateModificationCommands(operation, model).ToList(), 0); diff --git a/src/EFCore.SqlServer/Update/Internal/ISqlServerUpdateSqlGenerator.cs b/src/EFCore.SqlServer/Update/Internal/ISqlServerUpdateSqlGenerator.cs index 4099dd73dec..c7f92fb002a 100644 --- a/src/EFCore.SqlServer/Update/Internal/ISqlServerUpdateSqlGenerator.cs +++ b/src/EFCore.SqlServer/Update/Internal/ISqlServerUpdateSqlGenerator.cs @@ -27,5 +27,18 @@ public interface ISqlServerUpdateSqlGenerator : IUpdateSqlGenerator ResultSetMapping AppendBulkInsertOperation( StringBuilder commandStringBuilder, IReadOnlyList modificationCommands, - int commandPosition); + int commandPosition, + out bool requiresTransaction); + + /// + /// 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. + /// + ResultSetMapping AppendBulkInsertOperation( + StringBuilder commandStringBuilder, + IReadOnlyList modificationCommands, + int commandPosition) + => AppendBulkInsertOperation(commandStringBuilder, modificationCommands, commandPosition, out _); } diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs index c775d2b83f0..18e4ee6d501 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs +++ b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs @@ -85,7 +85,8 @@ protected override bool IsCommandTextValid() { if (--_commandsLeftToLengthCheck < 0) { - var commandTextLength = GetCommandText().Length; + UpdateCachedCommandText(); + var commandTextLength = CachedCommandText.Length; if (commandTextLength >= MaxScriptLength) { return false; @@ -138,28 +139,24 @@ private static int CountParameters(IReadOnlyModificationCommand modificationComm protected override void ResetCommandText() { base.ResetCommandText(); + _bulkInsertCommands.Clear(); } - /// - /// 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. - /// - protected override string GetCommandText() - => base.GetCommandText() + GetBulkInsertCommandText(ModificationCommands.Count); - - private string GetBulkInsertCommandText(int lastIndex) + private void AppendBulkInsertCommandText(int lastIndex) { if (_bulkInsertCommands.Count == 0) { - return string.Empty; + return; } - var stringBuilder = new StringBuilder(); + var wasCachedCommandTextEmpty = IsCachedCommandTextEmpty; + var resultSetMapping = UpdateSqlGenerator.AppendBulkInsertOperation( - stringBuilder, _bulkInsertCommands, lastIndex - _bulkInsertCommands.Count); + CachedCommandText, _bulkInsertCommands, lastIndex - _bulkInsertCommands.Count, out var requiresTransaction); + + SetRequiresTransaction(!wasCachedCommandTextEmpty || requiresTransaction); + for (var i = lastIndex - _bulkInsertCommands.Count; i < lastIndex; i++) { CommandResultSet[i] = resultSetMapping; @@ -169,8 +166,19 @@ private string GetBulkInsertCommandText(int lastIndex) { CommandResultSet[lastIndex - 1] = ResultSetMapping.LastInResultSet; } + } - return stringBuilder.ToString(); + /// + /// 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. + /// + protected override void UpdateCachedCommandText() + { + base.UpdateCachedCommandText(); + + AppendBulkInsertCommandText(ModificationCommands.Count); } /// @@ -188,7 +196,9 @@ protected override void UpdateCachedCommandText(int commandPosition) if (_bulkInsertCommands.Count > 0 && !CanBeInsertedInSameStatement(_bulkInsertCommands[0], newModificationCommand)) { - CachedCommandText.Append(GetBulkInsertCommandText(commandPosition)); + // The new Add command cannot be added to the pending bulk insert commands (e.g. different table). + // Write out the pending commands before starting a new pending chain. + AppendBulkInsertCommandText(commandPosition); _bulkInsertCommands.Clear(); } @@ -198,8 +208,14 @@ protected override void UpdateCachedCommandText(int commandPosition) } else { - CachedCommandText.Append(GetBulkInsertCommandText(commandPosition)); - _bulkInsertCommands.Clear(); + // If we have any pending bulk insert commands, write them out before the next non-Add command + if (_bulkInsertCommands.Count > 0) + { + // Note that we don't care about the transactionality of the bulk insert SQL, since there's the additional non-Add + // command coming right afterwards, and so a transaction is required in any case. + AppendBulkInsertCommandText(commandPosition); + _bulkInsertCommands.Clear(); + } base.UpdateCachedCommandText(commandPosition); } diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs index 709e7558e76..d746b5762cc 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs +++ b/src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs @@ -35,7 +35,8 @@ public SqlServerUpdateSqlGenerator( public virtual ResultSetMapping AppendBulkInsertOperation( StringBuilder commandStringBuilder, IReadOnlyList modificationCommands, - int commandPosition) + int commandPosition, + out bool requiresTransaction) { var table = StoreObjectIdentifier.Table(modificationCommands[0].TableName, modificationCommands[0].Schema); if (modificationCommands.Count == 1) @@ -45,13 +46,16 @@ public virtual ResultSetMapping AppendBulkInsertOperation( !o.IsKey || !o.IsRead || o.Property?.GetValueGenerationStrategy(table) == SqlServerValueGenerationStrategy.IdentityColumn) - ? AppendInsertOperation(commandStringBuilder, modificationCommands[0], commandPosition) + // Do a regular INSERT+SELECT for IDENTITY, but not if there are any non-IDENTITY generated columns + ? AppendInsertOperation(commandStringBuilder, modificationCommands[0], commandPosition, out requiresTransaction) + // If we have a non-identity generated column, do INSERT ... OUTPUT INTO @inserted; SELECT ... FROM @inserted : AppendInsertOperationWithServerKeys( commandStringBuilder, modificationCommands[0], modificationCommands[0].ColumnModifications.Where(o => o.IsKey).ToList(), modificationCommands[0].ColumnModifications.Where(o => o.IsRead).ToList(), - commandPosition); + commandPosition, + out requiresTransaction); } var readOperations = modificationCommands[0].ColumnModifications.Where(o => o.IsRead).ToList(); @@ -68,9 +72,11 @@ public virtual ResultSetMapping AppendBulkInsertOperation( if (nonIdentityOperations.Count == 0 || readOperations.Count == 0) { + requiresTransaction = false; foreach (var modification in modificationCommands) { - AppendInsertOperation(commandStringBuilder, modification, commandPosition); + AppendInsertOperation(commandStringBuilder, modification, commandPosition, out var localRequiresTransaction); + requiresTransaction = requiresTransaction || localRequiresTransaction; } return readOperations.Count == 0 @@ -86,23 +92,30 @@ public virtual ResultSetMapping AppendBulkInsertOperation( if (readOperations.Count == 0) { - return AppendBulkInsertWithoutServerValues(commandStringBuilder, modificationCommands, writeOperations); + return AppendBulkInsertWithoutServerValues( + commandStringBuilder, modificationCommands, writeOperations, out requiresTransaction); } if (defaultValuesOnly) { + requiresTransaction = true; + return AppendBulkInsertWithServerValuesOnly( - commandStringBuilder, modificationCommands, commandPosition, nonIdentityOperations, keyOperations, readOperations); + commandStringBuilder, modificationCommands, commandPosition, nonIdentityOperations, keyOperations, readOperations, + out requiresTransaction); } if (modificationCommands[0].Entries.SelectMany(e => e.EntityType.GetAllBaseTypesInclusive()) .Any(e => e.IsMemoryOptimized())) { + requiresTransaction = false; + if (!nonIdentityOperations.Any(o => o.IsRead && o.IsKey)) { foreach (var modification in modificationCommands) { - AppendInsertOperation(commandStringBuilder, modification, commandPosition++); + AppendInsertOperation(commandStringBuilder, modification, commandPosition++, out var localRequiresTransaction); + requiresTransaction = requiresTransaction || localRequiresTransaction; } } else @@ -110,7 +123,9 @@ public virtual ResultSetMapping AppendBulkInsertOperation( foreach (var modification in modificationCommands) { AppendInsertOperationWithServerKeys( - commandStringBuilder, modification, keyOperations, readOperations, commandPosition++); + commandStringBuilder, modification, keyOperations, readOperations, commandPosition++, + out var localRequiresTransaction); + requiresTransaction = requiresTransaction || localRequiresTransaction; } } @@ -118,13 +133,15 @@ public virtual ResultSetMapping AppendBulkInsertOperation( } return AppendBulkInsertWithServerValues( - commandStringBuilder, modificationCommands, commandPosition, writeOperations, keyOperations, readOperations); + commandStringBuilder, modificationCommands, commandPosition, writeOperations, keyOperations, readOperations, + out requiresTransaction); } private ResultSetMapping AppendBulkInsertWithoutServerValues( StringBuilder commandStringBuilder, IReadOnlyList modificationCommands, - List writeOperations) + List writeOperations, + out bool requiresTransaction) { Check.DebugAssert(writeOperations.Count > 0, $"writeOperations.Count is {writeOperations.Count}"); @@ -143,6 +160,8 @@ private ResultSetMapping AppendBulkInsertWithoutServerValues( commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator); + requiresTransaction = false; + return ResultSetMapping.NoResultSet; } @@ -158,7 +177,8 @@ private ResultSetMapping AppendBulkInsertWithServerValues( int commandPosition, List writeOperations, List keyOperations, - List readOperations) + List readOperations, + out bool requiresTransaction) { AppendDeclareTable( commandStringBuilder, @@ -190,6 +210,8 @@ private ResultSetMapping AppendBulkInsertWithServerValues( commandStringBuilder, readOperations, keyOperations, InsertedTableBaseName, commandPosition, name, schema, orderColumn: PositionColumnName); + requiresTransaction = false; + return ResultSetMapping.NotLastInResultSet; } @@ -199,7 +221,8 @@ private ResultSetMapping AppendBulkInsertWithServerValuesOnly( int commandPosition, List nonIdentityOperations, List keyOperations, - List readOperations) + List readOperations, + out bool requiresTransaction) { AppendDeclareTable(commandStringBuilder, InsertedTableBaseName, commandPosition, keyOperations); @@ -219,6 +242,8 @@ private ResultSetMapping AppendBulkInsertWithServerValuesOnly( AppendSelectCommand(commandStringBuilder, readOperations, keyOperations, InsertedTableBaseName, commandPosition, name, schema); + requiresTransaction = false; + return ResultSetMapping.NotLastInResultSet; } @@ -399,7 +424,8 @@ private ResultSetMapping AppendInsertOperationWithServerKeys( IReadOnlyModificationCommand command, IReadOnlyList keyOperations, IReadOnlyList readOperations, - int commandPosition) + int commandPosition, + out bool requiresTransaction) { var name = command.TableName; var schema = command.Schema; @@ -415,6 +441,8 @@ private ResultSetMapping AppendInsertOperationWithServerKeys( AppendValues(commandStringBuilder, name, schema, writeOperations); commandStringBuilder.Append(SqlGenerationHelper.StatementTerminator); + requiresTransaction = true; + return AppendSelectCommand( commandStringBuilder, readOperations, keyOperations, InsertedTableBaseName, commandPosition, name, schema); } @@ -515,6 +543,19 @@ public override void AppendBatchHeader(StringBuilder commandStringBuilder) .Append("SET NOCOUNT ON") .AppendLine(SqlGenerationHelper.StatementTerminator); + /// + /// 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. + /// + public override void PrependEnsureAutocommit(StringBuilder commandStringBuilder) + { + // SQL Server allows turning off autocommit via the IMPLICIT_TRANSACTIONS setting (see + // https://docs.microsoft.com/sql/t-sql/statements/set-implicit-transactions-transact-sql). + commandStringBuilder.Insert(0, $"SET IMPLICIT_TRANSACTIONS OFF{SqlGenerationHelper.StatementTerminator}{Environment.NewLine}"); + } + /// /// 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/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/SaveChangesScenariosContext.cs b/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/SaveChangesScenariosContext.cs new file mode 100644 index 00000000000..a38a42c016f --- /dev/null +++ b/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/SaveChangesScenariosContext.cs @@ -0,0 +1,30 @@ +// 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.TestModels.SaveChangesTransactionModel; + +public class SaveChangesScenariosContext : PoolableDbContext +{ + public SaveChangesScenariosContext(DbContextOptions options) + : base(options) + { + } + + public DbSet WithDatabaseGenerated { get; set; } + public DbSet WithoutDatabaseGenerated { get; set; } + + public void Reseed() + { + RemoveRange(WithDatabaseGenerated); + RemoveRange(WithoutDatabaseGenerated); + + SaveChanges(); + + ChangeTracker.Clear(); + + WithDatabaseGenerated.AddRange(new() { Data = 1 }, new() { Data = 2 }); + WithoutDatabaseGenerated.AddRange(new() { Id = 1, Data = 1 }, new() { Id = 2, Data = 2 }); + + SaveChanges(); + } +} diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithDatabaseGenerated.cs b/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithDatabaseGenerated.cs new file mode 100644 index 00000000000..48183d1a899 --- /dev/null +++ b/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithDatabaseGenerated.cs @@ -0,0 +1,11 @@ +// 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.TestModels.SaveChangesTransactionModel; + +public class WithDatabaseGenerated +{ + public int Id { get; set; } + public int Data { get; set; } + public int GeneratedOnUpdate { get; } +} diff --git a/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithoutDatabaseGenerated.cs b/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithoutDatabaseGenerated.cs new file mode 100644 index 00000000000..f5a48c36181 --- /dev/null +++ b/test/EFCore.Relational.Specification.Tests/TestModels/SaveChangesTransactionModel/WithoutDatabaseGenerated.cs @@ -0,0 +1,10 @@ +// 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.TestModels.SaveChangesTransactionModel; + +public class WithoutDatabaseGenerated +{ + public int Id { get; set; } + public int Data { get; set; } +} diff --git a/test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosFixtureBase.cs b/test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosFixtureBase.cs new file mode 100644 index 00000000000..7145ba0eedc --- /dev/null +++ b/test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosFixtureBase.cs @@ -0,0 +1,31 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.EntityFrameworkCore.TestModels.SaveChangesTransactionModel; + +namespace Microsoft.EntityFrameworkCore.Update; + +public abstract class SaveChangesScenariosFixtureBase : SharedStoreFixtureBase +{ + protected override string StoreName { get; } = "SaveChangesScenariosTest"; + + protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context) + { + modelBuilder + .Entity() + .Property(w => w.Id) + .ValueGeneratedNever(); + + modelBuilder + .Entity() + .Property(w => w.GeneratedOnUpdate) + .HasComputedColumnSql("80"); + } + + protected override bool ShouldLogCategory(string logCategory) + => logCategory == DbLoggerCategory.Database.Transaction.Name + || logCategory == DbLoggerCategory.Database.Command.Name; + + public TestSqlLoggerFactory TestSqlLoggerFactory + => (TestSqlLoggerFactory)ListLoggerFactory; +} diff --git a/test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosTestBase.cs b/test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosTestBase.cs new file mode 100644 index 00000000000..c90691e9cc2 --- /dev/null +++ b/test/EFCore.Relational.Specification.Tests/Update/SaveChangesScenariosTestBase.cs @@ -0,0 +1,220 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.EntityFrameworkCore.TestModels.SaveChangesTransactionModel; + +namespace Microsoft.EntityFrameworkCore.Update; + +public abstract class SaveChangesScenariosTestBase : IClassFixture + where TFixture : SaveChangesScenariosFixtureBase +{ + protected SaveChangesScenariosTestBase(TFixture fixture) + { + Fixture = fixture; + + using var context = CreateContext(); + context.Reseed(); + } + + [ConditionalTheory] + [InlineData(EntityState.Added, true)] + [InlineData(EntityState.Added, false)] + [InlineData(EntityState.Modified, true)] + [InlineData(EntityState.Modified, false)] + [InlineData(EntityState.Deleted, true)] + [InlineData(EntityState.Deleted, false)] + public virtual Task Single_change_with_generated_values(EntityState changeType, bool async) + => Test(changeType, secondOperationType: null, withGeneratedValues: true, async); + + [ConditionalTheory] + [InlineData(EntityState.Added, true)] + [InlineData(EntityState.Added, false)] + [InlineData(EntityState.Modified, true)] + [InlineData(EntityState.Modified, false)] + [InlineData(EntityState.Deleted, true)] + [InlineData(EntityState.Deleted, false)] + public virtual Task Single_change_without_generated_values(EntityState changeType, bool async) + => Test(changeType, secondOperationType: null, withGeneratedValues: false, async); + + [ConditionalTheory] + [InlineData(EntityState.Added, EntityState.Added, true)] + [InlineData(EntityState.Added, EntityState.Added, false)] + [InlineData(EntityState.Modified, EntityState.Modified, true)] + [InlineData(EntityState.Modified, EntityState.Modified, false)] + [InlineData(EntityState.Deleted, EntityState.Deleted, true)] + [InlineData(EntityState.Deleted, EntityState.Deleted, false)] + public virtual Task Two_changes_with_same_entity_type_and_with_key_generation( + EntityState firstOperationType, + EntityState secondOperationType, + bool async) + => Test(firstOperationType, secondOperationType, withGeneratedValues: true, async, withSameEntityType: true); + + [ConditionalTheory] + [InlineData(EntityState.Added, EntityState.Added, true)] + [InlineData(EntityState.Added, EntityState.Added, false)] + [InlineData(EntityState.Modified, EntityState.Modified, true)] + [InlineData(EntityState.Modified, EntityState.Modified, false)] + [InlineData(EntityState.Deleted, EntityState.Deleted, true)] + [InlineData(EntityState.Deleted, EntityState.Deleted, false)] + public virtual Task Two_changes_with_same_entity_type_and_without_key_generation( + EntityState firstOperationType, + EntityState secondOperationType, + bool async) + => Test(firstOperationType, secondOperationType, withGeneratedValues: false, async, withSameEntityType: true); + + [ConditionalTheory] + [InlineData(EntityState.Added, EntityState.Added, true)] + [InlineData(EntityState.Added, EntityState.Added, false)] + [InlineData(EntityState.Modified, EntityState.Modified, true)] + [InlineData(EntityState.Modified, EntityState.Modified, false)] + [InlineData(EntityState.Deleted, EntityState.Deleted, true)] + [InlineData(EntityState.Deleted, EntityState.Deleted, false)] + public virtual Task Two_changes_with_different_entity_types_and_with_key_generation( + EntityState firstOperationType, + EntityState secondOperationType, + bool async) + => Test(firstOperationType, secondOperationType, withGeneratedValues: true, async, withSameEntityType: false); + + [ConditionalTheory] + [InlineData(EntityState.Added, EntityState.Added, true)] + [InlineData(EntityState.Added, EntityState.Added, false)] + [InlineData(EntityState.Modified, EntityState.Modified, true)] + [InlineData(EntityState.Modified, EntityState.Modified, false)] + [InlineData(EntityState.Deleted, EntityState.Deleted, true)] + [InlineData(EntityState.Deleted, EntityState.Deleted, false)] + public virtual Task Two_changes_with_different_entity_types_and_without_key_generation( + EntityState firstOperationType, + EntityState secondOperationType, + bool async) + => Test(firstOperationType, secondOperationType, withGeneratedValues: false, async, withSameEntityType: false); + + protected virtual async Task Test( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withGeneratedValues, + bool async, + bool withSameEntityType = true) + { + using var context = CreateContext(); + + switch (firstOperationType) + { + case EntityState.Added: + context.Add(withGeneratedValues ? new WithDatabaseGenerated() : new WithoutDatabaseGenerated { Id = 100 }); + break; + + case EntityState.Modified: + if (withGeneratedValues) + { + context.WithDatabaseGenerated.OrderBy(w => w.Id).First().Data = 10; + } + else + { + context.WithoutDatabaseGenerated.OrderBy(w => w.Id).First().Data = 10; + } + break; + + case EntityState.Deleted: + context.Remove( + withGeneratedValues + ? context.WithDatabaseGenerated.OrderBy(w => w.Id).First() + : context.WithoutDatabaseGenerated.OrderBy(w => w.Id).First()); + break; + + default: + throw new ArgumentOutOfRangeException(nameof(firstOperationType)); + } + + switch (secondOperationType) + { + case EntityState.Added: + context.Add(withGeneratedValues ? new WithDatabaseGenerated() : new WithoutDatabaseGenerated { Id = 101 }); + break; + + case EntityState.Modified: + if (withGeneratedValues) + { + context.WithDatabaseGenerated.OrderBy(w => w.Id).Skip(1).First().Data = 11; + } + else + { + context.WithoutDatabaseGenerated.OrderBy(w => w.Id).Skip(1).First().Data = 11; + } + break; + + case EntityState.Deleted: + context.Remove( + withGeneratedValues + ? context.WithDatabaseGenerated.OrderBy(w => w.Id).Skip(1).First() + : context.WithoutDatabaseGenerated.OrderBy(w => w.Id).Skip(1).First()); + break; + + case null: + break; + + default: + throw new ArgumentOutOfRangeException(nameof(firstOperationType)); + } + + Fixture.ListLoggerFactory.Clear(); + + if (async) + { + await context.SaveChangesAsync(); + } + else + { + context.SaveChanges(); + } + + if (ShouldCreateImplicitTransaction(firstOperationType, secondOperationType, withGeneratedValues, withSameEntityType)) + { + Assert.Contains(Fixture.ListLoggerFactory.Log, l => l.Id == RelationalEventId.TransactionStarted); + Assert.Contains(Fixture.ListLoggerFactory.Log, l => l.Id == RelationalEventId.TransactionCommitted); + } + else + { + Assert.DoesNotContain(Fixture.ListLoggerFactory.Log, l => l.Id == RelationalEventId.TransactionStarted); + Assert.DoesNotContain(Fixture.ListLoggerFactory.Log, l => l.Id == RelationalEventId.TransactionCommitted); + } + + Assert.Equal( + ShouldExecuteInNumberOfCommands(firstOperationType, secondOperationType, withGeneratedValues, withSameEntityType), + Fixture.ListLoggerFactory.Log.Count(l => l.Id == RelationalEventId.CommandExecuted)); + } + + // Providers can override this to specify when SaveChanges should create a transaction, and when not + protected virtual bool ShouldCreateImplicitTransaction( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withDatabaseGenerated, + bool withSameEntityType) + { + // By default, two changes require a transaction + if (secondOperationType is not null) + { + return true; + } + + // Deletes don't ever need to bring back database-generated values + if (firstOperationType == EntityState.Deleted) + { + return false; + } + + // By default, assume that fetching back database-generated values requires a transaction + return withDatabaseGenerated; + } + + protected virtual int ShouldExecuteInNumberOfCommands( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withDatabaseGenerated, + bool withSameEntityType) + => 1; + + protected TFixture Fixture { get; } + + protected SaveChangesScenariosContext CreateContext() + => Fixture.CreateContext(); +} diff --git a/test/EFCore.SqlServer.FunctionalTests/DataAnnotationSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/DataAnnotationSqlServerTest.cs index 5338e33bbb9..b2c5db89b7b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/DataAnnotationSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/DataAnnotationSqlServerTest.cs @@ -164,6 +164,7 @@ FROM [Sample] AS [s] @p1='00000000-0000-0000-0003-000000000001' @p3='00000001-0000-0000-0000-000000000001' +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Sample] SET [Name] = @p0, [RowVersion] = @p1 WHERE [Unique_No] = @p2 AND [RowVersion] = @p3; @@ -174,6 +175,7 @@ FROM [Sample] AS [s] @p1='00000000-0000-0000-0002-000000000001' @p3='00000001-0000-0000-0000-000000000001' +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Sample] SET [Name] = @p0, [RowVersion] = @p1 WHERE [Unique_No] = @p2 AND [RowVersion] = @p3; diff --git a/test/EFCore.SqlServer.FunctionalTests/OptimisticConcurrencySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/OptimisticConcurrencySqlServerTest.cs index bc860720142..469a2b2120b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/OptimisticConcurrencySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/OptimisticConcurrencySqlServerTest.cs @@ -166,6 +166,7 @@ FROM [Engines] AS [e] @p4='47.64491' (Nullable = true) @p5='-122.128101' (Nullable = true) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Engines] SET [Name] = @p0 WHERE [Id] = @p1 AND [EngineSupplierId] = @p2 AND [Name] = @p3 AND [StorageLocation_Latitude] = @p4 AND [StorageLocation_Longitude] = @p5; diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/IncompleteMappingInheritanceQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/IncompleteMappingInheritanceQuerySqlServerTest.cs index 26b289461a3..85a40748b6c 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/IncompleteMappingInheritanceQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/IncompleteMappingInheritanceQuerySqlServerTest.cs @@ -389,6 +389,7 @@ FROM [Countries] AS [c] @p5='True' (Nullable = true) @p6='Little spotted kiwi' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Animals] ([Species], [CountryId], [Discriminator], [EagleId], [FoundOn], [IsFlightless], [Name]) VALUES (@p0, @p1, @p2, @p3, @p4, @p5, @p6);", @@ -400,6 +401,7 @@ FROM [Animals] AS [a] @"@p1='Apteryx owenii' (Nullable = false) (Size = 100) @p0='Aquila chrysaetos canadensis' (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Animals] SET [EagleId] = @p0 WHERE [Species] = @p1; @@ -411,6 +413,7 @@ FROM [Animals] AS [a] // @"@p0='Apteryx owenii' (Nullable = false) (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DELETE FROM [Animals] WHERE [Species] = @p0; diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/InheritanceQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/InheritanceQuerySqlServerTest.cs index 155f1612578..3f58c6ef539 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/InheritanceQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/InheritanceQuerySqlServerTest.cs @@ -363,6 +363,7 @@ FROM [Countries] AS [c] @p5='True' (Nullable = true) @p6='Little spotted kiwi' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Animals] ([Species], [CountryId], [Discriminator], [EagleId], [FoundOn], [IsFlightless], [Name]) VALUES (@p0, @p1, @p2, @p3, @p4, @p5, @p6);", @@ -374,6 +375,7 @@ FROM [Animals] AS [a] @"@p1='Apteryx owenii' (Nullable = false) (Size = 100) @p0='Aquila chrysaetos canadensis' (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Animals] SET [EagleId] = @p0 WHERE [Species] = @p1; @@ -385,6 +387,7 @@ FROM [Animals] AS [a] // @"@p0='Apteryx owenii' (Nullable = false) (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DELETE FROM [Animals] WHERE [Species] = @p0; diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs index e1c16adb004..105d8f29ff1 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs @@ -9134,6 +9134,7 @@ public virtual async Task Batch_insert_with_sqlvariant_different_types_12482() @p2='String Value' (Size = 12) (DbType = Object) @p3='2020-01-01T00:00:00.0000000' (Nullable = true) (DbType = Object) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DECLARE @inserted0 TABLE ([Id] int, [_Position] [int]); MERGE [BaseEntities] USING ( diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/TPTInheritanceQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/TPTInheritanceQuerySqlServerTest.cs index dee1a0b6105..6c45fc18098 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/TPTInheritanceQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/TPTInheritanceQuerySqlServerTest.cs @@ -102,6 +102,7 @@ FROM [Countries] AS [c] @p1='1' @p2='Little spotted kiwi' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Animals] ([Species], [CountryId], [Name]) VALUES (@p0, @p1, @p2);", @@ -110,6 +111,7 @@ INSERT INTO [Animals] ([Species], [CountryId], [Name]) @p4=NULL (Size = 100) @p5='True' +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Birds] ([Species], [EagleId], [IsFlightless]) VALUES (@p3, @p4, @p5);", @@ -117,6 +119,7 @@ INSERT INTO [Birds] ([Species], [EagleId], [IsFlightless]) @"@p6='Apteryx owenii' (Nullable = false) (Size = 100) @p7='0' (Size = 1) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Kiwi] ([Species], [FoundOn]) VALUES (@p6, @p7);", @@ -130,6 +133,7 @@ FROM [Animals] AS [a] @"@p1='Apteryx owenii' (Nullable = false) (Size = 100) @p0='Aquila chrysaetos canadensis' (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Birds] SET [EagleId] = @p0 WHERE [Species] = @p1; @@ -143,6 +147,7 @@ FROM [Animals] AS [a] // @"@p0='Apteryx owenii' (Nullable = false) (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DELETE FROM [Kiwi] WHERE [Species] = @p0; @@ -150,6 +155,7 @@ DELETE FROM [Kiwi] // @"@p1='Apteryx owenii' (Nullable = false) (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DELETE FROM [Birds] WHERE [Species] = @p1; @@ -157,6 +163,7 @@ DELETE FROM [Birds] // @"@p2='Apteryx owenii' (Nullable = false) (Size = 100) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DELETE FROM [Animals] WHERE [Species] = @p2; @@ -527,6 +534,7 @@ FROM [Animals] AS [a] @p1='0' @p2='Bald eagle' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Animals] ([Species], [CountryId], [Name]) VALUES (@p0, @p1, @p2);"); diff --git a/test/EFCore.SqlServer.FunctionalTests/TPTTableSplittingSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/TPTTableSplittingSqlServerTest.cs index 0181ac8adef..4f9bbb9011a 100644 --- a/test/EFCore.SqlServer.FunctionalTests/TPTTableSplittingSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/TPTTableSplittingSqlServerTest.cs @@ -187,6 +187,7 @@ public override async Task Can_change_dependent_instance_non_derived() @"@p1='Trek Pro Fit Madone 6 Series' (Nullable = false) (Size = 450) @p0='repairman' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Vehicles] SET [Operator_Name] = @p0 WHERE [Name] = @p1; @@ -195,6 +196,7 @@ public override async Task Can_change_dependent_instance_non_derived() @"@p2='Trek Pro Fit Madone 6 Series' (Nullable = false) (Size = 450) @p3='Repair' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [LicensedOperators] ([VehicleName], [LicenseType]) VALUES (@p2, @p3);", @@ -228,6 +230,7 @@ public override async Task Can_change_principal_instance_non_derived() @"@p1='Trek Pro Fit Madone 6 Series' (Nullable = false) (Size = 450) @p0='2' +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Vehicles] SET [SeatingCapacity] = @p0 WHERE [Name] = @p1; diff --git a/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs index a80051258f6..4fa4ba61080 100644 --- a/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/TableSplittingSqlServerTest.cs @@ -210,6 +210,7 @@ public override async Task Can_change_dependent_instance_non_derived() @p1='Repair' (Size = 4000) @p2='repairman' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Vehicles] SET [Operator_Discriminator] = @p0, [LicenseType] = @p1, [Operator_Name] = @p2 WHERE [Name] = @p3; @@ -233,6 +234,7 @@ public override async Task Can_change_principal_instance_non_derived() @"@p1='Trek Pro Fit Madone 6 Series' (Nullable = false) (Size = 450) @p0='2' +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Vehicles] SET [SeatingCapacity] = @p0 WHERE [Name] = @p1; diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosIdentitySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosIdentitySqlServerTest.cs new file mode 100644 index 00000000000..acdd3c4a1d9 --- /dev/null +++ b/test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosIdentitySqlServerTest.cs @@ -0,0 +1,43 @@ +// 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 SaveChangesScenariosIdentitySqlServerTest : SaveChangesScenariosTestBase< + SaveChangesScenariosIdentitySqlServerTest.SaveChangesScenariosIdentitySqlServerFixture> +{ + public SaveChangesScenariosIdentitySqlServerTest(SaveChangesScenariosIdentitySqlServerFixture fixture) + : base(fixture) + { + } + + protected override int ShouldExecuteInNumberOfCommands( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withSameEntityType, + bool withDatabaseGenerated) + => secondOperationType is null ? 1 : 2; + + protected override async Task Test( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withGeneratedValues, + bool async, + bool withSameEntityType = true) + { + await base.Test(firstOperationType, secondOperationType, withGeneratedValues, async, withSameEntityType); + + if (!ShouldCreateImplicitTransaction(firstOperationType, secondOperationType, withGeneratedValues, withSameEntityType)) + { + Assert.Contains("SET IMPLICIT_TRANSACTIONS OFF", Fixture.TestSqlLoggerFactory.SqlStatements[0]); + } + } + + public class SaveChangesScenariosIdentitySqlServerFixture : SaveChangesScenariosFixtureBase + { + protected override string StoreName { get; } = "SaveChangesScenariosIdentityTest"; + + protected override ITestStoreFactory TestStoreFactory + => SqlServerTestStoreFactory.Instance; + } +} diff --git a/test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosSequenceSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosSequenceSqlServerTest.cs new file mode 100644 index 00000000000..6f295bfea72 --- /dev/null +++ b/test/EFCore.SqlServer.FunctionalTests/Update/SaveChangesScenariosSequenceSqlServerTest.cs @@ -0,0 +1,42 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.EntityFrameworkCore.TestModels.SaveChangesTransactionModel; + +namespace Microsoft.EntityFrameworkCore.Update; + +public class SaveChangesScenariosSequenceSqlServerTest : SaveChangesScenariosTestBase< + SaveChangesScenariosSequenceSqlServerTest.SaveChangesScenariosSqlServerFixture> +{ + public SaveChangesScenariosSequenceSqlServerTest(SaveChangesScenariosSqlServerFixture fixture) + : base(fixture) + { + } + + protected override int ShouldExecuteInNumberOfCommands( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withSameEntityType, + bool withDatabaseGenerated) + => secondOperationType is null ? 1 : 2; + + public class SaveChangesScenariosSqlServerFixture : SaveChangesScenariosFixtureBase + { + protected override string StoreName { get; } = "SaveChangesScenariosSequenceTest"; + + protected override ITestStoreFactory TestStoreFactory + => SqlServerTestStoreFactory.Instance; + + protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context) + { + base.OnModelCreating(modelBuilder, context); + + modelBuilder.HasSequence("Ids"); + + modelBuilder + .Entity() + .Property(w => w.Id) + .HasDefaultValueSql("NEXT VALUE FOR [Ids]"); + } + } +} diff --git a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTest.cs index 4320cebe7ae..e2873211f58 100644 --- a/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerTest.cs @@ -44,6 +44,7 @@ public virtual void Save_with_shared_foreign_key() @p1=NULL (Size = 4000) @p2='777' +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; INSERT INTO [Categories] ([Id], [Name], [PrincipalId]) VALUES (@p0, @p1, @p2);", @@ -106,6 +107,7 @@ FROM [Person] @p8='7' (Size = 4000) @p9='3' (Nullable = true) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; DECLARE @inserted0 TABLE ([PersonId] int, [_Position] [int]); MERGE [Person] USING ( @@ -131,6 +133,7 @@ public override void Save_replaced_principal() @"@p1='78' @p0='New Category' (Size = 4000) +SET IMPLICIT_TRANSACTIONS OFF; SET NOCOUNT ON; UPDATE [Categories] SET [Name] = @p0 WHERE [Id] = @p1; diff --git a/test/EFCore.Sqlite.FunctionalTests/Update/SaveChangesScenariosSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Update/SaveChangesScenariosSqliteTest.cs new file mode 100644 index 00000000000..1a48c3c59ae --- /dev/null +++ b/test/EFCore.Sqlite.FunctionalTests/Update/SaveChangesScenariosSqliteTest.cs @@ -0,0 +1,26 @@ +// 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 SaveChangesScenariosSqliteTest : SaveChangesScenariosTestBase< + SaveChangesScenariosSqliteTest.SaveChangesScenariosSqliteFixture> +{ + public SaveChangesScenariosSqliteTest(SaveChangesScenariosSqliteFixture fixture) + : base(fixture) + { + } + + protected override int ShouldExecuteInNumberOfCommands( + EntityState firstOperationType, + EntityState? secondOperationType, + bool withSameEntityType, + bool withDatabaseGenerated) + => secondOperationType is null ? 1 : 2; + + public class SaveChangesScenariosSqliteFixture : SaveChangesScenariosFixtureBase + { + protected override ITestStoreFactory TestStoreFactory + => SqliteTestStoreFactory.Instance; + } +}