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-5143: FP validator gets an instance of ITerminologyService to pass to FhirEvaluationContext #188

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/Firely.Fhir.Validation/Impl/FhirPathValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

using Hl7.Fhir.ElementModel;
using Hl7.Fhir.FhirPath;
using Hl7.Fhir.Model;
using Hl7.Fhir.Specification.Terminology;
using Hl7.Fhir.Support;
using Hl7.Fhir.Utility;
using Hl7.Fhir.Validation;
Expand All @@ -14,6 +16,7 @@
using Newtonsoft.Json.Linq;
using System;
using System.Runtime.Serialization;
using System.Threading.Tasks;
using static Hl7.Fhir.Model.OperationOutcome;

namespace Firely.Fhir.Validation
Expand Down Expand Up @@ -99,7 +102,11 @@ protected override (bool, ResultReport?) RunInvariant(ITypedElement input, Valid
try
{
var node = input as ScopedNode ?? new ScopedNode(input);
return (predicate(input, new EvaluationContext(node.ResourceContext), vc), null);
var context = new FhirEvaluationContext(node.ResourceContext)
{
TerminologyService = new ValidateCodeServiceToTerminologyServiceAdapter(vc.ValidateCodeService)
Copy link
Member

Choose a reason for hiding this comment

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

Should we make TerminologyService an ICodeValidationTerminologyService maybe? We do not need more at this point - but we might later of course. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be better, but that is a breaking change in the SDK. So that's why I came up with this solution. Or do you have a cunning plan to avoid the breaking change? @ewoutkramer

Copy link
Member

Choose a reason for hiding this comment

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

Nope. I know that if you assign a TermSErvice to one of its subinterfaces, it will work, but indeed, for code reading the property this is a breaking change. It is in practice a breaking change, at runtime, since we're throwing in all not implemented functions, but yes, it does not formally break ;-)

So we should reconsider this for the next major version....

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, then I will create a SDK issue for this and we leave this PR as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

};
return (predicate(input, context, vc), null);
}
catch (Exception e)
{
Expand Down Expand Up @@ -152,5 +159,28 @@ private bool predicate(ITypedElement input, EvaluationContext context, Validatio

return compiledExpression.IsTrue(input, context);
}

/// <summary>
/// An adapter between the <see cref="ICodeValidationTerminologyService"/> and <see cref="ITerminologyService"/>. Be careful to use
/// this adapter, because it does only implement the <see cref="ICodeValidationTerminologyService"/> methods. The other methods, like those
/// from <see cref="IMappingTerminologyService"/> are not implemented will raise an exception.
/// </summary>
private class ValidateCodeServiceToTerminologyServiceAdapter : ITerminologyService
{
private readonly ICodeValidationTerminologyService _service;

public ValidateCodeServiceToTerminologyServiceAdapter(ICodeValidationTerminologyService service)
{
_service = service;
}

public Task<Resource> Closure(Parameters parameters, bool useGet = false) => throw new NotImplementedException();
public Task<Parameters> CodeSystemValidateCode(Parameters parameters, string? id = null, bool useGet = false) => throw new NotImplementedException();
public Task<Resource> Expand(Parameters parameters, string? id = null, bool useGet = false) => throw new NotImplementedException();
public Task<Parameters> Lookup(Parameters parameters, bool useGet = false) => throw new NotImplementedException();
public Task<Parameters> Subsumes(Parameters parameters, string? id = null, bool useGet = false) => _service.Subsumes(parameters, id, useGet);
public Task<Parameters> Translate(Parameters parameters, string? id = null, bool useGet = false) => throw new NotImplementedException();
public Task<Parameters> ValueSetValidateCode(Parameters parameters, string? id = null, bool useGet = false) => _service.ValueSetValidateCode(parameters, id, useGet);
}
}
}
46 changes: 0 additions & 46 deletions src/Firely.Fhir.Validation/Schema/FhirEvaluationContext.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright (C) 2021, Firely ([email protected]) - All Rights Reserved
* Proprietary and confidential. Unauthorized copying of this file,
* via any medium is strictly prohibited.
*/

