Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #230: Fix schema (in)sensitive casing issue #234

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Generic;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Dapper;
Expand All @@ -22,7 +22,7 @@ public async Task Returns_The_New_Version_Id()
var db = TestConfig.RandomDatabase();

GrateMigrator? migrator;

var parent = CreateRandomTempDirectory();
var knownFolders = FoldersConfiguration.Default(null);
CreateDummySql(parent, knownFolders[Sprocs]);
Expand All @@ -49,7 +49,7 @@ public async Task Does_Not_Create_Versions_When_Dryrun()
{
//for bug #204 - when running --baseline and --dryrun on a new db it shouldn't create the grate schema's etc
var db = TestConfig.RandomDatabase();

var parent = CreateRandomTempDirectory();
var knownFolders = FoldersConfiguration.Default(null);

Expand Down Expand Up @@ -80,7 +80,7 @@ public async Task Creates_A_New_Version_In_Progress()
CreateDummySql(parent, knownFolders[Up]);

long newVersionId = 0;

await using (migrator = Context.GetMigrator(db, parent, knownFolders))
{
//Calling migrate here to setup the database and such.
Expand All @@ -100,4 +100,5 @@ public async Task Creates_A_New_Version_In_Progress()
var version = entries.Single(x => x.version == dbVersion);
version.status.Should().Be(MigrationStatus.InProgress);
}

}
Original file line number Diff line number Diff line change
@@ -1,12 +1,47 @@
using System.Threading.Tasks;
using grate.Configuration;
using grate.unittests.TestInfrastructure;
using NUnit.Framework;

using static grate.Configuration.KnownFolderKeys;

namespace grate.unittests.SqlServer.Running_MigrationScripts;

