diff --git a/src/grate.sqlserver/Migration/SqlServerDatabase.cs b/src/grate.sqlserver/Migration/SqlServerDatabase.cs index 491ef41e..eb9a045a 100644 --- a/src/grate.sqlserver/Migration/SqlServerDatabase.cs +++ b/src/grate.sqlserver/Migration/SqlServerDatabase.cs @@ -27,17 +27,16 @@ protected override DbConnection GetSqlConnection(string? connectionString) // If pooling is not explicitly mentioned in the connection string, turn it off, as enabling it // might lead to problems in more scenarios than it (potentially) solves, in the most // common grate scenarios. - if (!(connectionString ?? "").Contains("Pooling", StringComparison.InvariantCultureIgnoreCase)) - { - var builder = new SqlConnectionStringBuilder(connectionString) { Pooling = false }; - connectionString = builder.ConnectionString; - } - + // if (!(connectionString ?? "").Contains("Pooling", StringComparison.InvariantCultureIgnoreCase)) + // { + // var builder = new SqlConnectionStringBuilder(connectionString) { Pooling = false }; + // connectionString = builder.ConnectionString; + // } + // var conn = new SqlConnection(connectionString) { AccessToken = AccessToken }; - return conn; } protected string? AccessToken { get; private set; } diff --git a/src/grate/Commands/FoldersCommand.cs b/src/grate/Commands/FoldersCommand.cs index 2c39113c..98b45bd4 100644 --- a/src/grate/Commands/FoldersCommand.cs +++ b/src/grate/Commands/FoldersCommand.cs @@ -121,7 +121,7 @@ private static void ApplyConfig(MigrationsFolder folder, string folderConfig) } var parsed = (propertyType?.IsEnum ?? false) - ? Enum.Parse(propertyType, value) + ? Enum.Parse(propertyType, value, true) : value; setter.Invoke(folder, new[] { parsed }); diff --git a/unittests/Basic_tests/CommandLineParsing/FolderConfiguration_.cs b/unittests/Basic_tests/CommandLineParsing/FolderConfiguration_.cs index 76585c10..5de4a771 100644 --- a/unittests/Basic_tests/CommandLineParsing/FolderConfiguration_.cs +++ b/unittests/Basic_tests/CommandLineParsing/FolderConfiguration_.cs @@ -6,6 +6,8 @@ using FluentAssertions; using grate.Commands; using grate.Configuration; +using grate.Migration; +using static grate.Configuration.KnownFolderKeys; namespace Basic_tests.CommandLineParsing; @@ -23,6 +25,19 @@ public async Task Default() AssertEquivalent(expected.Values, actual?.Values); } + + [Fact] + public async Task Default_with_overridden_transaction_handling_for_one_folder() + { + var cfg = await ParseGrateConfiguration("--folders=runAfterCreateDatabase=transactionHandling:autonomous"); + + var expected = FoldersConfiguration.Default(); + expected[RunAfterCreateDatabase] = expected[RunAfterCreateDatabase]! with { TransactionHandling = TransactionHandling.Autonomous }; + + var actual = cfg?.Folders; + actual![RunAfterCreateDatabase]!.TransactionHandling.Should().Be(TransactionHandling.Autonomous); + AssertEquivalent(expected.Values, actual.Values); + } [Theory] [MemberData(nameof(FoldersCommandLines))] diff --git a/unittests/SqlServer/Basic_tests/SqlServerDatabase_.cs b/unittests/SqlServer/Basic_tests/SqlServerDatabase_.cs index e3759005..39335716 100644 --- a/unittests/SqlServer/Basic_tests/SqlServerDatabase_.cs +++ b/unittests/SqlServer/Basic_tests/SqlServerDatabase_.cs @@ -8,17 +8,19 @@ namespace SqlServer.Basic_tests; // ReSharper disable once InconsistentNaming public class SqlServerDatabase_(InspectableSqlServerDatabase sqlServerDatabase) { - [Fact] - public async Task Disables_pooling_if_not_explicitly_set_in_connection_string() - { - var connStr = "Server=dummy"; - var cfg = new GrateConfiguration() { ConnectionString = connStr }; - await sqlServerDatabase.InitializeConnections(cfg); - - var conn = sqlServerDatabase.GetConnection(); - var builder = new SqlConnectionStringBuilder(conn.ConnectionString); - builder.Pooling.Should().BeFalse(); - } + // [Fact(Skip = "This is turned off now, as disabling pooling explicitly by default was causing performance issues")] + // Please see the test Real_world_issues.cs#Bug232_Timeout_v1U002E4U002E0_Regression for more information, and how + // to work around this issue, setting the transaction handling for the RunAfterCreateDatabase folder to autonomous. + // public async Task Disables_pooling_if_not_explicitly_set_in_connection_string() + // { + // var connStr = "Server=dummy"; + // var cfg = new GrateConfiguration() { ConnectionString = connStr }; + // await sqlServerDatabase.InitializeConnections(cfg); + // + // var conn = sqlServerDatabase.GetConnection(); + // var builder = new SqlConnectionStringBuilder(conn.ConnectionString); + // builder.Pooling.Should().BeFalse(); + // } [Fact] public async Task Leaves_pooling_as_configured_if_set_explicitly_in_connection_string() diff --git a/unittests/SqlServer/Running_MigrationScripts/Real_world_issues.cs b/unittests/SqlServer/Running_MigrationScripts/Real_world_issues.cs new file mode 100644 index 00000000..a02271ba --- /dev/null +++ b/unittests/SqlServer/Running_MigrationScripts/Real_world_issues.cs @@ -0,0 +1,74 @@ +using grate.Configuration; +using grate.Migration; +using TestCommon.Generic.Running_MigrationScripts; +using TestCommon.TestInfrastructure; +using static grate.Configuration.KnownFolderKeys; + +namespace SqlServer.Running_MigrationScripts; + +// ReSharper disable once InconsistentNaming +/// +/// Issues that have been encountered in the real world. +/// Create tests to reproduce the issue and then fix the issue, and keep the test to ensure it doesn't regress. +/// +[Collection(nameof(SqlServerTestContainer))] +public class Real_world_issues(IGrateTestContext context, ITestOutputHelper testOutput) : MigrationsScriptsBase(context, testOutput) +{ + private const string Bug232Sql = @" +ALTER DATABASE {{DatabaseName}} SET ALLOW_SNAPSHOT_ISOLATION ON; +ALTER DATABASE {{DatabaseName}} SET READ_COMMITTED_SNAPSHOT ON"; + + /// + /// Regression in 1.4.0 made us disable connection pooling if not explicitly set in the connection string. + /// however, this makes connections a lot slower, especially if using Azure AD authentication, where obtaining + /// the token takes a while. Disabling pooling means we have to get the token every time we open a connection, + /// as the connection is actually closed, not just returned to the pool. + /// + /// To run the "RunAfterCreateDatabase" scripts in its own transaction from the command line, use the following: + /// + /// --folders=runAfterCreateDatabase=transactionHandling:autonomous + /// + /// + /// + [Fact] + public async Task Bug232_Timeout_v1U002E4U002E0_Regression() + { + // V1.4 regressed something, trying to repro + + var db = TestConfig.RandomDatabase(); + + var parent = CreateRandomTempDirectory(); + var knownFolders = FoldersConfiguration.Default(); + + // Use autonomous transactions for the RunAfterCreateDatabase folder makes this work without having to + // disable connection pooling + knownFolders[RunAfterCreateDatabase] = knownFolders[RunAfterCreateDatabase]! with { TransactionHandling = TransactionHandling.Autonomous }; + + var path = new DirectoryInfo(Path.Combine(parent.ToString(), knownFolders[RunAfterCreateDatabase]?.Path ?? throw new Exception("Config Fail"))); + + WriteSql(path, "token.sql", Bug232Sql); + + var config = GrateConfigurationBuilder.Create(Context.DefaultConfiguration) + .WithConnectionString(Context.ConnectionString(db)) + .WithFolders(knownFolders) + .WithSqlFilesDirectory(parent) + .Build(); + + await using (var migrator = Context.Migrator.WithConfiguration(config)) + { + await migrator.Migrate(); + } + + // Now drop it and do it again + config = config with + { + Drop = true + }; + + await using (var migrator = Context.Migrator.WithConfiguration(config)) + { + await migrator.Migrate(); + } + } + +} diff --git a/unittests/SqlServer/Running_MigrationScripts/TokenScripts.cs b/unittests/SqlServer/Running_MigrationScripts/TokenScripts.cs index dfed21ea..19288801 100644 --- a/unittests/SqlServer/Running_MigrationScripts/TokenScripts.cs +++ b/unittests/SqlServer/Running_MigrationScripts/TokenScripts.cs @@ -7,3 +7,4 @@ namespace SqlServer.Running_MigrationScripts; public class TokenScripts(IGrateTestContext testContext, ITestOutputHelper testOutput) : TestCommon.Generic.Running_MigrationScripts.TokenScripts(testContext, testOutput); + diff --git a/unittests/TestCommon/Generic/GenericDatabase.cs b/unittests/TestCommon/Generic/GenericDatabase.cs index 8caf212f..50438c88 100644 --- a/unittests/TestCommon/Generic/GenericDatabase.cs +++ b/unittests/TestCommon/Generic/GenericDatabase.cs @@ -161,7 +161,6 @@ protected virtual async Task CreateDatabaseFromConnectionString(string db, strin try { using var conn = Context.CreateAdminDbConnection(); - conn.Open(); try {