diff --git a/src/Umbraco.Core/Constants-HealthChecks.cs b/src/Umbraco.Core/Constants-HealthChecks.cs index e0b637f39d86..726d2416580c 100644 --- a/src/Umbraco.Core/Constants-HealthChecks.cs +++ b/src/Umbraco.Core/Constants-HealthChecks.cs @@ -47,6 +47,17 @@ public static class LiveEnvironment public const string RuntimeModeCheck = "https://docs.umbraco.com/umbraco-cms/fundamentals/setup/server-setup/runtime-modes"; } + /// + /// Contains documentation links for data health checks. + /// + public static class Data + { + /// + /// The documentation link for untrusted database constraints check. + /// + public const string UntrustedConstraintsCheck = "https://umbra.co/healthchecks-untrusted-constraints"; + } + /// /// Contains documentation links for configuration health checks. /// diff --git a/src/Umbraco.Infrastructure/HealthChecks/Checks/Data/UntrustedDatabaseConstraintsCheck.cs b/src/Umbraco.Infrastructure/HealthChecks/Checks/Data/UntrustedDatabaseConstraintsCheck.cs new file mode 100644 index 000000000000..29d63baccc22 --- /dev/null +++ b/src/Umbraco.Infrastructure/HealthChecks/Checks/Data/UntrustedDatabaseConstraintsCheck.cs @@ -0,0 +1,105 @@ +using System.Net; +using System.Text; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.HealthChecks; +using Umbraco.Cms.Infrastructure.Persistence; + +namespace Umbraco.Cms.Infrastructure.HealthChecks.Checks.Data; + +/// +/// Health check that detects untrusted foreign key and check constraints on SQL Server. +/// +/// +/// An untrusted constraint (is_not_trusted = 1) means SQL Server has not verified that +/// all existing rows satisfy the constraint. This prevents the query optimizer from using the +/// constraint for join elimination and cardinality estimation, and indicates that orphaned or +/// otherwise invalid rows may exist. The RetrustForeignKeyAndCheckConstraints migration +/// attempts to re-trust these constraints automatically but logs a warning and continues when +/// existing data violates a constraint, leaving the issue for manual resolution. +/// +[HealthCheck( + "0B1E71E4-8D37-4F9B-A9A4-86C5B9EA5B0B", + "Untrusted database constraints", + Description = "Checks for untrusted foreign key or check constraints on SQL Server, which indicate pre-existing data integrity issues that must be resolved manually.", + Group = "Data Integrity")] +public class UntrustedDatabaseConstraintsCheck : HealthCheck +{ + private readonly IUmbracoDatabaseFactory _databaseFactory; + + /// + /// Initializes a new instance of the class. + /// + /// The database factory used to create a connection for the check. + public UntrustedDatabaseConstraintsCheck(IUmbracoDatabaseFactory databaseFactory) + => _databaseFactory = databaseFactory; + + /// + public override Task> GetStatusAsync() + => Task.FromResult>([CheckConstraints()]); + + private HealthCheckStatus CheckConstraints() + { + if (_databaseFactory.Configured is false || _databaseFactory.CanConnect is false) + { + return new HealthCheckStatus("Cannot connect to the database.") + { + ResultType = StatusResultType.Info, + }; + } + + using IUmbracoDatabase db = _databaseFactory.CreateDatabase(); + +#pragma warning disable CS0618 // Type or member is obsolete - justification: in this case we do want to check the database type before running SQL Server-specific queries. + if (db.DatabaseType.IsSqlServer() is false) + { + return new HealthCheckStatus("Not applicable — this check only runs on SQL Server.") + { + ResultType = StatusResultType.Info, + }; + } +#pragma warning restore CS0618 // Type or member is obsolete + + List untrustedConstraints = + db.Fetch(UntrustedConstraintsQuery.Sql); + + if (untrustedConstraints.Count == 0) + { + return new HealthCheckStatus("All Umbraco database constraints are trusted.") + { + ResultType = StatusResultType.Success, + }; + } + + return new HealthCheckStatus(BuildMessage(untrustedConstraints)) + { + ResultType = StatusResultType.Warning, + ReadMoreLink = Constants.HealthChecks.DocumentationLinks.Data.UntrustedConstraintsCheck, + }; + } + + private static string BuildMessage(IReadOnlyList constraints) + { + var sb = new StringBuilder(); + sb.Append("