[TestFixture]
[Category("SqlServer")]
[NonParallelizable]
// ReSharper disable once InconsistentNaming
public class Versioning_The_Database: Generic.Running_MigrationScripts.Versioning_The_Database
{
protected override IGrateTestContext Context => GrateTestContext.SqlServer;


[Test]
public async Task Bug230_Uses_Server_Casing_Rules_For_Schema()
{
//for bug #230 - when targeting an existing schema use the servers casing rules, not .Net's
var db = TestConfig.RandomDatabase();
var parent = CreateRandomTempDirectory();
var knownFolders = FoldersConfiguration.Default(null);

CreateDummySql(parent, knownFolders[Sprocs]); // make sure there's something that could be logged...

await using (var migrator = Context.GetMigrator(db, parent, knownFolders))
{
await migrator.Migrate();
Assert.True(await migrator.DbMigrator.Database.VersionTableExists()); // we migrated into the `grate` schema.
}

// Now we'll run again with the same name but different cased schema
var grateConfig = Context.GetConfiguration(db, parent, knownFolders) with
{
SchemaName = "GRATE"
};

await using (var migrator = Context.GetMigrator(grateConfig))
{
await migrator.Migrate(); // should either reuse the existing schema if a case-insensitive server, or create a new second schema for use if case-sensitive.
Assert.True(await migrator.DbMigrator.Database.VersionTableExists()); // we migrated into the `GRATE` schema, which may be the same as 'grate' depending on server settings.
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace grate.unittests.SqlServerCaseSensitive.Running_MigrationScripts
[TestFixture]
[Category("SqlServerCaseSensitive")]
// ReSharper disable once InconsistentNaming
public class Versioning_The_Database : Generic.Running_MigrationScripts.Versioning_The_Database
public class Versioning_The_Database : grate.unittests.SqlServer.Running_MigrationScripts.Versioning_The_Database
{
protected override IGrateTestContext Context => GrateTestContext.SqlServerCaseSensitive;
}
Expand Down
32 changes: 16 additions & 16 deletions grate/Migration/AnsiSqlDatabase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private async Task<string> ExistingOrDefault(string schemaName, string tableName
protected abstract DbConnection GetSqlConnection(string? connectionString);

protected DbConnection AdminConnection => _adminConnection ??= GetSqlConnection(AdminConnectionString);

protected DbConnection Connection => _connection ??= GetSqlConnection(ConnectionString);

public DbConnection ActiveConnection { protected get; set; } = default!;
Expand Down Expand Up @@ -129,8 +129,8 @@ protected async Task<TResult> RunInAutonomousTransaction<TResult>(string? connec
{
TResult res;
using (var s = new TransactionScope(
TransactionScopeOption.Suppress,
new TransactionOptions() { IsolationLevel = IsolationLevel.ReadUncommitted} ,
TransactionScopeOption.Suppress,
new TransactionOptions() { IsolationLevel = IsolationLevel.ReadUncommitted },
TransactionScopeAsyncFlowOption.Enabled))
{
await using (var connection = GetSqlConnection(connectionString))
Expand All @@ -143,7 +143,7 @@ protected async Task<TResult> RunInAutonomousTransaction<TResult>(string? connec
}
return res;
}

protected async Task RunInAutonomousTransaction(string? connectionString, Func<DbConnection, Task> func)
{
using (var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled))
Expand All @@ -157,7 +157,7 @@ protected async Task RunInAutonomousTransaction(string? connectionString, Func<D
s.Complete();
}
}

public async Task OpenActiveConnection()
{
await Open(ActiveConnection);
Expand Down Expand Up @@ -189,9 +189,9 @@ public async Task CreateDatabase()
if (!await DatabaseExists())
{
Logger.LogTrace("Creating database {DatabaseName}", DatabaseName);

await OpenAdminConnection();

var sql = _syntax.CreateDatabase(DatabaseName, Password);
await ExecuteNonQuery(AdminConnection, sql, Config?.AdminCommandTimeout);
}
Expand Down Expand Up @@ -275,7 +275,7 @@ private async Task<bool> RunSchemaExists()
{
string sql = $"SELECT s.SCHEMA_NAME FROM INFORMATION_SCHEMA.SCHEMATA s WHERE s.SCHEMA_NAME = '{SchemaName}'";
var res = await ExecuteScalarAsync<string>(ActiveConnection, sql);
return res == SchemaName;
return res != null; // #230: If the server found a record that's good enough for us
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be null, or can it be DBNull?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK handling the DBNull returned from the sql client is Dapper's job, and it converts to either null or default() depending on the type it's trying to map to.

If you have any info to the contrary I'd be very keen to know about it!

}

// TODO: Change MySQL/MariaDB from using schemas to using grate_ prefix
Expand Down Expand Up @@ -379,7 +379,7 @@ ALTER TABLE {VersionTable}
string existsSql = ExistsSql(tableSchema, fullTableName);

var res = await ExecuteScalarAsync<object>(ActiveConnection, existsSql);

var name = (!DBNull.Value.Equals(res) && res is not null) ? (string) res : null;

var prefix = SupportsSchemas ? string.Empty : _syntax.TableWithSchema(schemaName, string.Empty);
Expand Down Expand Up @@ -487,27 +487,27 @@ public void Rollback()

public async Task RunSql(string sql, ConnectionType connectionType, TransactionHandling transactionHandling)
{
Logger.LogTrace("[SQL] Running (on connection '{ConnType}', transaction handling '{TransactionHandling}'): \n{Sql}",
connectionType.ToString(),
Logger.LogTrace("[SQL] Running (on connection '{ConnType}', transaction handling '{TransactionHandling}'): \n{Sql}",
connectionType.ToString(),
transactionHandling,
sql);

int? timeout = GetTimeout(connectionType);
var connection = GetDbConnection(connectionType);

await ExecuteNonQuery(connection, sql, timeout);
}

private DbConnection GetDbConnection(ConnectionType connectionType) =>

private DbConnection GetDbConnection(ConnectionType connectionType) =>
connectionType switch
{
ConnectionType.Default => ActiveConnection,
ConnectionType.Admin => AdminConnection,
_ => throw new UnknownConnectionType(connectionType)
};
private int? GetTimeout(ConnectionType connectionType) =>

private int? GetTimeout(ConnectionType connectionType) =>
connectionType switch
{
ConnectionType.Default => Config?.CommandTimeout,
Expand Down
16 changes: 15 additions & 1 deletion grate/Migration/PostgreSqlDatabase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace grate.Migration;

public class PostgreSqlDatabase : AnsiSqlDatabase
{
public PostgreSqlDatabase(ILogger<PostgreSqlDatabase> logger)
public PostgreSqlDatabase(ILogger<PostgreSqlDatabase> logger)
: base(logger, new PostgreSqlSyntax())
{ }

Expand All @@ -20,4 +20,18 @@ public override Task RestoreDatabase(string backupPath)
{
throw new System.NotImplementedException("Restoring a database from file is not currently supported for Postgresql.");
}

protected override string ExistsSql(string tableSchema, string fullTableName)
{
// For #230. Postgres tables are lowercase by default unless you quote them when created, which we do. We _don't_ quote the schema though, so it will always be lowercase
// Ensure the table check uses the lowercase version of anything we're passed, as that's what we would have created.
return base.ExistsSql(tableSchema.ToLower(), fullTableName);
}

protected override string ExistsSql(string tableSchema, string fullTableName, string columnName)
{
// For #230. Postgres tables are lowercase by default unless you quote them when created, which we do. We _don't_ quote the schema though, so it will always be lowercase
// Ensure the table check uses the lowercase version of anything we're passed, as that's what we would have created.
return base.ExistsSql(tableSchema.ToLower(), fullTableName, columnName);
}
}
8 changes: 4 additions & 4 deletions grate/Migration/SqLiteDatabase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ namespace grate.Migration;
public class SqliteDatabase : AnsiSqlDatabase
{
private static readonly SqliteSyntax Syntax = new();
public SqliteDatabase(ILogger<SqliteDatabase> logger)


public SqliteDatabase(ILogger<SqliteDatabase> logger)
: base(logger, Syntax)
{ }

Expand Down Expand Up @@ -47,7 +47,7 @@ public override Task DropDatabase()
{
File.Delete(db);
}

return Task.CompletedTask;
}

Expand Down