FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
821 stars 342 forks source link

Internal logic failure: Unknown symbol 'resolve' #1730

Closed DarthGizka closed 3 years ago

DarthGizka commented 3 years ago

Firely throws the following errors when validating the first example bundle of project eRezeptAbgabedaten @ simplifier.net:

[FATAL] Internal logic failure: Unknown symbol 'resolve' (at Bundle.entry[0].resource[0])
[WARNING] Terminology service failed while validating code 'EUR' (system ''): Cannot resolve canonical reference 'urn:iso:std:iso:4217' to CodeSystem (at Bundle.entry[3].resource[0].lineItem[0].priceComponent[0].extension[1].extension[1].value[0].currency[0])
...
[ERROR] Element does not match any slice, but the group at 'Bundle.entry' is closed. (at Bundle.entry[0])
[ERROR] Instance count for 'Bundle.entry:ERezeptAbgabedaten' is 0, which is not within the specified cardinality of 1..1 (at Bundle)

Reproduction is easy - just hit the 'Validation' button for the example mentioned above (or any of the others there) at simplifier.net. The error messages there are essentially the same.

I am not certain whether the failure to validate the currency code (urn:iso:std:iso:4217) is related to the problem at hand or not, so I left the message in.

Other validators - e.g. HAPI - seem to have no problems validating the resource in question.

ewoutkramer commented 3 years ago

Which version of the SDK are you using? We have been fixing the resolve error recently....

DarthGizka commented 3 years ago

I'm using the current release version, via the official NuGet packages:

hl7.fhir.elementmodel.3.2.0.nupkg        1.908.230  2021-05-16 09:23:27
hl7.fhir.r4.3.2.0.nupkg                 51.848.260  2021-05-16 09:22:27
hl7.fhir.serialization.3.2.0.nupkg       1.601.460  2021-05-16 09:25:27
hl7.fhir.specification.r4.3.2.0.nupkg   62.419.430  2021-05-16 09:21:31
hl7.fhir.support.3.2.0.nupkg               875.920  2021-05-16 09:23:19
hl7.fhir.support.poco.3.2.0.nupkg        4.284.220  2021-05-16 09:25:55
hl7.fhirpath.3.2.0.nupkg                 2.157.680  2021-05-16 09:26:18
newtonsoft.json.13.0.1.nupkg            20.657.870  2021-05-16 09:23:09
system.net.http.4.3.4.nupkg              6.206.440  2021-05-16 10:33:13
system.valuetuple.4.5.0.nupkg            2.049.040  2021-05-16 09:22:59

I guess that would make it branch release/3.2.0-r4. The packages were still current when I posted the report.

ewoutkramer commented 3 years ago

Thanks. I took at look at our resolve() bugfix, but that has not been published yet (probably in 3.3).

Even so, [WARNING] Terminology service failed while validating code 'EUR' (system ''): Cannot resolve canonical reference 'urn:iso:std:iso:4217' to CodeSystem isn't something we can easily fix. The codesystem urn:iso:std:iso:4217 is not available as a FHIR ValueSet, so the "light-weight" terminology service available for free in the SDK does not know how to validate that code.

It is possible to configure the SDK so it will use a more powerful "external" terminology service that might know how to handle it (there are a few commercial systems that do). Since this works in HAPI, I am assuming the open source terminology endpoint at tx.fhir.org does support it as well.

If this is of interest to you, we can dive a bit deeper and help you make a connection from the SDK to tx.fhir.org

DarthGizka commented 3 years ago

Thanks, Ewout, that is very interesting. I wasn't even aware that urn:iso:std:iso:4217 is such a thorny problem!

However, we are working in a context (German health care) where the number of possible values is very, very small and for the foreseeable future there can only be one single value, EUR. I've already planned to stub this out. Either as a miniature fake conformance resource in a fixups directory or by making a small terminology server stub for Firely. There are other value sets that need to be faked or stubbed as well, since the specifications are not yet complete (despite the fact that the health minister has ordered the project go live country-wide on July 1st).