Found ") + .Append(constraints.Count) + .AppendLine(" untrusted constraint(s). These indicate pre-existing data integrity issues that need to be resolved manually.

"); + sb.AppendLine( + "

Untrusted constraints prevent the SQL Server query optimizer from using them and indicate that orphaned or invalid rows may exist. Resolve the underlying data issue, then re-trust the constraint with ALTER TABLE ... WITH CHECK CHECK CONSTRAINT.

"); + sb.AppendLine("
    "); + foreach (UntrustedConstraintsQuery.UntrustedConstraintDto constraint in constraints) + { + sb.Append("
  • ") + .Append(WebUtility.HtmlEncode(constraint.ConstraintType)) + .Append(" ") + .Append(WebUtility.HtmlEncode(constraint.ConstraintName)) + .Append(" on ") + .Append(WebUtility.HtmlEncode(constraint.SchemaName)) + .Append('.') + .Append(WebUtility.HtmlEncode(constraint.TableName)) + .AppendLine("
    • "); + } + + sb.AppendLine("
"); + return sb.ToString(); + } +} 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 index c75682cb2cd6..a201c5bfd9ac 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_3_0/RetrustForeignKeyAndCheckConstraints.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_17_3_0/RetrustForeignKeyAndCheckConstraints.cs @@ -51,28 +51,8 @@ protected override Task MigrateAsync() private void RetrustConstraints() { - // Only target Umbraco tables (prefixed "umbraco" or "cms") to avoid touching - // custom or third-party tables that share the same database. - 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 - AND (OBJECT_NAME(fk.parent_object_id) LIKE 'umbraco%' OR OBJECT_NAME(fk.parent_object_id) LIKE 'cms%') - - 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 - AND (OBJECT_NAME(cc.parent_object_id) LIKE 'umbraco%' OR OBJECT_NAME(cc.parent_object_id) LIKE 'cms%')"); + List untrustedConstraints = + Database.Fetch(UntrustedConstraintsQuery.Sql); if (untrustedConstraints.Count == 0) { @@ -95,7 +75,7 @@ FROM sys.check_constraints cc using IUmbracoDatabase db = _databaseFactory.CreateDatabase(); EnsureLongCommandTimeout(db); - foreach (UntrustedConstraintDto constraint in untrustedConstraints) + foreach (UntrustedConstraintsQuery.UntrustedConstraintDto constraint in untrustedConstraints) { // Leading semicolon prevents NPoco's auto-select from prepending // "SELECT ... FROM []" based on the empty [TableName("")] attribute. @@ -140,19 +120,6 @@ BEGIN CATCH 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!; - } - [TableName("")] private class RetrustResultDto { diff --git a/src/Umbraco.Infrastructure/Persistence/UntrustedConstraintsQuery.cs b/src/Umbraco.Infrastructure/Persistence/UntrustedConstraintsQuery.cs new file mode 100644 index 000000000000..c3b80f383321 --- /dev/null +++ b/src/Umbraco.Infrastructure/Persistence/UntrustedConstraintsQuery.cs @@ -0,0 +1,69 @@ +using NPoco; + +namespace Umbraco.Cms.Infrastructure.Persistence; + +/// +/// Shared SQL and DTO for identifying untrusted foreign key and check constraints on SQL Server. +/// Consumed by the RetrustForeignKeyAndCheckConstraints migration and the +/// UntrustedDatabaseConstraintsCheck health check. +/// +internal static class UntrustedConstraintsQuery +{ + /// + /// Returns one row per untrusted constraint (is_not_trusted = 1) on Umbraco tables + /// (those prefixed "umbraco" or "cms"), across both foreign keys and check constraints. + /// Other tables in the same database are deliberately excluded. + /// + public const string Sql = @"SELECT + 'Foreign key' AS ConstraintType, + 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 + AND (OBJECT_NAME(fk.parent_object_id) LIKE 'umbraco%' OR OBJECT_NAME(fk.parent_object_id) LIKE 'cms%') + +UNION ALL + +SELECT + 'Check constraint' AS ConstraintType, + 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 + AND (OBJECT_NAME(cc.parent_object_id) LIKE 'umbraco%' OR OBJECT_NAME(cc.parent_object_id) LIKE 'cms%')"; + + /// + /// A row returned by , describing a single untrusted constraint. + /// + [TableName("")] + public class UntrustedConstraintDto + { + /// + /// Gets or sets the kind of constraint — either "Foreign key" or "Check constraint". + /// + [Column("ConstraintType")] + public string ConstraintType { get; set; } = null!; + + /// + /// Gets or sets the schema name of the table the constraint belongs to (e.g. "dbo"). + /// + [Column("SchemaName")] + public string SchemaName { get; set; } = null!; + + /// + /// Gets or sets the name of the table the constraint belongs to. + /// + [Column("TableName")] + public string TableName { get; set; } = null!; + + /// + /// Gets or sets the name of the untrusted constraint. + /// + [Column("ConstraintName")] + public string ConstraintName { get; set; } = null!; + } +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/HealthChecks/UntrustedDatabaseConstraintsCheckTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/HealthChecks/UntrustedDatabaseConstraintsCheckTests.cs new file mode 100644 index 000000000000..de6fad671161 --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/HealthChecks/UntrustedDatabaseConstraintsCheckTests.cs @@ -0,0 +1,89 @@ +using Microsoft.Extensions.DependencyInjection; +using NUnit.Framework; +using Umbraco.Cms.Core.HealthChecks; +using Umbraco.Cms.Infrastructure.HealthChecks.Checks.Data; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Testing; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.HealthChecks; + +[TestFixture] +[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] +internal sealed class UntrustedDatabaseConstraintsCheckTests : UmbracoIntegrationTest +{ + private const string TestConstraintName = "FK_umbracoRelation_umbracoNode"; + + private UntrustedDatabaseConstraintsCheck CreateSut() + => new(Services.GetRequiredService()); + + [Test] + public async Task FreshDatabase_ReportsSuccessOnSqlServer() + { + if (BaseTestDatabase.IsSqlite()) + { + Assert.Ignore("Untrusted constraints are a SQL Server concept and do not apply to SQLite."); + return; + } + + HealthCheckStatus status = (await CreateSut().GetStatusAsync()).Single(); + + Assert.That(status.ResultType, Is.EqualTo(StatusResultType.Success)); + } + + [Test] + public async Task UntrustedConstraint_ReportsWarningAndIncludesConstraintName() + { + if (BaseTestDatabase.IsSqlite()) + { + Assert.Ignore("Untrusted constraints are a SQL Server concept and do not apply to SQLite."); + return; + } + + MakeConstraintUntrusted(TestConstraintName); + + try + { + HealthCheckStatus status = (await CreateSut().GetStatusAsync()).Single(); + + Assert.Multiple(() => + { + Assert.That(status.ResultType, Is.EqualTo(StatusResultType.Warning)); + Assert.That(status.Message, Does.Contain(TestConstraintName)); + Assert.That(status.ReadMoreLink, Is.Not.Null.And.Not.Empty); + }); + } + finally + { + RetrustConstraint(TestConstraintName); + } + } + + [Test] + public async Task SqliteDatabase_ReportsInfoShortCircuit() + { + if (BaseTestDatabase.IsSqlite() is false) + { + Assert.Ignore("This test verifies the SQLite short-circuit path."); + return; + } + + HealthCheckStatus status = (await CreateSut().GetStatusAsync()).Single(); + + Assert.That(status.ResultType, Is.EqualTo(StatusResultType.Info)); + } + + private void MakeConstraintUntrusted(string constraintName) + => ExecuteNonQuery( + $"ALTER TABLE [umbracoRelation] NOCHECK CONSTRAINT [{constraintName}];" + + $"ALTER TABLE [umbracoRelation] WITH NOCHECK CHECK CONSTRAINT [{constraintName}];"); + + private void RetrustConstraint(string constraintName) + => ExecuteNonQuery($"ALTER TABLE [umbracoRelation] WITH CHECK CHECK CONSTRAINT [{constraintName}];"); + + private void ExecuteNonQuery(string sql) + { + using IUmbracoDatabase db = Services.GetRequiredService().CreateDatabase(); + db.Execute(sql); + } +}