Skip to content

Commit

Permalink
Bug erikbra#219: Scripts that should always run, even on failure, do …
Browse files Browse the repository at this point in the history 
…not run on failure (erikbra#220)

* Fixed transaction handling - SQL connections that should run in an autonomous inner transaction
  (not affected by rollback of any other active 'outer' transactions) need to be opened after
  a TransactionScope with `TransactionScopeOptions.Suppress` is started.
* Added a few (failing) tests on scripts to run even on failure
* Fixed flow in GrateMigrator to run 'Run even on failure' scripts even if other scripts fail
  • Loading branch information
@erikbra
erikbra authored Jul 24, 2022
1 parent 6b9d3fd commit 2b5feb9
Show file tree
Hide file tree
Showing 7 changed files with 186 additions and 94 deletions.
127 changes: 88 additions & 39 deletions grate.unittests/Generic/Running_MigrationScripts/Failing_Scripts.cs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Data.Common;
using System.Data.Common;
using System.IO;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using System.Transactions;
using Dapper;
Expand Down Expand Up @@ -36,7 +37,7 @@ public async Task Aborts_the_run_giving_an_error_message()
}

[Test]
public async Task Are_Inserted_Into_ScriptRunErrors_Table()
public async Task Inserts_Failed_Scripts_Into_ScriptRunErrors_Table()
{
var db = TestConfig.RandomDatabase();

Expand Down Expand Up @@ -67,7 +68,7 @@ public async Task Are_Inserted_Into_ScriptRunErrors_Table()

scripts.Should().HaveCount(1);
}

[Test]
public void Ensure_Command_Timeout_Fires()
{
Expand Down Expand Up @@ -126,44 +127,92 @@ public void Ensure_AdminCommand_Timeout_Fires()
});
}

// This does not work for MySql/MariaDB, as it does not support DDL transactions
// [Test]
// public async Task Makes_Whole_Transaction_Rollback()
// {
// var db = TestConfig.RandomDatabase();
//
// GrateMigrator? migrator;
//
// var knownFolders = KnownFolders.In(CreateRandomTempDirectory());
// CreateDummySql(knownFolders.Up);
// CreateInvalidSql(knownFolders.Up);
//
// await using (migrator = GrateTestContext.GetMigrator(db, true, knownFolders))
// {
// try
// {
// await migrator.Migrate();
// }
// catch (DbException)
// {
// }
// }
//
// string[] scripts;
// string sql = $"SELECT script_name FROM {GrateTestContext.Syntax.TableWithSchema("grate", "ScriptsRun")}";
//
// await using (var conn = GrateTestContext.CreateDbConnection(db))
// {
// scripts = (await conn.QueryAsync<string>(sql)).ToArray();
// }
//
// scripts.Should().BeEmpty();
// }

private static void CreateInvalidSql(MigrationsFolder? folder)
[Test]
[TestCaseSource(nameof(ShouldStillBeRunOnRollback))]
public virtual async Task Still_Runs_The_Scripts_In(MigrationsFolder folder)
{
var filename = folder.Name + "_jalla1.sql";
var scripts = await RunMigration(folder, filename);
scripts.Should().Contain(filename);
}

[Test]
[TestCaseSource(nameof(ShouldNotBeRunOnRollback))]
public virtual async Task Rolls_Back_Runs_Of_Scripts_In(MigrationsFolder folder)
{
if (!Context.SupportsTransaction)
{
Assert.Ignore("DDL transactions not supported, skipping tests");
}

var scripts = await RunMigration(folder, folder.Name + "_jalla1.sql");
scripts.Should().BeEmpty();
}


private async Task<string[]> RunMigration(MigrationsFolder folder, string filename)
{
string[] scripts;

var db = TestConfig.RandomDatabase();

GrateMigrator? migrator;

var knownFolders = Folders;
CreateDummySql(folder, filename);
CreateInvalidSql(knownFolders.Up);

await using (migrator = Context.GetMigrator(db, knownFolders, true))
{
try
{
await migrator.Migrate();
}
catch (DbException)
{
}
}

string sql = $"SELECT script_name FROM {Context.Syntax.TableWithSchema("grate", "ScriptsRun")}";

await using (var conn = Context.CreateDbConnection(db))
{
scripts = (await conn.QueryAsync<string>(sql)).ToArray();
}

return scripts;
}