Anyway, I've filed your information nugget away for future reference, in case the number of references to tx.fhir.org (or their volatility) should exceed what can be handled by stubbing. Thanks again!

The most pressing matter at the moment is to resolve the 'resolve' thingy. Is there anything I can do to help?

ewoutkramer commented 3 years ago

@DarthGizka - I took a look at the errors - the fact that it says "unknown symbol 'resolve'" tells me the FhirPath compiler is not configured correctly. I think, from your description, that these errors did not come from your own code, but from Simplifier? I know Simplifier has an issue of not loading this "extra" set of functions (including 'resolve') at the moment.

If this is in your own code, we must be able to get this working for you as we use resolve() a lot ourselves, it is available.

As for the currency valueset - I think from the discussion on Stackoverflow it has become clear that this is available in a HL7-published package - we don't directly use these packages (yet) but I have written a solution on Stackoverflow: we can include this valueset in your validator by adding a new DirectorySource.

If this is mostly about getting this to work in Simplifier, we need to get our Simplifier friends into this discussion (they are monitoring it already) - if this is about validating in your own code, I think we should be able to solve these issues!

ewoutkramer commented 3 years ago

I also saw your comments on resolving within bundles. It seems that this did not always work correctly: https://github.com/FirelyTeam/firely-net-sdk/issues/1525.

We have just fixed this last week, and will publish a solution in version 3.3. It might, however, take a while for this to get into Simplifier.

DarthGizka commented 3 years ago

@ewoutkramer - yes, the errors came from the Firely SDK validation logic, but they are exactly the same as those reported by the builtin validator at Simplifier (just without the pretty styling). Validation via the Firely Terminal also gives essentially the same errors, but less detailed. That's why I linked to the example on Simplifier - it gives you the exact same errors as the Firely SDK, with just a few clicks and without writing a single line of code. ;-)

