Use RETURNING/OUTPUT clause for UPDATE/DELETE (dotnet#27663)

Closes dotnet#27547

src/EFCore.Relational/Update/AffectedCountModificationCommandBatch.cs

@@ -5,8 +5,7 @@ namespace Microsoft.EntityFrameworkCore.Update;
 
 /// <summary>
 ///     <para>
-///         A <see cref="ReaderModificationCommandBatch" /> for providers which append an SQL query to find out
-///         how many rows were affected (see <see cref="UpdateSqlGenerator.AppendSelectAffectedCountCommand" />).
+///         A <see cref="ReaderModificationCommandBatch" /> for providers which return values to find out how many rows were affected.
 ///     </para>
 ///     <para>
 ///         This type is typically used by database providers; it is generally not used in application code.

src/EFCore.Relational/Update/UpdateSqlGenerator.cs

@@ -97,20 +97,13 @@ public virtual ResultSetMapping AppendUpdateOperation(
         var conditionOperations = operations.Where(o => o.IsCondition).ToList();
         var readOperations = operations.Where(o => o.IsRead).ToList();
 
-        AppendUpdateCommand(commandStringBuilder, name, schema, writeOperations, conditionOperations);
-
-        if (readOperations.Count > 0)
-        {
-            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);
+        AppendUpdateCommand(
+            commandStringBuilder, name, schema, writeOperations, readOperations, conditionOperations,
+            additionalReadValues: readOperations.Count == 0 ? "1" : null);
+
+        return ResultSetMapping.LastInResultSet;
     }
 
     /// <summary>
@@ -131,11 +124,12 @@ public virtual ResultSetMapping AppendDeleteOperation(
         var schema = command.Schema;
         var conditionOperations = command.ColumnModifications.Where(o => o.IsCondition).ToList();
 
-        AppendDeleteCommand(commandStringBuilder, name, schema, conditionOperations);
-
         requiresTransaction = false;
 
-        return AppendSelectAffectedCountCommand(commandStringBuilder, name, schema, commandPosition);
+        AppendDeleteCommand(
+            commandStringBuilder, name, schema, Array.Empty<IColumnModification>(), conditionOperations, additionalReadValues: "1");
+
+        return ResultSetMapping.LastInResultSet;
     }
 
     /// <summary>
@@ -167,16 +161,21 @@ protected virtual void AppendInsertCommand(
     /// <param name="name">The name of the table.</param>
     /// <param name="schema">The table schema, or <see langword="null" /> to use the default schema.</param>
     /// <param name="writeOperations">The operations for each column.</param>
+    /// <param name="readOperations">The operations for column values to be read back.</param>
     /// <param name="conditionOperations">The operations used to generate the <c>WHERE</c> clause for the update.</param>
+    /// <param name="additionalReadValues">Additional values to be read back.</param>
     protected virtual void AppendUpdateCommand(
         StringBuilder commandStringBuilder,
         string name,
         string? schema,
         IReadOnlyList<IColumnModification> writeOperations,
-        IReadOnlyList<IColumnModification> conditionOperations)
+        IReadOnlyList<IColumnModification> readOperations,
+        IReadOnlyList<IColumnModification> conditionOperations,
+        string? additionalReadValues = null)
     {
         AppendUpdateCommandHeader(commandStringBuilder, name, schema, writeOperations);
         AppendWhereClause(commandStringBuilder, conditionOperations);
+        AppendReturningClause(commandStringBuilder, readOperations, additionalReadValues);
         commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator);
     }
 