using Microsoft.VisualStudio.TestTools.UnitTesting;

// Ensure that the test data source is refreshed with each execution. This requirement applies to the unit test class named ValidationManifestTests.
[assembly: TestDataSourceDiscovery(TestDataSourceDiscoveryOption.DuringExecution)]
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright (C) 2021, Firely ([email protected]) - All Rights Reserved
* Proprietary and confidential. Unauthorized copying of this file,
* via any medium is strictly prohibited.
*/

using Microsoft.VisualStudio.TestTools.UnitTesting;

// Ensure that the test data source is refreshed with each execution. This requirement applies to the unit test class named ValidationManifestTests.
[assembly: TestDataSourceDiscovery(TestDataSourceDiscoveryOption.DuringExecution)]
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright (C) 2021, Firely ([email protected]) - All Rights Reserved
* Proprietary and confidential. Unauthorized copying of this file,
* via any medium is strictly prohibited.
*/

using Microsoft.VisualStudio.TestTools.UnitTesting;

// Ensure that the test data source is refreshed with each execution. This requirement applies to the unit test class named ValidationManifestTests.
[assembly: TestDataSourceDiscovery(TestDataSourceDiscoveryOption.DuringExecution)]
45 changes: 45 additions & 0 deletions test/Firely.Fhir.Validation.Tests/Impl/FhirPathValidatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,18 @@

using FluentAssertions;
using Hl7.Fhir.ElementModel;
using Hl7.Fhir.FhirPath;
using Hl7.Fhir.Model;
using Hl7.Fhir.Specification.Terminology;
using Hl7.Fhir.Support;
using Hl7.FhirPath;
using Hl7.FhirPath.Expressions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using static Hl7.Fhir.Model.OperationOutcome;
using Task = System.Threading.Tasks.Task;

namespace Firely.Fhir.Validation.Tests
{
Expand All @@ -24,6 +30,7 @@ public FhirPathValidatorTests()
{
var symbolTable = new SymbolTable();
symbolTable.AddStandardFP();
symbolTable.AddFhirExtensions();
_fpCompiler = new FhirPathCompiler(symbolTable);
}

Expand Down Expand Up @@ -94,5 +101,43 @@ public void ValidateNullableInput()

result?.IsSuccessful.Should().Be(false, because: "FhirPath epressions resulting in null should return false in the FhirPathValidator");
}

[TestMethod]
public void ValidateMemberOf()
{
var validatable = new FhirPathValidator("memberof", "memberOf('http://hl7.org/fhir/ValueSet/iso3166-1-2')", "This expression should use FP memberOf()", IssueSeverity.Error, false);

var input = ElementNodeAdapter.Root("Coding");
input.Add("system", "http://hl7.org/fhir/ValueSet/iso3166-1-2", "uri");
input.Add("code", "NL", "string");

var minimalContextWithFp = ValidationContext.BuildMinimalContext(fpCompiler: _fpCompiler);
minimalContextWithFp.ValidateCodeService = new AlwaysSuccessfulTS();
var result = validatable.Validate(input, minimalContextWithFp);

result?.Warnings.Should().BeEmpty(because: "There should be no warnings about the terminology service.");
result?.IsSuccessful.Should().BeTrue();
}

/// <summary>
/// A CodeValidationTerminologyService that always return a success with the method <see cref="ValueSetValidateCode"/>. Only meant for
/// testing purposes.
/// </summary>
private class AlwaysSuccessfulTS : ICodeValidationTerminologyService
{
public Task<Parameters> Subsumes(Parameters parameters, string? id = null, bool useGet = false) =>
throw new System.NotImplementedException();

public Task<Parameters> ValueSetValidateCode(Parameters parameters, string? id = null, bool useGet = false) =>
Task.FromResult(new Parameters
{
Parameter = new List<Parameters.ParameterComponent>() {
new Parameters.ParameterComponent()
{
Name = "result",
Value = new FhirBoolean(true)}
}
});
}
}
}