From 8f926467db257d13b14129702a44f6e5791723a8 Mon Sep 17 00:00:00 2001 From: Wokket Date: Tue, 6 Sep 2022 18:05:43 +1000 Subject: [PATCH 1/2] fix #230: Fix schema (in)sensitive casing issue --- .../Versioning_The_Database.cs | 35 +++++++++++++++++-- grate/Migration/AnsiSqlDatabase.cs | 32 ++++++++--------- grate/Migration/PostgreSqlDatabase.cs | 18 ++++++++-- grate/Migration/SqLiteDatabase.cs | 13 +++---- 4 files changed, 71 insertions(+), 27 deletions(-) diff --git a/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs b/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs index 7a880db2..5eccbed0 100644 --- a/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs +++ b/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs @@ -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]); @@ -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); @@ -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. @@ -100,4 +100,33 @@ public async Task Creates_A_New_Version_In_Progress() var version = entries.Single(x => x.version == dbVersion); version.status.Should().Be(MigrationStatus.InProgress); } + + [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. + } + } } diff --git a/grate/Migration/AnsiSqlDatabase.cs b/grate/Migration/AnsiSqlDatabase.cs index 8ff60797..74b68b2c 100644 --- a/grate/Migration/AnsiSqlDatabase.cs +++ b/grate/Migration/AnsiSqlDatabase.cs @@ -69,7 +69,7 @@ public virtual Task InitializeConnections(GrateConfiguration configuration) 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!; @@ -110,8 +110,8 @@ protected async Task RunInAutonomousTransaction(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)) @@ -124,7 +124,7 @@ protected async Task RunInAutonomousTransaction(string? connec } return res; } - + protected async Task RunInAutonomousTransaction(string? connectionString, Func func) { using (var s = new TransactionScope(TransactionScopeOption.Suppress, TransactionScopeAsyncFlowOption.Enabled)) @@ -138,7 +138,7 @@ protected async Task RunInAutonomousTransaction(string? connectionString, Func RunSchemaExists() { string sql = $"SELECT s.schema_name FROM information_schema.schemata s WHERE s.schema_name = '{SchemaName}'"; var res = await ExecuteScalarAsync(ActiveConnection, sql); - return res == SchemaName; + return res != null; // #230: If the server found a record that's good enough for us } // TODO: Change MySQL/MariaDB from using schemas to using grate_ prefix @@ -348,7 +348,7 @@ public async Task TableExists(string schemaName, string tableName) string existsSql = ExistsSql(tableSchema, fullTableName); var res = await ExecuteScalarAsync(ActiveConnection, existsSql); - + return !DBNull.Value.Equals(res) && res is not null; } @@ -453,27 +453,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, diff --git a/grate/Migration/PostgreSqlDatabase.cs b/grate/Migration/PostgreSqlDatabase.cs index 2d7e7377..5ac994e8 100644 --- a/grate/Migration/PostgreSqlDatabase.cs +++ b/grate/Migration/PostgreSqlDatabase.cs @@ -8,7 +8,7 @@ namespace grate.Migration; public class PostgreSqlDatabase : AnsiSqlDatabase { - public PostgreSqlDatabase(ILogger logger) + public PostgreSqlDatabase(ILogger logger) : base(logger, new PostgreSqlSyntax()) { } @@ -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."); } -} \ No newline at end of file + + 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); + } +} diff --git a/grate/Migration/SqLiteDatabase.cs b/grate/Migration/SqLiteDatabase.cs index c087af24..166324a2 100644 --- a/grate/Migration/SqLiteDatabase.cs +++ b/grate/Migration/SqLiteDatabase.cs @@ -10,9 +10,9 @@ namespace grate.Migration; public class SqliteDatabase : AnsiSqlDatabase { private static readonly SqliteSyntax Syntax = new(); - - - public SqliteDatabase(ILogger logger) + + + public SqliteDatabase(ILogger logger) : base(logger, Syntax) { } @@ -24,8 +24,9 @@ protected override string ExistsSql(string tableSchema, string fullTableName) => $@" SELECT name FROM sqlite_master WHERE type ='table' AND -name = '{fullTableName}'; -"; +name = '{fullTableName}' COLLATE NOCASE; +"; // #230: Correct mismatched schema casing, sqllite is case-insensitive but the string comparisons in queries _are_ case sensitive by default + protected override string ExistsSql(string tableSchema, string fullTableName, string columnName) => $@"SELECT * FROM pragma_table_info('{fullTableName}') @@ -47,7 +48,7 @@ public override Task DropDatabase() { File.Delete(db); } - + return Task.CompletedTask; } From 973fc4895f28c6bf79a0ad3f18cd30b08b765dda Mon Sep 17 00:00:00 2001 From: Tim Thompson Date: Wed, 7 Jun 2023 22:14:07 +0200 Subject: [PATCH 2/2] chore: Made test for sql serve schema issue sql-server specific. I suspect the failing tests indicate the bug is still present on other DBMS's, but I don't know enough to go chasing that right now. --- .../Versioning_The_Database.cs | 30 +--------------- .../Versioning_The_Database.cs | 35 +++++++++++++++++++ .../Versioning_The_Database.cs | 2 +- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs b/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs index 5eccbed0..f833d985 100644 --- a/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs +++ b/grate.unittests/Generic/Running_MigrationScripts/Versioning_The_Database.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using Dapper; @@ -101,32 +101,4 @@ public async Task Creates_A_New_Version_In_Progress() version.status.Should().Be(MigrationStatus.InProgress); } - [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. - } - } } diff --git a/grate.unittests/SqlServer/Running_MigrationScripts/Versioning_The_Database.cs b/grate.unittests/SqlServer/Running_MigrationScripts/Versioning_The_Database.cs index 6d4094b2..0b7797df 100644 --- a/grate.unittests/SqlServer/Running_MigrationScripts/Versioning_The_Database.cs +++ b/grate.unittests/SqlServer/Running_MigrationScripts/Versioning_The_Database.cs @@ -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. + } + } } diff --git a/grate.unittests/SqlServerCaseSensitive/Running_MigrationScripts/Versioning_The_Database.cs b/grate.unittests/SqlServerCaseSensitive/Running_MigrationScripts/Versioning_The_Database.cs index ac04af59..8d88ba4a 100644 --- a/grate.unittests/SqlServerCaseSensitive/Running_MigrationScripts/Versioning_The_Database.cs +++ b/grate.unittests/SqlServerCaseSensitive/Running_MigrationScripts/Versioning_The_Database.cs @@ -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; }