From cc57034180100c604013a383329301ed14ecae1c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 20:51:57 +0000 Subject: [PATCH 1/2] Run type root invariants also when the element carries its own invariants 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 Claude-Session: https://claude.ai/code/session_01J2SNRG6mrRVURXPQCPiau5 --- .../SchemaBuilder.cs | 8 ++-- .../BaseTypeInvariantConstraintsValidator.cs | 21 ++++++++- .../PublicAPI.Unshipped.txt | 1 + .../SimpleValidationsTests.cs | 25 ++++++++++ .../TestProfileArtifactSource.cs | 46 +++++++++++++++++++ 5 files changed, 97 insertions(+), 4 deletions(-) diff --git a/src/Firely.Fhir.Validation.Compilation.Shared/SchemaBuilder.cs b/src/Firely.Fhir.Validation.Compilation.Shared/SchemaBuilder.cs index fb1f4f33..f2bfebf1 100644 --- a/src/Firely.Fhir.Validation.Compilation.Shared/SchemaBuilder.cs +++ b/src/Firely.Fhir.Validation.Compilation.Shared/SchemaBuilder.cs @@ -263,10 +263,12 @@ internal List ConvertElement(ElementDefinitionNavigator nav, Subsche // Type we're working with was pulled into definition, and we're not in a slice // so we need to validate any invariant rules we might find as well, but we shouldn't pull them twice. // We can skip backbone elements though, as there's nothing to copy. - if (schemaMembers.OfType().Any() && !schemaMembers.OfType().Any() - && string.IsNullOrEmpty(nav.Current.SliceName) && !nav.Current.IsBackboneElement()) + // Invariants that are already on this element (declared in the profile, or copied into the snapshot) are + // not validated a second time, but their presence must not suppress the root invariants of the type + // (https://github.com/FirelyTeam/firely-validator-api/issues/567). + if (schemaMembers.OfType().Any() && string.IsNullOrEmpty(nav.Current.SliceName) && !nav.Current.IsBackboneElement()) { - schemaMembers.Add(new BaseTypeInvariantConstraintsValidator()); + schemaMembers.Add(new BaseTypeInvariantConstraintsValidator(schemaMembers.OfType().Select(x => x.Key))); } } } diff --git a/src/Firely.Fhir.Validation/Impl/BaseTypeInvariantConstraintsValidator.cs b/src/Firely.Fhir.Validation/Impl/BaseTypeInvariantConstraintsValidator.cs index 660ae985..b7055130 100644 --- a/src/Firely.Fhir.Validation/Impl/BaseTypeInvariantConstraintsValidator.cs +++ b/src/Firely.Fhir.Validation/Impl/BaseTypeInvariantConstraintsValidator.cs @@ -26,6 +26,25 @@ namespace Firely.Fhir.Validation; #endif public class BaseTypeInvariantConstraintsValidator : IValidatable { + private readonly string[] _alreadyValidatedKeys; + + /// + /// Creates a validator that runs all invariants found on the root of the element's type. + /// + public BaseTypeInvariantConstraintsValidator() : this([]) + { + } + + /// + /// Creates a validator that runs the invariants found on the root of the element's type, + /// except for those whose key is already validated on the element itself. + /// + /// Keys of the invariants that are already part of the element's schema. + public BaseTypeInvariantConstraintsValidator(System.Collections.Generic.IEnumerable alreadyValidatedKeys) + { + _alreadyValidatedKeys = alreadyValidatedKeys.ToArray(); + } + /// /// Validate input against the expected context and invariants. /// @@ -38,7 +57,7 @@ public ResultReport Validate(PocoNode input, ValidationSettings vc, ValidationSt var typeProfile = vc.TypeNameMapper.MapTypeName(input.Poco.TypeName); return FhirSchemaGroupAnalyzer.FetchSchema(vc.ElementSchemaResolver, state, typeProfile, input.GetLocation()) switch { - (var schema, null, _) => ResultReport.Combine(schema!.Members.Where(vc.Filter).OfType().Select(x => x.ValidateOne(input, vc, state)).ToList()), + (var schema, null, _) => ResultReport.Combine(schema!.Members.Where(vc.Filter).OfType().Where(x => !_alreadyValidatedKeys.Contains(x.Key)).Select(x => x.ValidateOne(input, vc, state)).ToList()), (_, var error, _) => error }; } diff --git a/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt b/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt index 74c26f28..90131119 100644 --- a/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt +++ b/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt @@ -34,6 +34,7 @@ Firely.Fhir.Validation.BaseType.ToJson() -> Newtonsoft.Json.Linq.JToken! Firely.Fhir.Validation.BaseType.Type.get -> string! Firely.Fhir.Validation.BaseTypeInvariantConstraintsValidator Firely.Fhir.Validation.BaseTypeInvariantConstraintsValidator.BaseTypeInvariantConstraintsValidator() -> void +Firely.Fhir.Validation.BaseTypeInvariantConstraintsValidator.BaseTypeInvariantConstraintsValidator(System.Collections.Generic.IEnumerable! alreadyValidatedKeys) -> void Firely.Fhir.Validation.BaseTypeInvariantConstraintsValidator.ToJson() -> Newtonsoft.Json.Linq.JToken! Firely.Fhir.Validation.BaseTypeInvariantConstraintsValidator.Validate(Hl7.Fhir.Model.PocoNode! input, Firely.Fhir.Validation.ValidationSettings! vc, Firely.Fhir.Validation.ValidationState! state) -> Firely.Fhir.Validation.ResultReport! Firely.Fhir.Validation.BasicValidator diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs index 7456e782..bd3390db 100644 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs @@ -270,6 +270,31 @@ public void ValidateProfiledRunsInvariants(string url) } #endif + [Theory] + [InlineData(TestProfileArtifactSource.PROFILEDPERIODCHILDREN)] + [InlineData(TestProfileArtifactSource.PROFILEDPERIODCHILDRENWITHINVARIANT)] + public void ValidateRootInvariantsOfDatatypeWhoseChildrenAreProfiled(string url) + { + // Encounter.period is not a choice element and not sliced, but the profile walks into it. + // The root invariants of Period (per-1) must still be evaluated (issue 567). + var encounter = new Encounter() + { + Meta = new Meta { Profile = [ url ] }, +#if STU3 + Status = Encounter.EncounterStatus.Finished, +#else + Status = Encounter.EncounterStatus.Finished, +#endif + Period = new Period(new("2017-06-02"), new("2017-06-01")) + }; + + var schema = _fixture.SchemaResolver.GetSchema(url)!; + var result = schema.Validate(encounter.ToTypedElement(), _fixture.NewValidationSettings()); + var oo = result.ToOperationOutcome(); + oo.Success.Should().Be(false); + oo.Issue.Exists(i => i.Details?.Text != null && i.Details.Text.Contains("per-1")).Should().BeTrue($"the Period root invariant per-1 must be reported for {url}"); + } + [Fact] public void ValidateUriStringsInExtension() { diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs index c1bb9766..f85ad5c5 100644 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs @@ -49,6 +49,9 @@ internal class TestProfileArtifactSource : IResourceResolver public const string PROFILEDEXTENSIONTYPEWITHCHILDREN = "http://validationtest.org/fhir/StructureDefinition/ExtensionValueXChildren"; public const string PROFILEDEXTENSIONTYPEWITHSLICE = "http://validationtest.org/fhir/StructureDefinition/ExtensionValuePeriodSlice"; + public const string PROFILEDPERIODCHILDREN = "http://validationtest.org/fhir/StructureDefinition/EncounterPeriodChildren"; + public const string PROFILEDPERIODCHILDRENWITHINVARIANT = "http://validationtest.org/fhir/StructureDefinition/EncounterPeriodChildrenWithInvariant"; + public const string PROFILEDINVARIANTRESOLVE = "http://validationtest.org/fhir/StructureDefinition/ObservationSubjectPatientActive"; public const string EMPTYSNAPSHOTUNKNOWNBASE = "http://validationtest.org/fhir/StructureDefinition/EmptySnapshotDueToUnknownBaseProfile"; @@ -95,6 +98,8 @@ internal class TestProfileArtifactSource : IResourceResolver buildExtensionValueChildConstraints(PROFILEDEXTENSIONTYPEWITHCHILDREN, "value[x]"), buildExtensionValueChildConstraints(PROFILEDEXTENSIONTYPEWITHSLICE, "valuePeriod"), buildExtensionValueChildConstraints(), + buildEncounterPeriodChildren(PROFILEDPERIODCHILDREN, withOwnInvariant: false), + buildEncounterPeriodChildren(PROFILEDPERIODCHILDRENWITHINVARIANT, withOwnInvariant: true), buildEmptySnapshotWithUnknownBaseProfile(), buildSliceWithRepeatingChildrenTestcase(), @@ -129,6 +134,47 @@ private static StructureDefinition buildExtensionValueChildConstraints() return result; } + /// + /// A profile on Encounter that walks into the (non-choice) element Encounter.period by constraining + /// its child start, optionally also adding an invariant on Encounter.period itself. The root + /// invariants of the Period datatype (per-1) must still be run. + /// See: https://github.com/FirelyTeam/firely-validator-api/issues/567 + /// + private static StructureDefinition buildEncounterPeriodChildren(string uri, bool withOwnInvariant) + { + var result = createTestSD( + uri, + "Encounter that makes period.start mandatory", + "Encounter that makes period.start mandatory", + FHIRAllTypes.Encounter + ); + + var period = new ElementDefinition("Encounter.period"); + if (withOwnInvariant) + period.Constraint = + [ + new() + { +#if STU3 + Severity = ElementDefinition.ConstraintSeverity.Error, +#else + Severity = ConstraintSeverity.Error, +#endif + Key = "enc-test", + Expression = "start.exists()", + Human = "Test invariant" + } + ]; + + result.Differential!.Element = + [ + period, + new("Encounter.period.start") { Min = 1 } + ]; + + return result; + } + private static StructureDefinition buildExtensionValueChildConstraints(string uri, string property) { var result = createTestSD( From 7b03f4f32d87fc154d5e7ce4417b17adc81b5618 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 30 Sep 2026 20:59:15 +0000 Subject: [PATCH 2/2] Use Coverage.period in the #567 test so it compiles for R5 and STU3 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 Claude-Session: https://claude.ai/code/session_01J2SNRG6mrRVURXPQCPiau5 --- .../SimpleValidationsTests.cs | 12 +++------ .../TestProfileArtifactSource.cs | 26 +++++++++---------- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs index bd3390db..29fc11c7 100644 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/SimpleValidationsTests.cs @@ -275,21 +275,17 @@ public void ValidateProfiledRunsInvariants(string url) [InlineData(TestProfileArtifactSource.PROFILEDPERIODCHILDRENWITHINVARIANT)] public void ValidateRootInvariantsOfDatatypeWhoseChildrenAreProfiled(string url) { - // Encounter.period is not a choice element and not sliced, but the profile walks into it. + // Coverage.period is not a choice element and not sliced, but the profile walks into it. // The root invariants of Period (per-1) must still be evaluated (issue 567). - var encounter = new Encounter() + // Other required elements are deliberately left out: only the presence of per-1 is asserted. + var coverage = new Coverage() { Meta = new Meta { Profile = [ url ] }, -#if STU3 - Status = Encounter.EncounterStatus.Finished, -#else - Status = Encounter.EncounterStatus.Finished, -#endif Period = new Period(new("2017-06-02"), new("2017-06-01")) }; var schema = _fixture.SchemaResolver.GetSchema(url)!; - var result = schema.Validate(encounter.ToTypedElement(), _fixture.NewValidationSettings()); + var result = schema.Validate(coverage.ToTypedElement(), _fixture.NewValidationSettings()); var oo = result.ToOperationOutcome(); oo.Success.Should().Be(false); oo.Issue.Exists(i => i.Details?.Text != null && i.Details.Text.Contains("per-1")).Should().BeTrue($"the Period root invariant per-1 must be reported for {url}"); diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs index f85ad5c5..661c8493 100644 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/TestProfileArtifactSource.cs @@ -49,8 +49,8 @@ internal class TestProfileArtifactSource : IResourceResolver public const string PROFILEDEXTENSIONTYPEWITHCHILDREN = "http://validationtest.org/fhir/StructureDefinition/ExtensionValueXChildren"; public const string PROFILEDEXTENSIONTYPEWITHSLICE = "http://validationtest.org/fhir/StructureDefinition/ExtensionValuePeriodSlice"; - public const string PROFILEDPERIODCHILDREN = "http://validationtest.org/fhir/StructureDefinition/EncounterPeriodChildren"; - public const string PROFILEDPERIODCHILDRENWITHINVARIANT = "http://validationtest.org/fhir/StructureDefinition/EncounterPeriodChildrenWithInvariant"; + public const string PROFILEDPERIODCHILDREN = "http://validationtest.org/fhir/StructureDefinition/CoveragePeriodChildren"; + public const string PROFILEDPERIODCHILDRENWITHINVARIANT = "http://validationtest.org/fhir/StructureDefinition/CoveragePeriodChildrenWithInvariant"; public const string PROFILEDINVARIANTRESOLVE = "http://validationtest.org/fhir/StructureDefinition/ObservationSubjectPatientActive"; @@ -98,8 +98,8 @@ internal class TestProfileArtifactSource : IResourceResolver buildExtensionValueChildConstraints(PROFILEDEXTENSIONTYPEWITHCHILDREN, "value[x]"), buildExtensionValueChildConstraints(PROFILEDEXTENSIONTYPEWITHSLICE, "valuePeriod"), buildExtensionValueChildConstraints(), - buildEncounterPeriodChildren(PROFILEDPERIODCHILDREN, withOwnInvariant: false), - buildEncounterPeriodChildren(PROFILEDPERIODCHILDRENWITHINVARIANT, withOwnInvariant: true), + buildCoveragePeriodChildren(PROFILEDPERIODCHILDREN, withOwnInvariant: false), + buildCoveragePeriodChildren(PROFILEDPERIODCHILDRENWITHINVARIANT, withOwnInvariant: true), buildEmptySnapshotWithUnknownBaseProfile(), buildSliceWithRepeatingChildrenTestcase(), @@ -135,21 +135,21 @@ private static StructureDefinition buildExtensionValueChildConstraints() } /// - /// A profile on Encounter that walks into the (non-choice) element Encounter.period by constraining - /// its child start, optionally also adding an invariant on Encounter.period itself. The root + /// A profile on Coverage that walks into the (non-choice) element Coverage.period by constraining + /// its child start, optionally also adding an invariant on Coverage.period itself. The root /// invariants of the Period datatype (per-1) must still be run. /// See: https://github.com/FirelyTeam/firely-validator-api/issues/567 /// - private static StructureDefinition buildEncounterPeriodChildren(string uri, bool withOwnInvariant) + private static StructureDefinition buildCoveragePeriodChildren(string uri, bool withOwnInvariant) { var result = createTestSD( uri, - "Encounter that makes period.start mandatory", - "Encounter that makes period.start mandatory", - FHIRAllTypes.Encounter + "Coverage that makes period.start mandatory", + "Coverage that makes period.start mandatory", + FHIRAllTypes.Coverage ); - var period = new ElementDefinition("Encounter.period"); + var period = new ElementDefinition("Coverage.period"); if (withOwnInvariant) period.Constraint = [ @@ -160,7 +160,7 @@ private static StructureDefinition buildEncounterPeriodChildren(string uri, bool #else Severity = ConstraintSeverity.Error, #endif - Key = "enc-test", + Key = "cov-test", Expression = "start.exists()", Human = "Test invariant" } @@ -169,7 +169,7 @@ private static StructureDefinition buildEncounterPeriodChildren(string uri, bool result.Differential!.Element = [ period, - new("Encounter.period.start") { Min = 1 } + new("Coverage.period.start") { Min = 1 } ]; return result;