Delimit savepoint names as identifiers (#23036)

And move savepoint management generation logic to ISqlGenerationHelper.

Fixes #23021

All.sln.DotSettings

@@ -220,6 +220,7 @@ Licensed under the Apache License, Version 2.0. See License.txt in the project r
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=remapper/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=requiredness/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=retriable/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/UserDictionary/Words/=savepoints/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=shaper/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=spatialite/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=sproc/@EntryIndexedValue">True</s:Boolean>

src/EFCore.Relational/Storage/ISqlGenerationHelper.cs

@@ -120,5 +120,26 @@ public interface ISqlGenerationHelper
         /// <param name="text"> The comment text. </param>
         /// <returns> The generated SQL. </returns>
         string GenerateComment([NotNull] string text);
+
+        /// <summary>
+        ///     Generates an SQL statement which creates a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be created. </param>
+        /// <returns> An SQL string to create the savepoint. </returns>
+        string GenerateCreateSavepointStatement([NotNull] string name);
+
+        /// <summary>
+        ///     Generates an SQL statement which which rolls back to a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be rolled back to. </param>
+        /// <returns> An SQL string to roll back the savepoint. </returns>
+        string GenerateRollbackToSavepointStatement([NotNull] string name);
+
+        /// <summary>
+        ///     Generates an SQL statement which which releases a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be released. </param>
+        /// <returns> An SQL string to release the savepoint. </returns>
+        string GenerateReleaseSavepointStatement([NotNull] string name);
     }
 }

src/EFCore.Relational/Storage/RelationalSqlGenerationHelper.cs

@@ -199,5 +199,29 @@ public virtual string GenerateComment(string text)
 
             return builder.ToString();
         }
+
+        /// <summary>
+        ///     Generates an SQL statement which creates a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be created. </param>
+        /// <returns> An SQL string to create the savepoint. </returns>
+        public virtual string GenerateCreateSavepointStatement(string name)
+            => "SAVEPOINT " + DelimitIdentifier(name) + StatementTerminator;
+
+        /// <summary>
+        ///     Generates an SQL statement which which rolls back to a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be rolled back to. </param>
+        /// <returns> An SQL string to roll back the savepoint. </returns>
+        public virtual string GenerateRollbackToSavepointStatement(string name)
+            => "ROLLBACK TO " + DelimitIdentifier(name) + StatementTerminator;
+
+        /// <summary>
+        ///     Generates an SQL statement which which releases a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be released. </param>
+        /// <returns> An SQL string to release the savepoint. </returns>
+        public virtual string GenerateReleaseSavepointStatement(string name)
+            => "RELEASE SAVEPOINT " + DelimitIdentifier(name) + StatementTerminator;
     }
 }

src/EFCore.Relational/Storage/RelationalTransaction.cs