@@ -186,33 +185,23 @@ protected virtual void AppendUpdateCommand(
     /// <param name="commandStringBuilder">The builder to which the SQL should be appended.</param>
     /// <param name="name">The name of the table.</param>
     /// <param name="schema">The table schema, or <see langword="null" /> to use the default schema.</param>
+    /// <param name="readOperations">The operations for column values to be read back.</param>
     /// <param name="conditionOperations">The operations used to generate the <c>WHERE</c> clause for the delete.</param>
+    /// <param name="additionalReadValues">Additional values to be read back.</param>
     protected virtual void AppendDeleteCommand(
         StringBuilder commandStringBuilder,
         string name,
         string? schema,
-        IReadOnlyList<IColumnModification> conditionOperations)
+        IReadOnlyList<IColumnModification> readOperations,
+        IReadOnlyList<IColumnModification> conditionOperations,
+        string? additionalReadValues = null)
     {
         AppendDeleteCommandHeader(commandStringBuilder, name, schema);
         AppendWhereClause(commandStringBuilder, conditionOperations);
+        AppendReturningClause(commandStringBuilder, readOperations, additionalReadValues);
         commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator);
     }
 
-    /// <summary>
-    ///     Appends a SQL command for selecting the number of rows affected.
-    /// </summary>
-    /// <param name="commandStringBuilder">The builder to which the SQL should be appended.</param>
-    /// <param name="name">The name of the table.</param>
-    /// <param name="schema">The table schema, or <see langword="null" /> to use the default schema.</param>
-    /// <param name="commandPosition">The ordinal of the command for which rows affected it being returned.</param>
-    /// <returns>The <see cref="ResultSetMapping" /> for this command.</returns>
-    protected virtual ResultSetMapping AppendSelectAffectedCountCommand(
-        StringBuilder commandStringBuilder,
-        string name,
-        string? schema,
-        int commandPosition)
-        => ResultSetMapping.NoResultSet;
-
     /// <summary>
     ///     Appends a SQL command for selecting affected data.
     /// </summary>
@@ -411,11 +400,13 @@ protected virtual void AppendValues(
     /// </summary>
     /// <param name="commandStringBuilder">The builder to which the SQL should be appended.</param>
     /// <param name="operations">The operations for column values to be read back.</param>
+    /// <param name="additionalValues">Additional values to be read back.</param>
     protected virtual void AppendReturningClause(
         StringBuilder commandStringBuilder,
-        IReadOnlyList<IColumnModification> operations)
+        IReadOnlyList<IColumnModification> operations,
+        string? additionalValues = null)
     {
-        if (operations.Count > 0)
+        if (operations.Count > 0 || additionalValues is not null)
         {
             commandStringBuilder
                 .AppendLine()
@@ -424,6 +415,16 @@ protected virtual void AppendReturningClause(
                     operations,
                     SqlGenerationHelper,
                     (sb, o, helper) => helper.DelimitIdentifier(sb, o.ColumnName));
+
+            if (additionalValues is not null)
+            {
+                if (operations.Count > 0)
+                {
+                    commandStringBuilder.Append(", ");
+                }
+
+                commandStringBuilder.Append(additionalValues);
+            }
         }
     }
 

src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs

@@ -47,8 +47,7 @@ public override ResultSetMapping AppendInsertOperation(
         // If no database-generated columns need to be read back, just do a simple INSERT (default behavior).
         // If there are generated columns but there are no triggers defined on the table, we can do a simple INSERT ... OUTPUT
         // (without INTO), which is also the default behavior, doesn't require a transaction and is the most efficient.
-        if (command.ColumnModifications.All(o => !o.IsRead)
-            || !command.Entries[0].EntityType.Model.GetRelationalModel().FindTable(command.TableName, command.Schema)!.Triggers.Any())
+        if (command.ColumnModifications.All(o => !o.IsRead) || !HasAnyTriggers(command))
         {
             return base.AppendInsertOperation(commandStringBuilder, command, commandPosition, out requiresTransaction);
         }
@@ -94,6 +93,124 @@ protected override void AppendInsertCommand(
         commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator);
     }
 
