Repository navigation
Run datatype root invariants also when the element has its own invariants (#567) - #696
Merged
ewoutkramer merged 2 commits intoOct 1, 2026
Merged
Conversation
…ants The extra BaseTypeInvariantConstraintsValidator was only added when the element had no FhirPathValidator at all, so a profile that walks into a datatype (e.g. Encounter.period.start) and also declares an invariant on that element silently lost the root invariants of the datatype (per-1). Add the validator regardless, but skip the keys already validated on the element so nothing runs twice. Fixes #567 (first point) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2SNRG6mrRVURXPQCPiau5
Encounter.period and Encounter.EncounterStatus do not exist in R5, which broke the R5 test project build. Coverage.period exists in all versions, and the test only asserts the presence of per-1. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2SNRG6mrRVURXPQCPiau5
ewoutkramer
approved these changes
Sep 30, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation addresses issue #567 while avoiding duplicate validation and includes suitable multi-version regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Ensures datatype root invariants remain active when profiles expand datatype children and define their own invariants.
Changes:
- Always adds the base-type invariant validator for eligible expanded datatypes.
- Skips invariant keys already validated by the element.
- Adds cross-version regression coverage and updates the public API baseline.
| File | Description |
|---|---|
SchemaBuilder.cs |
Adds base-type validation with duplicate-key suppression. |
BaseTypeInvariantConstraintsValidator.cs |
Supports excluding previously validated invariant keys. |
PublicAPI.Unshipped.txt |
Records the new constructor overload. |
TestProfileArtifactSource.cs |
Defines Coverage profiles for regression testing. |
SimpleValidationsTests.cs |
Verifies per-1 runs with and without an element invariant. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
ewoutkramer
deleted the
claude/fix-567-root-invariants-expanded-datatypes
branch
October 1, 2026 00:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Ewout Kramer · Slack thread
Description
Issue #567 asks that the root invariants of a datatype (e.g.
per-1onPeriod) also run when a profile walks into that datatype's children. On currentdevelopthis already works for any (non-choice, non-sliced, non-backbone) element that has no invariants of its own, butSchemaBuilderonly added theBaseTypeInvariantConstraintsValidatorwhen the element had noFhirPathValidatorat all. So a profile that walks into e.g.Coverage.period.startand declares an invariant onCoverage.periodsilently lostper-1.This PR adds the validator regardless, but passes it the keys of the invariants already on the element so they are skipped there and nothing is validated twice (the original reason for the guard).
Not addressed: the second bullet of #567 (whether to replace the special validator by a mode of the
SchemaReferencevalidator). That is a refactor; a question about it is posted on the issue. Per the maintainer's answer on the issue, the special root-invariant validator is kept, so this PR closes #567.Related issues
Closes #567
Testing
ValidateRootInvariantsOfDatatypeWhoseChildrenAreProfiledwith two new test profiles (Coverage.period.startmandatory, with and without an own invariant onCoverage.period). The "with own invariant" case fails before the change, passes after. Passes on the STU3, R4, R4B and R5 test projects (the first push used Encounter, which does not compile for R5; fixed in 7b03f4f).Firely.Fhir.Validation.Compilation.Tests.R4: all pass except 3 tests that need theFhirTestCasessubmodule, which is not checked out in this environment (unrelated).PublicAPI.Unshipped.txtupdated for the new (still unshipped) constructor overload.🤖 Generated with Claude Code
https://claude.ai/code/session_01J2SNRG6mrRVURXPQCPiau5