From 2d0a8359098fa252cd66f12003dcd924320eec4a Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Fri, 13 Nov 2020 10:06:14 +0200 Subject: [PATCH] Disable savepoints on SQL Server when MARS is enabled Fixes #23269 --- All.sln.DotSettings | 3 ++ .../Internal/SqlServerLoggingDefinitions.cs | 8 ++++ .../Diagnostics/SqlServerEventId.cs | 19 ++++++++ .../Internal/SqlServerLoggerExtensions.cs | 24 ++++++++++ .../Properties/SqlServerStrings.Designer.cs | 24 ++++++++++ .../Properties/SqlServerStrings.resx | 4 ++ .../Storage/Internal/SqlServerTransaction.cs | 19 ++++++++ .../Diagnostics/WarningsConfiguration.cs | 2 +- .../MigrationsInfrastructureSqlServerTest.cs | 6 +-- .../TestUtilities/SqlServerTestStore.cs | 5 +- .../TransactionSqlServerTest.cs | 48 ++++++++++++++++++- 11 files changed, 156 insertions(+), 6 deletions(-) diff --git a/All.sln.DotSettings b/All.sln.DotSettings index 10b8b9be4a6..7f278518d5d 100644 --- a/All.sln.DotSettings +++ b/All.sln.DotSettings @@ -136,6 +136,7 @@ Licensed under the Apache License, Version 2.0. See License.txt in the project r <Policy Inspect="True" Prefix="" Suffix="" Style="aa_bb" /> True FK + MARS $object$_On$event$ <Policy Inspect="True" Prefix="_" Suffix="" Style="aaBb" /> <Policy><Descriptor Staticness="Static, Instance" AccessRightKinds="Public" Description="Test Methods"><ElementKinds><Kind Name="TEST_MEMBER" /></ElementKinds></Descriptor><Policy Inspect="True" Prefix="" Suffix="" Style="Aa_bb" /></Policy> @@ -219,6 +220,8 @@ Licensed under the Apache License, Version 2.0. See License.txt in the project r True True True + True + True True True True diff --git a/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs b/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs index 2cea968ff22..f9e155987fa 100644 --- a/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs +++ b/src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs @@ -149,6 +149,14 @@ public class SqlServerLoggingDefinitions : RelationalLoggingDefinitions /// public EventDefinitionBase LogReflexiveConstraintIgnored; + /// + /// 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 EventDefinitionBase LogSavepointsDisabledBecauseOfMARS; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs b/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs index 702e7bd9895..1b02de29ab5 100644 --- a/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs +++ b/src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs @@ -29,6 +29,9 @@ private enum Id ConflictingValueGenerationStrategiesWarning, DecimalTypeKeyWarning, + // Transaction events + SavepointsDisabledBecauseOfMARS, + // Scaffolding events ColumnFound = CoreEventId.ProviderDesignBaseId, ColumnNotNamedWarning, @@ -121,6 +124,22 @@ private static EventId MakeValidationId(Id id) public static readonly EventId ConflictingValueGenerationStrategiesWarning = MakeValidationId(Id.ConflictingValueGenerationStrategiesWarning); + private static readonly string _transactionPrefix = DbLoggerCategory.Database.Transaction.Name + "."; + + private static EventId MakeTransactionId(Id id) + => new EventId((int)id, _transactionPrefix + id); + + /// + /// + /// Savepoints have been disabled when saving changes with an external transaction, because Multiple Active Result Sets is + /// enabled. + /// + /// + /// This event is in the category. + /// + /// + public static readonly EventId SavepointsDisabledBecauseOfMARS = MakeTransactionId(Id.SavepointsDisabledBecauseOfMARS); + private static readonly string _scaffoldingPrefix = DbLoggerCategory.Scaffolding.Name + "."; private static EventId MakeScaffoldingId(Id id) diff --git a/src/EFCore.SqlServer/Internal/SqlServerLoggerExtensions.cs b/src/EFCore.SqlServer/Internal/SqlServerLoggerExtensions.cs index 1b3fa22ed4e..983a2600fd3 100644 --- a/src/EFCore.SqlServer/Internal/SqlServerLoggerExtensions.cs +++ b/src/EFCore.SqlServer/Internal/SqlServerLoggerExtensions.cs @@ -510,5 +510,29 @@ public static void ReflexiveConstraintIgnored( // No DiagnosticsSource events because these are purely design-time messages } + + /// + /// Logs for the event. + /// + /// The diagnostics logger to use. + public static void SavepointsDisabledBecauseOfMARS( + [NotNull] this IDiagnosticsLogger diagnostics) + { + var definition = SqlServerResources.LogSavepointsDisabledBecauseOfMARS(diagnostics); + + if (diagnostics.ShouldLog(definition)) + { + definition.Log(diagnostics); + } + + if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled)) + { + var eventData = new EventData( + definition, + (d, p) => ((EventDefinition)d).GenerateMessage()); + + diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled); + } + } } } diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs index 365da24ea07..c6ce97732c6 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs @@ -660,5 +660,29 @@ public static EventDefinition LogReflexiveConstraintIgnored([Not return (EventDefinition)definition; } + + /// + /// Savepoints are disabled because Multiple Active Result Sets (MARS) is enabled. If 'SaveChanges' fails, then the transaction cannot be automatically rolled back to a known clean state. Instead, the transaction should be rolled back by the application before retrying 'SaveChanges'. See https://go.microsoft.com/fwlink/?linkid=2149338 for more information. To identify the code which triggers this warning, call 'ConfigureWarnings(w => w.Throw(RelationalEventId.SavepointsDisabledBecauseOfMARS))'. + /// + public static EventDefinition LogSavepointsDisabledBecauseOfMARS([NotNull] IDiagnosticsLogger logger) + { + var definition = ((Diagnostics.Internal.SqlServerLoggingDefinitions)logger.Definitions).LogSavepointsDisabledBecauseOfMARS; + if (definition == null) + { + definition = LazyInitializer.EnsureInitialized( + ref ((Diagnostics.Internal.SqlServerLoggingDefinitions)logger.Definitions).LogSavepointsDisabledBecauseOfMARS, + () => new EventDefinition( + logger.Options, + SqlServerEventId.SavepointsDisabledBecauseOfMARS, + LogLevel.Warning, + "SqlServerEventId.SavepointsDisabledBecauseOfMARS", + level => LoggerMessage.Define( + level, + SqlServerEventId.SavepointsDisabledBecauseOfMARS, + _resourceManager.GetString("LogSavepointsDisabledBecauseOfMARS")))); + } + + return (EventDefinition)definition; + } } } diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx index 2b60ac5ec04..291e35472a7 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx @@ -249,6 +249,10 @@ Skipping foreign key '{foreignKeyName}' on table '{tableName}' since all of its columns reference themselves. Debug SqlServerEventId.ReflexiveConstraintIgnored string string + + Savepoints are disabled because Multiple Active Result Sets (MARS) is enabled. If 'SaveChanges' fails, then the transaction cannot be automatically rolled back to a known clean state. Instead, the transaction should be rolled back by the application before retrying 'SaveChanges'. See https://go.microsoft.com/fwlink/?linkid=2149338 for more information. To identify the code which triggers this warning, call 'ConfigureWarnings(w => w.Throw(RelationalEventId.SavepointsDisabledBecauseOfMARS))'. + Warning SqlServerEventId.SavepointsDisabledBecauseOfMARS + The properties {properties} are configured to use 'Identity' value generator and are mapped to the same table '{table}'. Only one column per table can be configured as 'Identity'. Call 'ValueGeneratedNever' for properties that should not use 'Identity'. diff --git a/src/EFCore.SqlServer/Storage/Internal/SqlServerTransaction.cs b/src/EFCore.SqlServer/Storage/Internal/SqlServerTransaction.cs index 56ca777fa36..81fa31b5873 100644 --- a/src/EFCore.SqlServer/Storage/Internal/SqlServerTransaction.cs +++ b/src/EFCore.SqlServer/Storage/Internal/SqlServerTransaction.cs @@ -7,6 +7,7 @@ using System.Threading.Tasks; using JetBrains.Annotations; using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.SqlServer.Internal; using Microsoft.EntityFrameworkCore.Storage; namespace Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal @@ -43,6 +44,24 @@ protected override string GetCreateSavepointSql(string name) protected override string GetRollbackToSavepointSql(string name) => "ROLLBACK TRANSACTION " + name; + /// + public override bool SupportsSavepoints + { + get + { + if (Connection is ISqlServerConnection sqlServerConnection && sqlServerConnection.IsMultipleActiveResultSetsEnabled) + { + Logger.SavepointsDisabledBecauseOfMARS(); + + return false; + } + + return true; + } + } + + // SQL Server doesn't support releasing savepoints. Override to do nothing. + /// public override void ReleaseSavepoint(string name) { } diff --git a/src/EFCore/Diagnostics/WarningsConfiguration.cs b/src/EFCore/Diagnostics/WarningsConfiguration.cs index 7832bbe80e2..e89b81c44d4 100644 --- a/src/EFCore/Diagnostics/WarningsConfiguration.cs +++ b/src/EFCore/Diagnostics/WarningsConfiguration.cs @@ -172,7 +172,7 @@ public virtual long GetServiceProviderHashCode() if (_explicitBehaviors != null) { - hashCode = _explicitBehaviors.Aggregate( + hashCode = _explicitBehaviors.OrderBy(b => b.Key).Aggregate( hashCode, (t, e) => (t * 397) ^ (((long)e.Value.GetHashCode() * 3163) ^ (long)e.Key.GetHashCode())); } diff --git a/test/EFCore.SqlServer.FunctionalTests/MigrationsInfrastructureSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/MigrationsInfrastructureSqlServerTest.cs index d7de3d560eb..47a2a1898b3 100644 --- a/test/EFCore.SqlServer.FunctionalTests/MigrationsInfrastructureSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/MigrationsInfrastructureSqlServerTest.cs @@ -6,6 +6,7 @@ using System.Threading.Tasks; using Identity30.Data; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Metadata; using Microsoft.EntityFrameworkCore.Migrations; @@ -1410,9 +1411,8 @@ IF EXISTS(select * from sys.databases where name='TransactionSuppressed') public override MigrationsContext CreateContext() { - var options = AddOptions( - new DbContextOptionsBuilder() - .UseSqlServer(TestStore.ConnectionString, b => b.ApplyConfiguration())) + var options = AddOptions(TestStore.AddProviderOptions(new DbContextOptionsBuilder())) + .UseSqlServer(TestStore.ConnectionString, b => b.ApplyConfiguration()) .UseInternalServiceProvider(ServiceProvider) .Options; return new MigrationsContext(options); diff --git a/test/EFCore.SqlServer.FunctionalTests/TestUtilities/SqlServerTestStore.cs b/test/EFCore.SqlServer.FunctionalTests/TestUtilities/SqlServerTestStore.cs index b0967348b46..77727f336b9 100644 --- a/test/EFCore.SqlServer.FunctionalTests/TestUtilities/SqlServerTestStore.cs +++ b/test/EFCore.SqlServer.FunctionalTests/TestUtilities/SqlServerTestStore.cs @@ -11,6 +11,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.Data.SqlClient; +using Microsoft.EntityFrameworkCore.Diagnostics; #pragma warning disable IDE0022 // Use block body for methods // ReSharper disable SuggestBaseTypeForParameter @@ -98,7 +99,9 @@ protected override void Initialize(Func createContext, Action builder.UseSqlServer(Connection, b => b.ApplyConfiguration()); + => builder + .UseSqlServer(Connection, b => b.ApplyConfiguration()) + .ConfigureWarnings(b => b.Ignore(SqlServerEventId.SavepointsDisabledBecauseOfMARS)); private bool CreateDatabase(Action clean) { diff --git a/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs index fce8f6710fe..bce94b84951 100644 --- a/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs @@ -1,9 +1,15 @@ // 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 System.Threading.Tasks; +using Microsoft.Data.SqlClient; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal; using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; + +// ReSharper disable MethodHasAsyncOverload namespace Microsoft.EntityFrameworkCore { @@ -14,6 +20,41 @@ public TransactionSqlServerTest(TransactionSqlServerFixture fixture) { } + [ConditionalTheory] + [InlineData(true)] + [InlineData(false)] + public virtual async Task Savepoints_are_disabled_with_MARS(bool async) + { + await using var context = CreateContextWithConnectionString( + SqlServerTestStore.CreateConnectionString(TestStore.Name, multipleActiveResultSets: true)); + + await using var transaction = await context.Database.BeginTransactionAsync(); + + var orderId = 300; + foreach (var _ in context.Set()) + { + context.Add(new TransactionOrder { Id = orderId++, Name = "Order " + orderId }); + if (async) + { + await context.SaveChangesAsync(); + } + else + { + context.SaveChanges(); + } + } + + await transaction.CommitAsync(); + + Assert.Contains(Fixture.ListLoggerFactory.Log, t => t.Id == SqlServerEventId.SavepointsDisabledBecauseOfMARS); + } + + // Test relies on savepoints, which are disabled when MARS is enabled + public override Task SaveChanges_uses_explicit_transaction_with_failure_behavior(bool async, bool autoTransaction) + => new SqlConnectionStringBuilder(TestStore.ConnectionString).MultipleActiveResultSets + ? Task.CompletedTask + : base.SaveChanges_uses_explicit_transaction_with_failure_behavior(async, autoTransaction); + protected override bool SnapshotSupported => true; @@ -21,12 +62,16 @@ protected override bool AmbientTransactionsSupported => true; protected override DbContext CreateContextWithConnectionString() + => CreateContextWithConnectionString(null); + + protected DbContext CreateContextWithConnectionString(string connectionString) { var options = Fixture.AddOptions( new DbContextOptionsBuilder() .UseSqlServer( - TestStore.ConnectionString, + connectionString ?? TestStore.ConnectionString, b => b.ApplyConfiguration().ExecutionStrategy(c => new SqlServerExecutionStrategy(c)))) + .ConfigureWarnings(b => b.Log(SqlServerEventId.SavepointsDisabledBecauseOfMARS)) .UseInternalServiceProvider(Fixture.ServiceProvider); return new DbContext(options.Options); @@ -60,6 +105,7 @@ public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder build new SqlServerDbContextOptionsBuilder( base.AddOptions(builder)) .ExecutionStrategy(c => new SqlServerExecutionStrategy(c)); + builder.ConfigureWarnings(b => b.Log(SqlServerEventId.SavepointsDisabledBecauseOfMARS)); return builder; } }