FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
829 stars 345 forks source link

Incorrect validation warnings for slice names #413

Closed wmrutten closed 7 years ago

wmrutten commented 7 years ago

Chris Munro reported the following issue:

I've noticed that even with <sliceName value="Test"/>

I still get a warning of [WARNING] Evaluation of FhirPath for constraint 'eld-16' failed: Invocation of 'binary.or' failed: Invocation of 'matches' failed: parsing "^[a-zA-Z0-9-_]+$" - Unrecognized escape sequence _. (at StructureDefinition.differential[0].element[6])

This looks to be because in the spec there are escape characters (https://www.hl7.org/fhir/elementdefinition-definitions.html) :

eld-16: sliceName must be composed of proper tokens separated by "/" (expression : sliceName.empty() or sliceName.matches('^[a-zA-Z0-9\/\-\]+$'), xpath: not(exists(f:sliceName/@value)) or matches(f:sliceName/@value, '^[a-zA-Z0-9\/\-\]+$'))

and the way java/c# process the escaped character in the regular expression?

It appears that the API is stumbling over the regex escaping...? Note that C# escaping rules are different. In Forge, I validate eld-16 using the following regex: @"^[a-zA-Z0-9/-_]+$"

ewoutkramer commented 7 years ago

This is not present in the C# code, the eld-16 fhirpath expression has an invalid escape sequence \_, this is not allowed in the grammar. I don't know why this shows up now - the grammar has not changed and we run all constraints in the spec - so a unit-test should have caught this. Correct, @brianpos?

ewoutkramer commented 7 years ago

Easy fix would be to

ewoutkramer commented 7 years ago

Actually, what we see might be garbled because of all the escaping - it's a regex escaped in a FHIRPath statement and then pasted into Zulip.... have to be careful to find out exactly where the mistake is...

ewoutkramer commented 7 years ago

Hmmmm.....cannot reproduce it in 0.93.5-b3 :-( What I did is just validate a Patient StructureDefinition - and another StructureDefinition from out testset - this should have triggered it?

wmrutten commented 7 years ago

Did you put a sliceName on the StructureDefs?

Escaping is hard. Here's the fhirpath expression eld-16 on ElementDefinition from profiles-types.xml: sliceName.empty() or sliceName.matches('^[a-zA-Z0-9\\/\\-\\_]+$') Note the double "\" chars. These are not visible in Chris' original message above. [EDIT] Nevermind, the original post contains double backslash; apparently github rendering performs unescaping too...

What are the rules about fhirpath regex escaping?

ewoutkramer commented 7 years ago

Traced it down all from the parsed FHIRPath expression, into the expression tree, into the actual call to the .NET regex evaluator. Turns out the message comes from the .NET regex evaluator, not from the FHIR API. And the .NET regex evaluator is right, \_ is /not/ a valid escape sequence in regexes. We need to fix the spec.