+    /// <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
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    public override ResultSetMapping AppendUpdateOperation(
+        StringBuilder commandStringBuilder,
+        IReadOnlyModificationCommand command,
+        int commandPosition,
+        out bool requiresTransaction)
+    {
+        // We normally do a simple UPDATE with an OUTPUT clause (either for the generated columns, or for "1" for concurrency checking).
+        // However, if there are triggers defined, OUTPUT (without INTO) is not supported, so we do UPDATE+SELECT.
+        if (!HasAnyTriggers(command))
+        {
+            return base.AppendUpdateOperation(commandStringBuilder, command, commandPosition, out requiresTransaction);
+        }
+
+        var name = command.TableName;
+        var schema = command.Schema;
+        var operations = command.ColumnModifications;
+
+        var writeOperations = operations.Where(o => o.IsWrite).ToList();
+        var conditionOperations = operations.Where(o => o.IsCondition).ToList();
+        var readOperations = operations.Where(o => o.IsRead).ToList();
+
+        AppendUpdateCommand(commandStringBuilder, name, schema, writeOperations, Array.Empty<IColumnModification>(), conditionOperations);
+
+        if (readOperations.Count > 0)
+        {
+            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);
+    }
+
+    /// <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
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    protected override void AppendUpdateCommand(
+        StringBuilder commandStringBuilder,
+        string name,
+        string? schema,
+        IReadOnlyList<IColumnModification> writeOperations,
+        IReadOnlyList<IColumnModification> readOperations,
+        IReadOnlyList<IColumnModification> conditionOperations,
+        string? additionalReadValues = null)
+    {
+        // In SQL Server the OUTPUT clause is placed differently (before the WHERE instead of at the end)
+        AppendUpdateCommandHeader(commandStringBuilder, name, schema, writeOperations);
+        AppendOutputClause(commandStringBuilder, readOperations, additionalReadValues);
+        AppendWhereClause(commandStringBuilder, conditionOperations);
+        commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator);
+    }
+
+    /// <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
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    public override ResultSetMapping AppendDeleteOperation(
+        StringBuilder commandStringBuilder,
+        IReadOnlyModificationCommand command,
+        int commandPosition,
+        out bool requiresTransaction)
+    {
+        // We normally do a simple DELETE, with an OUTPUT clause emitting "1" for concurrency checking.
+        // However, if there are triggers defined, OUTPUT (without INTO) is not supported, so we do UPDATE+SELECT.
+        if (!HasAnyTriggers(command))
+        {
+            return base.AppendDeleteOperation(commandStringBuilder, command, commandPosition, out requiresTransaction);
+        }
+
+        var name = command.TableName;
+        var schema = command.Schema;
+        var operations = command.ColumnModifications;
+
+        var conditionOperations = operations.Where(o => o.IsCondition).ToList();
+
+        requiresTransaction = false;
+
+        AppendDeleteCommand(commandStringBuilder, name, schema, Array.Empty<IColumnModification>(), conditionOperations);
+
+        return AppendSelectAffectedCountCommand(commandStringBuilder, name, schema, commandPosition);
+    }
+
+    /// <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
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    protected override void AppendDeleteCommand(
+        StringBuilder commandStringBuilder,
+        string name,
+        string? schema,
+        IReadOnlyList<IColumnModification> readOperations,
+        IReadOnlyList<IColumnModification> conditionOperations,
+        string? additionalReadValues = null)
+    {
+        // In SQL Server the OUTPUT clause is placed differently (before the WHERE instead of at the end)
+        AppendDeleteCommandHeader(commandStringBuilder, name, schema);
+        AppendOutputClause(commandStringBuilder, readOperations, additionalReadValues);
+        AppendWhereClause(commandStringBuilder, conditionOperations);
+        commandStringBuilder.AppendLine(SqlGenerationHelper.StatementTerminator);
+    }
+
     /// <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
@@ -200,8 +317,7 @@ public virtual ResultSetMapping AppendBulkInsertOperation(
         // We default to using MERGE ... OUTPUT (without INTO), projecting back a synthetic _Position column to know the order back
         // at the client and propagate database-generated values correctly. However, if any triggers are defined, OUTPUT without INTO
         // doesn't work.
-        if (!firstCommand.Entries[0].EntityType.Model.GetRelationalModel().FindTable(firstCommand.TableName, firstCommand.Schema)!
-                .Triggers.Any())
+        if (!HasAnyTriggers(firstCommand))
         {
             // MERGE ... OUTPUT returns rows whose ordering isn't guaranteed. So this technique projects back a position int with each row,
             // to allow mapping the rows back for value propagation.
@@ -578,16 +694,17 @@ private static string GetTypeNameForCopy(IProperty property)
     /// </summary>
     protected override void AppendReturningClause(
         StringBuilder commandStringBuilder,
-        IReadOnlyList<IColumnModification> operations)
-        => AppendOutputClause(commandStringBuilder, operations);
+        IReadOnlyList<IColumnModification> operations,
+        string? additionalValues = null)
+        => AppendOutputClause(commandStringBuilder, operations, additionalValues);
 
     // ReSharper disable once ParameterTypeCanBeEnumerable.Local
     private void AppendOutputClause(
         StringBuilder commandStringBuilder,
         IReadOnlyList<IColumnModification> operations,
-        string? additionalColumns = null)
+        string? additionalReadValues = null)
     {
-        if (operations.Count > 0 || additionalColumns is not null)
+        if (operations.Count > 0 || additionalReadValues is not null)
         {
             commandStringBuilder
                 .AppendLine()
@@ -601,14 +718,14 @@ private void AppendOutputClause(
                         helper.DelimitIdentifier(sb, o.ColumnName);
                     });
 
-            if (additionalColumns != null)
+            if (additionalReadValues is not null)
             {
                 if (operations.Count > 0)
                 {
                     commandStringBuilder.Append(", ");
                 }
 
-                commandStringBuilder.Append(additionalColumns);
+                commandStringBuilder.Append(additionalReadValues);
             }
         }
     }
@@ -728,7 +845,7 @@ private ResultSetMapping AppendSelectCommand(
     ///     any release. You should only use it directly in your code with extreme caution and knowing that
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
-    protected override ResultSetMapping AppendSelectAffectedCountCommand(
+    protected virtual ResultSetMapping AppendSelectAffectedCountCommand(
         StringBuilder commandStringBuilder,
         string name,
         string? schema,
@@ -790,4 +907,10 @@ protected override void AppendRowsAffectedWhereCondition(StringBuilder commandSt
         => commandStringBuilder
             .Append("@@ROWCOUNT = ")
             .Append(expectedRowsAffected.ToString(CultureInfo.InvariantCulture));
+
+    private static bool HasAnyTriggers(IReadOnlyModificationCommand command)
+        // Data seeding doesn't provide any entries, so we we don't know if the table has triggers; assume it does to generate SQL
+        // that works everywhere.
+        => command.Entries.Count == 0
+            || command.Entries[0].EntityType.Model.GetRelationalModel().FindTable(command.TableName, command.Schema)!.Triggers.Any();
 }

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

@@ -38,26 +38,6 @@ protected override void AppendIdentityWhereCondition(StringBuilder commandString
             .Append("last_insert_rowid()");
     }
 
-    /// <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
-    ///     any release. You should only use it directly in your code with extreme caution and knowing that
-    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
-    /// </summary>
-    protected override ResultSetMapping AppendSelectAffectedCountCommand(
-        StringBuilder commandStringBuilder,
-        string name,
-        string? schema,
-        int commandPosition)
-    {
-        commandStringBuilder
-            .Append("SELECT changes()")
-            .AppendLine(SqlGenerationHelper.StatementTerminator)
-            .AppendLine();
-
-        return ResultSetMapping.LastInResultSet;
-    }
-
     /// <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