@@ -26,6 +26,7 @@ public class RelationalTransaction : IDbContextTransaction, IInfrastructure<DbTr
     {
         private readonly DbTransaction _dbTransaction;
         private readonly bool _transactionOwned;
+        private readonly ISqlGenerationHelper _sqlGenerationHelper;
 
         private bool _connectionClosed;
         private bool _disposed;
@@ -40,16 +41,19 @@ public class RelationalTransaction : IDbContextTransaction, IInfrastructure<DbTr
         /// <param name="transactionOwned">
         ///     A value indicating whether the transaction is owned by this class (i.e. if it can be disposed when this class is disposed).
         /// </param>
+        /// <param name="sqlGenerationHelper"> The SQL generation helper to use. </param>
         public RelationalTransaction(
             [NotNull] IRelationalConnection connection,
             [NotNull] DbTransaction transaction,
             Guid transactionId,
             [NotNull] IDiagnosticsLogger<DbLoggerCategory.Database.Transaction> logger,
-            bool transactionOwned)
+            bool transactionOwned,
+            [NotNull] ISqlGenerationHelper sqlGenerationHelper)
         {
             Check.NotNull(connection, nameof(connection));
             Check.NotNull(transaction, nameof(transaction));
             Check.NotNull(logger, nameof(logger));
+            Check.NotNull(sqlGenerationHelper, nameof(sqlGenerationHelper));
 
             if (connection.DbConnection != transaction.Connection)
             {
@@ -62,6 +66,7 @@ public RelationalTransaction(
             _dbTransaction = transaction;
             Logger = logger;
             _transactionOwned = transactionOwned;
+            _sqlGenerationHelper = sqlGenerationHelper;
         }
 
         /// <summary>
@@ -279,7 +284,7 @@ public virtual void CreateSavepoint(string name)
                 {
                     using var command = Connection.DbConnection.CreateCommand();
                     command.Transaction = _dbTransaction;
-                    command.CommandText = GetCreateSavepointSql(name);
+                    command.CommandText = _sqlGenerationHelper.GenerateCreateSavepointStatement(name);
                     command.ExecuteNonQuery();
                 }
 
@@ -323,7 +328,7 @@ public virtual async Task CreateSavepointAsync(string name, CancellationToken ca
                 {
                     using var command = Connection.DbConnection.CreateCommand();
                     command.Transaction = _dbTransaction;
-                    command.CommandText = GetCreateSavepointSql(name);
+                    command.CommandText = _sqlGenerationHelper.GenerateCreateSavepointStatement(name);
                     await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);
                 }
 
@@ -350,15 +355,6 @@ await Logger.TransactionErrorAsync(
             }
         }
 
-        /// <summary>
-        ///     When implemented in a provider supporting transaction savepoints, this method should return an
-        ///     SQL statement which creates a savepoint with the given name.
-        /// </summary>
-        /// <param name="name"> The name of the savepoint to be created. </param>
-        /// <returns> An SQL string to create the savepoint. </returns>
-        protected virtual string GetCreateSavepointSql([NotNull] string name)
-            => "SAVEPOINT " + name;
-
         /// <inheritdoc />
         public virtual void RollbackToSavepoint(string name)
         {
@@ -377,7 +373,7 @@ public virtual void RollbackToSavepoint(string name)
                 {
                     using var command = Connection.DbConnection.CreateCommand();
                     command.Transaction = _dbTransaction;
-                    command.CommandText = GetRollbackToSavepointSql(name);
+                    command.CommandText = _sqlGenerationHelper.GenerateRollbackToSavepointStatement(name);
                     command.ExecuteNonQuery();
                 }
 
@@ -421,7 +417,7 @@ public virtual async Task RollbackToSavepointAsync(string name, CancellationToke
                 {
                     using var command = Connection.DbConnection.CreateCommand();
                     command.Transaction = _dbTransaction;
-                    command.CommandText = GetRollbackToSavepointSql(name);
+                    command.CommandText = _sqlGenerationHelper.GenerateRollbackToSavepointStatement(name);
                     await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);
                 }
 
@@ -448,15 +444,6 @@ await Logger.TransactionErrorAsync(
             }
         }
 
-        /// <summary>
-        ///     When implemented in a provider supporting transaction savepoints, this method should return an
-        ///     SQL statement which rolls back a savepoint with the given name.
-        /// </summary>
-        /// <param name="name"> The name of the savepoint to be created. </param>
-        /// <returns> An SQL string to create the savepoint. </returns>
-        protected virtual string GetRollbackToSavepointSql([NotNull] string name)
-            => "ROLLBACK TO " + name;
-
         /// <inheritdoc />
         public virtual void ReleaseSavepoint(string name)
         {
@@ -475,7 +462,7 @@ public virtual void ReleaseSavepoint(string name)
                 {
                     using var command = Connection.DbConnection.CreateCommand();
                     command.Transaction = _dbTransaction;
-                    command.CommandText = GetReleaseSavepointSql(name);
+                    command.CommandText = _sqlGenerationHelper.GenerateReleaseSavepointStatement(name);
                     command.ExecuteNonQuery();
                 }
 
@@ -519,7 +506,7 @@ public virtual async Task ReleaseSavepointAsync(string name, CancellationToken c
                 {
                     using var command = Connection.DbConnection.CreateCommand();
                     command.Transaction = _dbTransaction;
-                    command.CommandText = GetReleaseSavepointSql(name);
+                    command.CommandText = _sqlGenerationHelper.GenerateReleaseSavepointStatement(name);
                     await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);
                 }
 
@@ -546,21 +533,6 @@ await Logger.TransactionErrorAsync(
             }
         }
 
-        /// <summary>
-        ///     <para>
-        ///         When implemented in a provider supporting transaction savepoints, this method should return an
-        ///         SQL statement which releases a savepoint with the given name.
-        ///     </para>
-        ///     <para>
-        ///         If savepoint release isn't supported, <see cref="ReleaseSavepoint " /> and <see cref="ReleaseSavepointAsync " /> should
-        ///         be overridden to do nothing.
-        ///     </para>
-        /// </summary>
-        /// <param name="name"> The name of the savepoint to be created. </param>
-        /// <returns> An SQL string to create the savepoint. </returns>
-        protected virtual string GetReleaseSavepointSql([NotNull] string name)
-            => "RELEASE SAVEPOINT " + name;
-
         /// <inheritdoc />
         public virtual bool SupportsSavepoints
             => true;

src/EFCore.Relational/Storage/RelationalTransactionFactory.cs

@@ -58,6 +58,7 @@ public virtual RelationalTransaction Create(
             Guid transactionId,
             IDiagnosticsLogger<DbLoggerCategory.Database.Transaction> logger,
             bool transactionOwned)
-            => new RelationalTransaction(connection, transaction, transactionId, logger, transactionOwned);
+            => new RelationalTransaction(
+                connection, transaction, transactionId, logger, transactionOwned, Dependencies.SqlGenerationHelper);
     }
 }

