Bug #450: Re-enable using connection pooling by default for SQL server (

#451)

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; }

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 });

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))]

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()

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
+/// <summary>
+/// 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.
+/// </summary>
+[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";
+
+ /// <summary>
+ /// 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
+ /// 
+ /// </summary>
+ /// <exception cref="Exception"></exception>
+ [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();
+ }
+ }
+
+}

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);
 
+

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
  {