protected static void CreateInvalidSql(MigrationsFolder? folder)
{
var dummySql = "SELECT TOP";
var path = MakeSurePathExists(folder);
WriteSql(path, "2_failing.sql", dummySql);
}

private static readonly DirectoryInfo Root = TestConfig.CreateRandomTempDirectory();
private static readonly KnownFolders Folders = KnownFolders.In(Root);

private static readonly object?[] ShouldStillBeRunOnRollback =
{
GetTestCase(Folders.BeforeMigration), GetTestCase(Folders.AlterDatabase), GetTestCase(Folders.Permissions),
GetTestCase(Folders.AfterMigration),
};

private static readonly object?[] ShouldNotBeRunOnRollback =
{
GetTestCase(Folders.RunAfterCreateDatabase), GetTestCase(Folders.RunBeforeUp), GetTestCase(Folders.Up),
GetTestCase(Folders.RunFirstAfterUp), GetTestCase(Folders.Functions), GetTestCase(Folders.Views),
GetTestCase(Folders.Sprocs), GetTestCase(Folders.Triggers), GetTestCase(Folders.Indexes),
GetTestCase(Folders.RunAfterOtherAnyTimeScripts),
};

private static TestCaseData GetTestCase(
MigrationsFolder? folder,
[CallerArgumentExpression("folder")] string migrationsFolderDefinitionName = ""
) =>
new TestCaseData(folder)
.SetArgDisplayNames(
migrationsFolderDefinitionName
);
}
16 changes: 14 additions & 2 deletions grate.unittests/TestInfrastructure/IGrateTestContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,28 @@ public GrateMigrator GetMigrator(GrateConfiguration config)

public GrateMigrator GetMigrator(string databaseName, KnownFolders knownFolders)
{
return GetMigrator(databaseName, knownFolders, null);
return GetMigrator(databaseName, knownFolders, null, false);
}


public GrateMigrator GetMigrator(string databaseName, KnownFolders knownFolders, bool runInTransaction)
{
return GetMigrator(databaseName, knownFolders, null, runInTransaction);
}


public GrateMigrator GetMigrator(string databaseName, KnownFolders knownFolders, string? env)
{
return GetMigrator(databaseName, knownFolders, env, false);
}

public GrateMigrator GetMigrator(string databaseName, KnownFolders knownFolders, string? env, bool runInTransaction)
{
var config = DefaultConfiguration with
{
ConnectionString = ConnectionString(databaseName),
KnownFolders = knownFolders,
Environment = env != null ? new GrateEnvironment(env) : null,
Transaction = runInTransaction
};

return GetMigrator(config);
Expand Down
53 changes: 31 additions & 22 deletions grate/Migration/AnsiSqlDatabase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ public abstract class AnsiSqlDatabase : IDatabase
protected GrateConfiguration? Config { get; private set; }

protected ILogger Logger { get; }
// ReSharper disable once InconsistentNaming
protected DbConnection? _connection;
private DbConnection? _connection;
private DbConnection? _adminConnection;
private readonly ISyntax _syntax;

Expand Down Expand Up @@ -72,11 +71,34 @@ public virtual Task InitializeConnections(GrateConfiguration configuration)

public async Task OpenConnection() => await Open(Connection);
// Don't use the properties, they can open a connection just to dispose it!
public async Task CloseConnection() => await Close(_connection);
public async Task CloseConnection()
{
await Close(_connection);
_connection = null;
}

public async Task OpenAdminConnection() => await Open(AdminConnection);
// Don't use the properties, they can open a connection just to dispose it!
public async Task CloseAdminConnection() => await Close(_adminConnection);
public async Task CloseAdminConnection()
{
await Close(_adminConnection);
_adminConnection = null;
}

protected async Task<DbConnection> OpenNewConnection()
{
var conn = GetSqlConnection(ConnectionString);
await Open(conn);
return conn;
}

protected async Task<DbConnection> OpenNewAdminConnection()
{
var conn = GetSqlConnection(AdminConnectionString);
await Open(conn);
return conn;
}


public async Task CreateDatabase()
{
Expand All @@ -85,9 +107,10 @@ public async Task CreateDatabase()
Logger.LogTrace("Creating database {DatabaseName}", DatabaseName);

using var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled);
await using var conn = await OpenNewAdminConnection();
var sql = _syntax.CreateDatabase(DatabaseName, Password);

await ExecuteNonQuery(AdminConnection, sql, Config?.AdminCommandTimeout);
await ExecuteNonQuery(conn, sql, Config?.AdminCommandTimeout);
s.Complete();
}