src/EFCore.Relational/Storage/RelationalTransactionFactoryDependencies.cs

@@ -1,7 +1,9 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Utilities;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Microsoft.EntityFrameworkCore.Storage
@@ -51,8 +53,24 @@ public sealed class RelationalTransactionFactoryDependencies
         ///     </para>
         /// </summary>
         [EntityFrameworkInternal]
-        public RelationalTransactionFactoryDependencies()
+        public RelationalTransactionFactoryDependencies([NotNull] ISqlGenerationHelper sqlGenerationHelper)
         {
+            Check.NotNull(sqlGenerationHelper, nameof(sqlGenerationHelper));
+
+            SqlGenerationHelper = sqlGenerationHelper;
         }
+
+        /// <summary>
+        ///     Helpers for SQL generation.
+        /// </summary>
+        public ISqlGenerationHelper SqlGenerationHelper { get; }
+
+        /// <summary>
+        ///     Clones this dependency parameter object with one service replaced.
+        /// </summary>
+        /// <param name="sqlGenerationHelper"> A replacement for the current dependency of this type. </param>
+        /// <returns> A new parameter object with the given service replaced. </returns>
+        public RelationalTransactionFactoryDependencies With([NotNull] ISqlGenerationHelper sqlGenerationHelper)
+            => new RelationalTransactionFactoryDependencies(sqlGenerationHelper);
     }
 }

src/EFCore.SqlServer/Storage/Internal/SqlServerSqlGenerationHelper.cs

@@ -101,5 +101,29 @@ public override void DelimitIdentifier(StringBuilder builder, string identifier)
             EscapeIdentifier(builder, identifier);
             builder.Append(']');
         }
+
+        /// <summary>
+        ///     Generates an SQL statement which creates a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be created. </param>
+        /// <returns> An SQL string to create the savepoint. </returns>
+        public override string GenerateCreateSavepointStatement(string name)
+            => "SAVE TRANSACTION " + DelimitIdentifier(name) + StatementTerminator;
+
+        /// <summary>
+        ///     Generates an SQL statement which which rolls back to a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be rolled back to. </param>
+        /// <returns> An SQL string to roll back the savepoint. </returns>
+        public override string GenerateRollbackToSavepointStatement(string name)
+            => "ROLLBACK TRANSACTION " + DelimitIdentifier(name) + StatementTerminator;
+
+        /// <summary>
+        ///     Generates an SQL statement which which releases a savepoint with the given name.
+        /// </summary>
+        /// <param name="name"> The name of the savepoint to be released. </param>
+        /// <returns> An SQL string to release the savepoint. </returns>
+        public override string GenerateReleaseSavepointStatement(string name)
+            => throw new NotSupportedException("SQL Server does not support releasing a savepoint");
     }
 }

src/EFCore.SqlServer/Storage/Internal/SqlServerTransaction.cs

