diff --git a/src/Umbraco.Cms.Persistence.SqlServer/Services/MicrosoftSqlSyntaxProviderBase.cs b/src/Umbraco.Cms.Persistence.SqlServer/Services/MicrosoftSqlSyntaxProviderBase.cs index bc1a7afc6b85..6757ccfd3fa8 100644 --- a/src/Umbraco.Cms.Persistence.SqlServer/Services/MicrosoftSqlSyntaxProviderBase.cs +++ b/src/Umbraco.Cms.Persistence.SqlServer/Services/MicrosoftSqlSyntaxProviderBase.cs @@ -31,6 +31,9 @@ protected MicrosoftSqlSyntaxProviderBase() BlobColumnDefinition = "VARBINARY(MAX)"; } + public override string CreateForeignKeyConstraint => + "ALTER TABLE {0} WITH CHECK ADD CONSTRAINT {1} FOREIGN KEY ({2}) REFERENCES {3} ({4}){5}{6}"; + public override string RenameTable => "sp_rename '{0}', '{1}'"; public override string AddColumn => "ALTER TABLE {0} ADD {1}"; diff --git a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerBulkSqlInsertProvider.cs b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerBulkSqlInsertProvider.cs index c6ae4021478f..eda51d7a2ed8 100644 --- a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerBulkSqlInsertProvider.cs +++ b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerBulkSqlInsertProvider.cs @@ -58,7 +58,7 @@ private static int BulkInsertRecordsSqlServer(IUmbracoDatabase database, Poco throw new NotSupportedException("SqlSyntax must be SqlServerSyntaxProvider."); } - using (var copy = new SqlBulkCopy(tConnection, SqlBulkCopyOptions.Default, tTransaction) + using (var copy = new SqlBulkCopy(tConnection, SqlBulkCopyOptions.CheckConstraints, tTransaction) { // 0 = no bulk copy timeout. If a timeout occurs it will be an connection/command timeout. BulkCopyTimeout = 0, diff --git a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerSyntaxProvider.cs b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerSyntaxProvider.cs index 1464b049dac1..bea25ce08b1b 100644 --- a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerSyntaxProvider.cs +++ b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerSyntaxProvider.cs @@ -88,10 +88,10 @@ public override DatabaseType GetUpdatedDatabaseType(DatabaseType current, string if (_logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Debug)) { - _logger.LogDebug("SqlServer {SqlServerVersion}, DatabaseType is {DatabaseType} ({Source}).", versionName, DatabaseType.SqlServer2012, fromSettings ? "settings" : "detected"); + _logger.LogDebug("SqlServer {SqlServerVersion}, DatabaseType is {DatabaseType} ({Source}).", versionName, nameof(UmbracoSqlServerDatabaseType), fromSettings ? "settings" : "detected"); } - return DatabaseType.SqlServer2012; + return new UmbracoSqlServerDatabaseType(); } private static VersionName MapProductVersion(string productVersion) diff --git a/src/Umbraco.Cms.Persistence.SqlServer/Services/UmbracoSqlServerDatabaseType.cs b/src/Umbraco.Cms.Persistence.SqlServer/Services/UmbracoSqlServerDatabaseType.cs new file mode 100644 index 000000000000..d6475bfc3058 --- /dev/null +++ b/src/Umbraco.Cms.Persistence.SqlServer/Services/UmbracoSqlServerDatabaseType.cs @@ -0,0 +1,27 @@ +using Microsoft.Data.SqlClient; +using NPoco; +using NPoco.DatabaseTypes; +using NPoco.SqlServer; + +namespace Umbraco.Cms.Persistence.SqlServer.Services; + +/// +/// Custom SQL Server database type that ensures FK constraints remain trusted +/// during bulk insert operations by using . +/// +/// +/// NPoco's default and uses +/// which bypasses FK validation during operations, causing +/// SQL Server to mark constraints as untrusted (is_not_trusted = 1). +/// Untrusted constraints prevent the query optimizer from using them for join elimination and cardinality estimation. +/// +internal sealed class UmbracoSqlServerDatabaseType : SqlServer2012DatabaseType +{ + /// + public override void InsertBulk(IDatabase db, IEnumerable pocos, InsertBulkOptions? options) + => SqlBulkCopyHelper.BulkInsert(db, pocos, SqlBulkCopyOptions.CheckConstraints, options); + + /// + public override Task InsertBulkAsync(IDatabase db, IEnumerable pocos, InsertBulkOptions? options, CancellationToken cancellationToken = default) + => SqlBulkCopyHelper.BulkInsertAsync(db, pocos, SqlBulkCopyOptions.CheckConstraints, options ?? new InsertBulkOptions(), cancellationToken); +} diff --git a/src/Umbraco.Infrastructure/Migrations/AsyncMigrationBase.Database.cs b/src/Umbraco.Infrastructure/Migrations/AsyncMigrationBase.Database.cs index 21ea92377981..2d52e5579c64 100644 --- a/src/Umbraco.Infrastructure/Migrations/AsyncMigrationBase.Database.cs +++ b/src/Umbraco.Infrastructure/Migrations/AsyncMigrationBase.Database.cs @@ -10,7 +10,24 @@ namespace Umbraco.Cms.Infrastructure.Migrations; /// public abstract partial class AsyncMigrationBase { - // provides extra methods for migrations + /// + /// Ensures that the command timeout for the specified database is set to a minimum of 300 seconds. + /// + /// + /// Adjusts the command timeout to prevent potential timeouts during long-running + /// database operations. + /// If the command timeout is already longer, applied via the connection string with "Connect Timeout={timeout}" we leave it as is. + /// + /// The database instance for which the command timeout is being ensured. + protected static void EnsureLongCommandTimeout(NPoco.IDatabase database) + { + const int CommandTimeoutInSeconds = 300; + if (database.CommandTimeout < CommandTimeoutInSeconds) + { + database.CommandTimeout = CommandTimeoutInSeconds; + } + } + protected void AddColumn(string columnName) { TableDefinition? table = DefinitionFactory.GetTableDefinition(typeof(T), SqlSyntax); diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs index 970b8a7ed53e..91068eceb870 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs @@ -153,6 +153,7 @@ protected virtual void DefinePlan() // To 17.3.0 To("{B2F4A1C3-8D5E-4F6A-9B7C-3E1D2A4F5B6C}"); + To("{0638E0E0-D914-4ACA-8A4B-9551A3AAB91F}"); // To 18.0.0 // TODO (V18): Enable on 18 branch diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs index 6f290d455f7b..e0769cd83af0 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs @@ -97,12 +97,7 @@ protected override void Migrate() using IDisposable notificationSuppression = scope.Notifications.Suppress(); // Ensure we have a long command timeout as this migration can take a while on large tables within the database. - // If the command timeout is already longer, applied via the connection string with "Connect Timeout={timeout}" we leave it as is. - const int CommandTimeoutInSeconds = 300; - if (scope.Database.CommandTimeout < CommandTimeoutInSeconds) - { - scope.Database.CommandTimeout = CommandTimeoutInSeconds; - } + EnsureLongCommandTimeout(scope.Database); MigrateDateColumn(scope, "cmsMember", "emailConfirmedDate", timeZone); MigrateDateColumn(scope, "cmsMember", "lastLoginDate", timeZone); diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_3_0/RetrustForeignKeyAndCheckConstraints.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_3_0/RetrustForeignKeyAndCheckConstraints.cs new file mode 100644 index 000000000000..53ff084ab486 --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_3_0/RetrustForeignKeyAndCheckConstraints.cs @@ -0,0 +1,119 @@ +using Microsoft.Extensions.Logging; +using NPoco; +using Umbraco.Cms.Core; + +namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_17_3_0; + +/// +/// Re-trusts all untrusted foreign key and check constraints on SQL Server. +/// Untrusted constraints (where is_not_trusted = 1) prevent the query optimizer from using them +/// for join elimination, cardinality estimation, and index selection. They can result from historical +/// upgrades, EF Core migrations, or DBA operations such as backup restores. +/// +public class RetrustForeignKeyAndCheckConstraints : AsyncMigrationBase +{ + /// + /// Initializes a new instance of the class. + /// + /// The migration context. + public RetrustForeignKeyAndCheckConstraints(IMigrationContext context) + : base(context) + { + } + + /// + protected override Task MigrateAsync() + { + if (DatabaseType == DatabaseType.SQLite) + { + return Task.CompletedTask; + } + + RetrustConstraints(); + + return Task.CompletedTask; + } + + private void RetrustConstraints() + { + List untrustedConstraints = Database.Fetch( + @"SELECT + s.name AS SchemaName, + OBJECT_NAME(fk.parent_object_id) AS TableName, + fk.name AS ConstraintName + FROM sys.foreign_keys fk + INNER JOIN sys.schemas s ON fk.schema_id = s.schema_id + WHERE fk.is_not_trusted = 1 + + UNION ALL + + SELECT + s.name AS SchemaName, + OBJECT_NAME(cc.parent_object_id) AS TableName, + cc.name AS ConstraintName + FROM sys.check_constraints cc + INNER JOIN sys.schemas s ON cc.schema_id = s.schema_id + WHERE cc.is_not_trusted = 1"); + + if (untrustedConstraints.Count == 0) + { + Logger.LogInformation("No untrusted constraints found."); + return; + } + + Logger.LogInformation("Found {Count} untrusted constraint(s) to re-trust.", untrustedConstraints.Count); + + // Ensure we have a long command timeout, in case the migration targets a huge table. + EnsureLongCommandTimeout(Database); + + var retrusted = 0; + var failed = 0; + + foreach (UntrustedConstraintDto constraint in untrustedConstraints) + { + var sql = $"ALTER TABLE [{constraint.SchemaName}].[{constraint.TableName}] WITH CHECK CHECK CONSTRAINT [{constraint.ConstraintName}]"; + + try + { + Database.Execute(sql); + retrusted++; + Logger.LogDebug( + "Re-trusted constraint [{ConstraintName}] on [{SchemaName}].[{TableName}].", + constraint.ConstraintName, + constraint.SchemaName, + constraint.TableName); + } + catch (Exception ex) + { + failed++; + Logger.LogWarning( + ex, + "Could not re-trust constraint [{ConstraintName}] on [{SchemaName}].[{TableName}]. " + + "This likely means existing data violates the constraint. " + + "Please investigate and fix the data integrity issue manually.", + constraint.ConstraintName, + constraint.SchemaName, + constraint.TableName); + } + } + + Logger.LogInformation( + "Constraint re-trust complete: {Retrusted} succeeded, {Failed} failed out of {Total} total.", + retrusted, + failed, + untrustedConstraints.Count); + } + + [TableName("")] + private class UntrustedConstraintDto + { + [Column("SchemaName")] + public string SchemaName { get; set; } = null!; + + [Column("TableName")] + public string TableName { get; set; } = null!; + + [Column("ConstraintName")] + public string ConstraintName { get; set; } = null!; + } +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoBulkInsertTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoBulkInsertTests.cs index b85d08f443d5..12ea23999a31 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoBulkInsertTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/NPocoTests/NPocoBulkInsertTests.cs @@ -1,21 +1,21 @@ // Copyright (c) Umbraco. // See LICENSE for more details. -using System.Collections.Generic; using System.Data; -using System.Linq; using System.Text.RegularExpressions; +using Microsoft.Extensions.DependencyInjection; using NUnit.Framework; +using Umbraco.Cms.Core; using Umbraco.Cms.Core.Logging; +using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Implementations; using Umbraco.Cms.Tests.Integration.Testing; -using Umbraco.Extensions; namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence.NPocoTests; -// TODO: npoco - is this still appropriate? [TestFixture] [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] internal sealed class NPocoBulkInsertTests : UmbracoIntegrationTest @@ -206,4 +206,122 @@ public void Generate_Bulk_Import_Sql_Exceeding_Max_Params() Assert.LessOrEqual(Regex.Matches(s, "@\\d+").Count, 2000); } } + + [Test] + public async Task InsertBulk_Does_Not_Create_Untrusted_Foreign_Key_Constraints() + { + if (BaseTestDatabase.IsSqlServer() is false) + { + Assert.Ignore("This test only applies to SQL Server (SQLite has no constraint trust concept)."); + } + + // Updating property data invokes SQL bulk insert, so we'll replicate that scenario for testing. + + // Create a content item so we have valid FK references for PropertyDataDto. + var contentTypeService = Services.GetRequiredService(); + var contentService = Services.GetRequiredService(); + var templateService = Services.GetRequiredService(); + + var template = TemplateBuilder.CreateTextPageTemplate("defaultTemplate"); + await templateService.CreateAsync(template, Constants.Security.SuperUserKey); + + var contentType = ContentTypeBuilder.CreateSimpleContentType("testPage", "Test Page", defaultTemplateId: template.Id); + await contentTypeService.CreateAsync(contentType, Constants.Security.SuperUserKey); + + var content = ContentBuilder.CreateSimpleContent(contentType); + contentService.Save(content); + + // Replicate what ContentRepositoryBase.ReplacePropertyValues does: + // delete existing property data for the version, then re-insert via InsertBulk. + // + // NPoco's Database.InsertBulk() dispatches to DatabaseType.InsertBulk() → + // SqlBulkCopyHelper.BulkInsert(). Without our UmbracoSqlServerDatabaseType override, + // NPoco uses SqlBulkCopyOptions.Default which bypasses FK validation during SqlBulkCopy, + // causing SQL Server to mark constraints as untrusted (is_not_trusted = 1). + using (ScopeProvider.CreateScope(autoComplete: true)) + { + var db = ScopeAccessor.AmbientScope!.Database; + + // Fetch existing property data for this content's version. + var versionId = content.VersionId; + var existing = db.Fetch( + db.SqlContext.Sql() + .Select("*") + .From() + .Where(x => x.VersionId == versionId)); + Assert.That(existing, Has.Count.GreaterThan(0), "Content save should have created property data."); + + // Delete existing rows, then re-insert them via InsertBulk (mirroring ReplacePropertyValues). + db.Execute( + db.SqlContext.Sql() + .Delete() + .Where(x => x.VersionId == versionId)); + + foreach (var dto in existing) + { + dto.Id = 0; // reset PK so InsertBulk generates new identity values + } + + db.InsertBulk(existing); + + var untrustedCount = db.ExecuteScalar( + @"SELECT COUNT(*) + FROM sys.foreign_keys + WHERE is_not_trusted = 1 + AND OBJECT_NAME(parent_object_id) = @0", + "umbracoPropertyData"); + + Assert.That( + untrustedCount, + Is.EqualTo(0), + "FK constraints on umbracoPropertyData should be trusted after InsertBulk. " + + "NPoco's InsertBulk with SqlBulkCopyOptions.Default causes constraints to become untrusted."); + } + } + + [Test] + public async Task InsertBulkAsync_Does_Not_Create_Untrusted_Foreign_Key_Constraints() + { + if (BaseTestDatabase.IsSqlServer() is false) + { + Assert.Ignore("This test only applies to SQL Server (SQLite has no constraint trust concept)."); + } + + // Replicate what WebhookRepository does when saving a webhook with associated events: + // insert the webhook row, then InsertBulkAsync the event associations. + using (ScopeProvider.CreateScope(autoComplete: true)) + { + var db = ScopeAccessor.AmbientScope!.Database; + + // Insert a parent webhook row so the FK reference is valid. + var webhookDto = new WebhookDto + { + Key = Guid.NewGuid(), + Url = "https://example.com/hook", + Enabled = true, + }; + await db.InsertAsync(webhookDto); + + // InsertBulkAsync the event associations (mirrors WebhookRepository.CreateAsync). + var eventDtos = new List + { + new() { WebhookId = webhookDto.Id, Event = "Umbraco.ContentPublish" }, + new() { WebhookId = webhookDto.Id, Event = "Umbraco.ContentUnpublish" }, + }; + await db.InsertBulkAsync(eventDtos); + + var untrustedCount = db.ExecuteScalar( + @"SELECT COUNT(*) + FROM sys.foreign_keys + WHERE is_not_trusted = 1 + AND OBJECT_NAME(parent_object_id) = @0", + Constants.DatabaseSchema.Tables.Webhook2Events); + + Assert.That( + untrustedCount, + Is.EqualTo(0), + "FK constraints on umbracoWebhook2Events should be trusted after InsertBulkAsync. " + + "NPoco's InsertBulkAsync with SqlBulkCopyOptions.Default causes constraints to become untrusted."); + } + } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentCacheServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentCacheServiceTests.cs index 3e3d80b48f94..e1ced1b7d7f3 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentCacheServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentCacheServiceTests.cs @@ -354,6 +354,36 @@ public async Task Rebuild_Includes_Composed_Properties_In_Cache() } } + [Test] + public void Rebuild_Does_Not_Create_Untrusted_Foreign_Key_Constraints() + { + if (BaseTestDatabase.IsSqlServer() is false) + { + Assert.Ignore("This test only applies to SQL Server (SQLite has no constraint trust concept)."); + } + + // Arrange - publish content so the rebuild inserts both draft and published rows. + ContentService.Publish(Textpage, ["*"]); + + // Act - Rebuild the cache (uses SqlBulkCopy internally). + DocumentCacheService.Rebuild([ContentType.Id]); + + // Assert - Verify no untrusted FK constraints exist on the cmsContentNu table. + using var scope = ScopeProvider.CreateScope(autoComplete: true); + var untrustedCount = ScopeAccessor.AmbientScope!.Database.ExecuteScalar( + @"SELECT COUNT(*) + FROM sys.foreign_keys + WHERE is_not_trusted = 1 + AND OBJECT_NAME(parent_object_id) = @0", + "cmsContentNu"); + + Assert.That( + untrustedCount, + Is.EqualTo(0), + "FK constraints on cmsContentNu should be trusted after rebuild. " + + "SqlBulkCopy with SqlBulkCopyOptions.Default causes constraints to become untrusted."); + } + [GeneratedRegex(@"""dt"":""[^""]+""")] private static partial Regex RemoveDatesFromJsonSerialization(); }