From 290af1583bbeee3af85d1c1ead8282821319367e Mon Sep 17 00:00:00 2001 From: John Simons Date: Mon, 7 Sep 2026 15:48:01 +1000 Subject: [PATCH 1/2] Add Database/Schema support to SQL Server and PostgreSQL persisters Each test now takes its own schema in one shared database rather than its own database, which exercises the same Database/Schema setting offered to customers. Migrations are stamped with the configured schema at generation time via a custom IMigrationsSqlGenerator, raw SQL in the dialects is qualified using the schema-aware table name helper, and the migrators require the schema to exist rather than silently creating it. SchemaName validates the value before it reaches any DDL interpolation. --- docs/testing-persistence.md | 13 ++- .../AcceptanceTestStorageConfiguration.cs | 32 ++----- ...eControl.AcceptanceTests.PostgreSql.csproj | 1 + .../AcceptanceTestStorageConfiguration.cs | 38 ++------ ...ceControl.AcceptanceTests.SqlServer.csproj | 1 + .../FullTextSearchSql.cs | 34 +++++++- .../PostgreSqlDatabaseMigrator.cs | 21 +++++ .../PostgreSqlDialect.cs | 7 ++ ...tgreSqlFailedMessageIngestionSqlDialect.cs | 32 +++---- .../PostgreSqlPersistence.cs | 24 ++++++ .../PostgreSqlRetryBatchSqlDialect.cs | 2 +- ...emaStampingNpgsqlMigrationsSqlGenerator.cs | 47 ++++++++++ .../FullTextSearchSql.cs | 73 +++++++++++++--- ...StampingSqlServerMigrationsSqlGenerator.cs | 41 +++++++++ .../SqlServerDatabaseMigrator.cs | 21 +++++ .../SqlServerDialect.cs | 7 ++ ...lServerFailedMessageIngestionSqlDialect.cs | 8 +- .../SqlServerPersistence.cs | 24 ++++++ .../SqlServerRetryBatchSqlDialect.cs | 2 +- .../EFPersistenceConfigurationBase.cs | 21 +++++ .../Abstractions/EFPersisterSettings.cs | 11 +++ .../Abstractions/SchemaName.cs | 40 +++++++++ .../DbContexts/ServiceControlDbContext.cs | 12 +++ .../Infrastructure/MigrationSchemaStamper.cs | 86 +++++++++++++++++++ .../SchemaModelCacheKeyFactory.cs | 16 ++++ .../Infrastructure/SchemaOptionsExtension.cs | 45 ++++++++++ .../SchemaQualifiedTableName.cs | 32 +++++++ .../MigrationSqlIsSchemaAwareTests.cs | 33 +++++++ .../PendingModelChangesTests.cs | 22 +++++ .../PersistenceTestsContext.cs | 25 ++---- .../PostgreSqlSharedContainer.cs | 42 ++++++++- .../SchemaMustExistTests.cs | 37 ++++++++ .../TestSchema.cs | 24 ++++++ .../MigrationSqlIsSchemaAwareTests.cs | 33 +++++++ .../PendingModelChangesTests.cs | 22 +++++ .../PersistenceTestsContext.cs | 31 ++----- .../SchemaMustExistTests.cs | 37 ++++++++ .../SqlServerSharedContainer.cs | 34 +++++++- .../TestSchema.cs | 65 ++++++++++++++ .../EFCore/SchemaNameTests.cs | 35 ++++++++ 40 files changed, 994 insertions(+), 137 deletions(-) create mode 100644 src/ServiceControl.Persistence.EFCore.PostgreSql/SchemaStampingNpgsqlMigrationsSqlGenerator.cs create mode 100644 src/ServiceControl.Persistence.EFCore.SqlServer/SchemaStampingSqlServerMigrationsSqlGenerator.cs create mode 100644 src/ServiceControl.Persistence.EFCore/Abstractions/SchemaName.cs create mode 100644 src/ServiceControl.Persistence.EFCore/Infrastructure/MigrationSchemaStamper.cs create mode 100644 src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaModelCacheKeyFactory.cs create mode 100644 src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaOptionsExtension.cs create mode 100644 src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaQualifiedTableName.cs create mode 100644 src/ServiceControl.Persistence.Tests.PostgreSql/MigrationSqlIsSchemaAwareTests.cs create mode 100644 src/ServiceControl.Persistence.Tests.PostgreSql/PendingModelChangesTests.cs create mode 100644 src/ServiceControl.Persistence.Tests.PostgreSql/SchemaMustExistTests.cs create mode 100644 src/ServiceControl.Persistence.Tests.PostgreSql/TestSchema.cs create mode 100644 src/ServiceControl.Persistence.Tests.SqlServer/MigrationSqlIsSchemaAwareTests.cs create mode 100644 src/ServiceControl.Persistence.Tests.SqlServer/PendingModelChangesTests.cs create mode 100644 src/ServiceControl.Persistence.Tests.SqlServer/SchemaMustExistTests.cs create mode 100644 src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs create mode 100644 src/ServiceControl.Persistence.Tests/EFCore/SchemaNameTests.cs diff --git a/docs/testing-persistence.md b/docs/testing-persistence.md index 52c7dde116..954196b20c 100644 --- a/docs/testing-persistence.md +++ b/docs/testing-persistence.md @@ -8,6 +8,15 @@ ServiceControl supports multiple persistence types All persistence test projects can be run with `dotnet test` against the corresponding test project in `src/`. +## Test isolation on the SQL persisters + +The SQL Server and PostgreSQL suites give each test its own **schema** in one shared database, named `sc_test_` for persistence tests and `sc_at_` for acceptance tests, and drop it on teardown. This uses the same `Database/Schema` setting that is offered to customers, so every run exercises that feature. + +Two consequences are worth knowing: + +* The connection string is used exactly as it is given, so **the database it names must already exist**. The test containers create one; a server you point the environment variable at will not. +* A test failure that leaves a process behind can leave a schema behind with it. `SELECT nspname FROM pg_namespace WHERE nspname LIKE 'sc\_%'` and `SELECT name FROM sys.schemas WHERE name LIKE 'sc[_]%'` will find any strays. + ## RavenDB RavenDB persistence tests start an embedded RavenDB instance for the duration of the test run. @@ -23,7 +32,7 @@ Build that image locally before running SQL Server persistence tests: docker buildx build --platform=linux/amd64 --tag particular/servicecontrol-testing-sqlserver:latest ./src/Scripts/Docker/servicecontrol-testing-sqlserver ``` -If you want to use an existing SQL Server instance instead of a test container, set the `ServiceControl_Persistence_SqlServer_ConnectionString` environment variable to a valid SQL Server connection string. +If you want to use an existing SQL Server instance instead of a test container, set the `ServiceControl_Persistence_SqlServer_ConnectionString` environment variable to a valid SQL Server connection string. It must name a database that exists, not `master`, because the tests create their schemas in whatever database it points at. The test container creates a `ServiceControlTests` database for this. ## PostgreSQL @@ -35,4 +44,4 @@ If you want to use an existing PostgreSQL instance instead of a test container, ServiceControl_Persistence_PostgreSql_ConnectionString ``` -to a valid PostgreSQL connection string. +to a valid PostgreSQL connection string. It must name a database that exists, because the tests create their schemas in whatever database it points at. The test container creates a `servicecontroltests` database for this, so that test schemas do not end up in the `postgres` maintenance database. diff --git a/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs b/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs index eb22d27223..76a10a305a 100644 --- a/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs +++ b/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs @@ -4,7 +4,6 @@ namespace ServiceControl.AcceptanceTests.PostgreSql; using System.IO; using System.Threading; using System.Threading.Tasks; -using Npgsql; using ServiceBus.Management.Infrastructure.Settings; using ServiceControl.AcceptanceTests.TestSupport; using ServiceControl.Persistence.EFCore.Abstractions; @@ -17,19 +16,16 @@ public class AcceptanceTestStorageConfiguration : IAcceptanceTestStorageConfigur public async Task CustomizeSettings(Settings settings, CancellationToken cancellationToken = default) { - databaseName = $"sc_at_{Guid.NewGuid():n}"; - serverConnectionString = await PostgreSqlSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false); - - var connectionStringBuilder = new NpgsqlConnectionStringBuilder(serverConnectionString) - { - Database = databaseName - }; + schema = $"sc_at_{Guid.NewGuid():n}"; + connectionString = await PostgreSqlSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false); + await TestSchema.Create(connectionString, schema, cancellationToken).ConfigureAwait(false); bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName; settings.PersisterSpecificSettings = new PostgreSqlPersisterSettings { - ConnectionString = connectionStringBuilder.ConnectionString, + ConnectionString = connectionString, + Schema = schema, ErrorRetentionPeriod = TimeSpan.FromDays(10), BodyStorage = new FileSystemBodyStorageSettings { StoragePath = bodyStoragePath } }; @@ -44,22 +40,12 @@ public async Task Cleanup(CancellationToken cancellationToken = default) try { - if (serverConnectionString == null || databaseName == null) + if (connectionString == null || schema == null) { return; } - var connection = new NpgsqlConnection(serverConnectionString); - await using (connection.ConfigureAwait(false)) - { - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); - var command = connection.CreateCommand(); - await using (command.ConfigureAwait(false)) - { - command.CommandText = $"DROP DATABASE IF EXISTS \"{databaseName}\" WITH (FORCE)"; - await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); - } - } + await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false); } finally { @@ -91,8 +77,8 @@ public void Dispose() } } - string serverConnectionString; - string databaseName; + string connectionString; + string schema; string bodyStoragePath; int cleanupStarted; } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj b/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj index 56a4054a16..718b22bc78 100644 --- a/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj +++ b/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj @@ -32,6 +32,7 @@ + diff --git a/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs b/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs index 4cf4c442c9..48270fde85 100644 --- a/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs +++ b/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs @@ -4,7 +4,6 @@ namespace ServiceControl.AcceptanceTests.SqlServer; using System.IO; using System.Threading; using System.Threading.Tasks; -using Microsoft.Data.SqlClient; using ServiceBus.Management.Infrastructure.Settings; using ServiceControl.AcceptanceTests.TestSupport; using ServiceControl.Persistence.EFCore.Abstractions; @@ -17,19 +16,16 @@ public class AcceptanceTestStorageConfiguration : IAcceptanceTestStorageConfigur public async Task CustomizeSettings(Settings settings, CancellationToken cancellationToken = default) { - databaseName = $"sc_at_{Guid.NewGuid():n}"; - serverConnectionString = await SqlServerSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false); - - var connectionStringBuilder = new SqlConnectionStringBuilder(serverConnectionString) - { - InitialCatalog = databaseName - }; + schema = $"sc_at_{Guid.NewGuid():n}"; + connectionString = await SqlServerSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false); + await TestSchema.Create(connectionString, schema, cancellationToken).ConfigureAwait(false); bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName; settings.PersisterSpecificSettings = new SqlServerPersisterSettings { - ConnectionString = connectionStringBuilder.ConnectionString, + ConnectionString = connectionString, + Schema = schema, ErrorRetentionPeriod = TimeSpan.FromDays(10), BodyStorage = new FileSystemBodyStorageSettings { StoragePath = bodyStoragePath } }; @@ -44,28 +40,12 @@ public async Task Cleanup(CancellationToken cancellationToken = default) try { - if (serverConnectionString == null || databaseName == null) + if (connectionString == null || schema == null) { return; } - var connection = new SqlConnection(serverConnectionString); - await using (connection.ConfigureAwait(false)) - { - await connection.OpenAsync(cancellationToken).ConfigureAwait(false); - var command = connection.CreateCommand(); - await using (command.ConfigureAwait(false)) - { - command.CommandText = $""" - IF DB_ID('{databaseName}') IS NOT NULL - BEGIN - ALTER DATABASE [{databaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; - DROP DATABASE [{databaseName}]; - END - """; - await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); - } - } + await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false); } finally { @@ -97,8 +77,8 @@ public void Dispose() } } - string serverConnectionString; - string databaseName; + string connectionString; + string schema; string bodyStoragePath; int cleanupStarted; } diff --git a/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj b/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj index ce485ced3f..cbc66d5b4a 100644 --- a/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj +++ b/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj @@ -32,6 +32,7 @@ + diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/FullTextSearchSql.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/FullTextSearchSql.cs index f8d25c91d4..1887885ec9 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/FullTextSearchSql.cs +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/FullTextSearchSql.cs @@ -1,5 +1,7 @@ namespace ServiceControl.Persistence.EFCore.PostgreSql; +using Microsoft.EntityFrameworkCore.Migrations.Operations; + /// /// Full text search DDL for the failed messages table. EF Core cannot model a GIN index over an /// expression, so it is applied by the AddFullTextSearch migration. The statements live here, and @@ -9,6 +11,7 @@ namespace ServiceControl.Persistence.EFCore.PostgreSql; static class FullTextSearchSql { const string IndexName = "ix_failed_messages_full_text"; + const string TableName = "failed_messages"; // 'simple' rather than 'english': message and header content is technical, stemming and // stopword removal do more harm than good. @@ -26,7 +29,34 @@ static class FullTextSearchSql public const string IndexedExpression = $"""to_tsvector('{Configuration}', headers_json || ' ' || COALESCE(body_text, '') || ' ' || replace(replace(COALESCE(message_type, ''), '.', ' '), '+', ' '))"""; - public const string Up = $"CREATE INDEX {IndexName} ON failed_messages USING GIN ({IndexedExpression})"; + public static readonly string Up = CreateIndexSql(null); + + public static readonly string Down = DropIndexSql(null); + + /// + /// Re-renders the statement the migration carries, this time with the configured schema in it. + /// Anything else is left alone: EF Core builds the migrations history table's own SQL through + /// the same generator, already pointed at the right schema. MigrationSqlIsSchemaAwareTests is + /// what catches a statement of ours that should have been listed here. + /// + public static MigrationOperation Rewrite(SqlOperation operation, string schema) => + operation.Sql switch + { + var sql when sql == Up => WithSql(operation, CreateIndexSql(schema)), + var sql when sql == Down => WithSql(operation, DropIndexSql(schema)), + _ => operation + }; + + public static bool IsHandled(string sql) => sql == Up || sql == Down; + + static string CreateIndexSql(string? schema) => + $"CREATE INDEX {IndexName} ON {Qualify(schema, TableName)} USING GIN ({IndexedExpression})"; + + // An index belongs to its table's schema, so it is the index that gets qualified here. + static string DropIndexSql(string? schema) => $"DROP INDEX IF EXISTS {Qualify(schema, IndexName)}"; + + static string Qualify(string? schema, string name) => schema is null ? name : $"\"{schema}\".{name}"; - public const string Down = $"DROP INDEX IF EXISTS {IndexName}"; + static SqlOperation WithSql(SqlOperation operation, string sql) => + new() { Sql = sql, SuppressTransaction = operation.SuppressTransaction }; } diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDatabaseMigrator.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDatabaseMigrator.cs index b0ea89fa1b..1a87d914ec 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDatabaseMigrator.cs +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDatabaseMigrator.cs @@ -14,10 +14,31 @@ public async Task ApplyMigrations(CancellationToken cancellationToken = default) var previousTimeout = dbContext.Database.GetCommandTimeout(); dbContext.Database.SetCommandTimeout(EFPersisterSettings.MigrationCommandTimeout); + await RequireSchema(cancellationToken); await dbContext.Database.MigrateAsync(cancellationToken); dbContext.Database.SetCommandTimeout(previousTimeout); logger.LogInformation("PostgreSQL database migration completed"); } + + // EF Core would create the schema on its way to creating the migrations history table, which + // would turn a misspelled Database/Schema into a silently empty instance rather than an error. + async Task RequireSchema(CancellationToken cancellationToken) + { + if (dbContext.Schema is null) + { + return; + } + + var exists = await dbContext.Database + .SqlQueryRaw("""SELECT CASE WHEN EXISTS (SELECT 1 FROM pg_namespace WHERE nspname = {0}) THEN 1 ELSE 0 END AS "Value" """, dbContext.Schema) + .SingleAsync(cancellationToken); + + if (exists == 0) + { + throw new InvalidOperationException( + $"The configured schema '{dbContext.Schema}' does not exist in the database. ServiceControl does not create schemas, the same way it does not create the database. Create the schema, grant the configured user rights on it, and run setup again."); + } + } } diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDialect.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDialect.cs index 5e387119d2..16d80e5035 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDialect.cs +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlDialect.cs @@ -4,9 +4,16 @@ namespace ServiceControl.Persistence.EFCore.PostgreSql; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Storage; using ServiceControl.Persistence.EFCore.DbContexts; +using ServiceControl.Persistence.EFCore.Infrastructure; abstract class PostgreSqlDialect { + /// + /// The delimited, schema qualified name of the table an entity is mapped to. Every statement + /// below names its target this way so that a configured schema reaches the raw SQL too. + /// + protected static string Table(ServiceControlDbContext dbContext) => SchemaQualifiedTableName.For(dbContext); + protected static async Task Execute(ServiceControlDbContext dbContext, string sql, IEnumerable rows, CancellationToken cancellationToken = default) { await using var command = dbContext.Database.GetDbConnection().CreateCommand(); diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlFailedMessageIngestionSqlDialect.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlFailedMessageIngestionSqlDialect.cs index daf82efc03..108c674083 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlFailedMessageIngestionSqlDialect.cs +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlFailedMessageIngestionSqlDialect.cs @@ -10,6 +10,8 @@ namespace ServiceControl.Persistence.EFCore.PostgreSql; // when two writers insert the same key concurrently, ON CONFLICT cannot. All references to the // target table inside DO UPDATE read the pre-update row, so the guards are consistent within one // atomic statement. Rows are chunked to keep statement texts down to a few reusable shapes. +// The target is aliased as t, so t. is the stored row and excluded. the incoming one, and the +// statement bodies stay the same whichever schema the table is in. class PostgreSqlFailedMessageIngestionSqlDialect : PostgreSqlDialect, IFailedMessageIngestionSqlDialect { public async Task UpsertFailedMessages(ServiceControlDbContext dbContext, IReadOnlyList rows, CancellationToken cancellationToken = default) @@ -19,7 +21,7 @@ public async Task UpsertFailedMessages(ServiceControlDbContext dbContext, IReadO await Execute( dbContext, $""" - INSERT INTO failed_messages ({FailedMessageColumnList}) + INSERT INTO {Table(dbContext)} AS t ({FailedMessageColumnList}) VALUES {ParameterRows(chunk.Length, FailedMessageColumns.Length)} {OnConflictUpdate} @@ -36,7 +38,7 @@ public async Task InsertGroups(ServiceControlDbContext dbContext, IReadOnlyList< await Execute( dbContext, $""" - INSERT INTO failed_message_groups (failed_message_unique_id, group_id, title, type) + INSERT INTO {Table(dbContext)} (failed_message_unique_id, group_id, title, type) VALUES {ParameterRows(chunk.Length, 4)} ON CONFLICT (failed_message_unique_id, group_id) DO NOTHING @@ -53,7 +55,7 @@ public async Task InsertMissingKnownEndpoints(ServiceControlDbContext dbContext, await Execute( dbContext, $""" - INSERT INTO known_endpoints (id, name, host_id, host, monitored) + INSERT INTO {Table(dbContext)} (id, name, host_id, host, monitored) VALUES {ParameterRows(chunk.Length, 5)} ON CONFLICT (id) DO NOTHING @@ -72,15 +74,15 @@ public async Task ResolveRetriedMessages(ServiceControlDbContext dbContext, IRea await Execute( dbContext, $""" - UPDATE failed_messages SET + UPDATE {Table(dbContext)} AS t SET status = {resolved}, status_changed_at = @p0, last_modified = @p0 FROM (VALUES {ConfirmedRetryRows(chunk.Length)} ) AS s (unique_message_id, succeeded_at) - WHERE failed_messages.unique_message_id = s.unique_message_id - AND failed_messages.last_attempted_at <= s.succeeded_at + WHERE t.unique_message_id = s.unique_message_id + AND t.last_attempted_at <= s.succeeded_at """, [[now], .. chunk.Select(retry => new object?[] { retry.UniqueMessageId, retry.SucceededAt })], cancellationToken); @@ -129,7 +131,7 @@ .. PayloadColumns // Strictly newer, where the payload columns take the incoming attempt on a tie as well. A // redelivery of the attempt already stored is not news, and must not undo a resolve or an // archive that happened after it. - const string IsNewerAttempt = "excluded.last_attempted_at > failed_messages.last_attempted_at"; + const string IsNewerAttempt = "excluded.last_attempted_at > t.last_attempted_at"; static string BuildOnConflictUpdate() { @@ -138,22 +140,22 @@ static string BuildOnConflictUpdate() var sql = new StringBuilder( $""" ON CONFLICT (unique_message_id) DO UPDATE SET - status = CASE WHEN {IsNewerAttempt} THEN {unresolved} ELSE failed_messages.status END, - status_changed_at = CASE WHEN {IsNewerAttempt} AND failed_messages.status <> {unresolved} THEN excluded.status_changed_at ELSE failed_messages.status_changed_at END, + status = CASE WHEN {IsNewerAttempt} THEN {unresolved} ELSE t.status END, + status_changed_at = CASE WHEN {IsNewerAttempt} AND t.status <> {unresolved} THEN excluded.status_changed_at ELSE t.status_changed_at END, last_modified = excluded.last_modified, - number_of_processing_attempts = failed_messages.number_of_processing_attempts - + CASE WHEN excluded.last_attempted_at <> failed_messages.last_attempted_at THEN excluded.number_of_processing_attempts ELSE 0 END, - first_time_of_failure = LEAST(failed_messages.first_time_of_failure, excluded.first_time_of_failure), - last_time_of_failure = GREATEST(failed_messages.last_time_of_failure, excluded.last_time_of_failure), + number_of_processing_attempts = t.number_of_processing_attempts + + CASE WHEN excluded.last_attempted_at <> t.last_attempted_at THEN excluded.number_of_processing_attempts ELSE 0 END, + first_time_of_failure = LEAST(t.first_time_of_failure, excluded.first_time_of_failure), + last_time_of_failure = GREATEST(t.last_time_of_failure, excluded.last_time_of_failure), """); foreach (var column in PayloadColumns) { sql.AppendLine().Append( - $" {column} = CASE WHEN excluded.last_attempted_at >= failed_messages.last_attempted_at THEN excluded.{column} ELSE failed_messages.{column} END,"); + $" {column} = CASE WHEN excluded.last_attempted_at >= t.last_attempted_at THEN excluded.{column} ELSE t.{column} END,"); } - sql.AppendLine().Append(" last_attempted_at = GREATEST(failed_messages.last_attempted_at, excluded.last_attempted_at)"); + sql.AppendLine().Append(" last_attempted_at = GREATEST(t.last_attempted_at, excluded.last_attempted_at)"); return sql.ToString(); } diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlPersistence.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlPersistence.cs index 803151fce8..a7ced14e91 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlPersistence.cs +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlPersistence.cs @@ -1,6 +1,9 @@ namespace ServiceControl.Persistence.EFCore.PostgreSql; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.Extensions.DependencyInjection; using ServiceControl.Persistence.EFCore.Abstractions; using ServiceControl.Persistence.EFCore.DbContexts; @@ -43,6 +46,15 @@ void ConfigureDbContext(IServiceCollection services) options.UseNpgsql(settings.ConnectionString, npgsqlOptions => { npgsqlOptions.CommandTimeout(settings.CommandTimeout); + + if (settings.Schema is not null) + { + // Its own history table per schema, so schemas sharing a database migrate + // independently. Without this EF leaves the history table unqualified and every + // schema reads the same one. + npgsqlOptions.MigrationsHistoryTable(HistoryRepository.DefaultTableName, settings.Schema); + } + if (settings.EnableRetryOnFailure) { npgsqlOptions.EnableRetryOnFailure( @@ -52,6 +64,18 @@ void ConfigureDbContext(IServiceCollection services) } }); + if (settings.Schema is not null) + { + ((IDbContextOptionsBuilderInfrastructure)options).AddOrUpdateExtension(new SchemaOptionsExtension(settings.Schema)); + options.ReplaceService(); + options.ReplaceService(); + + // HasDefaultSchema moves every table, so the model is meant to differ from the + // snapshot the migrations were scaffolded against. A default installation keeps the + // check, where a difference really would mean a migration is missing. + options.ConfigureWarnings(warnings => warnings.Ignore(RelationalEventId.PendingModelChangesWarning)); + } + if (settings.EnableSensitiveDataLogging) { options.EnableSensitiveDataLogging(); diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlRetryBatchSqlDialect.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlRetryBatchSqlDialect.cs index 3de74ce12a..9003d9fd68 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlRetryBatchSqlDialect.cs +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/PostgreSqlRetryBatchSqlDialect.cs @@ -13,7 +13,7 @@ public async Task InsertMissingRetryClaims(ServiceControlDbContext dbContext, IR await Execute( dbContext, $""" - INSERT INTO failed_message_retries (unique_message_id, retry_batch_id, stage_attempts) + INSERT INTO {Table(dbContext)} (unique_message_id, retry_batch_id, stage_attempts) VALUES {ParameterRows(chunk.Length, 3)} ON CONFLICT (unique_message_id) DO NOTHING diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/SchemaStampingNpgsqlMigrationsSqlGenerator.cs b/src/ServiceControl.Persistence.EFCore.PostgreSql/SchemaStampingNpgsqlMigrationsSqlGenerator.cs new file mode 100644 index 0000000000..fe2945a4e0 --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/SchemaStampingNpgsqlMigrationsSqlGenerator.cs @@ -0,0 +1,47 @@ +namespace ServiceControl.Persistence.EFCore.PostgreSql; + +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Migrations.Operations; +using Npgsql.EntityFrameworkCore.PostgreSQL.Infrastructure.Internal; +using Npgsql.EntityFrameworkCore.PostgreSQL.Migrations; +using ServiceControl.Persistence.EFCore.Infrastructure; + +/// +/// Points the scaffolded migrations at the configured schema as they are turned into SQL. Without +/// this they name no schema and every table would land wherever the search path happens to point. +/// +// Npgsql's own generator takes INpgsqlSingletonOptions, which it ships as an internal API, so +// deriving from it cannot be done without naming that type. The alternative is reimplementing +// PostgreSQL DDL generation, which is far worse. +#pragma warning disable EF1001 // Internal EF Core API usage +sealed class SchemaStampingNpgsqlMigrationsSqlGenerator( + MigrationsSqlGeneratorDependencies dependencies, + INpgsqlSingletonOptions npgsqlSingletonOptions, + IDbContextOptions contextOptions) + : NpgsqlMigrationsSqlGenerator(dependencies, npgsqlSingletonOptions) +{ + readonly string? schema = contextOptions.FindExtension()?.Schema; + + public override IReadOnlyList Generate( + IReadOnlyList operations, + IModel? model = null, + MigrationsSqlGenerationOptions options = MigrationsSqlGenerationOptions.Default) + { + if (schema is null) + { + return base.Generate(operations, model, options); + } + + MigrationOperation[] stamped = + [ + .. operations.Select(operation => operation is SqlOperation sql + ? FullTextSearchSql.Rewrite(sql, schema) + : MigrationSchemaStamper.Stamp(operation, schema)) + ]; + + return base.Generate(stamped, model, options); + } +} +#pragma warning restore EF1001 diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/FullTextSearchSql.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/FullTextSearchSql.cs index a7140f672f..e502a3d6ca 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/FullTextSearchSql.cs +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/FullTextSearchSql.cs @@ -1,5 +1,7 @@ namespace ServiceControl.Persistence.EFCore.SqlServer; +using Microsoft.EntityFrameworkCore.Migrations.Operations; + /// /// Full text search DDL for the failed messages table. EF Core has no full text support for SQL /// Server, so it is applied by the AddFullTextSearch migration. The statements live here, and not @@ -9,6 +11,7 @@ namespace ServiceControl.Persistence.EFCore.SqlServer; static class FullTextSearchSql { const string CatalogName = "ServiceControlFullTextCatalog"; + const string TableName = "FailedMessages"; // Message search is not optional, so an instance without Full-Text Search installed is not a // degraded instance, it is a broken one: every /messages/search request would fail on a missing @@ -20,40 +23,82 @@ IF SERVERPROPERTY('IsFullTextInstalled') <> 1 END """; + // A catalog belongs to the database, not to a schema, so instances configured with different + // schemas in one database share this one and can reach the CREATE together. The re-check in the + // CATCH is what makes losing that race harmless. // The statements are idempotent so that a re-run is harmless. They also cannot run inside a // transaction, so the migration passes suppressTransaction. public const string CreateCatalog = $""" IF NOT EXISTS (SELECT 1 FROM sys.fulltext_catalogs WHERE name = '{CatalogName}') BEGIN - EXEC('CREATE FULLTEXT CATALOG {CatalogName}'); + BEGIN TRY + EXEC('CREATE FULLTEXT CATALOG {CatalogName}'); + END TRY + BEGIN CATCH + IF NOT EXISTS (SELECT 1 FROM sys.fulltext_catalogs WHERE name = '{CatalogName}') THROW; + END CATCH + END + """; + + // Only when this is the last index in the catalog, otherwise rolling back one schema's + // migration would take full text search away from every other schema in the database. + public const string DropCatalog = $""" + IF EXISTS (SELECT 1 FROM sys.fulltext_catalogs WHERE name = '{CatalogName}') + AND NOT EXISTS (SELECT 1 + FROM sys.fulltext_indexes i + JOIN sys.fulltext_catalogs c ON i.fulltext_catalog_id = c.fulltext_catalog_id + WHERE c.name = '{CatalogName}') + BEGIN + DROP FULLTEXT CATALOG {CatalogName}; END """; + public static readonly string CreateIndex = CreateIndexSql(null); + + public static readonly string DropIndex = DropIndexSql(null); + + /// + /// Re-renders the statement the migration carries, this time with the configured schema in it. + /// Anything else is left alone: EF Core builds the migrations history table's own SQL through + /// the same generator, already pointed at the right schema. MigrationSqlIsSchemaAwareTests is + /// what catches a statement of ours that should have been listed here. + /// + public static MigrationOperation Rewrite(SqlOperation operation, string schema) => + operation.Sql switch + { + var sql when sql == CreateIndex => WithSql(operation, CreateIndexSql(schema)), + var sql when sql == DropIndex => WithSql(operation, DropIndexSql(schema)), + _ => operation + }; + + // The catalog statements count as handled without being rewritten: they are server and database + // scoped, so no schema reaches them. + public static bool IsHandled(string sql) => + sql == RequireFullTextSearch || sql == CreateCatalog || sql == DropCatalog || sql == CreateIndex || sql == DropIndex; + // LANGUAGE 0 (neutral) and STOPLIST = OFF keep the word breaker from applying language rules // and from dropping stopwords, both of which lose matches on technical content. // The message type needs no dedicated column here: the word breaker splits dotted names, and // the headers already carry the type. - public const string CreateIndex = $""" - IF NOT EXISTS (SELECT 1 FROM sys.fulltext_indexes WHERE object_id = OBJECT_ID('FailedMessages')) + static string CreateIndexSql(string? schema) => $""" + IF NOT EXISTS (SELECT 1 FROM sys.fulltext_indexes WHERE object_id = OBJECT_ID('{Qualify(schema)}')) BEGIN - EXEC('CREATE FULLTEXT INDEX ON FailedMessages(HeadersJson LANGUAGE 0, BodyText LANGUAGE 0) - KEY INDEX PK_FailedMessages + EXEC('CREATE FULLTEXT INDEX ON {Qualify(schema)}(HeadersJson LANGUAGE 0, BodyText LANGUAGE 0) + KEY INDEX PK_{TableName} ON {CatalogName} WITH (CHANGE_TRACKING AUTO, STOPLIST = OFF)'); END """; - public const string DropIndex = """ - IF EXISTS (SELECT 1 FROM sys.fulltext_indexes WHERE object_id = OBJECT_ID('FailedMessages')) + static string DropIndexSql(string? schema) => $""" + IF EXISTS (SELECT 1 FROM sys.fulltext_indexes WHERE object_id = OBJECT_ID('{Qualify(schema)}')) BEGIN - DROP FULLTEXT INDEX ON FailedMessages; + DROP FULLTEXT INDEX ON {Qualify(schema)}; END """; - public const string DropCatalog = $""" - IF EXISTS (SELECT 1 FROM sys.fulltext_catalogs WHERE name = '{CatalogName}') - BEGIN - DROP FULLTEXT CATALOG {CatalogName}; - END - """; + static string Qualify(string? schema) => schema is null ? TableName : $"[{schema}].[{TableName}]"; + + static SqlOperation WithSql(SqlOperation operation, string sql) => + new() { Sql = sql, SuppressTransaction = operation.SuppressTransaction }; } diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/SchemaStampingSqlServerMigrationsSqlGenerator.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/SchemaStampingSqlServerMigrationsSqlGenerator.cs new file mode 100644 index 0000000000..832ba51a13 --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/SchemaStampingSqlServerMigrationsSqlGenerator.cs @@ -0,0 +1,41 @@ +namespace ServiceControl.Persistence.EFCore.SqlServer; + +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Migrations.Operations; +using Microsoft.EntityFrameworkCore.Update; +using ServiceControl.Persistence.EFCore.Infrastructure; + +/// +/// Points the scaffolded migrations at the configured schema as they are turned into SQL. Without +/// this they name no schema and every table would land in dbo, whatever the model says. +/// +sealed class SchemaStampingSqlServerMigrationsSqlGenerator( + MigrationsSqlGeneratorDependencies dependencies, + ICommandBatchPreparer commandBatchPreparer, + IDbContextOptions contextOptions) + : SqlServerMigrationsSqlGenerator(dependencies, commandBatchPreparer) +{ + readonly string? schema = contextOptions.FindExtension()?.Schema; + + public override IReadOnlyList Generate( + IReadOnlyList operations, + IModel? model = null, + MigrationsSqlGenerationOptions options = MigrationsSqlGenerationOptions.Default) + { + if (schema is null) + { + return base.Generate(operations, model, options); + } + + MigrationOperation[] stamped = + [ + .. operations.Select(operation => operation is SqlOperation sql + ? FullTextSearchSql.Rewrite(sql, schema) + : MigrationSchemaStamper.Stamp(operation, schema)) + ]; + + return base.Generate(stamped, model, options); + } +} diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDatabaseMigrator.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDatabaseMigrator.cs index dfa0f8e4b8..bdd297c290 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDatabaseMigrator.cs +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDatabaseMigrator.cs @@ -14,10 +14,31 @@ public async Task ApplyMigrations(CancellationToken cancellationToken = default) var previousTimeout = dbContext.Database.GetCommandTimeout(); dbContext.Database.SetCommandTimeout(EFPersisterSettings.MigrationCommandTimeout); + await RequireSchema(cancellationToken); await dbContext.Database.MigrateAsync(cancellationToken); dbContext.Database.SetCommandTimeout(previousTimeout); logger.LogInformation("SQL Server database migration completed"); } + + // EF Core would create the schema on its way to creating the migrations history table, which + // would turn a misspelled Database/Schema into a silently empty instance rather than an error. + async Task RequireSchema(CancellationToken cancellationToken) + { + if (dbContext.Schema is null) + { + return; + } + + var exists = await dbContext.Database + .SqlQueryRaw("SELECT CASE WHEN SCHEMA_ID({0}) IS NULL THEN 0 ELSE 1 END AS [Value]", dbContext.Schema) + .SingleAsync(cancellationToken); + + if (exists == 0) + { + throw new InvalidOperationException( + $"The configured schema '{dbContext.Schema}' does not exist in the database. ServiceControl does not create schemas, the same way it does not create the database. Create the schema, grant the configured user rights on it, and run setup again."); + } + } } diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDialect.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDialect.cs index 8742404dce..5835c1f39c 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDialect.cs +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerDialect.cs @@ -5,9 +5,16 @@ namespace ServiceControl.Persistence.EFCore.SqlServer; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Storage; using ServiceControl.Persistence.EFCore.DbContexts; +using ServiceControl.Persistence.EFCore.Infrastructure; abstract class SqlServerDialect { + /// + /// The delimited, schema qualified name of the table an entity is mapped to. Every statement + /// below names its target this way so that a configured schema reaches the raw SQL too. + /// + protected static string Table(ServiceControlDbContext dbContext) => SchemaQualifiedTableName.For(dbContext); + protected static async Task Execute(ServiceControlDbContext dbContext, string sql, IEnumerable rows, CancellationToken cancellationToken = default) { await using var command = dbContext.Database.GetDbConnection().CreateCommand(); diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerFailedMessageIngestionSqlDialect.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerFailedMessageIngestionSqlDialect.cs index bafb3234e5..24f41d1bd9 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerFailedMessageIngestionSqlDialect.cs +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerFailedMessageIngestionSqlDialect.cs @@ -20,7 +20,7 @@ public async Task UpsertFailedMessages(ServiceControlDbContext dbContext, IReadO await Execute( dbContext, $""" - MERGE [FailedMessages] WITH (HOLDLOCK) AS t + MERGE {Table(dbContext)} WITH (HOLDLOCK) AS t USING (VALUES {ParameterRows(chunk.Length, FailedMessageColumns.Length)} ) AS s ({FailedMessageColumnList}) @@ -42,7 +42,7 @@ public async Task InsertGroups(ServiceControlDbContext dbContext, IReadOnlyList< await Execute( dbContext, $""" - MERGE [FailedMessageGroups] WITH (HOLDLOCK) AS t + MERGE {Table(dbContext)} WITH (HOLDLOCK) AS t USING (VALUES {ParameterRows(chunk.Length, 4)} ) AS s ([FailedMessageUniqueId], [GroupId], [Title], [Type]) @@ -63,7 +63,7 @@ public async Task InsertMissingKnownEndpoints(ServiceControlDbContext dbContext, await Execute( dbContext, $""" - MERGE [KnownEndpoints] WITH (HOLDLOCK) AS t + MERGE {Table(dbContext)} WITH (HOLDLOCK) AS t USING (VALUES {ParameterRows(chunk.Length, 5)} ) AS s ([Id], [Name], [HostId], [Host], [Monitored]) @@ -91,7 +91,7 @@ UPDATE t SET [Status] = {resolved}, [StatusChangedAt] = @p0, [LastModified] = @p0 - FROM [FailedMessages] AS t + FROM {Table(dbContext)} AS t INNER JOIN (VALUES {ConfirmedRetryRows(chunk.Length)} ) AS s ([UniqueMessageId], [SucceededAt]) diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerPersistence.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerPersistence.cs index 6c67df2065..79102e3723 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerPersistence.cs +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerPersistence.cs @@ -1,6 +1,9 @@ namespace ServiceControl.Persistence.EFCore.SqlServer; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.Extensions.DependencyInjection; using ServiceControl.Persistence.EFCore.Abstractions; using ServiceControl.Persistence.EFCore.DbContexts; @@ -43,6 +46,15 @@ void ConfigureDbContext(IServiceCollection services) options.UseSqlServer(settings.ConnectionString, sqlOptions => { sqlOptions.CommandTimeout(settings.CommandTimeout); + + if (settings.Schema is not null) + { + // Its own history table per schema, so schemas sharing a database migrate + // independently. Without this EF leaves the history table unqualified and every + // schema reads the same one. + sqlOptions.MigrationsHistoryTable(HistoryRepository.DefaultTableName, settings.Schema); + } + if (settings.EnableRetryOnFailure) { sqlOptions.EnableRetryOnFailure( @@ -52,6 +64,18 @@ void ConfigureDbContext(IServiceCollection services) } }); + if (settings.Schema is not null) + { + ((IDbContextOptionsBuilderInfrastructure)options).AddOrUpdateExtension(new SchemaOptionsExtension(settings.Schema)); + options.ReplaceService(); + options.ReplaceService(); + + // HasDefaultSchema moves every table, so the model is meant to differ from the + // snapshot the migrations were scaffolded against. A default installation keeps the + // check, where a difference really would mean a migration is missing. + options.ConfigureWarnings(warnings => warnings.Ignore(RelationalEventId.PendingModelChangesWarning)); + } + if (settings.EnableSensitiveDataLogging) { options.EnableSensitiveDataLogging(); diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerRetryBatchSqlDialect.cs b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerRetryBatchSqlDialect.cs index 1e8de7a9c5..b0fc5a353b 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerRetryBatchSqlDialect.cs +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/SqlServerRetryBatchSqlDialect.cs @@ -14,7 +14,7 @@ public async Task InsertMissingRetryClaims(ServiceControlDbContext dbContext, IR await Execute( dbContext, $""" - MERGE [FailedMessageRetries] WITH (HOLDLOCK) AS t + MERGE {Table(dbContext)} WITH (HOLDLOCK) AS t USING (VALUES {ParameterRows(chunk.Length, 3)} ) AS s ([UniqueMessageId], [RetryBatchId], [StageAttempts]) diff --git a/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersistenceConfigurationBase.cs b/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersistenceConfigurationBase.cs index dba12e9fe1..727aef4b98 100644 --- a/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersistenceConfigurationBase.cs +++ b/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersistenceConfigurationBase.cs @@ -7,6 +7,7 @@ namespace ServiceControl.Persistence.EFCore.Abstractions; public abstract class EFPersistenceConfigurationBase : PersistenceConfiguration, IPersistenceConfiguration { const string ConnectionStringKey = "Database/ConnectionString"; + const string SchemaKey = "Database/Schema"; const string CommandTimeoutKey = "Database/CommandTimeout"; const string BodyStorageTypeKey = "MessageBody/StorageType"; const string FileSystemStoragePathKey = "MessageBody/FileSystem/StoragePath"; @@ -37,6 +38,7 @@ public PersistenceSettings CreateSettings(SettingsRootNamespace settingsRootName GetRequiredSetting(settingsRootNamespace, ConnectionStringKey), CreateBodyStorageSettings(settingsRootNamespace)); + settings.Schema = ReadSchema(settingsRootNamespace); settings.CommandTimeout = SettingsReader.Read(settingsRootNamespace, CommandTimeoutKey, EFPersisterSettings.DefaultCommandTimeout); settings.ErrorRetentionPeriod = GetRequiredSetting(settingsRootNamespace, ErrorRetentionPeriodKey); settings.EventsRetentionPeriod = SettingsReader.Read(settingsRootNamespace, EventsRetentionPeriodKey, EFPersisterSettings.DefaultEventsRetentionPeriod); @@ -51,6 +53,25 @@ public PersistenceSettings CreateSettings(SettingsRootNamespace settingsRootName protected abstract EFPersisterSettings CreateSettings(string connectionString, BodyStorageSettings bodyStorage); + static string? ReadSchema(SettingsRootNamespace settingsRootNamespace) + { + var schema = SettingsReader.Read(settingsRootNamespace, SchemaKey); + + if (string.IsNullOrWhiteSpace(schema)) + { + return null; + } + + try + { + return SchemaName.Validate(schema); + } + catch (ArgumentException e) + { + throw new Exception($"Setting {SchemaKey} is invalid. {e.Message}", e); + } + } + static BodyStorageSettings CreateBodyStorageSettings(SettingsRootNamespace settingsRootNamespace) { var storageType = ReadBodyStorageType(settingsRootNamespace); diff --git a/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersisterSettings.cs b/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersisterSettings.cs index 52596db0a9..1d57e8de46 100644 --- a/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersisterSettings.cs +++ b/src/ServiceControl.Persistence.EFCore/Abstractions/EFPersisterSettings.cs @@ -11,6 +11,17 @@ public abstract class EFPersisterSettings : PersistenceSettings public static readonly TimeSpan DefaultSubscriptionCacheDuration = TimeSpan.FromSeconds(60); public required string ConnectionString { get; set; } + + /// + /// The schema the persister owns, or null to use the provider default. Validated on assignment + /// because it reaches raw DDL and dialect SQL by interpolation. + /// + public string? Schema + { + get; + set => field = value is null ? null : SchemaName.Validate(value); + } + public int CommandTimeout { get; set; } = DefaultCommandTimeout; public TimeSpan ErrorRetentionPeriod { get; set; } public TimeSpan EventsRetentionPeriod { get; set; } = DefaultEventsRetentionPeriod; diff --git a/src/ServiceControl.Persistence.EFCore/Abstractions/SchemaName.cs b/src/ServiceControl.Persistence.EFCore/Abstractions/SchemaName.cs new file mode 100644 index 0000000000..e2c5c4262c --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Abstractions/SchemaName.cs @@ -0,0 +1,40 @@ +namespace ServiceControl.Persistence.EFCore.Abstractions; + +using System.Text.RegularExpressions; + +/// +/// Validation for the configured database schema. A schema name cannot be passed as a parameter, +/// it is interpolated into DDL and into the raw SQL the dialects build, so the allowlist here is +/// what makes that safe. +/// +public static partial class SchemaName +{ + /// + /// PostgreSQL truncates identifiers at 63 bytes and SQL Server allows 128, so the lower limit + /// applies to both and the same configured name works on either provider. + /// + public const int MaxLength = 63; + + public static string Validate(string schema) + { + if (string.IsNullOrWhiteSpace(schema)) + { + throw new ArgumentException("A database schema name cannot be empty.", nameof(schema)); + } + + if (schema.Length > MaxLength) + { + throw new ArgumentException($"Database schema name '{schema}' is longer than the {MaxLength} character limit.", nameof(schema)); + } + + if (!AllowedName().IsMatch(schema)) + { + throw new ArgumentException($"Database schema name '{schema}' is not valid. Use a letter or an underscore followed by letters, digits or underscores.", nameof(schema)); + } + + return schema; + } + + [GeneratedRegex("^[A-Za-z_][A-Za-z0-9_]*$")] + private static partial Regex AllowedName(); +} diff --git a/src/ServiceControl.Persistence.EFCore/DbContexts/ServiceControlDbContext.cs b/src/ServiceControl.Persistence.EFCore/DbContexts/ServiceControlDbContext.cs index 09fec536e4..3717c6a7f3 100644 --- a/src/ServiceControl.Persistence.EFCore/DbContexts/ServiceControlDbContext.cs +++ b/src/ServiceControl.Persistence.EFCore/DbContexts/ServiceControlDbContext.cs @@ -3,9 +3,16 @@ namespace ServiceControl.Persistence.EFCore.DbContexts; using Microsoft.EntityFrameworkCore; using ServiceControl.Persistence.EFCore.Entities; using ServiceControl.Persistence.EFCore.EntityConfigurations; +using ServiceControl.Persistence.EFCore.Infrastructure; public abstract class ServiceControlDbContext(DbContextOptions options) : DbContext(options) { + /// + /// The configured schema, or null when the provider default is in use. Null keeps the model + /// identical to the one the migrations were scaffolded against. + /// + public string? Schema { get; } = options.FindExtension()?.Schema; + public DbSet CustomChecks { get; set; } public DbSet EndpointSettings { get; set; } public DbSet KnownEndpoints { get; set; } @@ -35,6 +42,11 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) { base.OnModelCreating(modelBuilder); + if (Schema is not null) + { + modelBuilder.HasDefaultSchema(Schema); + } + modelBuilder.ApplyConfiguration(new CustomCheckConfiguration()); modelBuilder.ApplyConfiguration(new EndpointSettingsConfiguration()); modelBuilder.ApplyConfiguration(new FailedErrorImportConfiguration()); diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/MigrationSchemaStamper.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/MigrationSchemaStamper.cs new file mode 100644 index 0000000000..6b8f35b10a --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/MigrationSchemaStamper.cs @@ -0,0 +1,86 @@ +namespace ServiceControl.Persistence.EFCore.Infrastructure; + +using Microsoft.EntityFrameworkCore.Migrations.Operations; + +/// +/// Scaffolded migrations carry no schema, so every table they name resolves to the connection's +/// default schema. Stamping the configured schema onto the operations just before they become SQL +/// is what lets one set of migrations build the schema an installation was configured with, +/// without regenerating them or making the schema a scaffold-time decision. +/// +public static class MigrationSchemaStamper +{ + /// + /// Sets the schema on the operation and on anything nested inside it, leaving a schema the + /// migration set for itself alone. Throws for an operation type that has not been considered + /// rather than letting it run against the wrong schema. + /// + public static MigrationOperation Stamp(MigrationOperation operation, string schema) + { + switch (operation) + { + case CreateTableOperation createTable: + createTable.Schema ??= schema; + foreach (var column in createTable.Columns) + { + column.Schema ??= schema; + } + createTable.PrimaryKey?.Schema ??= schema; + foreach (var foreignKey in createTable.ForeignKeys) + { + foreignKey.Schema ??= schema; + foreignKey.PrincipalSchema ??= schema; + } + foreach (var uniqueConstraint in createTable.UniqueConstraints) + { + uniqueConstraint.Schema ??= schema; + } + foreach (var checkConstraint in createTable.CheckConstraints) + { + checkConstraint.Schema ??= schema; + } + break; + + case DropTableOperation dropTable: + dropTable.Schema ??= schema; + break; + + case CreateIndexOperation createIndex: + createIndex.Schema ??= schema; + break; + + case DropIndexOperation dropIndex: + dropIndex.Schema ??= schema; + break; + + case AddColumnOperation addColumn: + addColumn.Schema ??= schema; + break; + + case AlterColumnOperation alterColumn: + alterColumn.Schema ??= schema; + alterColumn.OldColumn.Schema ??= schema; + break; + + case DropColumnOperation dropColumn: + dropColumn.Schema ??= schema; + break; + + case InsertDataOperation insertData: + insertData.Schema ??= schema; + break; + + // The schema is the operation's own subject, not something it sits inside. The history + // repository emits one of these to create the schema it keeps its table in. + case EnsureSchemaOperation: + case DropSchemaOperation: + break; + + default: + throw new InvalidOperationException( + $"Migration operation {operation.GetType().Name} is not handled by {nameof(MigrationSchemaStamper)}, so it would run against the connection's default schema instead of '{schema}'. Add a case for it."); + } + + return operation; + } +} diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaModelCacheKeyFactory.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaModelCacheKeyFactory.cs new file mode 100644 index 0000000000..ad533b0a14 --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaModelCacheKeyFactory.cs @@ -0,0 +1,16 @@ +namespace ServiceControl.Persistence.EFCore.Infrastructure; + +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using ServiceControl.Persistence.EFCore.DbContexts; + +/// +/// EF Core keys its model cache on the context type alone, so contexts configured with different +/// schemas would share one model and every one after the first would read and write the wrong +/// schema's tables. +/// +public sealed class SchemaModelCacheKeyFactory : IModelCacheKeyFactory +{ + public object Create(DbContext context, bool designTime) => + (context.GetType(), (context as ServiceControlDbContext)?.Schema, designTime); +} diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaOptionsExtension.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaOptionsExtension.cs new file mode 100644 index 0000000000..fa87c17920 --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaOptionsExtension.cs @@ -0,0 +1,45 @@ +namespace ServiceControl.Persistence.EFCore.Infrastructure; + +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.Extensions.DependencyInjection; + +/// +/// Carries the configured schema into EF Core, where the model builder and the migrations SQL +/// generator both need it. An options extension rather than a constructor argument because the +/// migrations generator is resolved from the provider's own service provider and cannot see +/// application services. Present only when a schema is configured, so a default installation +/// builds exactly the model and the SQL it did before. +/// +public sealed class SchemaOptionsExtension(string schema) : IDbContextOptionsExtension +{ + public string Schema { get; } = schema; + + public DbContextOptionsExtensionInfo Info => field ??= new ExtensionInfo(this); + + public void ApplyServices(IServiceCollection services) + { + } + + public void Validate(IDbContextOptions options) + { + } + + sealed class ExtensionInfo(SchemaOptionsExtension extension) : DbContextOptionsExtensionInfo(extension) + { + public override bool IsDatabaseProvider => false; + + public override string LogFragment => $"using schema {Extension.Schema} "; + + public override void PopulateDebugInfo(IDictionary debugInfo) => + debugInfo["ServiceControl:" + nameof(Schema)] = Extension.Schema; + + // Every schema shares one internal service provider. The services that read the schema are + // scoped and reach it through IDbContextOptions, so keying the provider on the schema would + // only build a provider per schema, which is ruinous in a test run that uses one per test. + public override int GetServiceProviderHashCode() => 0; + + public override bool ShouldUseSameServiceProvider(DbContextOptionsExtensionInfo other) => other is ExtensionInfo; + + new SchemaOptionsExtension Extension => (SchemaOptionsExtension)base.Extension; + } +} diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaQualifiedTableName.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaQualifiedTableName.cs new file mode 100644 index 0000000000..c52980818e --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/SchemaQualifiedTableName.cs @@ -0,0 +1,32 @@ +namespace ServiceControl.Persistence.EFCore.Infrastructure; + +using System.Collections.Concurrent; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Storage; + +/// +/// The delimited, schema qualified table name an entity is mapped to, for the raw SQL the dialects +/// build. Taking the name from the model rather than writing it out keeps the dialects correct +/// when the schema is configured, and keeps them from restating the naming convention that decides +/// the table names in the first place. +/// +public static class SchemaQualifiedTableName +{ + public static string For(DbContext dbContext) => + // The model cache key includes the schema, so each schema has its own model and therefore + // its own entry here. + cache.GetOrAdd((dbContext.Model, typeof(TEntity)), static (key, context) => + { + var entityType = key.Model.FindEntityType(key.EntityType) + ?? throw new InvalidOperationException($"{key.EntityType.Name} is not part of the model."); + + var tableName = entityType.GetTableName() + ?? throw new InvalidOperationException($"{key.EntityType.Name} is not mapped to a table."); + + return context.GetService().DelimitIdentifier(tableName, entityType.GetSchema()); + }, dbContext); + + static readonly ConcurrentDictionary<(IModel Model, Type EntityType), string> cache = new(); +} diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/MigrationSqlIsSchemaAwareTests.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/MigrationSqlIsSchemaAwareTests.cs new file mode 100644 index 0000000000..91447327d3 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/MigrationSqlIsSchemaAwareTests.cs @@ -0,0 +1,33 @@ +// ReSharper disable once CheckNamespace +namespace ServiceControl.Persistence.Tests; + +using System.Linq; +using EFCore.PostgreSql; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Migrations.Operations; +using Microsoft.Extensions.DependencyInjection; +using NUnit.Framework; + +class MigrationSqlIsSchemaAwareTests : PersistenceTestBase +{ + [Test] + public void Every_hand_written_migration_statement_is_schema_aware() + { + using var scope = ServiceProvider.CreateScope(); + var dbContext = scope.ServiceProvider.GetRequiredService(); + var migrations = dbContext.GetService(); + + var unrecognised = migrations.Migrations + .Select(migration => migrations.CreateMigration(migration.Value, dbContext.Database.ProviderName)) + .SelectMany(migration => migration.UpOperations.Concat(migration.DownOperations)) + .OfType() + .Select(operation => operation.Sql) + .Where(sql => !FullTextSearchSql.IsHandled(sql)) + .ToArray(); + + Assert.That(unrecognised, Is.Empty, + $"A migration runs SQL that {nameof(FullTextSearchSql)}.{nameof(FullTextSearchSql.Rewrite)} does not recognise. It would run against the connection's search path, whatever Database/Schema is set to. Add it to Rewrite, and to IsHandled if it needs no qualifying."); + } +} diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/PendingModelChangesTests.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/PendingModelChangesTests.cs new file mode 100644 index 0000000000..c6896844e4 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/PendingModelChangesTests.cs @@ -0,0 +1,22 @@ +// ReSharper disable once CheckNamespace +namespace ServiceControl.Persistence.Tests; + +using EFCore.PostgreSql; +using Microsoft.EntityFrameworkCore; +using NUnit.Framework; + +// EF Core makes this check itself during Migrate, but only when no schema is configured: a +// configured schema moves every table and so has to suppress it. Every test configures a schema, +// so without this the suites would no longer notice a model change that has no migration. +[TestFixture] +class PendingModelChangesTests +{ + [Test] + public void The_model_matches_the_migrations() + { + using var dbContext = new PostgreSqlServiceControlDbContextFactory().CreateDbContext([]); + + Assert.That(dbContext.Database.HasPendingModelChanges(), Is.False, + "The model has changed and no migration matches it. Run 'dotnet ef migrations add ' in ServiceControl.Persistence.EFCore.PostgreSql."); + } +} diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs index e17d723297..4ce91496d5 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs @@ -12,14 +12,14 @@ namespace ServiceControl.Persistence.Tests; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; -using Npgsql; using ServiceControl.Persistence.EFCore.Abstractions; using ServiceControl.Persistence.EFCore.Infrastructure; public partial class PersistenceTestsContext : IPersistenceTestsContext { IHost host; - string databaseName; + string connectionString; + string schema; string bodyStoragePath; public void AdvanceClock(TimeSpan by) => FakeTime.Advance(by); @@ -28,18 +28,16 @@ public partial class PersistenceTestsContext : IPersistenceTestsContext public async Task Setup(IHostApplicationBuilder hostBuilder) { - databaseName = $"sc_test_{Guid.NewGuid():n}"; - - var connectionStringBuilder = new NpgsqlConnectionStringBuilder(await PostgreSqlSharedContainer.GetConnectionStringAsync()) - { - Database = databaseName - }; + schema = $"sc_test_{Guid.NewGuid():n}"; + connectionString = await PostgreSqlSharedContainer.GetConnectionStringAsync(); + await TestSchema.Create(connectionString, schema); bodyStoragePath = Directory.CreateTempSubdirectory("sc_test_bodies_").FullName; PersistenceSettings = new PostgreSqlPersisterSettings { - ConnectionString = connectionStringBuilder.ConnectionString, + ConnectionString = connectionString, + Schema = schema, BodyStorage = new FileSystemBodyStorageSettings { StoragePath = bodyStoragePath }, ErrorRetentionPeriod = DefaultRetentionPeriod, EventsRetentionPeriod = DefaultRetentionPeriod @@ -58,19 +56,14 @@ public async Task PostSetup(IHost host) this.host = host; using var scope = host.Services.CreateScope(); - var db = scope.ServiceProvider.GetRequiredService(); - await db.Database.MigrateAsync(); + await scope.ServiceProvider.GetRequiredService().ApplyMigrations(); } public async Task TearDown() { DeleteBodyStorage(); - await using var connection = new NpgsqlConnection(await PostgreSqlSharedContainer.GetConnectionStringAsync()); - await connection.OpenAsync(); - await using var command = connection.CreateCommand(); - command.CommandText = $"DROP DATABASE IF EXISTS \"{databaseName}\" WITH (FORCE)"; - await command.ExecuteNonQueryAsync(); + await TestSchema.Drop(connectionString, schema); } // Drain every insert-only reconciler so that ingested data is visible to the data stores, diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/PostgreSqlSharedContainer.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/PostgreSqlSharedContainer.cs index 441c05a9e5..4c0dea908f 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/PostgreSqlSharedContainer.cs +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/PostgreSqlSharedContainer.cs @@ -3,12 +3,20 @@ namespace ServiceControl.Persistence.Tests; using System; using System.Threading; using System.Threading.Tasks; +using Npgsql; using Testcontainers.PostgreSql; static class PostgreSqlSharedContainer { const string docsPath = "docs/testing-persistence.md#postgresql"; + /// + /// Tests share one database and take a schema each, so the connection string is used as it + /// comes and the database it names has to exist. The container hands back its maintenance + /// database until this creates one to keep test schemas out of it. + /// + const string testDatabaseName = "servicecontroltests"; + public static async Task GetConnectionStringAsync(CancellationToken cancellationToken = default) { var envConnStr = Environment.GetEnvironmentVariable("ServiceControl_Persistence_PostgreSql_ConnectionString"); @@ -17,16 +25,21 @@ public static async Task GetConnectionStringAsync(CancellationToken canc return envConnStr; } - if (container != null) + if (connectionString != null) { - return container.GetConnectionString(); + return connectionString; } await semaphore.WaitAsync(cancellationToken); try { - container ??= await StartContainerAsync(cancellationToken); - return container.GetConnectionString(); + if (connectionString == null) + { + container ??= await StartContainerAsync(cancellationToken); + connectionString = await CreateTestDatabase(container.GetConnectionString(), cancellationToken); + } + + return connectionString; } finally { @@ -34,6 +47,26 @@ public static async Task GetConnectionStringAsync(CancellationToken canc } } + static async Task CreateTestDatabase(string maintenanceConnectionString, CancellationToken cancellationToken) + { + await using (var connection = new NpgsqlConnection(maintenanceConnectionString)) + { + await connection.OpenAsync(cancellationToken); + + await using var command = connection.CreateCommand(); + // CREATE DATABASE cannot run inside a transaction and has no IF NOT EXISTS, so the + // existence check is a separate statement. + command.CommandText = $"SELECT 1 FROM pg_database WHERE datname = '{testDatabaseName}'"; + if (await command.ExecuteScalarAsync(cancellationToken) is null) + { + command.CommandText = $"CREATE DATABASE {testDatabaseName}"; + await command.ExecuteNonQueryAsync(cancellationToken); + } + } + + return new NpgsqlConnectionStringBuilder(maintenanceConnectionString) { Database = testDatabaseName }.ConnectionString; + } + public static async Task Stop(CancellationToken cancellationToken = default) => await (container?.DisposeAsync() ?? ValueTask.CompletedTask); static async Task StartContainerAsync(CancellationToken cancellationToken) @@ -59,5 +92,6 @@ static async Task StartContainerAsync(CancellationToken can } static PostgreSqlContainer container; + static string connectionString; static readonly SemaphoreSlim semaphore = new(1, 1); } diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/SchemaMustExistTests.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/SchemaMustExistTests.cs new file mode 100644 index 0000000000..26f51e2a5b --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/SchemaMustExistTests.cs @@ -0,0 +1,37 @@ +// ReSharper disable once CheckNamespace +namespace ServiceControl.Persistence.Tests; + +using System; +using System.IO; +using System.Threading.Tasks; +using EFCore.PostgreSql; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using NUnit.Framework; +using ServiceControl.Persistence.EFCore.Abstractions; + +[TestFixture] +class SchemaMustExistTests +{ + [Test] + public async Task Migration_fails_when_the_configured_schema_does_not_exist() + { + var settings = new PostgreSqlPersisterSettings + { + ConnectionString = await PostgreSqlSharedContainer.GetConnectionStringAsync(), + Schema = $"sc_absent_{Guid.NewGuid():n}", + BodyStorage = new FileSystemBodyStorageSettings { StoragePath = Path.GetTempPath() } + }; + + var services = new ServiceCollection(); + services.AddLogging(); + new PostgreSqlPersistenceConfiguration().Create(settings).AddInstaller(services); + + await using var provider = services.BuildServiceProvider(); + using var scope = provider.CreateScope(); + var migrator = scope.ServiceProvider.GetRequiredService(); + + var exception = Assert.ThrowsAsync(() => migrator.ApplyMigrations()); + Assert.That(exception.Message, Does.Contain(settings.Schema).And.Contain("does not exist")); + } +} diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/TestSchema.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/TestSchema.cs new file mode 100644 index 0000000000..ced8cf43e0 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/TestSchema.cs @@ -0,0 +1,24 @@ +namespace ServiceControl.Persistence.Tests; + +using System.Threading; +using System.Threading.Tasks; +using Npgsql; + +static class TestSchema +{ + public static Task Create(string connectionString, string schema, CancellationToken cancellationToken = default) => + Execute(connectionString, $"CREATE SCHEMA IF NOT EXISTS \"{schema}\"", cancellationToken); + + public static Task Drop(string connectionString, string schema, CancellationToken cancellationToken = default) => + Execute(connectionString, $"DROP SCHEMA IF EXISTS \"{schema}\" CASCADE", cancellationToken); + + static async Task Execute(string connectionString, string sql, CancellationToken cancellationToken) + { + await using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(cancellationToken); + + await using var command = connection.CreateCommand(); + command.CommandText = sql; + await command.ExecuteNonQueryAsync(cancellationToken); + } +} diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/MigrationSqlIsSchemaAwareTests.cs b/src/ServiceControl.Persistence.Tests.SqlServer/MigrationSqlIsSchemaAwareTests.cs new file mode 100644 index 0000000000..75e56ef52e --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.SqlServer/MigrationSqlIsSchemaAwareTests.cs @@ -0,0 +1,33 @@ +// ReSharper disable once CheckNamespace +namespace ServiceControl.Persistence.Tests; + +using System.Linq; +using EFCore.SqlServer; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Migrations.Operations; +using Microsoft.Extensions.DependencyInjection; +using NUnit.Framework; + +class MigrationSqlIsSchemaAwareTests : PersistenceTestBase +{ + [Test] + public void Every_hand_written_migration_statement_is_schema_aware() + { + using var scope = ServiceProvider.CreateScope(); + var dbContext = scope.ServiceProvider.GetRequiredService(); + var migrations = dbContext.GetService(); + + var unrecognised = migrations.Migrations + .Select(migration => migrations.CreateMigration(migration.Value, dbContext.Database.ProviderName)) + .SelectMany(migration => migration.UpOperations.Concat(migration.DownOperations)) + .OfType() + .Select(operation => operation.Sql) + .Where(sql => !FullTextSearchSql.IsHandled(sql)) + .ToArray(); + + Assert.That(unrecognised, Is.Empty, + $"A migration runs SQL that {nameof(FullTextSearchSql)}.{nameof(FullTextSearchSql.Rewrite)} does not recognise. It would run against the connection's default schema, whatever Database/Schema is set to. Add it to Rewrite, and to IsHandled if it needs no qualifying."); + } +} diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/PendingModelChangesTests.cs b/src/ServiceControl.Persistence.Tests.SqlServer/PendingModelChangesTests.cs new file mode 100644 index 0000000000..cb39144845 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.SqlServer/PendingModelChangesTests.cs @@ -0,0 +1,22 @@ +// ReSharper disable once CheckNamespace +namespace ServiceControl.Persistence.Tests; + +using EFCore.SqlServer; +using Microsoft.EntityFrameworkCore; +using NUnit.Framework; + +// EF Core makes this check itself during Migrate, but only when no schema is configured: a +// configured schema moves every table and so has to suppress it. Every test configures a schema, +// so without this the suites would no longer notice a model change that has no migration. +[TestFixture] +class PendingModelChangesTests +{ + [Test] + public void The_model_matches_the_migrations() + { + using var dbContext = new SqlServerServiceControlDbContextFactory().CreateDbContext([]); + + Assert.That(dbContext.Database.HasPendingModelChanges(), Is.False, + "The model has changed and no migration matches it. Run 'dotnet ef migrations add ' in ServiceControl.Persistence.EFCore.SqlServer."); + } +} diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs index fa21a3266b..9ded3182bc 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs @@ -7,7 +7,6 @@ namespace ServiceControl.Persistence.Tests; using System.Threading.Tasks; using EFCore.SqlServer; using MessageFailures; -using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.DependencyInjection; @@ -18,7 +17,8 @@ namespace ServiceControl.Persistence.Tests; public partial class PersistenceTestsContext : IPersistenceTestsContext { IHost host; - string databaseName; + string connectionString; + string schema; string bodyStoragePath; public void AdvanceClock(TimeSpan by) => FakeTime.Advance(by); @@ -27,18 +27,16 @@ public partial class PersistenceTestsContext : IPersistenceTestsContext public async Task Setup(IHostApplicationBuilder hostBuilder) { - databaseName = $"sc_test_{Guid.NewGuid():n}"; - - var connectionStringBuilder = new SqlConnectionStringBuilder(await SqlServerSharedContainer.GetConnectionStringAsync()) - { - InitialCatalog = databaseName - }; + schema = $"sc_test_{Guid.NewGuid():n}"; + connectionString = await SqlServerSharedContainer.GetConnectionStringAsync(); + await TestSchema.Create(connectionString, schema); bodyStoragePath = Directory.CreateTempSubdirectory("sc_test_bodies_").FullName; PersistenceSettings = new SqlServerPersisterSettings { - ConnectionString = connectionStringBuilder.ConnectionString, + ConnectionString = connectionString, + Schema = schema, BodyStorage = new FileSystemBodyStorageSettings { StoragePath = bodyStoragePath }, ErrorRetentionPeriod = DefaultRetentionPeriod, EventsRetentionPeriod = DefaultRetentionPeriod @@ -57,25 +55,14 @@ public async Task PostSetup(IHost host) this.host = host; using var scope = host.Services.CreateScope(); - var db = scope.ServiceProvider.GetRequiredService(); - await db.Database.MigrateAsync(); + await scope.ServiceProvider.GetRequiredService().ApplyMigrations(); } public async Task TearDown() { DeleteBodyStorage(); - await using var connection = new SqlConnection(await SqlServerSharedContainer.GetConnectionStringAsync()); - await connection.OpenAsync(); - await using var command = connection.CreateCommand(); - command.CommandText = $""" - IF DB_ID('{databaseName}') IS NOT NULL - BEGIN - ALTER DATABASE [{databaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; - DROP DATABASE [{databaseName}]; - END - """; - await command.ExecuteNonQueryAsync(); + await TestSchema.Drop(connectionString, schema); } // Drain every insert-only reconciler so that ingested data is visible to the data stores, diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/SchemaMustExistTests.cs b/src/ServiceControl.Persistence.Tests.SqlServer/SchemaMustExistTests.cs new file mode 100644 index 0000000000..32a09c87e4 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.SqlServer/SchemaMustExistTests.cs @@ -0,0 +1,37 @@ +// ReSharper disable once CheckNamespace +namespace ServiceControl.Persistence.Tests; + +using System; +using System.IO; +using System.Threading.Tasks; +using EFCore.SqlServer; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using NUnit.Framework; +using ServiceControl.Persistence.EFCore.Abstractions; + +[TestFixture] +class SchemaMustExistTests +{ + [Test] + public async Task Migration_fails_when_the_configured_schema_does_not_exist() + { + var settings = new SqlServerPersisterSettings + { + ConnectionString = await SqlServerSharedContainer.GetConnectionStringAsync(), + Schema = $"sc_absent_{Guid.NewGuid():n}", + BodyStorage = new FileSystemBodyStorageSettings { StoragePath = Path.GetTempPath() } + }; + + var services = new ServiceCollection(); + services.AddLogging(); + new SqlServerPersistenceConfiguration().Create(settings).AddInstaller(services); + + await using var provider = services.BuildServiceProvider(); + using var scope = provider.CreateScope(); + var migrator = scope.ServiceProvider.GetRequiredService(); + + var exception = Assert.ThrowsAsync(() => migrator.ApplyMigrations()); + Assert.That(exception.Message, Does.Contain(settings.Schema).And.Contain("does not exist")); + } +} diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/SqlServerSharedContainer.cs b/src/ServiceControl.Persistence.Tests.SqlServer/SqlServerSharedContainer.cs index 35ac127bd3..18bcfcc45b 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/SqlServerSharedContainer.cs +++ b/src/ServiceControl.Persistence.Tests.SqlServer/SqlServerSharedContainer.cs @@ -3,12 +3,20 @@ namespace ServiceControl.Persistence.Tests; using System; using System.Threading; using System.Threading.Tasks; +using Microsoft.Data.SqlClient; using Testcontainers.MsSql; static class SqlServerSharedContainer { const string docsPath = "docs/testing-persistence.md#sql-server"; + /// + /// Tests share one database and take a schema each, so the connection string is used as it + /// comes and the database it names has to exist. The container has only master until this + /// creates one. + /// + const string testDatabaseName = "ServiceControlTests"; + public static async Task GetConnectionStringAsync(CancellationToken cancellationToken = default) { var envConnStr = Environment.GetEnvironmentVariable("ServiceControl_Persistence_SqlServer_ConnectionString"); @@ -17,16 +25,21 @@ public static async Task GetConnectionStringAsync(CancellationToken canc return envConnStr; } - if (container != null) + if (connectionString != null) { - return container.GetConnectionString(); + return connectionString; } await semaphore.WaitAsync(cancellationToken); try { - container ??= await StartContainerAsync(cancellationToken); - return container.GetConnectionString(); + if (connectionString == null) + { + container ??= await StartContainerAsync(cancellationToken); + connectionString = await CreateTestDatabase(container.GetConnectionString(), cancellationToken); + } + + return connectionString; } finally { @@ -57,6 +70,19 @@ static async Task StartContainerAsync(CancellationToken cancella return c; } + static async Task CreateTestDatabase(string serverConnectionString, CancellationToken cancellationToken) + { + await using var connection = new SqlConnection(serverConnectionString); + await connection.OpenAsync(cancellationToken); + + await using var command = connection.CreateCommand(); + command.CommandText = $"IF DB_ID(N'{testDatabaseName}') IS NULL CREATE DATABASE [{testDatabaseName}]"; + await command.ExecuteNonQueryAsync(cancellationToken); + + return new SqlConnectionStringBuilder(serverConnectionString) { InitialCatalog = testDatabaseName }.ConnectionString; + } + static MsSqlContainer container; + static string connectionString; static readonly SemaphoreSlim semaphore = new(1, 1); } diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs b/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs new file mode 100644 index 0000000000..6962bad30b --- /dev/null +++ b/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs @@ -0,0 +1,65 @@ +namespace ServiceControl.Persistence.Tests; + +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Data.SqlClient; + +static class TestSchema +{ + public static Task Create(string connectionString, string schema, CancellationToken cancellationToken = default) => + Execute(connectionString, CreateSchemaSql, schema, cancellationToken); + + // SQL Server has no DROP SCHEMA CASCADE, so the objects have to go first, and the full-text + // index has to go before the table it is on. + public static Task Drop(string connectionString, string schema, CancellationToken cancellationToken = default) => + Execute(connectionString, DropSchemaSql, schema, cancellationToken); + + static async Task Execute(string connectionString, string sql, string schema, CancellationToken cancellationToken) + { + await using var connection = new SqlConnection(connectionString); + await connection.OpenAsync(cancellationToken); + + await using var command = connection.CreateCommand(); + command.CommandText = sql; + command.Parameters.AddWithValue("@schema", schema); + await command.ExecuteNonQueryAsync(cancellationToken); + } + + // CREATE SCHEMA has to be the only statement in its batch, and EXEC will not take a + // concatenated expression, so the statement is built into a variable first. + const string CreateSchemaSql = """ + IF SCHEMA_ID(@schema) IS NULL + BEGIN + DECLARE @create nvarchar(max) = N'CREATE SCHEMA ' + QUOTENAME(@schema); + EXEC sp_executesql @create; + END + """; + + const string DropSchemaSql = """ + DECLARE @sql nvarchar(max) = N''; + + SELECT @sql = @sql + N'DROP FULLTEXT INDEX ON ' + QUOTENAME(s.name) + N'.' + QUOTENAME(t.name) + N';' + FROM sys.fulltext_indexes fi + JOIN sys.tables t ON fi.object_id = t.object_id + JOIN sys.schemas s ON t.schema_id = s.schema_id + WHERE s.name = @schema; + + SELECT @sql = @sql + N'ALTER TABLE ' + QUOTENAME(s.name) + N'.' + QUOTENAME(t.name) + N' DROP CONSTRAINT ' + QUOTENAME(f.name) + N';' + FROM sys.foreign_keys f + JOIN sys.tables t ON f.parent_object_id = t.object_id + JOIN sys.schemas s ON t.schema_id = s.schema_id + WHERE s.name = @schema; + + SELECT @sql = @sql + N'DROP TABLE ' + QUOTENAME(s.name) + N'.' + QUOTENAME(t.name) + N';' + FROM sys.tables t + JOIN sys.schemas s ON t.schema_id = s.schema_id + WHERE s.name = @schema; + + IF SCHEMA_ID(@schema) IS NOT NULL + BEGIN + SET @sql = @sql + N'DROP SCHEMA ' + QUOTENAME(@schema) + N';'; + END + + EXEC sp_executesql @sql, N'@schema sysname', @schema = @schema; + """; +} diff --git a/src/ServiceControl.Persistence.Tests/EFCore/SchemaNameTests.cs b/src/ServiceControl.Persistence.Tests/EFCore/SchemaNameTests.cs new file mode 100644 index 0000000000..e6c0a6cd45 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/EFCore/SchemaNameTests.cs @@ -0,0 +1,35 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using NUnit.Framework; +using ServiceControl.Persistence.EFCore.Abstractions; + +[TestFixture] +class SchemaNameTests +{ + [TestCase("dbo")] + [TestCase("public")] + [TestCase("_private")] + [TestCase("ServiceControl")] + [TestCase("sc_test_9d0d0a1d3d4b4d1ab3f1a1b2c3d4e5f6")] + public void Accepts(string schema) => Assert.That(SchemaName.Validate(schema), Is.EqualTo(schema)); + + [TestCase("", Description = "empty")] + [TestCase(" ", Description = "whitespace")] + [TestCase("1schema", Description = "leading digit")] + [TestCase("my schema", Description = "space")] + [TestCase("my-schema", Description = "hyphen")] + [TestCase("my.schema", Description = "dot")] + [TestCase("\"quoted\"", Description = "quotes")] + [TestCase("sc]; DROP TABLE FailedMessages--", Description = "injection through a closing bracket")] + [TestCase("sc'; DROP TABLE FailedMessages--", Description = "injection through a closing quote")] + public void Rejects(string schema) => Assert.Throws(() => SchemaName.Validate(schema)); + + [Test] + public void Rejects_a_name_longer_than_PostgreSql_allows() => + Assert.Throws(() => SchemaName.Validate(new string('a', SchemaName.MaxLength + 1))); + + [Test] + public void Accepts_a_name_at_the_limit() => + Assert.DoesNotThrow(() => SchemaName.Validate(new string('a', SchemaName.MaxLength))); +} From 8d9d33ab5c15690e4ef975d297f5d8bb2edfc693 Mon Sep 17 00:00:00 2001 From: John Simons Date: Wed, 9 Sep 2026 09:55:46 +1000 Subject: [PATCH 2/2] Fix schemas stranded in shared database when tests run multiple scenarios Tests that run more than one scenario call CustomizeSettings multiple times, creating a new schema each time, but the old single-field approach only tracked the most recent. When Cleanup ran, earlier schemas were left behind in the shared database. Switching to ConcurrentBag accumulates every schema and body storage path created across all scenarios, so Cleanup drains and removes all of them. The one-shot cleanupStarted guard is also removed, since iterating the bag is already idempotent. Schema setup and teardown on SQL Server also now retries on deadlock (error 1205), which occurs when many tests concurrently hit the system catalogs with DDL. DropSchema additionally switches to READ UNCOMMITTED to avoid taking shared locks on catalog reads that were themselves entering deadlocks. --- .../AcceptanceTestStorageConfiguration.cs | 31 ++++++++-------- .../AcceptanceTestStorageConfiguration.cs | 29 ++++++++------- .../TestSchema.cs | 36 +++++++++++++++---- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs b/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs index 76a10a305a..786d9b4cb4 100644 --- a/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs +++ b/src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs @@ -1,6 +1,7 @@ namespace ServiceControl.AcceptanceTests.PostgreSql; using System; +using System.Collections.Concurrent; using System.IO; using System.Threading; using System.Threading.Tasks; @@ -16,11 +17,17 @@ public class AcceptanceTestStorageConfiguration : IAcceptanceTestStorageConfigur public async Task CustomizeSettings(Settings settings, CancellationToken cancellationToken = default) { - schema = $"sc_at_{Guid.NewGuid():n}"; + var schema = $"sc_at_{Guid.NewGuid():n}"; + var bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName; + connectionString = await PostgreSqlSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false); await TestSchema.Create(connectionString, schema, cancellationToken).ConfigureAwait(false); - bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName; + // A test that runs more than one scenario comes back through here, and the runner cleans up + // after each one. Recording everything created, rather than keeping only the most recent, + // is what stops the earlier schema being stranded in the shared database. + schemas.Add(schema); + bodyStoragePaths.Add(bodyStoragePath); settings.PersisterSpecificSettings = new PostgreSqlPersisterSettings { @@ -33,23 +40,16 @@ public async Task CustomizeSettings(Settings settings, CancellationToken cancell public async Task Cleanup(CancellationToken cancellationToken = default) { - if (Interlocked.Exchange(ref cleanupStarted, 1) != 0) - { - return; - } - try { - if (connectionString == null || schema == null) + while (schemas.TryTake(out var schema)) { - return; + await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false); } - - await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false); } finally { - if (bodyStoragePath != null) + while (bodyStoragePaths.TryTake(out var bodyStoragePath)) { try { @@ -77,8 +77,7 @@ public void Dispose() } } + readonly ConcurrentBag schemas = []; + readonly ConcurrentBag bodyStoragePaths = []; string connectionString; - string schema; - string bodyStoragePath; - int cleanupStarted; -} \ No newline at end of file +} diff --git a/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs b/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs index 48270fde85..fd453c930a 100644 --- a/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs +++ b/src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs @@ -1,6 +1,7 @@ namespace ServiceControl.AcceptanceTests.SqlServer; using System; +using System.Collections.Concurrent; using System.IO; using System.Threading; using System.Threading.Tasks; @@ -16,11 +17,17 @@ public class AcceptanceTestStorageConfiguration : IAcceptanceTestStorageConfigur public async Task CustomizeSettings(Settings settings, CancellationToken cancellationToken = default) { - schema = $"sc_at_{Guid.NewGuid():n}"; + var schema = $"sc_at_{Guid.NewGuid():n}"; + var bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName; + connectionString = await SqlServerSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false); await TestSchema.Create(connectionString, schema, cancellationToken).ConfigureAwait(false); - bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName; + // A test that runs more than one scenario comes back through here, and the runner cleans up + // after each one. Recording everything created, rather than keeping only the most recent, + // is what stops the earlier schema being stranded in the shared database. + schemas.Add(schema); + bodyStoragePaths.Add(bodyStoragePath); settings.PersisterSpecificSettings = new SqlServerPersisterSettings { @@ -33,23 +40,16 @@ public async Task CustomizeSettings(Settings settings, CancellationToken cancell public async Task Cleanup(CancellationToken cancellationToken = default) { - if (Interlocked.Exchange(ref cleanupStarted, 1) != 0) - { - return; - } - try { - if (connectionString == null || schema == null) + while (schemas.TryTake(out var schema)) { - return; + await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false); } - - await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false); } finally { - if (bodyStoragePath != null) + while (bodyStoragePaths.TryTake(out var bodyStoragePath)) { try { @@ -77,8 +77,7 @@ public void Dispose() } } + readonly ConcurrentBag schemas = []; + readonly ConcurrentBag bodyStoragePaths = []; string connectionString; - string schema; - string bodyStoragePath; - int cleanupStarted; } diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs b/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs index 6962bad30b..4dcd38c039 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs +++ b/src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs @@ -1,5 +1,6 @@ namespace ServiceControl.Persistence.Tests; +using System; using System.Threading; using System.Threading.Tasks; using Microsoft.Data.SqlClient; @@ -14,15 +15,33 @@ public static Task Create(string connectionString, string schema, CancellationTo public static Task Drop(string connectionString, string schema, CancellationToken cancellationToken = default) => Execute(connectionString, DropSchemaSql, schema, cancellationToken); + // Tests share one database now, so a schema being set up or torn down contends on the system + // catalogs with every other test's CREATE and DROP, and with the transport tests, which CI points + // at the same server. SQL Server settles that by picking a victim and asking it to try again, + // which is what error 1205 means. + const int DeadlockVictim = 1205; + const int MaxAttempts = 5; + static async Task Execute(string connectionString, string sql, string schema, CancellationToken cancellationToken) { - await using var connection = new SqlConnection(connectionString); - await connection.OpenAsync(cancellationToken); + for (var attempt = 1; ; attempt++) + { + try + { + await using var connection = new SqlConnection(connectionString); + await connection.OpenAsync(cancellationToken); - await using var command = connection.CreateCommand(); - command.CommandText = sql; - command.Parameters.AddWithValue("@schema", schema); - await command.ExecuteNonQueryAsync(cancellationToken); + await using var command = connection.CreateCommand(); + command.CommandText = sql; + command.Parameters.AddWithValue("@schema", schema); + await command.ExecuteNonQueryAsync(cancellationToken); + return; + } + catch (SqlException e) when (e.Number == DeadlockVictim && attempt < MaxAttempts) + { + await Task.Delay(TimeSpan.FromMilliseconds(100 * attempt), cancellationToken); + } + } } // CREATE SCHEMA has to be the only statement in its batch, and EXEC will not take a @@ -36,6 +55,11 @@ IF SCHEMA_ID(@schema) IS NULL """; const string DropSchemaSql = """ + -- Only this test's own schema is read, and no other session creates or drops anything in it, + -- so a dirty read cannot be wrong here. It does mean the catalog reads take no shared locks, + -- which is what put them in a deadlock with other tests' DDL. + SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; + DECLARE @sql nvarchar(max) = N''; SELECT @sql = @sql + N'DROP FULLTEXT INDEX ON ' + QUOTENAME(s.name) + N'.' + QUOTENAME(t.name) + N';'