computed circular reference error messages #355
Conversation
# Conflicts: # src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs
| @@ -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 | |||
| /// </summary> | |||
| [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")] | |||
There was a problem hiding this comment.
Pretty sure you want to re-enable this Ignore[]?
| // 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 |
There was a problem hiding this comment.
What happens if existing.Result is null but the refCycleVisualizer() call returns null, it does not report anything? Maybe that makes sense after I have seen refCycleVisualizer(), but it feels wrong. Cannot this remain a string if()/else?
| 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 |
There was a problem hiding this comment.
I think this should be a private utility method inside the validationlogger, not an internal extension methods to ensure everyone understands this should never be called by anyone other than the author of the ValidationLogger.
|
|
||
| foreach (var child in current.Children()) | ||
| { | ||
| var childNode = child.ToScopedNode(); |
There was a problem hiding this comment.
Since the parent (current) is a ScopedNode, its children will be scoped nodes. This is a guarantee that the children will have their parent set correctly. It is better to explictly stating this knowledge (and assumption) by making this line var childName = (ScopedNode)child, instead of the call to ToScopedNode().
|
|
||
| if (childNode.InstanceType == "Reference") | ||
| { | ||
| var target = childNode.Resolve(url => vc.ResolveExternalReference is { } resolve ? resolve(url, childNode.Location) : null)?.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 |
| /// <returns>The result of calling the validator, or a historic result if there is one.</returns> | ||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| public ResultReport Start(ValidationState state, string profileUrl, Func<ResultReport> validator) | ||
| public ResultReport Start(ValidationState state, string profileUrl, Func<ResultReport> validator, Func<string?> refCycleVisualizer) |
There was a problem hiding this comment.
I must be blind but I don't see anything in this PR that actually supplies this extra parameter to the Start() call. It cannot be, as this PR compiles, but I just don't see it.
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Actually, I don't think we should do this. This function is simply too ugly and too inefficient. It will resolve every single reference in an instance, it uses fragile string matches on location, and it just looks like a hack. Cannot we expand the ValidationLogger or the location state so we keep track of the full path instead of constructing it?
…when they are not.
…reference-error-message # Conflicts: # test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases
Description
We should now correctly display a reference cycle in the error messages.
Related issues
Closes #286
Testing
Edited the circular reference unit test and the manifest