@@ -30,18 +30,13 @@ public SqlServerTransaction(
             [NotNull] DbTransaction transaction,
             Guid transactionId,
             [NotNull] IDiagnosticsLogger<DbLoggerCategory.Database.Transaction> logger,
-            bool transactionOwned)
-            : base(connection, transaction, transactionId, logger, transactionOwned)
+            bool transactionOwned,
+            [NotNull] ISqlGenerationHelper sqlGenerationHelper)
+            : base(connection, transaction, transactionId, logger, transactionOwned, sqlGenerationHelper)
         {
         }
 
-        /// <inheritdoc />
-        protected override string GetCreateSavepointSql(string name)
-            => "SAVE TRANSACTION " + name;
-
-        /// <inheritdoc />
-        protected override string GetRollbackToSavepointSql(string name)
-            => "ROLLBACK TRANSACTION " + name;
+        // SQL Server doesn't support releasing savepoints. Override to do nothing.
 
         /// <inheritdoc />
         public override void ReleaseSavepoint(string name) { }

src/EFCore.SqlServer/Storage/Internal/SqlServerTransactionFactory.cs

@@ -3,8 +3,10 @@
 
 using System;
 using System.Data.Common;
+using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Storage;
+using Microsoft.EntityFrameworkCore.Utilities;
 
 namespace Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal
 {
@@ -16,6 +18,22 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal
     /// </summary>
     public class SqlServerTransactionFactory : IRelationalTransactionFactory
     {
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="RelationalTransactionFactory" /> class.
+        /// </summary>
+        /// <param name="dependencies"> Parameter object containing dependencies for this service. </param>
+        public SqlServerTransactionFactory([NotNull] RelationalTransactionFactoryDependencies dependencies)
+        {
+            Check.NotNull(dependencies, nameof(dependencies));
+
+            Dependencies = dependencies;
+        }
+
+        /// <summary>
+        ///     Parameter object containing dependencies for this service.
+        /// </summary>
+        protected virtual RelationalTransactionFactoryDependencies Dependencies { get; }
+
         /// <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
@@ -28,6 +46,6 @@ public virtual RelationalTransaction Create(
             Guid transactionId,
             IDiagnosticsLogger<DbLoggerCategory.Database.Transaction> logger,
             bool transactionOwned)
-            => new SqlServerTransaction(connection, transaction, transactionId, logger, transactionOwned);
+            => new SqlServerTransaction(connection, transaction, transactionId, logger, transactionOwned, Dependencies.SqlGenerationHelper);
     }
 }

src/EFCore/Storage/IDbContextTransaction.cs

@@ -86,15 +86,27 @@ Task RollbackToSavepointAsync([NotNull] string name, CancellationToken cancellat
             => throw new NotSupportedException();
 
         /// <summary>
-        ///     Destroys a savepoint previously defined in the current transaction. This allows the system to
-        ///     reclaim some resources before the transaction ends.
+        ///     <para>
+        ///         Destroys a savepoint previously defined in the current transaction. This allows the system to
+        ///         reclaim some resources before the transaction ends.
+        ///     </para>
+        ///     <para>
+        ///         If savepoint release isn't supported, <see cref="ReleaseSavepoint " /> and <see cref="ReleaseSavepointAsync " /> should
+        ///         do nothing rather than throw. This is the default behavior.
+        ///     </para>
         /// </summary>
         /// <param name="name"> The name of the savepoint to release. </param>
         void ReleaseSavepoint([NotNull] string name) { }
 
         /// <summary>
-        ///     Destroys a savepoint previously defined in the current transaction. This allows the system to
-        ///     reclaim some resources before the transaction ends.
+        ///     <para>
+        ///         Destroys a savepoint previously defined in the current transaction. This allows the system to
+        ///         reclaim some resources before the transaction ends.
+        ///     </para>
+        ///     <para>
+        ///         If savepoint release isn't supported, <see cref="ReleaseSavepoint " /> and <see cref="ReleaseSavepointAsync " /> should
+        ///         do nothing rather than throw. This is the default behavior.
+        ///     </para>
         /// </summary>
         /// <param name="name"> The name of the savepoint to release. </param>
         /// <param name="cancellationToken"> The cancellation token. </param>