Migrations: Re-trust untrusted foreign key and check constraints on SQL Server and fix bulk inserts to prevent recurrence

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}";

src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerBulkSqlInsertProvider.cs

@@ -58,7 +58,7 @@ private static int BulkInsertRecordsSqlServer<T>(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,

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)

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;
+
+/// <summary>
+/// Custom SQL Server database type that ensures FK constraints remain trusted
+/// during bulk insert operations by using <see cref="SqlBulkCopyOptions.CheckConstraints"/>.
+/// </summary>
+/// <remarks>
+/// NPoco's default <see cref="SqlServerDatabaseType.InsertBulk{T}"/> and <see cref="SqlServerDatabaseType.InsertBulkAsync{T}"/> uses
+/// <see cref="SqlBulkCopyOptions.Default"/> which bypasses FK validation during <see cref="SqlBulkCopy"/> operations, causing
+/// SQL Server to mark constraints as untrusted (<c>is_not_trusted = 1</c>).
+/// Untrusted constraints prevent the query optimizer from using them for join elimination and cardinality estimation.
+/// </remarks>
+internal sealed class UmbracoSqlServerDatabaseType : SqlServer2012DatabaseType
+{
+    /// <inheritdoc />
+    public override void InsertBulk<T>(IDatabase db, IEnumerable<T> pocos, InsertBulkOptions? options)
+        => SqlBulkCopyHelper.BulkInsert(db, pocos, SqlBulkCopyOptions.CheckConstraints, options);
+
+    /// <inheritdoc />
+    public override Task InsertBulkAsync<T>(IDatabase db, IEnumerable<T> pocos, InsertBulkOptions? options, CancellationToken cancellationToken = default)
+        => SqlBulkCopyHelper.BulkInsertAsync(db, pocos, SqlBulkCopyOptions.CheckConstraints, options ?? new InsertBulkOptions(), cancellationToken);
+}

src/Umbraco.Infrastructure/Migrations/AsyncMigrationBase.Database.cs

@@ -1,4 +1,4 @@
 using Umbraco.Cms.Infrastructure.Migrations.Expressions.Execute.Expressions;
 using Umbraco.Cms.Infrastructure.Persistence.DatabaseModelDefinitions;
 using Umbraco.Cms.Infrastructure.Persistence.SqlSyntax;
 using Umbraco.Extensions;
@@ -10,7 +10,24 @@
 /// </summary>
 public abstract partial class AsyncMigrationBase
 {
-    // provides extra methods for migrations
+    /// <summary>
+    /// Ensures that the command timeout for the specified database is set to a minimum of 300 seconds.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    /// <param name="database">The database instance for which the command timeout is being ensured.</param>
+    protected static void EnsureLongCommandTimeout(NPoco.IDatabase database)
+    {
+        const int CommandTimeoutInSeconds = 300;
+        if (database.CommandTimeout < CommandTimeoutInSeconds)
+        {
+            database.CommandTimeout = CommandTimeoutInSeconds;
+        }
+    }
+
     protected void AddColumn<T>(string columnName)
     {
         TableDefinition? table = DefinitionFactory.GetTableDefinition(typeof(T), SqlSyntax);

src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs

@@ -153,6 +153,7 @@
 
         // To 17.3.0
         To<V_17_3_0.IncreaseSizeOfLongRunningOperationTypeColumn>("{B2F4A1C3-8D5E-4F6A-9B7C-3E1D2A4F5B6C}");
+        To<V_17_3_0.RetrustForeignKeyAndCheckConstraints>("{0638E0E0-D914-4ACA-8A4B-9551A3AAB91F}");
 
         // To 18.0.0
         // TODO (V18): Enable on 18 branch

src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_0_0/MigrateSystemDatesToUtc.cs

@@ -97,12 +97,7 @@
         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);

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;
+
+/// <summary>
+/// Re-trusts all untrusted foreign key and check constraints on SQL Server.
+/// Untrusted constraints (where <c>is_not_trusted = 1</c>) 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.
+/// </summary>
+public class RetrustForeignKeyAndCheckConstraints : AsyncMigrationBase
+{
+    /// <summary>
+    /// Initializes a new instance of the <see cref="RetrustForeignKeyAndCheckConstraints"/> class.
+    /// </summary>
+    /// <param name="context">The migration context.</param>
+    public RetrustForeignKeyAndCheckConstraints(IMigrationContext context)
+        : base(context)
+    {
+    }
+
+    /// <inheritdoc />
+    protected override Task MigrateAsync()
+    {
+        if (DatabaseType == DatabaseType.SQLite)
+        {
+            return Task.CompletedTask;
+        }
+
+        RetrustConstraints();
+
+        return Task.CompletedTask;
+    }
+
+    private void RetrustConstraints()
+    {
+        List<UntrustedConstraintDto> untrustedConstraints = Database.Fetch<UntrustedConstraintDto>(
+            @"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!;
+    }
+}

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 @@
             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<IContentTypeService>();
+        var contentService = Services.GetRequiredService<IContentService>();
+        var templateService = Services.GetRequiredService<ITemplateService>();
+
+        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<PropertyDataDto>(
+                db.SqlContext.Sql()
+                    .Select("*")
+                    .From<PropertyDataDto>()
+                    .Where<PropertyDataDto>(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<PropertyDataDto>()
+                    .Where<PropertyDataDto>(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<int>(
+                @"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<Webhook2EventsDto>
+            {
+                new() { WebhookId = webhookDto.Id, Event = "Umbraco.ContentPublish" },
+                new() { WebhookId = webhookDto.Id, Event = "Umbraco.ContentUnpublish" },
+            };
+            await db.InsertBulkAsync(eventDtos);
+
+            var untrustedCount = db.ExecuteScalar<int>(
+                @"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.");
+        }
+    }
 }