From 7790d16a2914466d6c640607bde509107db83263 Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Tue, 3 Sep 2024 17:21:49 +0200 Subject: [PATCH 01/15] Circular reference error should now have a proper location set --- src/Firely.Fhir.Validation/Schema/InstancePath.cs | 2 +- .../Schema/ValidationLogger.cs | 15 ++++++++++++++- .../Impl/CircularReferenceTests.cs | 1 + 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/Firely.Fhir.Validation/Schema/InstancePath.cs b/src/Firely.Fhir.Validation/Schema/InstancePath.cs index ef14e915..ddcd3464 100644 --- a/src/Firely.Fhir.Validation/Schema/InstancePath.cs +++ b/src/Firely.Fhir.Validation/Schema/InstancePath.cs @@ -16,7 +16,7 @@ namespace Firely.Fhir.Validation /// internal class InstancePath : PathStack { - private InstancePath(PathStackEvent? current) : base(current) + internal InstancePath(PathStackEvent? current) : base(current) { } diff --git a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs index 1707b8b3..dcd68039 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs @@ -83,7 +83,7 @@ public ResultReport Start(ValidationState state, string profileUrl, Func diff --git a/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs b/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs index f3a3497a..8f415187 100644 --- a/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs +++ b/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs @@ -10,6 +10,7 @@ using Hl7.Fhir.ElementModel; using Hl7.Fhir.Support; using Microsoft.VisualStudio.TestTools.UnitTesting; +using System.Linq; namespace Firely.Fhir.Validation.Tests { From 3d8b8971328f17403e370307ec5dcbe71405fabd Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Wed, 4 Sep 2024 11:25:29 +0200 Subject: [PATCH 02/15] Added a more elaborate error which should be correct in every case now --- .../Impl/ResourceSchema.cs | 3 +- .../Schema/ValidationLogger.cs | 43 ++++++++++++++++--- .../Impl/CircularReferenceTests.cs | 1 + 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs index c695210b..72c7df98 100644 --- a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs +++ b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs @@ -125,7 +125,8 @@ internal ResultReport ValidateResourceSchema(IScopedNode input, ValidationSettin { return state.Global.RunValidations.Start( state, - Id.ToString(), // is the same as the canonical for resource schemas + Id.ToString(), // is the same as the canonical for resource schemas + input, () => { state.Global.ResourcesValidated += 1; diff --git a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs index dcd68039..22565292 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs @@ -6,9 +6,11 @@ * available at https://github.com/FirelyTeam/firely-validator-api/blob/main/LICENSE */ +using Hl7.Fhir.ElementModel; using Hl7.Fhir.Support; using System; using System.Collections.Generic; +using System.Linq; #nullable enable @@ -67,10 +69,11 @@ public override string ToString() => /// /// The validation state /// Profile against which we are validating + /// The node that is being validated. We need this for computing circular references /// Validation to start when it has not been run before. /// The result of calling the validator, or a historic result if there is one. #pragma warning disable CS0618 // Type or member is obsolete - public ResultReport Start(ValidationState state, string profileUrl, Func validator) + public ResultReport Start(ValidationState state, string profileUrl, IScopedNode node, Func validator) #pragma warning restore CS0618 // Type or member is obsolete { var resourceUrl = state.Instance.ResourceUrl; @@ -82,8 +85,8 @@ public ResultReport Start(ValidationState state, string profileUrl, Func? followed = null) // this is expensive, but only executed when a loop is detected. We accept this #pragma warning restore CS0618 // Type or member is obsolete { - for(var current = state.Location.InstanceLocation.Current; current is not null; current = current.Previous) + + if (current.AtResource && followed?.Count(c => c.Item2 == current.Location) is 2) // if we followed the same reference twice, we have a loop { - if (current is InternalReferenceNavEvent ire) - return new InstancePath(ire.Previous).ToString(); + return string.Join(" | ", followed.Select(reference => $"{reference.Item1} -> {reference.Item2}")); + } + + followed ??= []; + + foreach (var child in current.Children()) + { + var childNode = child.ToScopedNode(); + + if (childNode.InstanceType == "Reference") + { + var target = childNode.Resolve(); + if (target is null) // possible external reference, we cannot check this + { + continue; + } + + followed.Add((childNode.Location, target.Location)); // add the reference to the list of followed references + + if(getCircularReferenceStructure(target, followed) is { } result) + return result; // if multiple paths are found, we only return the first one. Rerunning will show the next one. Let's hope that never happens. + } + + if (getCircularReferenceStructure(childNode, followed) is { } result2) // why is result still in scope? that makes no sense + { + return result2; + } } return null; diff --git a/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs b/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs index 8f415187..fc0813d2 100644 --- a/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs +++ b/test/Firely.Fhir.Validation.Tests/Impl/CircularReferenceTests.cs @@ -91,6 +91,7 @@ public void CircularInContainedResources() var result = test(SCHEMA, pat.DictionaryToTypedElement("Patient")); result.IsSuccessful.Should().BeTrue(); result.Evidence.Should().Contain(ass => (ass as IssueAssertion)!.IssueNumber == Issue.CONTENT_REFERENCE_CYCLE_DETECTED.Code); + result.Warnings.Should().ContainSingle(ass => ass.IssueNumber == Issue.CONTENT_REFERENCE_CYCLE_DETECTED.Code).Subject.Message.Should().ContainAll(["Patient.contained[1]", "Patient.contained[0]"]); } [TestMethod] From a0f9c280695a8643062108ec8da98051be98c44b Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Wed, 4 Sep 2024 13:14:11 +0200 Subject: [PATCH 03/15] no warnings for a contained resource that references the container --- .../Schema/ValidationLogger.cs | 26 +++++++++---------- .../FhirTests/ValidationManifestTest.cs | 4 +-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs index 22565292..3f56e978 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs @@ -83,17 +83,16 @@ public ResultReport Start(ValidationState state, string profileUrl, IScopedNode if (_data.TryGetValue(key, out var existing)) { - if (existing.Result is null) + if (existing.Result is null && computeReferenceCycle(node.ToScopedNode()) is { } referenceCycle) // check for loops: we do not include references to containers in the cycle return new IssueAssertion(Issue.CONTENT_REFERENCE_CYCLE_DETECTED, - $"Detected a loop: instance data inside '{fullLocation}' refers back to itself (cycle structure: {getCircularReferenceStructure(node.ToScopedNode())}).") + $"Detected a loop: instance data inside '{fullLocation}' refers back to itself (cycle structure: {referenceCycle}).") .AsResult(fullLocation); - else - { - // If the validation has been run before, return an outcome with the same result. - // Note: we don't keep a copy of the original outcome since it has been included in the - // total result (at the first run) and keeping them around costs a lot of memory. - return new ResultReport(existing.Result.Value, new TraceAssertion(fullLocation, $"Repeated validation at {fullLocation} against profile {profileUrl}.")); - } + + // If the validation has been run before, return an outcome with the same result. + // Note: we don't keep a copy of the original outcome since it has been included in the + // total result (at the first run) and keeping them around costs a lot of memory. + return existing.Result is not null ? + new ResultReport(existing.Result.Value, new TraceAssertion(fullLocation, $"Repeated validation at {fullLocation} against profile {profileUrl}.")) : ResultReport.SUCCESS; } else { @@ -110,7 +109,7 @@ public ResultReport Start(ValidationState state, string profileUrl, IScopedNode } #pragma warning disable CS0618 // Type or member is obsolete - string? getCircularReferenceStructure(ScopedNode current, IList<(string, string)>? followed = null) // this is expensive, but only executed when a loop is detected. We accept this + string? computeReferenceCycle(ScopedNode current, IList<(string, string)>? followed = null) // this is expensive, but only executed when a loop is detected. We accept this #pragma warning restore CS0618 // Type or member is obsolete { @@ -125,7 +124,8 @@ public ResultReport Start(ValidationState state, string profileUrl, IScopedNode { var childNode = child.ToScopedNode(); - if (childNode.InstanceType == "Reference") + // don't follow references to container: these are always circular and they are considered correct + if (childNode.InstanceType == "Reference" && childNode.Children("reference").SingleOrDefault()?.Value is string reference && reference != "#") { var target = childNode.Resolve(); if (target is null) // possible external reference, we cannot check this @@ -135,11 +135,11 @@ public ResultReport Start(ValidationState state, string profileUrl, IScopedNode followed.Add((childNode.Location, target.Location)); // add the reference to the list of followed references - if(getCircularReferenceStructure(target, followed) is { } result) + if(computeReferenceCycle(target, followed) is { } result) return result; // if multiple paths are found, we only return the first one. Rerunning will show the next one. Let's hope that never happens. } - if (getCircularReferenceStructure(childNode, followed) is { } result2) // why is result still in scope? that makes no sense + if (computeReferenceCycle(childNode, followed) is { } result2) // why is result still in scope? that makes no sense { return result2; } diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs index ec4540cc..3660dcb4 100644 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs @@ -32,7 +32,7 @@ public class ValidationManifestTest /// //[Ignore] [DataTestMethod] - [ValidationManifestDataSource(TEST_CASES_MANIFEST, singleTest: "mr-m-simple-nossystem")] + [ValidationManifestDataSource(TEST_CASES_MANIFEST, singleTest: "containedToContainer")] public void RunSingleTest(TestCase testCase, string baseDirectory) => _runner.RunTestCase(testCase, DotNetValidator.Create(), baseDirectory); @@ -59,7 +59,7 @@ public void RunFirelySdkTests(TestCase testCase, string baseDirectory) /// - The method `ClassCleanup` will gather all the testcases and serialize those to disk. The filename can be altered in /// that method /// - [Ignore("This test is only used to generate the Firely SDK results in the manifest. See the method for more info")] + // [Ignore("This test is only used to generate the Firely SDK results in the manifest. See the method for more info")] [TestMethod] public void AddFirelySdkValidatorResults() => _runner.AddOrEditValidatorResults(TEST_CASES_MANIFEST, new[] { DotNetValidator.Create() }); From ff3f7fe8912ad0f4b688d9fb03a7fc4cb0f842dc Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Wed, 4 Sep 2024 14:09:14 +0200 Subject: [PATCH 04/15] added compatibility with external resolvers (and circular references there) --- .../Impl/ResourceSchema.cs | 4 +- .../Schema/ValidationLogger.cs | 57 +++---------------- .../Support/IScopedNodeExtensions.cs | 39 +++++++++++++ .../FhirTests/ValidationManifestTest.cs | 2 +- 4 files changed, 50 insertions(+), 52 deletions(-) diff --git a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs index 72c7df98..2fb6f41b 100644 --- a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs +++ b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs @@ -126,12 +126,12 @@ internal ResultReport ValidateResourceSchema(IScopedNode input, ValidationSettin return state.Global.RunValidations.Start( state, Id.ToString(), // is the same as the canonical for resource schemas - input, () => { state.Global.ResourcesValidated += 1; return base.ValidateInternal(input, vc, state); - }); + }, + () => input.ComputeReferenceCycle(vc)); } /// diff --git a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs index 3f56e978..4d9888ab 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs @@ -16,8 +16,6 @@ namespace Firely.Fhir.Validation { - - /// /// This class is used to track which resources/contained resources/bundled resources are already /// validated, and what the previous result was. Since it keeps track of running validations, it @@ -69,11 +67,11 @@ public override string ToString() => /// /// The validation state /// Profile against which we are validating - /// The node that is being validated. We need this for computing circular references /// Validation to start when it has not been run before. + /// A function which visualises reference cycles when called /// The result of calling the validator, or a historic result if there is one. #pragma warning disable CS0618 // Type or member is obsolete - public ResultReport Start(ValidationState state, string profileUrl, IScopedNode node, Func validator) + public ResultReport Start(ValidationState state, string profileUrl, Func validator, Func refCycleVisualizer) #pragma warning restore CS0618 // Type or member is obsolete { var resourceUrl = state.Instance.ResourceUrl; @@ -83,16 +81,17 @@ public ResultReport Start(ValidationState state, string profileUrl, IScopedNode if (_data.TryGetValue(key, out var existing)) { - if (existing.Result is null && computeReferenceCycle(node.ToScopedNode()) is { } referenceCycle) // check for loops: we do not include references to containers in the cycle + if (existing.Result is null && refCycleVisualizer() is { } referenceCycle) // check for loops: we do not include references to containers in the cycle return new IssueAssertion(Issue.CONTENT_REFERENCE_CYCLE_DETECTED, - $"Detected a loop: instance data inside '{fullLocation}' refers back to itself (cycle structure: {referenceCycle}).") + $"Detected a loop: instance data inside '{fullLocation}' refers back to itself (cycle structure: {referenceCycle}).") .AsResult(fullLocation); - + // If the validation has been run before, return an outcome with the same result. // Note: we don't keep a copy of the original outcome since it has been included in the // total result (at the first run) and keeping them around costs a lot of memory. - return existing.Result is not null ? - new ResultReport(existing.Result.Value, new TraceAssertion(fullLocation, $"Repeated validation at {fullLocation} against profile {profileUrl}.")) : ResultReport.SUCCESS; + return existing.Result is not null + ? new ResultReport(existing.Result.Value, new TraceAssertion(fullLocation, $"Repeated validation at {fullLocation} against profile {profileUrl}.")) + : ResultReport.SUCCESS; } else { @@ -107,46 +106,6 @@ public ResultReport Start(ValidationState state, string profileUrl, IScopedNode return result; } - -#pragma warning disable CS0618 // Type or member is obsolete - string? computeReferenceCycle(ScopedNode current, IList<(string, string)>? followed = null) // this is expensive, but only executed when a loop is detected. We accept this -#pragma warning restore CS0618 // Type or member is obsolete - { - - if (current.AtResource && followed?.Count(c => c.Item2 == current.Location) is 2) // if we followed the same reference twice, we have a loop - { - return string.Join(" | ", followed.Select(reference => $"{reference.Item1} -> {reference.Item2}")); - } - - followed ??= []; - - foreach (var child in current.Children()) - { - var childNode = child.ToScopedNode(); - - // don't follow references to container: these are always circular and they are considered correct - if (childNode.InstanceType == "Reference" && childNode.Children("reference").SingleOrDefault()?.Value is string reference && reference != "#") - { - var target = childNode.Resolve(); - if (target is null) // possible external reference, we cannot check this - { - continue; - } - - followed.Add((childNode.Location, target.Location)); // add the reference to the list of followed references - - if(computeReferenceCycle(target, followed) is { } result) - return result; // if multiple paths are found, we only return the first one. Rerunning will show the next one. Let's hope that never happens. - } - - if (computeReferenceCycle(childNode, followed) is { } result2) // why is result still in scope? that makes no sense - { - return result2; - } - } - - return null; - } } /// diff --git a/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs b/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs index 83d4b9fb..13e85fe4 100644 --- a/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs +++ b/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs @@ -93,6 +93,45 @@ internal static bool IsExactlyEqualTo(this IScopedNode left, ITypedElement right (childL, childR) => childL.Name == childR.Name && childL.IsExactlyEqualTo(childR, ignoreOrder)).All(t => t); } + internal static string? ComputeReferenceCycle(this IScopedNode current, ValidationSettings vc) => + current.ToScopedNode().ComputeReferenceCycle(vc); + + internal static string? ComputeReferenceCycle(this ScopedNode current, ValidationSettings vc, IList<(string, string)>? followed = null) // this is expensive, but only executed when a loop is detected. We accept this + { + + if (current.AtResource && followed?.Count(c => c.Item2 == current.Location) is 2) // if we followed the same reference twice, we have a loop + { + return string.Join(" | ", followed.Select(reference => $"{reference.Item1} -> {reference.Item2}")); + } + + followed ??= []; + + foreach (var child in current.Children()) + { + var childNode = child.ToScopedNode(); + + if (childNode.InstanceType == "Reference") + { + var target = childNode.Resolve(url => vc.ResolveExternalReference is { } resolve ? resolve(url, childNode.Location) : null)?.ToScopedNode(); + if (target is null || (childNode.Location.StartsWith(target.Location + ".contained["))) // external reference, or reference to parent container: we do not include these in the cycle + { + continue; + } + + followed.Add((childNode.Location, target.Location)); // add the reference to the list of followed references + + if(ComputeReferenceCycle(target, vc, followed) is { } result) + return result; // if multiple paths are found, we only return the first one. Rerunning will show the next one. Let's hope that never happens. + } + + if (ComputeReferenceCycle(childNode, vc, followed) is { } result2) // why is result still in scope? that makes no sense + { + return result2; + } + } + + return null; + } } } diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs index 3660dcb4..33058798 100644 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTests/ValidationManifestTest.cs @@ -32,7 +32,7 @@ public class ValidationManifestTest /// //[Ignore] [DataTestMethod] - [ValidationManifestDataSource(TEST_CASES_MANIFEST, singleTest: "containedToContainer")] + [ValidationManifestDataSource(TEST_CASES_MANIFEST, singleTest: "message-empty-entry")] public void RunSingleTest(TestCase testCase, string baseDirectory) => _runner.RunTestCase(testCase, DotNetValidator.Create(), baseDirectory); From 1777753ad712634955439b8151d33f5a5cd7d9b5 Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Wed, 4 Sep 2024 14:13:08 +0200 Subject: [PATCH 05/15] removed nonsensical warnings in manifest --- .../FhirTestCases | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases index 64106cb6..0b996e83 160000 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases @@ -1 +1 @@ -Subproject commit 64106cb6a31dbb3f9d81b1f7384b44b198409ca3 +Subproject commit 0b996e83cbeff56018e71e9c8fd2977356022cef From d31c491e0c66ba3daf62aa89d0ebfd742fce85b1 Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Wed, 4 Sep 2024 14:30:09 +0200 Subject: [PATCH 06/15] cleaned up imports --- src/Firely.Fhir.Validation/Schema/ValidationLogger.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs index 4d9888ab..20f6e363 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs @@ -6,11 +6,9 @@ * available at https://github.com/FirelyTeam/firely-validator-api/blob/main/LICENSE */ -using Hl7.Fhir.ElementModel; using Hl7.Fhir.Support; using System; using System.Collections.Generic; -using System.Linq; #nullable enable From 29fe05e225c956ef354f8abdae470972c1d8672a Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Wed, 4 Sep 2024 14:41:28 +0200 Subject: [PATCH 07/15] Fixed behaviour and cleaned up unintended changes --- src/Firely.Fhir.Validation/Schema/InstancePath.cs | 2 +- .../Support/IScopedNodeExtensions.cs | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Firely.Fhir.Validation/Schema/InstancePath.cs b/src/Firely.Fhir.Validation/Schema/InstancePath.cs index ddcd3464..ef14e915 100644 --- a/src/Firely.Fhir.Validation/Schema/InstancePath.cs +++ b/src/Firely.Fhir.Validation/Schema/InstancePath.cs @@ -16,7 +16,7 @@ namespace Firely.Fhir.Validation /// internal class InstancePath : PathStack { - internal InstancePath(PathStackEvent? current) : base(current) + private InstancePath(PathStackEvent? current) : base(current) { } diff --git a/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs b/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs index 13e85fe4..3ee8b13e 100644 --- a/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs +++ b/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs @@ -98,12 +98,6 @@ internal static bool IsExactlyEqualTo(this IScopedNode left, ITypedElement right internal static string? ComputeReferenceCycle(this ScopedNode current, ValidationSettings vc, IList<(string, string)>? followed = null) // this is expensive, but only executed when a loop is detected. We accept this { - - if (current.AtResource && followed?.Count(c => c.Item2 == current.Location) is 2) // if we followed the same reference twice, we have a loop - { - return string.Join(" | ", followed.Select(reference => $"{reference.Item1} -> {reference.Item2}")); - } - followed ??= []; foreach (var child in current.Children()) @@ -120,6 +114,11 @@ internal static bool IsExactlyEqualTo(this IScopedNode left, ITypedElement right followed.Add((childNode.Location, target.Location)); // add the reference to the list of followed references + if (followed.Count(tuple => tuple == (childNode.Location, target.Location)) is 2) // if we followed the same reference twice, we have a loop + { + return string.Join(" | ", followed.Select(reference => $"{reference.Item1} -> {reference.Item2}")); + } + if(ComputeReferenceCycle(target, vc, followed) is { } result) return result; // if multiple paths are found, we only return the first one. Rerunning will show the next one. Let's hope that never happens. } From c946ad8c4fd61dee931a988bb16a3b32eb2981c6 Mon Sep 17 00:00:00 2001 From: mmsmits Date: Thu, 12 Sep 2024 15:41:54 +0200 Subject: [PATCH 08/15] update sdk to 5.10.1 --- firely-validator-api-tests.props | 2 +- firely-validator-api.props | 2 +- src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/firely-validator-api-tests.props b/firely-validator-api-tests.props index 2678c88b..c239dbf0 100644 --- a/firely-validator-api-tests.props +++ b/firely-validator-api-tests.props @@ -8,7 +8,7 @@ - 5.9.1 + 5.10.1 diff --git a/firely-validator-api.props b/firely-validator-api.props index fde66727..8cc680d8 100644 --- a/firely-validator-api.props +++ b/firely-validator-api.props @@ -27,7 +27,7 @@ - 5.9.1 + 5.10.1 diff --git a/src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs b/src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs index 6a7352f9..255b1844 100644 --- a/src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs +++ b/src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs @@ -110,7 +110,7 @@ internal override (bool, ResultReport?) RunInvariant(IScopedNode input, Validati try { ScopedNode node = input.ToScopedNode(); - var context = new FhirEvaluationContext(node.ResourceContext) + var context = new FhirEvaluationContext() { TerminologyService = new ValidateCodeServiceToTerminologyServiceAdapter(vc.ValidateCodeService) }; From c746197d307ec863c173e69fb73b412ca3df2b60 Mon Sep 17 00:00:00 2001 From: mmsmits Date: Thu, 12 Sep 2024 16:29:58 +0200 Subject: [PATCH 09/15] point to newer submodule --- .../FhirTestCases | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases index 0b996e83..235316f1 160000 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases @@ -1 +1 @@ -Subproject commit 0b996e83cbeff56018e71e9c8fd2977356022cef +Subproject commit 235316f166886099eedc07bdae555a294a7eab8d From 63fb492a13c74cea0590322a855b7680f63c95e6 Mon Sep 17 00:00:00 2001 From: mmsmits Date: Thu, 12 Sep 2024 17:22:00 +0200 Subject: [PATCH 10/15] point to proper branch again --- .../FhirTestCases | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases index 1efa325b..ab7e2ba7 160000 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases @@ -1 +1 @@ -Subproject commit 1efa325bef04bf17c377d313abd01b6997cbd18e +Subproject commit ab7e2ba7b444c631a041bf8d3969216843be1f3d From 83a160561b55f11d67a7190d227447ecbd07ca24 Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Tue, 24 Sep 2024 15:13:22 +0200 Subject: [PATCH 11/15] wip --- .../Impl/ElementSchema.cs | 6 ++- .../Impl/ResourceSchema.cs | 3 +- .../PublicAPI.Unshipped.txt | 1 + .../Schema/DefinitionPath.cs | 4 +- .../Schema/InstancePath.cs | 4 +- .../Schema/PathStack.cs | 20 ++++----- .../Schema/ValidationLogger.cs | 31 ++++++++++--- .../Schema/ValidationState.cs | 40 ++++++++--------- .../Support/IScopedNodeExtensions.cs | 45 ++----------------- .../FhirTestCases | 2 +- 10 files changed, 68 insertions(+), 88 deletions(-) diff --git a/src/Firely.Fhir.Validation/Impl/ElementSchema.cs b/src/Firely.Fhir.Validation/Impl/ElementSchema.cs index 7a730eba..50767403 100644 --- a/src/Firely.Fhir.Validation/Impl/ElementSchema.cs +++ b/src/Firely.Fhir.Validation/Impl/ElementSchema.cs @@ -20,7 +20,7 @@ namespace Firely.Fhir.Validation /// schema to be succesful. /// [DataContract] - public class ElementSchema : IGroupValidatable + public class ElementSchema : IGroupValidatable, IEquatable { /// /// The unique id for this schema. @@ -170,6 +170,10 @@ static JToken nest(JToken mem) => /// Whether the schema has members. /// internal bool IsEmpty() => !Members.Any(); + + + /// + public bool Equals(ElementSchema? other) => this.Id == other?.Id; } } #pragma warning restore CS0618 // Type or member is obsolete diff --git a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs index 2fb6f41b..ab4f4ed4 100644 --- a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs +++ b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs @@ -130,8 +130,7 @@ internal ResultReport ValidateResourceSchema(IScopedNode input, ValidationSettin { state.Global.ResourcesValidated += 1; return base.ValidateInternal(input, vc, state); - }, - () => input.ComputeReferenceCycle(vc)); + }); } /// diff --git a/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt b/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt index a6e21426..86a9b1c6 100644 --- a/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt +++ b/src/Firely.Fhir.Validation/PublicAPI.Unshipped.txt @@ -98,6 +98,7 @@ Firely.Fhir.Validation.DefinitionsAssertion.ToJson() -> Newtonsoft.Json.Linq.JTo Firely.Fhir.Validation.ElementSchema Firely.Fhir.Validation.ElementSchema.ElementSchema(Firely.Fhir.Validation.Canonical! id, params Firely.Fhir.Validation.IAssertion![]! members) -> void Firely.Fhir.Validation.ElementSchema.ElementSchema(Firely.Fhir.Validation.Canonical! id, System.Collections.Generic.IEnumerable! members) -> void +Firely.Fhir.Validation.ElementSchema.Equals(Firely.Fhir.Validation.ElementSchema? other) -> bool Firely.Fhir.Validation.ExtensionSchema Firely.Fhir.Validation.ExtensionSchema.ExtensionSchema(Firely.Fhir.Validation.StructureDefinitionInformation! structureDefinition, params Firely.Fhir.Validation.IAssertion![]! members) -> void Firely.Fhir.Validation.ExtensionSchema.ExtensionSchema(Firely.Fhir.Validation.StructureDefinitionInformation! structureDefinition, System.Collections.Generic.IEnumerable! members) -> void diff --git a/src/Firely.Fhir.Validation/Schema/DefinitionPath.cs b/src/Firely.Fhir.Validation/Schema/DefinitionPath.cs index a344a62a..61b8dbd4 100644 --- a/src/Firely.Fhir.Validation/Schema/DefinitionPath.cs +++ b/src/Firely.Fhir.Validation/Schema/DefinitionPath.cs @@ -14,7 +14,7 @@ namespace Firely.Fhir.Validation /// This class is also used to track the location of an instance. /// /// An example skeleton path is: "Resource(canonical).childA.childB[sliceB1].childC -> Datatype(canonical).childA" - internal class DefinitionPath : PathStack + internal record DefinitionPath : PathStack { private DefinitionPath(PathStackEvent? current) : base(current) { @@ -85,7 +85,7 @@ public bool TryGetSliceInfo(out string? sliceInfo) /// /// Start a new DefinitionPath. /// - public static DefinitionPath Start() => new(null); + public static DefinitionPath Start() => new(current: null); /// /// Update the path to include a move to a child element. diff --git a/src/Firely.Fhir.Validation/Schema/InstancePath.cs b/src/Firely.Fhir.Validation/Schema/InstancePath.cs index ef14e915..aa6eeea8 100644 --- a/src/Firely.Fhir.Validation/Schema/InstancePath.cs +++ b/src/Firely.Fhir.Validation/Schema/InstancePath.cs @@ -14,7 +14,7 @@ namespace Firely.Fhir.Validation /// /// A class representing the location of an instance. /// - internal class InstancePath : PathStack + internal record InstancePath : PathStack { private InstancePath(PathStackEvent? current) : base(current) { @@ -40,7 +40,7 @@ public override string ToString() /// /// Start a new InstancePath. /// - public static InstancePath Start() => new(null); + public static InstancePath Start() => new(current: null); /// /// Start the instance path with a reference to a resource. Can only be used at the beginning of the path. diff --git a/src/Firely.Fhir.Validation/Schema/PathStack.cs b/src/Firely.Fhir.Validation/Schema/PathStack.cs index fd67b28f..9362acff 100644 --- a/src/Firely.Fhir.Validation/Schema/PathStack.cs +++ b/src/Firely.Fhir.Validation/Schema/PathStack.cs @@ -19,7 +19,7 @@ namespace Firely.Fhir.Validation /// /// An example skeleton path is: "Resource(canonical).childA.childB[sliceB1].childC -> Datatype(canonical).childA" /// - internal abstract class PathStack + internal abstract record PathStack { /// /// Construct a new PathStack and place the current event on top of the stack. @@ -39,7 +39,7 @@ protected PathStack(PathStackEvent? current) /// /// A linked list of events that represent a path. /// - internal abstract class PathStackEvent + internal abstract record PathStackEvent { /// /// Add a new event to the path. @@ -60,7 +60,7 @@ internal abstract class PathStackEvent } - internal class ChildNavEvent : PathStackEvent + internal record ChildNavEvent : PathStackEvent { public ChildNavEvent(PathStackEvent? previous, string childName, string? choiceType) : base(previous) { @@ -77,7 +77,7 @@ protected internal override string Render() => public override string ToString() => $"ChildNavEvent: ChildName: {ChildName}, ChoiceType: {ChoiceType}"; } - internal class IndexNavEvent : PathStackEvent + internal record IndexNavEvent : PathStackEvent { public IndexNavEvent(PathStackEvent? previous, int index) : base(previous) { @@ -94,7 +94,7 @@ Previous is OriginalIndicesNavEvent originalIndices public override string ToString() => $"IndexNavEvent: Index: {Index}"; } - internal class OriginalIndicesNavEvent : PathStackEvent + internal record OriginalIndicesNavEvent : PathStackEvent { public OriginalIndicesNavEvent(PathStackEvent? previous, IEnumerable originalIndices) : base(previous) { @@ -108,7 +108,7 @@ public OriginalIndicesNavEvent(PathStackEvent? previous, IEnumerable origin public override string ToString() => $"OriginalIndicesNavEvent: Indices: {string.Join(',', OriginalIndices)}"; } - internal class InternalReferenceNavEvent : PathStackEvent + internal record InternalReferenceNavEvent : PathStackEvent { public InternalReferenceNavEvent(PathStackEvent? previous, string location) : base(previous) { @@ -128,14 +128,14 @@ static string removeIndexPart(string location) } #pragma warning disable CS0618 // Type or member is obsolete - internal class InvokeProfileEvent : PathStackEvent + internal record InvokeProfileEvent : PathStackEvent { public InvokeProfileEvent(PathStackEvent? previous, ElementSchema schema) : base(previous) { Schema = schema; } - public ElementSchema Schema { get; } + public ElementSchema Schema { get; } // implements iEquatable, so we can compare path stacks by value protected internal override string Render() { @@ -156,7 +156,7 @@ protected internal override string Render() } #pragma warning restore CS0618 // Type or member is obsolete - internal class CheckSliceEvent : PathStackEvent + internal record CheckSliceEvent : PathStackEvent { public CheckSliceEvent(PathStackEvent? previous, string sliceName) : base(previous) { @@ -168,7 +168,7 @@ public CheckSliceEvent(PathStackEvent? previous, string sliceName) : base(previo protected internal override string Render() => $"[{SliceName}]"; } - internal class StartResourceNavEvent : PathStackEvent + internal record StartResourceNavEvent : PathStackEvent { public StartResourceNavEvent(PathStackEvent? previous, string resource) : base(previous) { diff --git a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs index 20f6e363..7a42477b 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationLogger.cs @@ -9,6 +9,8 @@ using Hl7.Fhir.Support; using System; using System.Collections.Generic; +using System.Linq; +using System.Runtime.CompilerServices; #nullable enable @@ -66,22 +68,19 @@ public override string ToString() => /// The validation state /// Profile against which we are validating /// Validation to start when it has not been run before. - /// A function which visualises reference cycles when called /// The result of calling the validator, or a historic result if there is one. #pragma warning disable CS0618 // Type or member is obsolete - public ResultReport Start(ValidationState state, string profileUrl, Func validator, Func refCycleVisualizer) + public ResultReport Start(ValidationState state, string profileUrl, Func validator) #pragma warning restore CS0618 // Type or member is obsolete { - var resourceUrl = state.Instance.ResourceUrl; - var fullLocation = (resourceUrl is not null ? resourceUrl + "#" : "") + state.Location.InstanceLocation.ToString(); - + var fullLocation = getFullLocation(state); var key = (fullLocation, profileUrl); if (_data.TryGetValue(key, out var existing)) { - if (existing.Result is null && refCycleVisualizer() is { } referenceCycle) // check for loops: we do not include references to containers in the cycle + if (existing.Result is null) return new IssueAssertion(Issue.CONTENT_REFERENCE_CYCLE_DETECTED, - $"Detected a loop: instance data inside '{fullLocation}' refers back to itself (cycle structure: {referenceCycle}).") + $"Detected a loop: instance data inside '{fullLocation}' refers back to itself (cycle structure: {showRefCycle(state)}).") .AsResult(fullLocation); // If the validation has been run before, return an outcome with the same result. @@ -105,6 +104,24 @@ public ResultReport Start(ValidationState state, string profileUrl, Func(); +#pragma warning restore CS0618 // Type or member is obsolete + return string.Join( + " -> ", + state.Parents().TakeWhile(parentState => visited.Add(parentState)).Reverse().Select(getFullLocation) + ); + } + +#pragma warning disable CS0618 // Type or member is obsolete + private string getFullLocation(ValidationState state) => +#pragma warning restore CS0618 // Type or member is obsolete + state.Instance.ResourceUrl is null + ? state.Location.InstanceLocation.ToString() + : $"{state.Instance.ResourceUrl}#{state.Location.InstanceLocation}"; /// /// The number of run or running validations in this logger. diff --git a/src/Firely.Fhir.Validation/Schema/ValidationState.cs b/src/Firely.Fhir.Validation/Schema/ValidationState.cs index 24b37d73..a5c702cd 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationState.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationState.cs @@ -8,7 +8,9 @@ using Hl7.FhirPath; using System; +using System.Collections.Generic; using System.ComponentModel; +using System.Transactions; namespace Firely.Fhir.Validation @@ -57,7 +59,7 @@ internal class GlobalState /// (if the resource under validation references an external resource, /// the validation of that resource will have its own . /// - internal class InstanceState + internal record InstanceState { /// /// The URL where the current instance was retrieved (if known). @@ -65,6 +67,18 @@ internal class InstanceState public string? ResourceUrl { get; set; } } + internal ValidationState? Parent { get; private set; } + + internal IEnumerable Parents() + { + var current = this; + while (current.Parent != null) + { + yield return current.Parent; + current = current.Parent; + } + } + /// /// State to be kept while validating a single instance. /// @@ -78,6 +92,7 @@ internal ValidationState NewInstanceScope() => { // Global data is shared across ValidationState instances. Global = Global, + Parent = this }; /// @@ -107,26 +122,9 @@ internal class LocationState /// Update the location, returning a new state with the updated location. /// internal ValidationState UpdateLocation(Func pathStackUpdate) => - new() - { - Global = Global, - Instance = Instance, - Location = new LocationState - { - DefinitionPath = pathStackUpdate(Location.DefinitionPath), - InstanceLocation = Location.InstanceLocation // is this correct - } - }; + this with {Location = new LocationState { DefinitionPath = pathStackUpdate(Location.DefinitionPath), InstanceLocation = Location.InstanceLocation }}; + internal ValidationState UpdateInstanceLocation(Func pathStackUpdate) => - new() - { - Global = Global, - Instance = Instance, - Location = new LocationState - { - DefinitionPath = Location.DefinitionPath, // is this correct? - InstanceLocation = pathStackUpdate(Location.InstanceLocation) - } - }; + this with {Location = new LocationState { DefinitionPath = Location.DefinitionPath, InstanceLocation = pathStackUpdate(Location.InstanceLocation) }}; } } \ No newline at end of file diff --git a/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs b/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs index 3ee8b13e..412ea05c 100644 --- a/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs +++ b/src/Firely.Fhir.Validation/Support/IScopedNodeExtensions.cs @@ -27,8 +27,8 @@ internal static class IScopedNodeExtensions /// the methods and . /// [Obsolete("WARNING! For internal API use only. Turning an IScopedNode into an ITypedElement will cause problems for" + - "Location and Definitions. Those properties are not implemented using this method and can cause problems " + - "elsewhere. Please don't use this method unless you know what you are doing.")] + "Location and Definitions. Those properties are not implemented using this method and can cause problems " + + "elsewhere. Please don't use this method unless you know what you are doing.")] public static ITypedElement AsTypedElement(this IScopedNode node) => node switch { TypedElementToIScopedNodeToAdapter adapter => adapter.ScopedNode, @@ -50,7 +50,7 @@ internal static class IScopedNodeExtensions /// /// public static IEnumerable Children(this IEnumerable nodes, string? name = null) => - nodes.SelectMany(n => n.Children(name)); + nodes.SelectMany(n => n.Children(name)); internal static bool Matches(this IScopedNode value, ITypedElement pattern) { @@ -92,45 +92,6 @@ internal static bool IsExactlyEqualTo(this IScopedNode left, ITypedElement right return childrenL.Zip(childrenR, (childL, childR) => childL.Name == childR.Name && childL.IsExactlyEqualTo(childR, ignoreOrder)).All(t => t); } - - internal static string? ComputeReferenceCycle(this IScopedNode current, ValidationSettings vc) => - current.ToScopedNode().ComputeReferenceCycle(vc); - - internal static string? ComputeReferenceCycle(this ScopedNode current, ValidationSettings vc, IList<(string, string)>? followed = null) // this is expensive, but only executed when a loop is detected. We accept this - { - followed ??= []; - - foreach (var child in current.Children()) - { - var childNode = child.ToScopedNode(); - - if (childNode.InstanceType == "Reference") - { - var target = childNode.Resolve(url => vc.ResolveExternalReference is { } resolve ? resolve(url, childNode.Location) : null)?.ToScopedNode(); - if (target is null || (childNode.Location.StartsWith(target.Location + ".contained["))) // external reference, or reference to parent container: we do not include these in the cycle - { - continue; - } - - followed.Add((childNode.Location, target.Location)); // add the reference to the list of followed references - - if (followed.Count(tuple => tuple == (childNode.Location, target.Location)) is 2) // if we followed the same reference twice, we have a loop - { - return string.Join(" | ", followed.Select(reference => $"{reference.Item1} -> {reference.Item2}")); - } - - if(ComputeReferenceCycle(target, vc, followed) is { } result) - return result; // if multiple paths are found, we only return the first one. Rerunning will show the next one. Let's hope that never happens. - } - - if (ComputeReferenceCycle(childNode, vc, followed) is { } result2) // why is result still in scope? that makes no sense - { - return result2; - } - } - - return null; - } } } diff --git a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases index ab7e2ba7..99bf0f18 160000 --- a/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases +++ b/test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases @@ -1 +1 @@ -Subproject commit ab7e2ba7b444c631a041bf8d3969216843be1f3d +Subproject commit 99bf0f180abfdc87461216f94214c4387179341f From d00715bbf08ed83c9eba7a706d774c1d18a686fe Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Tue, 24 Sep 2024 16:08:41 +0200 Subject: [PATCH 12/15] wip --- .../Impl/ReferencedInstanceValidator.cs | 13 +++++-------- .../Schema/ValidationState.cs | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Firely.Fhir.Validation/Impl/ReferencedInstanceValidator.cs b/src/Firely.Fhir.Validation/Impl/ReferencedInstanceValidator.cs index 4304bc45..e883e8b5 100644 --- a/src/Firely.Fhir.Validation/Impl/ReferencedInstanceValidator.cs +++ b/src/Firely.Fhir.Validation/Impl/ReferencedInstanceValidator.cs @@ -227,19 +227,16 @@ private ResultReport validateReferencedResource(string reference, ValidationSett //result += Trace($"Starting validation of referenced resource {reference} ({encounteredKind})"); - // References within the instance are dealt with within the same validator, - // references to external entities will operate within a new instance of a validator (and hence a new tracking context). - // In both cases, the outcome is included in the result. + ValidationState? newState; if (resolution.ReferenceKind != AggregationMode.Referenced) - return Schema.ValidateOne(resolution.ReferencedResource.AsScopedNode(), vc, state.UpdateInstanceLocation(dp => dp.AddInternalReference(resolution.ReferencedResource.Location))); + newState = state.UpdateInstanceLocation(dp => dp.AddInternalReference(resolution.ReferencedResource.Location)) with {Parent = state}; else { - //TODO: We're using state to track the external URL, but this actually would be better - //implemented on the ScopedNode instead - add this (and combine with FullUrl?) there. - var newState = state.NewInstanceScope(); + newState = state.NewInstanceScope(); newState.Instance.ResourceUrl = reference; - return Schema.ValidateOne(resolution.ReferencedResource.AsScopedNode(), vc, newState); } + + return Schema.ValidateOne(resolution.ReferencedResource.AsScopedNode(), vc, newState); } /// diff --git a/src/Firely.Fhir.Validation/Schema/ValidationState.cs b/src/Firely.Fhir.Validation/Schema/ValidationState.cs index a5c702cd..e49b75e8 100644 --- a/src/Firely.Fhir.Validation/Schema/ValidationState.cs +++ b/src/Firely.Fhir.Validation/Schema/ValidationState.cs @@ -67,7 +67,7 @@ internal record InstanceState public string? ResourceUrl { get; set; } } - internal ValidationState? Parent { get; private set; } + internal ValidationState? Parent { get; set; } internal IEnumerable Parents() { From 443a8f6264c6b90d78b56bc9353b8b4f3f277241 Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Tue, 24 Sep 2024 17:15:27 +0200 Subject: [PATCH 13/15] fixed cases where internal references were detected as circular even when they are not. --- .../Schema/InstancePath.cs | 17 +++++++++++++++-- src/Firely.Fhir.Validation/Schema/PathStack.cs | 9 +-------- .../Impl/TestInstancePath.cs | 3 +-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/Firely.Fhir.Validation/Schema/InstancePath.cs b/src/Firely.Fhir.Validation/Schema/InstancePath.cs index ef14e915..a5373c9d 100644 --- a/src/Firely.Fhir.Validation/Schema/InstancePath.cs +++ b/src/Firely.Fhir.Validation/Schema/InstancePath.cs @@ -8,6 +8,7 @@ using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; namespace Firely.Fhir.Validation { @@ -78,8 +79,20 @@ public InstancePath AddOriginalIndices(IEnumerable indices) => : this; /// - /// Update the path to include a reference to an internal element. + /// Update the path to include a reference to an internal element. This may imply adding an index nav event as well /// - public InstancePath AddInternalReference(string location) => new(new InternalReferenceNavEvent(Current, location)); + public InstancePath AddInternalReference(string location) + { + var match = new Regex(@"(?.*)(\[(?\d+)\])?").Match(location); + var locationWithoutIndex = match.Groups["instance"].Value; + var hasIndex = match.Groups["index"].Success; + + var navEvt = new InternalReferenceNavEvent(Current, locationWithoutIndex); + if(!hasIndex) + return new(navEvt); + + var index = int.Parse(match.Groups["index"].Value); + return new (new IndexNavEvent(navEvt, index)); + } } } \ No newline at end of file diff --git a/src/Firely.Fhir.Validation/Schema/PathStack.cs b/src/Firely.Fhir.Validation/Schema/PathStack.cs index fd67b28f..0da6daa8 100644 --- a/src/Firely.Fhir.Validation/Schema/PathStack.cs +++ b/src/Firely.Fhir.Validation/Schema/PathStack.cs @@ -112,14 +112,7 @@ internal class InternalReferenceNavEvent : PathStackEvent { public InternalReferenceNavEvent(PathStackEvent? previous, string location) : base(previous) { - Location = location.EndsWith(']') ? removeIndexPart(location) : location; // remove last index - - - static string removeIndexPart(string location) - { - var indexStart = location.LastIndexOf('['); - return indexStart > 0 ? location[..indexStart] : location; - } + Location = location; // remove last index } public string Location { get; } diff --git a/test/Firely.Fhir.Validation.Tests/Impl/TestInstancePath.cs b/test/Firely.Fhir.Validation.Tests/Impl/TestInstancePath.cs index 44862de8..c2700f26 100644 --- a/test/Firely.Fhir.Validation.Tests/Impl/TestInstancePath.cs +++ b/test/Firely.Fhir.Validation.Tests/Impl/TestInstancePath.cs @@ -52,11 +52,10 @@ public void NavigateToInternalReference() .ToChild("resource") .ToChild("name") .AddInternalReference("Bundle.entry[29]") - .ToIndex(2) .ToChild("resource") .ToChild("name") .ToIndex(1); - testee.ToString().Should().Be("Bundle.entry[2].resource.name[1]"); + testee.ToString().Should().Be("Bundle.entry[29].resource.name[1]"); } [TestMethod] From 7c01e87cb7258ffae4329cc2843ab507efa8ca5e Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Tue, 24 Sep 2024 18:01:15 +0200 Subject: [PATCH 14/15] Let's see what falls over... this has been a ride to say the least --- src/Firely.Fhir.Validation/Impl/ResourceSchema.cs | 2 +- src/Firely.Fhir.Validation/Schema/InstancePath.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs index ab4f4ed4..5a2e627d 100644 --- a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs +++ b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs @@ -67,7 +67,7 @@ internal override ResultReport ValidateInternal(IEnumerable input, { // Schemas representing the root of a FHIR resource cannot meaningfully be used as a GroupValidatable, // so we'll turn this into a normal IValidatable. - var results = input.Select((i, index) => ValidateInternal(i, vc, state.UpdateInstanceLocation(d => d.ToIndex(index)))); + var results = input.Count() == 1 ? [ValidateInternal(input.First(), vc, state)] : input.Select((i, index) => ValidateInternal(i, vc, state.UpdateInstanceLocation(d => d.ToIndex(index)))); return ResultReport.Combine(results.ToList()); } diff --git a/src/Firely.Fhir.Validation/Schema/InstancePath.cs b/src/Firely.Fhir.Validation/Schema/InstancePath.cs index b51b574c..0188802d 100644 --- a/src/Firely.Fhir.Validation/Schema/InstancePath.cs +++ b/src/Firely.Fhir.Validation/Schema/InstancePath.cs @@ -83,7 +83,7 @@ public InstancePath AddOriginalIndices(IEnumerable indices) => /// public InstancePath AddInternalReference(string location) { - var match = new Regex(@"(?.*)(\[(?\d+)\])?").Match(location); + var match = new Regex(@"(?[^\[]+)(?:\[(?\d+)\])?").Match(location); var locationWithoutIndex = match.Groups["instance"].Value; var hasIndex = match.Groups["index"].Success; From 491fec7b385b84adc77badc2844f9696b80d6fe6 Mon Sep 17 00:00:00 2001 From: Kasdejong Date: Tue, 24 Sep 2024 18:02:16 +0200 Subject: [PATCH 15/15] little refactoring --- src/Firely.Fhir.Validation/Impl/ResourceSchema.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs index 5a2e627d..37b09728 100644 --- a/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs +++ b/src/Firely.Fhir.Validation/Impl/ResourceSchema.cs @@ -67,7 +67,9 @@ internal override ResultReport ValidateInternal(IEnumerable input, { // Schemas representing the root of a FHIR resource cannot meaningfully be used as a GroupValidatable, // so we'll turn this into a normal IValidatable. - var results = input.Count() == 1 ? [ValidateInternal(input.First(), vc, state)] : input.Select((i, index) => ValidateInternal(i, vc, state.UpdateInstanceLocation(d => d.ToIndex(index)))); + var results = input.Count() == 1 + ? [ValidateInternal(input.First(), vc, state)] + : input.Select((i, index) => ValidateInternal(i, vc, state.UpdateInstanceLocation(d => d.ToIndex(index)))); return ResultReport.Combine(results.ToList()); }