Expand All @@ -99,11 +122,10 @@ public virtual async Task DropDatabase()
{
if (await DatabaseExists())
{
using var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled);
await CloseConnection(); // try and ensure there's nobody else in there...
await OpenAdminConnection();
await ExecuteNonQuery(AdminConnection, _syntax.DropDatabase(DatabaseName), Config?.AdminCommandTimeout);
s.Complete();
using var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled);
await using var conn = await OpenNewAdminConnection();
await ExecuteNonQuery(conn, _syntax.DropDatabase(DatabaseName), Config?.AdminCommandTimeout);
}
}

Expand All @@ -118,7 +140,6 @@ public virtual async Task<bool> DatabaseExists()

try
{
await OpenConnection();
var databases = (await Connection.QueryAsync<string>(sql)).ToArray();

Logger.LogTrace("Current databases: ");
Expand All @@ -134,10 +155,6 @@ public virtual async Task<bool> DatabaseExists()
Logger.LogDebug(e, "Got error: {ErrorMessage}", e.Message);
return false;
}
finally
{
await CloseConnection();
}
}

protected async Task WaitUntilDatabaseIsReady()
Expand All @@ -163,15 +180,10 @@ protected async Task WaitUntilDatabaseIsReady()

public async Task RunSupportTasks()
{
using (var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled))
{
await CreateRunSchema();
await CreateScriptsRunTable();
await CreateScriptsRunErrorsTable();
await CreateVersionTable();
s.Complete();
}
await CloseConnection();
}

private async Task CreateRunSchema()
Expand Down Expand Up @@ -473,10 +485,7 @@ INSERT INTO {ScriptsRunErrorsTable}
usr = Environment.UserName,
};

using var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled);
await ExecuteAsync(Connection, insertSql, scriptRunErrors);

s.Complete();
}

private static async Task Close(DbConnection? conn)
Expand Down
9 changes: 8 additions & 1 deletion grate/Migration/DbMigrator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,14 @@ private async Task RunTheActualSql(
catch (Exception ex)
{
Database.Rollback();
await Database.CloseConnection();
Transaction.Current?.Dispose();

using var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled);
await Database.OpenConnection();
await RecordScriptInScriptsRunErrorsTable(scriptName, sql, statement, ex.Message, versionId);

await Database.CloseConnection();

throw;
}
}
Expand All @@ -277,8 +280,12 @@ private void LogScriptChangedWarning(string scriptName)
private async Task OneTimeScriptChanged(string sql, string scriptName, long versionId)
{
Database.Rollback();
await Database.CloseConnection();
Transaction.Current?.Dispose();

using var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled);
await Database.OpenConnection();

string errorMessage =
$"{scriptName} has changed since the last time it was run. By default this is not allowed - scripts that run once should never change. To change this behavior to a warning, please set warnOnOneTimeScriptChanges to true and run again. Stopping execution.";
await RecordScriptInScriptsRunErrorsTable(scriptName, sql, sql, errorMessage, versionId);
Expand Down
Loading

0 comments on commit 2b5feb9

Please sign in to comment.