The fact that the linked example also exhibits the problem which I talked about on Stackoverflow (Bundle-internal reference: relative URL “[type]/[id]” when resource has fullUrl “urn:uuid:[id]”, not “http://blah/blih/[type]/[id]”) is pure coincidence. The error messages remain the same even if the relative-URL-style reference is changed to uuid style, like the other internal references in the bundle.

Thank you for the tip regarding #1525. This opens up a new path of investigation.

I've put the problem with the currency value set on the back burner for now. For one thing, it looks like it can be solved by brute force (expanded value set file or an apposite implementation of ITerminologyServer - somehow we will conquer this one). For another, working 7/14 doesn't leave a lot of room for learning and investigation, which means I have to keep laser focus on the most important objectives/targets.

ewoutkramer commented 3 years ago

Great. So, if you have the 'resolve' missing in your code it means the FhirPath parser is misconfigured in the validator This can only happen in your own code if you have set the FhirPathCompiler in the ValidatorSettings to something else than the default. By default, the FhirPath compiler is configured like so:

 var symbolTable = new SymbolTable();
 symbolTable.AddStandardFP();
 symbolTable.AddFhirExtensions();

 _fpCompiler = new FhirPathCompiler(symbolTable);

The AddFhirExtensions() adds support for resolve to the FhirPath compiler. The reason this is not installed by default is that the FhirPath standard itself (http://hl7.org/fhirpath/) does not define resolve(), this is a FHIR-specific add-on (even though the language is called FhirPath it is used in non-FHIR contexts too). This appendix in the FHIR standard (http://hl7.org/fhir/fhirpath.html) defines the resolve() function.

This is why these functions need to be installed separately as shown above. But as you can see, for a default FhirValidator, this should already have happened.

Hope this helps.

DarthGizka commented 3 years ago

@ewoutkramer - it is unlikely that the 'resolve()' issue is caused by user code setting a misconfigured FhirPathCompiler or someone shooting the FHIR extensions out of the FpCompiler property. Moreover, the rumour mill has it that the 'resolve()' problem is caused by the slicing definition used by the resource in question. If it were a simple misconfiguration thing then simplifier.net would probably have fixed it long ago.

In any case, here's the code that produces the validation settings that are handed to the Validator constructor:

const string BASISPROFIL_DE = "de.basisprofil.r4#0.9.13";
const string EABGABEDATEN   = "de.abda.eRezeptAbgabedaten#1.0.3";

static CachedResolver create_combined_resolver_ ()
{
    var ds_settings = new DirectorySourceSettings();

    return new CachedResolver(new MultiResolver(
        ZipSource.CreateValidationSource(), 
        new DirectorySource(verified_path_in_HAPI_cache_(BASISPROFIL_DE), ds_settings),
        new DirectorySource(verified_path_in_HAPI_cache_(EABGABEDATEN), ds_settings)
    ));
}

static ValidationSettings validation_settings_ ()
{
    return new ValidationSettings()
    {
        ResourceResolver = create_combined_resolver_(),
        GenerateSnapshot = true,
        EnableXsdValidation = false,
        Trace = false,
        ResolveExternalReferences = false           
    };
}

As you can see, FhirPathCompiler does not get touched at all.

The code that creates the Validator looks like this:

const string ABGABEDATEN_NR1 = LocalTestUtil.SampleFile(@"ABDA\eRezeptAbgabedaten-1.0.3\72bd741c-7ad8-41d8-97c3-9aabbdd0f5b4.xml");

[TestMethod]
public void Abgabedaten_Nr1 ()
{
    var expect_success = false;  // needs to be changed when Firely is fixed

    using (var xml_reader = XmlReader.Create(new FileStream(ABGABEDATEN_NR1, FileMode.Open)))
    {
        var validator = new Validator(validation_settings_());
        var result = validator.Validate(xml_reader);

        Assert.AreEqual(expect_success, result.Success, "result.Success");

        Console.WriteLine(result);
    }
}

Reminder: the exact same error messages are produced if you go to the example resource at simplifier.net that I linked in my first message and use the Validate button right there on the resource's page.

ewoutkramer commented 3 years ago

the resolve() error "unknown symbol" tells me the FP compiler cannot find the method resolve, or the parameter count is incorrect. So, maybe that is the problem! If the resolve() has parameters, we would get the same error. Do we have any idea what the exact FP statement is that is offered to the compiler?

UnixNetworks commented 3 years ago

The Exception was thrown in Hl7.Fhir.Specification/Validation/Slicing/PathBasedDiscriminator.cs

public bool Matches(ITypedElement candidate)
        {
            ITypedElement[] values = candidate.Select(Path).ToArray();  // Excetion here. Path = "entry.resolve()"

            // Don't know how to handle a discriminating element that repeats - all? any?
            if (values.Length > 1)
                throw Error.NotImplemented($"The instance has multiple elements at '{candidate.Location}' " +
                    $"for the discriminator path '{Path}'. Don't know how to handle that.");
            else if (values.Length == 0)
                return false;
            else
                return MatchInternal(values.Single());
        }
UnixNetworks commented 3 years ago

@DarthGizka Have you solved this?

DarthGizka commented 3 years ago

@UnixNetworks: not yet. I had to put Firely on hold because the powers that be effectively decreed the HAPI validator to be normative for electronic prescriptions in Germany (which is somewhat understandable, given that Firely cannot process some of the profiles involved). HAPI being Java means additional work for integrating it into processes that are not based on Java. It doesn't help that the health minister has set deadlines that are near impossible to meet (although there was an unexpected reprieve and an additional grace period of 3 very short months).

As soon as I've got HAPI humming along on the corporation servers I'll dive back into Firely (aided by the fact that my validator API is validator-agnostic, allowing aggregation or mix-and-match use of any validator regardless of platform). However, there are other changes that affect billing in health care and it is probable that we will have to segue straight into other projects after HAPI, depending on whether other industry partners are able to meet their obligations or not. We have been working 7/12-15 since February, so our capacity for extracurricular work or sprints has shrunk to zero a long time ago...

ewoutkramer commented 3 years ago

@UnixNetworks - thanks for pointing that piece of code out! I know exactly what's going on now. Let me check if there is an easy work-around.

UnixNetworks commented 3 years ago

@DarthGizka Yes. I agree with that.

ewoutkramer commented 3 years ago

Yes, the problem lies in the fact that in "standard" FhirPath (http://hl7.org/fhirpath/ - which is a spec developed separately from FHIR) there is no resolve operation, it's added by FHIR itself (http://hl7.org/fhir/fhirpath.html#functions). So there are currently two "dialects" of FhirPath if you will.

The built-in FHIRpath compiler has no way of knowing which dialect we're dealing with here: you would normally pass it to the constructor of a FhirPathCompiler, but the bit of code you found won't let you do that. For these cases, you can change that by adding these functions to the "default" symbol table" by calling Hl7.FhirPath.FhirPathCompiler.DefaultSymbolTable.AddFhirExtensions(); somewhere at the start of your code.

In fact, you can completely replace the DefaultSymbolTable if you so desired:

var mySymTable = new SymbolTable();
mySymTable
     .AddStandardFP()     // the "normal" FP functions
     .AddFhirExtensions();   // add the specific FHIR functions
FhirPathCompiler.SetDefaultSymbolTable(new Lazy(mySymTable));

(of course you could make this really Lazy too)

ewoutkramer commented 3 years ago

Maybe this is a misdesign - this is certainly an FAQ. Then again, installing these functions by default would mean they become available when running FP in another context where they shouln't (e.g. CQL). I have no clear idea on how to improve the API to make this more natural at the moment....

UnixNetworks commented 3 years ago

Yes. Adding Hl7.FhirPath.FhirPathCompiler.DefaultSymbolTable.AddFhirExtensions(); solved this.

Is for using this an Internet Connection required?

ewoutkramer commented 3 years ago

No. resolve() in discriminators is hardly ever used for referring to resources outside of your current instance or application. If you do encounter it, you can configure the .NET stack to allow access to external resources by way of HTTP but that seems like something you would not normally do in production.

ABDA-FHIR commented 3 years ago

Is there any update on this issue? Our examples still won't validate on simplifier.net while being correct FHIR. This is a serious issue as it irritates implementers in the context of German electronic prescription (E-Rezept).

UnixNetworks commented 3 years ago

Only [WARNING]'s are not solved ("... validating code 'EUR' (system ''): Cannot resolve canoni ....")

ewoutkramer commented 3 years ago

@ABDA-FHIR - Adding Hl7.FhirPath.FhirPathCompiler.DefaultSymbolTable.AddFhirExtensions(); someone at the start of your program solves it.

ewoutkramer commented 3 years ago

@UnixNetworks [WARNING] Terminology service failed while validating code 'EUR' (system ''): Cannot resolve canonical reference 'urn:iso:std:iso:4217' to CodeSystem. - let's take a look at that.

@DarthGizka suggested a stub. In fact, that's a good idea. The terminology services in the SDK are pretty flexible, as you only have to implement the ValueSetValidateCode() function on the ITerminologyService interface passed to the validator. If you'd create an implementation of this interface that validates the code "EUR" only for that valueset and otherwise passes it on to the original terminology service, you are set.

Alternative, we could define a small valueset with just EUR and make sure the SDK can find that valueset. I cannot determine which option would work better in the German context....

ewoutkramer commented 3 years ago

I think it is actually administratively better if we separate this issue off from the resolve() problem and close this issue.

@ABDA-FHIR - Since this has been reported in the context of Simplifier, do you need this solved in Simplifier? I could contact the Simplifier team for you to find out whether there is an active issue for this in their support channels.

ABDA-FHIR commented 3 years ago

@ewoutkramer

@ABDA-FHIR - Since this has been reported in the context of Simplifier, do you need this solved in Simplifier? I could contact the Simplifier team for you to find out whether there is an active issue for this in their support channels.

We would like to see that solved in simplifier. If you could forward the issue to the simplifier team (and the corresponding bug tracker), that would be of great help.

ewoutkramer commented 3 years ago

@ABDA-FHIR - Done! The team should be in contact with you soon...