Health Check: Add check for untrusted database constraints on SQL Server#22592
Conversation
|
Claude finished @AndyButland's task in 5m 54s —— View job PR ReviewTarget: Adds a "Untrusted database constraints" health check to the Data Integrity group that queries
Suggestions
NotesA few observations that don't warrant findings but are worth calling out for context:
Approved with Suggestions for improvementGood to go, but please carefully consider the importance of the suggestions. |
There was a problem hiding this comment.
Pull request overview
Adds a new Data Integrity health check that surfaces untrusted SQL Server FK/check constraints (left behind when re-trust migration skips violated constraints), including a documentation “Read more” link and integration coverage.
Changes:
- Added
UntrustedDatabaseConstraintsCheckhealth check that queriessys.foreign_keys/sys.check_constraintsforis_not_trusted = 1onumbraco%/cms%tables and reports Success/Warning/Info accordingly. - Added a documentation-link constant for the new health check.
- Added integration tests covering: fresh DB (Success), deliberately untrusted FK (Warning), and SQLite short-circuit (Info).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Umbraco.Infrastructure/HealthChecks/Checks/Data/UntrustedDatabaseConstraintsCheck.cs | Implements the new SQL Server-only health check and renders a per-constraint HTML description + read-more link. |
| src/Umbraco.Core/Constants-HealthChecks.cs | Adds the documentation shortlink constant used by the new health check. |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/HealthChecks/UntrustedDatabaseConstraintsCheckTests.cs | Adds integration tests validating Success/Warning/Info outcomes across SQL Server vs SQLite. |
kjac
left a comment
There was a problem hiding this comment.
This is excellent, @AndyButland 💪
The short-link does not work, though - https://umbra.co/healthchecks-untrusted-constraints yields a 404. But if that's to be expected at this point, feel free to merge this in 👍
|
Thanks @kjac.
Yes, that's expected. I've raised umbraco/UmbracoDocs#8004 for the docs, so when that is merged in - and before 17.5 - we can prepare the short-link. |
Description
The
RetrustForeignKeyAndCheckConstraintsmigration introduced in 17.3 (see #21744 and #22488) re-trusts untrusted foreign key and check constraints so the SQL Server query optimizer can use them. Existing data integrity violations are logged as a warning and the migration continues.Although there's a warning in the upgrade log there is no other in-product signal of an issue.
This PR adds a new health check — "Untrusted database constraints" — in the existing Data Integrity group that surfaces this state and links to documentation for manual resolution.
The check does the same SQL Server query to find untrusted constraints that the migration does (limited to Umbraco tables).
It then reports details on any untrusted constraints found:
Or gives a success message if none are found:
There's no automatic fix to the constraints as it's likely orphaned records need to be removed (otherwise the migration would have fixed them). Instead there's a link to a docs page that needs to be written and the short-link setup.
Testing
Automated
Integration tests are added for the health-check. Set
Tests:Database:DatabaseTypeintests/Umbraco.Tests.Integration/appsettings.Tests.jsonto"LocalDb"to exercise it:Manual
Run a site against SQL Server.
1. Verify the happy path (no untrusted constraints).
2. Create an untrusted constraint (flips the trust flag without needing orphaned data):
Re-run the health check. Expected:
Foreign key FK_umbracoRelation_umbracoNode on dbo.umbracoRelation.3. Restore trust (returns the check to Success):
Re-run the health check — should return to Success.
4. Re-check the migration.
Prepare an untrusted constraint again (step 2).
Reset the migration state to the step prior to the trust constraints migration:
Restart Umbraco.
Re-run the health check — should show Success.