From 8b4161d145aad36e2472e503faa1542c86711402 Mon Sep 17 00:00:00 2001 From: John Simons Date: Mon, 7 Sep 2026 15:48:01 +1000 Subject: [PATCH 1/3] 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 | 26 ++---- .../PostgreSqlSharedContainer.cs | 42 ++++++++- .../SchemaMustExistTests.cs | 37 ++++++++ .../TestSchema.cs | 24 ++++++ .../MigrationSqlIsSchemaAwareTests.cs | 33 +++++++ .../PendingModelChangesTests.cs | 22 +++++ .../PersistenceTestsContext.cs | 32 ++----- .../SchemaMustExistTests.cs | 37 ++++++++ .../SqlServerSharedContainer.cs | 34 +++++++- .../TestSchema.cs | 65 ++++++++++++++ .../EFCore/SchemaNameTests.cs | 35 ++++++++ 40 files changed, 994 insertions(+), 139 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 385b61ddc5..61062382a2 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 a053469147..fffb23c766 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 89c3c5609a..a5db30d930 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]) @@ -90,7 +90,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 62529efbf2..62a2026029 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"; @@ -35,6 +36,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); @@ -48,6 +50,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 d05de5de27..72054b7fa7 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs @@ -8,17 +8,16 @@ namespace ServiceControl.Persistence.Tests; using System.Threading.Tasks; using EFCore.PostgreSql; using MessageFailures; -using Microsoft.EntityFrameworkCore; 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); @@ -27,18 +26,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 } }; @@ -55,19 +52,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 9077dd38dd..bae0c5d18a 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs @@ -7,8 +7,6 @@ namespace ServiceControl.Persistence.Tests; using System.Threading.Tasks; using EFCore.SqlServer; using MessageFailures; -using Microsoft.Data.SqlClient; -using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using ServiceControl.Persistence.EFCore.Abstractions; @@ -17,7 +15,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); @@ -26,18 +25,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 } }; @@ -54,25 +51,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 17e1e5069a4fc3eaf9b0b72e0c4d674ac24c2991 Mon Sep 17 00:00:00 2001 From: John Simons Date: Wed, 9 Sep 2026 09:55:46 +1000 Subject: [PATCH 2/3] 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';' From 3ebda7a11156b3dd6b0baf46eb0c0eea884e8fbd Mon Sep 17 00:00:00 2001 From: John Simons Date: Wed, 9 Sep 2026 08:24:30 +1000 Subject: [PATCH 3/3] Add GitHub Actions workflows and provisioning scripts for cloud database tests Introduces two workflows and supporting PowerShell scripts to run the persistence and acceptance test suites against managed cloud databases (Azure SQL, Azure Database for PostgreSQL, Aurora PostgreSQL, RDS SQL Server). The test workflow provisions a fresh database per run, executes the relevant test projects against it, and tears it down regardless of outcome. A nightly cleanup workflow sweeps any resources tagged `sc-cloud-test` that are older than four hours. --- .../cloud-database-tests-cleanup.yml | 113 +++++++++++++++ .github/workflows/cloud-database-tests.yml | 131 ++++++++++++++++++ tools/cloud/aurora-postgresql.ps1 | 95 +++++++++++++ tools/cloud/azure-postgresql.ps1 | 64 +++++++++ tools/cloud/azure-sql.ps1 | 73 ++++++++++ tools/cloud/common.ps1 | 109 +++++++++++++++ tools/cloud/rds-sqlserver.ps1 | 90 ++++++++++++ tools/cloud/verify-sqlserver.cs | 52 +++++++ 8 files changed, 727 insertions(+) create mode 100644 .github/workflows/cloud-database-tests-cleanup.yml create mode 100644 .github/workflows/cloud-database-tests.yml create mode 100644 tools/cloud/aurora-postgresql.ps1 create mode 100644 tools/cloud/azure-postgresql.ps1 create mode 100644 tools/cloud/azure-sql.ps1 create mode 100644 tools/cloud/common.ps1 create mode 100644 tools/cloud/rds-sqlserver.ps1 create mode 100644 tools/cloud/verify-sqlserver.cs diff --git a/.github/workflows/cloud-database-tests-cleanup.yml b/.github/workflows/cloud-database-tests-cleanup.yml new file mode 100644 index 0000000000..974022efc8 --- /dev/null +++ b/.github/workflows/cloud-database-tests-cleanup.yml @@ -0,0 +1,113 @@ +name: Cloud database tests cleanup +on: + schedule: + - cron: '0 3 * * *' + workflow_dispatch: +env: + DOTNET_NOLOGO: true +defaults: + run: + shell: pwsh +jobs: + azure: + name: Azure + runs-on: ubuntu-latest + steps: + - name: Check for secrets + env: + SECRETS_AVAILABLE: ${{ secrets.SECRETS_AVAILABLE }} + run: exit $(If ($env:SECRETS_AVAILABLE -eq 'true') { 0 } Else { 1 }) + - name: Azure login + uses: azure/login@v3.0.2 + with: + creds: ${{ secrets.AZURE_CLOUD_TEST_CREDENTIALS }} + - name: Delete leaked resource groups + run: | + $cutoff = [DateTimeOffset]::UtcNow.AddHours(-4).ToUnixTimeSeconds() + $groups = az group list --tag sc-cloud-test=true --query '[].{name:name, created:tags.created}' --output json | ConvertFrom-Json + + if (-not $groups) { + Write-Output 'Nothing tagged sc-cloud-test is left in this subscription.' + return + } + + foreach ($group in $groups) { + if (-not $group.created) { + Write-Warning "Resource group $($group.name) has no created tag, so its age is unknown. Leaving it, delete it by hand." + continue + } + + if ([long]$group.created -ge $cutoff) { + Write-Output "Leaving $($group.name), it belongs to a run that may still be going." + continue + } + + Write-Output "Deleting $($group.name)" + try { + az group delete --name $group.name --yes --no-wait --only-show-errors + } + catch { + Write-Warning "Could not delete $($group.name): $($_.Exception.Message)" + } + } + + aws: + name: AWS + runs-on: ubuntu-latest + steps: + - name: Check for secrets + env: + SECRETS_AVAILABLE: ${{ secrets.SECRETS_AVAILABLE }} + run: exit $(If ($env:SECRETS_AVAILABLE -eq 'true') { 0 } Else { 1 }) + - name: Setup AWS environment variables + run: | + echo "AWS_REGION=${{ secrets.AWS_REGION }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + echo "AWS_ACCESS_KEY_ID=${{ secrets.AWS_ACCESS_KEY_ID }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + echo "AWS_SECRET_ACCESS_KEY=${{ secrets.AWS_SECRET_ACCESS_KEY }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + - name: Delete leaked RDS resources and security groups + run: | + $cutoff = [DateTimeOffset]::UtcNow.AddHours(-4).ToUnixTimeSeconds() + $tagged = aws resourcegroupstaggingapi get-resources --tag-filters Key=sc-cloud-test,Values=true --output json | ConvertFrom-Json + + $stale = @() + foreach ($resource in $tagged.ResourceTagMappingList) { + $created = ($resource.Tags | Where-Object { $_.Key -eq 'created' }).Value + + if (-not $created) { + Write-Warning "$($resource.ResourceARN) has no created tag, so its age is unknown. Leaving it, delete it by hand." + continue + } + + if ([long]$created -ge $cutoff) { + Write-Output "Leaving $($resource.ResourceARN), it belongs to a run that may still be going." + continue + } + + $stale += $resource.ResourceARN + } + + if (-not $stale) { + Write-Output 'Nothing stale is tagged sc-cloud-test in this account.' + return + } + + # Instances first, then the clusters that hold them, then the security groups the instances + # were using. A security group still in use refuses to go, and is picked up by the next run. + $order = @(':db:', ':cluster:', 'security-group/') + + foreach ($pattern in $order) { + foreach ($arn in $stale | Where-Object { $_ -like "*$pattern*" }) { + $identifier = ($arn -split '[:/]')[-1] + Write-Output "Deleting $arn" + try { + switch -Wildcard ($arn) { + '*:db:*' { aws rds delete-db-instance --db-instance-identifier $identifier --skip-final-snapshot --delete-automated-backups --no-cli-pager --output none; break } + '*:cluster:*' { aws rds delete-db-cluster --db-cluster-identifier $identifier --skip-final-snapshot --no-cli-pager --output none; break } + '*security-group/*' { aws ec2 delete-security-group --group-id $identifier --output none; break } + } + } + catch { + Write-Warning "Could not delete ${arn}: $($_.Exception.Message)" + } + } + } diff --git a/.github/workflows/cloud-database-tests.yml b/.github/workflows/cloud-database-tests.yml new file mode 100644 index 0000000000..7dc8a603eb --- /dev/null +++ b/.github/workflows/cloud-database-tests.yml @@ -0,0 +1,131 @@ +name: Cloud database tests +on: + workflow_dispatch: + inputs: + target: + description: Which managed database to test against + required: true + default: all + type: choice + options: + - all + - azure-sql + - azure-postgresql + - aurora-postgresql + - rds-sqlserver + push: + tags: + - '[0-9]+.[0-9]+.[0-9]+' + - '[0-9]+.[0-9]+.[0-9]+-*' +env: + DOTNET_NOLOGO: true +defaults: + run: + shell: pwsh +concurrency: + # Deliberately not cancel-in-progress: cancelling a run mid-flight risks skipping the teardown + # step and leaving billable cloud resources behind. + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: false +jobs: + plan: + name: Plan + runs-on: ubuntu-latest + outputs: + targets: ${{ steps.select.outputs.targets }} + steps: + - name: Select targets + id: select + run: | + $all = @( + @{ target = 'azure-sql'; provider = 'SqlServer' } + @{ target = 'azure-postgresql'; provider = 'PostgreSql' } + @{ target = 'aurora-postgresql'; provider = 'PostgreSql' } + @{ target = 'rds-sqlserver'; provider = 'SqlServer' } + ) + + # A tag push has no input, and runs everything. + $requested = '${{ inputs.target }}' + $targets = if (-not $requested -or $requested -eq 'all') { $all } else { $all | Where-Object { $_.target -eq $requested } } + + if (-not $targets) { + throw "No cloud target named '$requested'." + } + + $targets | ForEach-Object { Write-Output "Will test against $($_.target)" } + + $matrix = @{ include = @($targets) } + "targets=$(ConvertTo-Json -InputObject $matrix -Compress -Depth 4)" | Out-File -FilePath $Env:GITHUB_OUTPUT -Encoding utf8 -Append + + test: + name: ${{ matrix.target }} + needs: plan + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: ${{ fromJSON(needs.plan.outputs.targets) }} + # The two suites that exercise the persister. Named outright rather than selected by test + # category, because the category also carries the transport tests, which test the queue + # transport against its own connection string and have no business running here. + env: + PERSISTENCE_PROJECT: src/ServiceControl.Persistence.Tests.${{ matrix.provider }}/ServiceControl.Persistence.Tests.${{ matrix.provider }}.csproj + ACCEPTANCE_PROJECT: src/ServiceControl.AcceptanceTests.${{ matrix.provider }}/ServiceControl.AcceptanceTests.${{ matrix.provider }}.csproj + steps: + - name: Check for secrets + env: + SECRETS_AVAILABLE: ${{ secrets.SECRETS_AVAILABLE }} + run: exit $(If ($env:SECRETS_AVAILABLE -eq 'true') { 0 } Else { 1 }) + - name: Checkout + uses: actions/checkout@v7.0.1 + with: + fetch-depth: 0 + - name: Setup .NET SDK + uses: actions/setup-dotnet@v6.0.0 + with: + global-json-file: global.json + - name: Determine runtime version + run: | + # Read settings from Custom.Build.props + [xml]$xml = Get-Content ./src/Custom.Build.props + $runtimeVersion = $xml.selectNodes('/Project/PropertyGroup/RuntimeFrameworkVersion').InnerText + if (-not ($runtimeVersion)) { + throw "Missing RuntimeFrameworkVersion setting in Custom.Build.props" + } + echo "RuntimeVersion=$runtimeVersion" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + - name: Install .NET Runtime + shell: bash + run: | + ./tools/dotnet-install.sh --skip-non-versioned-files --install-dir /usr/share/dotnet --runtime dotnet --version ${{ env.RuntimeVersion }} + ./tools/dotnet-install.sh --skip-non-versioned-files --install-dir /usr/share/dotnet --runtime aspnetcore --version ${{ env.RuntimeVersion }} + - name: Build + id: build + background: true + run: | + dotnet build $Env:PERSISTENCE_PROJECT --configuration Release + dotnet build $Env:ACCEPTANCE_PROJECT --configuration Release + - name: Azure login + uses: azure/login@v3.0.2 + if: startsWith(matrix.target, 'azure-') + with: + creds: ${{ secrets.AZURE_CLOUD_TEST_CREDENTIALS }} + - name: Setup AWS environment variables + if: startsWith(matrix.target, 'aurora-') || startsWith(matrix.target, 'rds-') + run: | + echo "AWS_REGION=${{ secrets.AWS_REGION }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + echo "AWS_ACCESS_KEY_ID=${{ secrets.AWS_ACCESS_KEY_ID }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + echo "AWS_SECRET_ACCESS_KEY=${{ secrets.AWS_SECRET_ACCESS_KEY }}" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append + # Unlike ci.yml, which holds its cloud resources back until the build is done, provisioning + # starts first: it is the long pole here at anywhere from 3 to 25 minutes, and the teardown + # step runs on failure regardless. + - name: Provision database + run: ./tools/cloud/${{ matrix.target }}.ps1 -Action Provision -Name sc-ct-${{ github.run_id }} + - name: Wait for build + wait: build + - name: Run tests + run: ./tools/run-tests.ps1 -Projects "$Env:PERSISTENCE_PROJECT`n$Env:ACCEPTANCE_PROJECT" -MaxParallel 2 + env: + ServiceControl_TESTS_FILTER: ${{ matrix.provider }} + PARTICULARSOFTWARE_LICENSE: ${{ secrets.LICENSETEXT }} + - name: Tear down database + if: always() + run: ./tools/cloud/${{ matrix.target }}.ps1 -Action Teardown -Name sc-ct-${{ github.run_id }} diff --git a/tools/cloud/aurora-postgresql.ps1 b/tools/cloud/aurora-postgresql.ps1 new file mode 100644 index 0000000000..464943b62f --- /dev/null +++ b/tools/cloud/aurora-postgresql.ps1 @@ -0,0 +1,95 @@ +# Provisions and tears down an Aurora PostgreSQL cluster for one run of the cloud database tests. + +param( + [Parameter(Mandatory)][ValidateSet('Provision', 'Teardown')][string]$Action, + [Parameter(Mandatory)][string]$Name, + [string]$DatabaseName = 'servicecontrol' +) + +. $PSScriptRoot/common.ps1 + +$instance = "$Name-1" +$adminUser = 'sctestadmin' + +if ($Action -eq 'Teardown') { + # Without waiting for the deletions to finish: an RDS instance takes minutes to disappear, and + # holding the job open for that costs more than the scheduled cleanup workflow does. + Invoke-Teardown "Deleting instance $instance" { + aws rds delete-db-instance --db-instance-identifier $instance --skip-final-snapshot --delete-automated-backups --no-cli-pager --output none + } + + Invoke-Teardown "Deleting cluster $Name" { + aws rds delete-db-cluster --db-cluster-identifier $Name --skip-final-snapshot --no-cli-pager --output none + } + + # Will refuse while the instance still holds it, which is the normal case. The cleanup workflow + # sweeps up whatever is left. + Invoke-Teardown "Deleting security group $Name" { + $groupId = aws ec2 describe-security-groups --filters "Name=group-name,Values=$Name" --query 'SecurityGroups[0].GroupId' --output text + if ($groupId -and $groupId -ne 'None') { + aws ec2 delete-security-group --group-id $groupId --output none + } + } + + return +} + +$password = New-AdminPassword +$runnerIp = Get-RunnerIpAddress +$created = Get-CreatedTimestamp + +$vpcId = aws ec2 describe-vpcs --filters 'Name=isDefault,Values=true' --query 'Vpcs[0].VpcId' --output text +if (-not $vpcId -or $vpcId -eq 'None') { + throw 'This AWS account has no default VPC in this region, so there is no public subnet group for the cluster to use.' +} + +Write-Step "Creating security group $Name in $vpcId, allowing $runnerIp on 5432" +$groupId = aws ec2 create-security-group ` + --group-name $Name ` + --description 'ServiceControl cloud database tests' ` + --vpc-id $vpcId ` + --tag-specifications "ResourceType=security-group,Tags=[{Key=sc-cloud-test,Value=true},{Key=run-id,Value=$Name},{Key=created,Value=$created}]" ` + --query GroupId --output text + +aws ec2 authorize-security-group-ingress ` + --group-id $groupId ` + --protocol tcp ` + --port 5432 ` + --cidr "$runnerIp/32" ` + --output none + +# The engine version is left to the AWS default so that this does not break every time a pinned +# minor version is retired. +Write-Step "Creating Aurora PostgreSQL cluster $Name" +aws rds create-db-cluster ` + --db-cluster-identifier $Name ` + --engine aurora-postgresql ` + --master-username $adminUser ` + --master-user-password $password ` + --database-name $DatabaseName ` + --vpc-security-group-ids $groupId ` + --no-deletion-protection ` + --backup-retention-period 1 ` + --tags "Key=sc-cloud-test,Value=true" "Key=run-id,Value=$Name" "Key=created,Value=$created" ` + --no-cli-pager --output none + +Write-Step "Creating instance $instance" +aws rds create-db-instance ` + --db-instance-identifier $instance ` + --db-cluster-identifier $Name ` + --engine aurora-postgresql ` + --db-instance-class db.t4g.medium ` + --publicly-accessible ` + --tags "Key=sc-cloud-test,Value=true" "Key=run-id,Value=$Name" "Key=created,Value=$created" ` + --no-cli-pager --output none + +Write-Step 'Waiting for the instance to become available' +aws rds wait db-instance-available --db-instance-identifier $instance + +$endpoint = aws rds describe-db-clusters --db-cluster-identifier $Name --query 'DBClusters[0].Endpoint' --output text + +# Trust Server Certificate because RDS presents an Amazon CA that is not in the runner's trust store. +# The connection is still encrypted; only the certificate chain goes unverified. +$connectionString = "Host=$endpoint;Port=5432;Database=$DatabaseName;Username=$adminUser;Password=$password;Ssl Mode=Require;Trust Server Certificate=true;Timeout=60" + +Set-PersistenceConnectionString -Provider PostgreSql -ConnectionString $connectionString diff --git a/tools/cloud/azure-postgresql.ps1 b/tools/cloud/azure-postgresql.ps1 new file mode 100644 index 0000000000..2a1bee0dfc --- /dev/null +++ b/tools/cloud/azure-postgresql.ps1 @@ -0,0 +1,64 @@ +# Provisions and tears down an Azure Database for PostgreSQL flexible server for one run of the +# cloud database tests. +# +# Everything lives in a single resource group named after the run, so teardown is one call and a +# partly provisioned run cleans up as completely as a successful one. + +param( + [Parameter(Mandatory)][ValidateSet('Provision', 'Teardown')][string]$Action, + [Parameter(Mandatory)][string]$Name, + [string]$Location = 'eastus2', + [string]$DatabaseName = 'servicecontrol' +) + +. $PSScriptRoot/common.ps1 + +$resourceGroup = $Name +$server = $Name +$adminUser = 'sctestadmin' + +if ($Action -eq 'Teardown') { + Invoke-Teardown "Deleting resource group $resourceGroup" { + az group delete --name $resourceGroup --yes --no-wait --only-show-errors + } + return +} + +$password = New-AdminPassword +$runnerIp = Get-RunnerIpAddress +$created = Get-CreatedTimestamp + +Write-Step "Creating resource group $resourceGroup in $Location" +az group create ` + --name $resourceGroup ` + --location $Location ` + --tags sc-cloud-test=true "run-id=$Name" "created=$created" ` + --only-show-errors --output none + +# Burstable B1ms is the cheapest tier, and the suites are latency bound rather than CPU bound. +# --public-access opens the firewall to just this runner as part of creation. +Write-Step "Creating PostgreSQL flexible server $server, allowing $runnerIp" +az postgres flexible-server create ` + --name $server ` + --resource-group $resourceGroup ` + --location $Location ` + --admin-user $adminUser ` + --admin-password $password ` + --tier Burstable ` + --sku-name Standard_B1ms ` + --storage-size 32 ` + --version 16 ` + --public-access $runnerIp ` + --yes ` + --only-show-errors --output none + +Write-Step "Creating database $DatabaseName" +az postgres flexible-server db create ` + --database-name $DatabaseName ` + --resource-group $resourceGroup ` + --server-name $server ` + --only-show-errors --output none + +$connectionString = "Host=$server.postgres.database.azure.com;Port=5432;Database=$DatabaseName;Username=$adminUser;Password=$password;Ssl Mode=Require;Timeout=60" + +Set-PersistenceConnectionString -Provider PostgreSql -ConnectionString $connectionString diff --git a/tools/cloud/azure-sql.ps1 b/tools/cloud/azure-sql.ps1 new file mode 100644 index 0000000000..c46e386196 --- /dev/null +++ b/tools/cloud/azure-sql.ps1 @@ -0,0 +1,73 @@ +# Provisions and tears down an Azure SQL Database for one run of the cloud database tests. +# +# Everything lives in a single resource group named after the run, so teardown is one call and a +# partly provisioned run cleans up as completely as a successful one. + +param( + [Parameter(Mandatory)][ValidateSet('Provision', 'Teardown')][string]$Action, + [Parameter(Mandatory)][string]$Name, + [string]$Location = 'eastus2', + [string]$DatabaseName = 'servicecontrol' +) + +. $PSScriptRoot/common.ps1 + +$resourceGroup = $Name +$server = $Name +$adminUser = 'sctestadmin' + +if ($Action -eq 'Teardown') { + # One call takes the server, the database and the firewall rule with it. --no-wait because + # nothing later in the run depends on the deletion having finished. + Invoke-Teardown "Deleting resource group $resourceGroup" { + az group delete --name $resourceGroup --yes --no-wait --only-show-errors + } + return +} + +$password = New-AdminPassword +$runnerIp = Get-RunnerIpAddress +$created = Get-CreatedTimestamp + +Write-Step "Creating resource group $resourceGroup in $Location" +az group create ` + --name $resourceGroup ` + --location $Location ` + --tags sc-cloud-test=true "run-id=$Name" "created=$created" ` + --only-show-errors --output none + +Write-Step "Creating SQL server $server" +az sql server create ` + --name $server ` + --resource-group $resourceGroup ` + --location $Location ` + --admin-user $adminUser ` + --admin-password $password ` + --only-show-errors --output none + +Write-Step "Allowing $runnerIp through the server firewall" +az sql server firewall-rule create ` + --name github-runner ` + --resource-group $resourceGroup ` + --server $server ` + --start-ip-address $runnerIp ` + --end-ip-address $runnerIp ` + --only-show-errors --output none + +# S0 is the cheapest tier that still provisions in a couple of minutes. Local backup redundancy +# because the database does not outlive the run. +Write-Step "Creating database $DatabaseName" +az sql db create ` + --name $DatabaseName ` + --resource-group $resourceGroup ` + --server $server ` + --service-objective S0 ` + --backup-storage-redundancy Local ` + --only-show-errors --output none + +$connectionString = "Server=tcp:$server.database.windows.net,1433;Initial Catalog=$DatabaseName;User ID=$adminUser;Password=$password;Encrypt=True;TrustServerCertificate=False;Connect Timeout=60" + +Write-Step 'Verifying the database is reachable and has Full-Text Search' +Invoke-SqlServerVerification -ConnectionString $connectionString + +Set-PersistenceConnectionString -Provider SqlServer -ConnectionString $connectionString diff --git a/tools/cloud/common.ps1 b/tools/cloud/common.ps1 new file mode 100644 index 0000000000..91f6dc225a --- /dev/null +++ b/tools/cloud/common.ps1 @@ -0,0 +1,109 @@ +# Shared helpers for the cloud database provisioning scripts in this folder. Dot-sourced, not run +# directly. + +# Native commands (az, aws) fail by returning a non-zero exit code rather than throwing, and a +# provisioning script that carries on after a failed step leaves half-built infrastructure behind. +$PSNativeCommandUseErrorActionPreference = $true +$ErrorActionPreference = 'Stop' + +function Write-Step { + param([Parameter(Mandatory)][string]$Message) + + Write-Output "==> $Message" +} + +# Satisfies the complexity rules of all four services at once, and avoids the characters that would +# have to be escaped in a connection string or are rejected outright by RDS (/ " @ and space). +function New-AdminPassword { + $upper = 'ABCDEFGHJKLMNPQRSTUVWXYZ' + $lower = 'abcdefghijkmnpqrstuvwxyz' + $digit = '23456789' + $symbol = '!#$%*()-_+' + $all = $upper + $lower + $digit + $symbol + + $characters = @( + Get-Random -InputObject $upper.ToCharArray() + Get-Random -InputObject $lower.ToCharArray() + Get-Random -InputObject $digit.ToCharArray() + Get-Random -InputObject $symbol.ToCharArray() + ) + $characters += 1..24 | ForEach-Object { Get-Random -InputObject $all.ToCharArray() } + + $password = -join ($characters | Sort-Object { Get-Random }) + + # Straight to stdout rather than through the output stream, which the caller is capturing as the + # return value. Everything the password is later embedded in, the connection string included, is + # redacted along with it. + [Console]::WriteLine("::add-mask::$password") + + return $password +} + +# The servers are reachable from the internet, so each one is firewalled to the single address this +# job connects from. +function Get-RunnerIpAddress { + try { + return (Invoke-RestMethod -Uri 'https://api.ipify.org' -TimeoutSec 30).Trim() + } + catch { + throw "Could not determine the runner's public IP address, which is needed to open the database firewall to it. $($_.Exception.Message)" + } +} + +# Neither an Azure resource group nor an EC2 security group records when it was created, so the +# scheduled cleanup workflow needs the provisioning scripts to stamp it. Epoch seconds because both +# clouds restrict the characters a tag value may contain. +function Get-CreatedTimestamp { + return [DateTimeOffset]::UtcNow.ToUnixTimeSeconds().ToString() +} + +function Set-PersistenceConnectionString { + param( + [Parameter(Mandatory)][ValidateSet('SqlServer', 'PostgreSql')][string]$Provider, + [Parameter(Mandatory)][string]$ConnectionString + ) + + $name = "ServiceControl_Persistence_${Provider}_ConnectionString" + + if (-not $Env:GITHUB_ENV) { + Write-Step "GITHUB_ENV is not set, so $name was not exported. Set it yourself to run the tests against this server." + return + } + + "$name=$ConnectionString" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf8 -Append + Write-Step "Exported $name" +} + +# Creates the test database if the service could not, and fails the run if the server has no +# Full-Text Search. +function Invoke-SqlServerVerification { + param([Parameter(Mandatory)][string]$ConnectionString) + + # Run from this folder: `dotnet run .cs` picks up any project file in the working + # directory, and by this point the repo root holds the tests.proj that select-test-projects.ps1 + # generated. + Push-Location $PSScriptRoot + try { + dotnet run ./verify-sqlserver.cs -- $ConnectionString + } + finally { + Pop-Location + } +} + +# Teardown runs even when provisioning failed part way, so it has to tolerate resources that were +# never created. +function Invoke-Teardown { + param( + [Parameter(Mandatory)][string]$Description, + [Parameter(Mandatory)][scriptblock]$Action + ) + + Write-Step $Description + try { + & $Action + } + catch { + Write-Warning "$Description failed: $($_.Exception.Message). The scheduled cloud-database-cleanup workflow will pick up anything left behind." + } +} diff --git a/tools/cloud/rds-sqlserver.ps1 b/tools/cloud/rds-sqlserver.ps1 new file mode 100644 index 0000000000..b5aa78ef95 --- /dev/null +++ b/tools/cloud/rds-sqlserver.ps1 @@ -0,0 +1,90 @@ +# Provisions and tears down an RDS SQL Server instance for one run of the cloud database tests. +# +# This is the slowest of the four targets to provision, around 15 to 25 minutes, so the workflow +# starts it before waiting on the build. + +param( + [Parameter(Mandatory)][ValidateSet('Provision', 'Teardown')][string]$Action, + [Parameter(Mandatory)][string]$Name, + [string]$DatabaseName = 'servicecontrol', + # Web edition is the cheapest licence-included option. Full-Text Search availability is what + # decides whether it is usable; verify-sqlserver.cs fails the run with a clear message if this + # edition turns out not to have it, in which case move to sqlserver-se. + [string]$Engine = 'sqlserver-web' +) + +. $PSScriptRoot/common.ps1 + +$adminUser = 'sctestadmin' + +if ($Action -eq 'Teardown') { + Invoke-Teardown "Deleting instance $Name" { + aws rds delete-db-instance --db-instance-identifier $Name --skip-final-snapshot --delete-automated-backups --no-cli-pager --output none + } + + Invoke-Teardown "Deleting security group $Name" { + $groupId = aws ec2 describe-security-groups --filters "Name=group-name,Values=$Name" --query 'SecurityGroups[0].GroupId' --output text + if ($groupId -and $groupId -ne 'None') { + aws ec2 delete-security-group --group-id $groupId --output none + } + } + + return +} + +$password = New-AdminPassword +$runnerIp = Get-RunnerIpAddress +$created = Get-CreatedTimestamp + +$vpcId = aws ec2 describe-vpcs --filters 'Name=isDefault,Values=true' --query 'Vpcs[0].VpcId' --output text +if (-not $vpcId -or $vpcId -eq 'None') { + throw 'This AWS account has no default VPC in this region, so there is no public subnet group for the instance to use.' +} + +Write-Step "Creating security group $Name in $vpcId, allowing $runnerIp on 1433" +$groupId = aws ec2 create-security-group ` + --group-name $Name ` + --description 'ServiceControl cloud database tests' ` + --vpc-id $vpcId ` + --tag-specifications "ResourceType=security-group,Tags=[{Key=sc-cloud-test,Value=true},{Key=run-id,Value=$Name},{Key=created,Value=$created}]" ` + --query GroupId --output text + +aws ec2 authorize-security-group-ingress ` + --group-id $groupId ` + --protocol tcp ` + --port 1433 ` + --cidr "$runnerIp/32" ` + --output none + +# No automated backups and no standby: the instance does not outlive the run, and both slow +# provisioning down. +Write-Step "Creating RDS SQL Server instance $Name" +aws rds create-db-instance ` + --db-instance-identifier $Name ` + --engine $Engine ` + --db-instance-class db.t3.small ` + --allocated-storage 20 ` + --master-username $adminUser ` + --master-user-password $password ` + --vpc-security-group-ids $groupId ` + --license-model license-included ` + --publicly-accessible ` + --no-multi-az ` + --backup-retention-period 0 ` + --tags "Key=sc-cloud-test,Value=true" "Key=run-id,Value=$Name" "Key=created,Value=$created" ` + --no-cli-pager --output none + +Write-Step 'Waiting for the instance to become available' +aws rds wait db-instance-available --db-instance-identifier $Name + +$endpoint = aws rds describe-db-instances --db-instance-identifier $Name --query 'DBInstances[0].Endpoint.Address' --output text + +# Trust Server Certificate because RDS presents an Amazon CA that is not in the runner's trust store. +# The connection is still encrypted; only the certificate chain goes unverified. +$connectionString = "Server=tcp:$endpoint,1433;Initial Catalog=$DatabaseName;User ID=$adminUser;Password=$password;Encrypt=True;TrustServerCertificate=True;Connect Timeout=60" + +# RDS cannot create a user database as part of the instance, unlike every other target here. +Write-Step "Creating database $DatabaseName and verifying Full-Text Search" +Invoke-SqlServerVerification -ConnectionString $connectionString + +Set-PersistenceConnectionString -Provider SqlServer -ConnectionString $connectionString diff --git a/tools/cloud/verify-sqlserver.cs b/tools/cloud/verify-sqlserver.cs new file mode 100644 index 0000000000..d7961f1d60 --- /dev/null +++ b/tools/cloud/verify-sqlserver.cs @@ -0,0 +1,52 @@ +#:package Microsoft.Data.SqlClient@6.1.1 + +// Creates the ServiceControl test database if the service could not create it during provisioning, +// and fails the run if the server has no Full-Text Search. Message search is not optional, so an +// instance without it would otherwise fail every search test twenty minutes later with a much less +// obvious error. + +using Microsoft.Data.SqlClient; + +if (args.Length != 1) +{ + Console.Error.WriteLine("usage: dotnet run verify-sqlserver.cs -- "); + return 1; +} + +var builder = new SqlConnectionStringBuilder(args[0]); +var database = builder.InitialCatalog; + +builder.InitialCatalog = "master"; + +try +{ + await using var master = new SqlConnection(builder.ConnectionString); + await master.OpenAsync(); + + await using (var create = master.CreateCommand()) + { + create.CommandText = $"IF DB_ID(N'{database}') IS NULL CREATE DATABASE [{database}]"; + create.CommandTimeout = 300; + await create.ExecuteNonQueryAsync(); + } + + await using (var fullText = master.CreateCommand()) + { + fullText.CommandText = "SELECT CONVERT(int, ISNULL(SERVERPROPERTY('IsFullTextInstalled'), 0))"; + if ((int)(await fullText.ExecuteScalarAsync())! != 1) + { + Console.Error.WriteLine($"{builder.DataSource} does not have SQL Server Full-Text Search installed, which ServiceControl requires. On RDS, check that the edition supports it and that the option group includes it."); + return 1; + } + } +} +catch (SqlException e) +{ + // Without the stack trace, which says nothing useful about a server that is unreachable or + // rejecting the login. + Console.Error.WriteLine($"Could not prepare {builder.DataSource}: {e.Message}"); + return 1; +} + +Console.WriteLine($"Database '{database}' is present on {builder.DataSource} and Full-Text Search is installed."); +return 0;