Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VONK-5093: Validation is using now IScopedNode as input #207

Merged
merged 35 commits into from
Dec 7, 2023

Conversation

marcovisserFurore
Copy link
Member

@marcovisserFurore marcovisserFurore commented Nov 14, 2023

Description

Instead of using ITypedElement as input for the validation, we use now IScopedNode. The validator requires more than is available in ITypedElement, which are (some of) the features provided by ScopedNode. These requirements are not expressed structurally in the method signatures of, say, Validate(), instead the engines sniff their implementation of ITypedElement to see whether the underlying implementation is a ScopedNode.
In this pull request, we now explicitly mandate the inclusion of all members through the newly introduced interface IScopedNode.

Remarks

  • This PR is using an unreleased version of the SDK.

@marcovisserFurore marcovisserFurore marked this pull request as draft November 14, 2023 14:28
@marcovisserFurore marcovisserFurore changed the title Feature/vonk 5093 Validation is using now IScopedNode as inpou VONK-5093: Validation is using now IScopedNode as input Nov 14, 2023
@marcovisserFurore marcovisserFurore marked this pull request as ready for review November 14, 2023 15:16
Copy link
Member

@ewoutkramer ewoutkramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more philosphical question, and a question about one of the Validate() overloads.

src/Firely.Fhir.Validation/Impl/FixedValidator.cs Outdated Show resolved Hide resolved
src/Hl7.Fhir.Validation.Shared/Validator.cs Outdated Show resolved Hide resolved
@ewoutkramer
Copy link
Member

I searched for "ITypedElement" in the codebase for the validator, and I see it appear on several places

  • Inside Hl7.Fhir.Validation.Shared, but I guess that's the shim around the old validator that we can remove now?
  • Many times in our tests, but that does not bother us too much
  • In Patterns, fixed values etc.

That last category is interesting, since the reason we still use ITypedElement is that the SDK has function like IsExactlyEqual that use ITypedElement, but could easily be made to work on something simpler. But we cannot use IBaseElementNavigator, since what would T be? It looks like we need a simple IBaseELementNavigator (no T) additionally (much like IEnumerable and IEnumerable), so we can express these simple cases. Something to ponder about. Otherwise, I think everything looks fine!

ewoutkramer and others added 5 commits November 21, 2023 11:59
# Conflicts:
#	src/Firely.Fhir.Validation/Impl/PatternValidator.cs
#	src/Firely.Fhir.Validation/Impl/ResourceSchema.cs
#	test/Firely.Fhir.Validation.Compilation.Tests.Shared/FhirTestCases
#	test/Firely.Fhir.Validation.Tests/Impl/ResourceSchemaTests.cs
@ewoutkramer
Copy link
Member

ewoutkramer commented Nov 21, 2023

In fact, the most interesting place is this one, in ValidationContext.cs

public Func<string, Task<ITypedElement?>>? ExternalReferenceResolver = null;`

This, of course, should also be an IScopedNode, otherwise we will force the caller of the validation to produce ITypedElements anyway. But that means you cannot do

return Schema.ValidateOne(resolution.ReferencedResource.AsScopedNode(), vc, state.UpdateInstanceLocation(dp => dp.AddInternalReference(resolution.ReferencedResource.Location)));

in ReferencedInstanceValidator.

@ewoutkramer
Copy link
Member

Also, this, in the PathSelectorValidator would not work once we've moved FhirPath off ITypedElement:

state = state.UpdateInstanceLocation(ip => ip.AddInternalReference(selected.First().Location));

@ewoutkramer
Copy link
Member

I was curious how we are keeping track of the location when we do an internal jump, but I guess this is the clue:

  private (IReadOnlyCollection<ResultReport>, ResolutionResult) fetchReference(IScopedNode input, string reference, ValidationContext vc, ValidationState s)
  {
      ResolutionResult resolution = new(null, null, null);
      List<ResultReport> evidence = new();

      if (input is not ScopedNode instance)
          throw new InvalidOperationException($"Cannot validate because input is not of type {nameof(ScopedNode)}.");

So - basically this means that if we are jumping to an internal node, we still use the facilities of ScopedNode to track the Location (and the Parent)? Does this mean that in the end we wrap every IScopedNode inside a ScopedNode?

@ewoutkramer
Copy link
Member

In AssertionValidators.cs, the overloads taking ITypedElement are only used by our tests, so we can move them there, and remove another reference to ITypedElement from our validator.

@marcovisserFurore
Copy link
Member Author

In AssertionValidators.cs, the overloads taking ITypedElement are only used by our tests, so we can move them there, and remove another reference to ITypedElement from our validator.

I agree.

@ewoutkramer
Copy link
Member

ewoutkramer commented Nov 21, 2023

I think we can remove Hl7.Fhir.Validation.Shared, it's the wrapper around the old validator!

(yes, I tried it, we can!)

@ewoutkramer
Copy link
Member

The reason the tests still use the overloads taking ITypedElement is that do not yet have an implementation of IScopedNode for Poco's. I guess we need that for people to be using the new validator. I'll see if I can put that magic into the public layer we will put around it? Since we don't actually use Parent yet, it is really easy to adapt.

ewoutkramer and others added 8 commits November 22, 2023 17:53
…nish-public-surface

# Conflicts:
#	src/Firely.Fhir.Validation/Impl/BindingValidator.cs
#	src/Firely.Fhir.Validation/Impl/ExtensionSchema.cs
#	src/Firely.Fhir.Validation/Impl/ResourceSchema.cs
#	src/Firely.Fhir.Validation/Schema/ValidationContext.cs
#	src/Hl7.Fhir.Validation.R4/Hl7.Fhir.Validation.R4.csproj
#	src/Hl7.Fhir.Validation.Shared/ValidationSettings.cs
#	test/Firely.Fhir.Validation.Tests/Impl/BindingValidatorTests.cs
#	test/Firely.Fhir.Validation.Tests/Impl/ResourceSchemaTests.cs
…r-r5

Makes the snapgen tests work for the R5 ElementSchema compiler.
@marcovisserFurore marcovisserFurore merged commit 6c6bf60 into develop Dec 7, 2023
2 checks passed
@marcovisserFurore marcovisserFurore deleted the feature/VONK-5093-use-IScopedNode branch December 7, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants