New serializer cannot parse quoted integers which was parsable in the Newtonsoft serializer

[almostchristian]

Describe the bug
The System.Text.Json Fhir deserializer throws an exception when numeric fields are quoted unlike in the Newtonsoft serializer.
This is breaking our automated UI tests after upgrading to the latest version of the SDK and using the newer serializer.

Setting the NumberHandling property in the JsonSerializerOptions to AllowReadingFromString has no effect.

To Reproduce
Create the following resource:

{
  "resourceType": "Patient",
  "multipleBirthInteger": "3",
}

Exception thrown:

Hl7.Fhir.Serialization.DeserializationFailedException: 'One or more errors occurred. (Expecting a Int32, but found a json string with value '3'. At line 15, position 30.)'

   at Hl7.Fhir.Serialization.FhirJsonPocoDeserializer.DeserializeResource(Utf8JsonReader& reader)
   at Hl7.Fhir.Serialization.FhirJsonConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonReaderState& readerState, Boolean isFinalBlock, ReadOnlySpan`1 buffer, JsonSerializerOptions options, ReadStack& state, JsonConverter converterBase)
   at System.Text.Json.JsonSerializer.ContinueDeserialize[TValue](ReadBufferState& bufferState, JsonReaderState& jsonReaderState, ReadStack& readStack, JsonConverter converter, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.<ReadAllAsync>d__65`1.MoveNext()
   at System.Threading.Tasks.ValueTask`1.get_Result()
   at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult()
   at Ihis.FhirEngine.Core.Utility.Serialization.SystemTextJsonFhirJsonSerializationUtility.<DeserializeAsync>d__6`1.MoveNext() in C:\Users\donnr\Source\Repos\FhirEngine\src\Ihis.FhirEngine.Core\Utility\Serialization\SystemTextJsonFhirJsonSerializationUtility.cs:line 47

Expected behavior
System.Text.Json should also be able to parse successfully when NumberHandling is set to AllowReadingFromString

Version used:

• FHIR Version: R4
• Version: 4.2.1

[ewoutkramer]

This is actually not a bug, but a feature. Using quotes around these integer is incorrect according to the FHIR specification, but the old parsers where by default operating in "permissive" mode, which made them accept recoverable mistakes. This was a historical decision in order not to break existing software, which would then stop accepting data that was acceptable before. You can change the behaviour in the old parsers by setting the "PermissiveParsing" option to "false" - you will then also get an error that these string decimals are incorrect. Having the default to "off" also meant the parsers accept data that is actually not fully correct.

With the new parsers we had the chance to rethink this For the new parsers we have rectified this situation. They will now always operate strictly - this is why you are getting this error. However, it is easy to get back the original behaviour: the exception returned now has coded errors and also a property PartialResult, which will contain the POCO. So you can now inspect the collection of errors, decide whether you want to accept them anyway by checking them by code (you can thus maintain your own list of acceptable errors), and then use the PartialResult as you would now. In this case, the partial result would be exactly the same as when would be returned by the old parsers in permissive mode.

See here: https://docs.fire.ly/projects/Firely-NET-SDK/parsing/system-text-json-deserialization.html#finding-errors-while-deserializing for more details.

Since this is a new approach, and we have just published these new parsers we are of course interested in feedback on the actual use of this new system, so let us know whether this works for you!

[almostchristian]

I am ok with fixing our tests to force the users to give the correct format. However it seems like both the firely and hapi test servers accept quoted numbers. Since consumers of our FHIR web services may be testing with public FHIR servers, they may be surprised once they use our services.

I also found initially that the System.Text.Json deserializer didn't like inline comments in the json payload. Since our developers and testers frequently add comments to their json payloads for testing, I had to support it. Luckily the comments could be handled by setting the property JsonSerializerOptions.ReadCommentHandling to Skip. Maybe have a setting in FhirJsonPocoDeserializerSettings that allows for the reading of numeric fields from string?

[ewoutkramer]

Since the new API will already continue parsing, it does actually parse with or without the quotes and it does produce a POCO. All the setting would do is decide whether or not a warning/error is added to the DeserializationFailedException.Exceptions list.

The goal of this new design is that many users of the API have different needs of what they want to allow, hence we went for a design where you can decide for yourself, based on the coded errors the parser resturns: there will never be enough settings to satisfy everyone otherwise (this is what we learned from the "old" API).

Maybe the fact that the parse results in an exception is putting you off? All the API tries to communicate is that a) there is an error in your document (there is!), but b) here is the coded list of warnings/errors - decide what you want to allow and c) here is the poco that resulted from deserialization.

In your concrete case, I can see that the error returned is "JSON110" - Unexpected Json token. We could split those warnings up into more codes (e.g. one for encountering a string where a number was expected - say JSON999), then you could do:

• Parse. If no errors, return the result
• Catch the DeserializationFailedException exception
• If the exception's "Exceptions" list has only JSON999 - you are fine, return the exception's PartialResult.
• Otherwise, rethrow the exception.

[ewoutkramer]

(by the way, it's interesting that the Java library accepts these strings too - it must mean they are operating in lenient mode - which then in turns means there must be databases full of incorrect FHIR data). I'll ask James about it next week at the WGM.

[almostchristian]

Thanks @ewoutkramer , I managed to resolve this issue by configuring the callback OnPrimitiveParseFailed in the FhirJsonPocoDeserializerSettings.

private static (object? Value, FhirJsonException? Exception) TryParseQuotedNumber(
    ref Utf8JsonReader reader,
    Type targetType,
    object? originalValue,
    FhirJsonException originalException)
{
    if (originalException.ErrorCode == FhirJsonException.UNEXPECTED_JSON_TOKEN_CODE &&
        reader.TokenType == JsonTokenType.String &&
        originalValue is string str)
    {
        if (targetType == typeof(int))
        {
            return (int.Parse(str), null);
        }
        else if (targetType == typeof(decimal))
        {
            return (decimal.Parse(str), null);
        }
    }

    return (null, originalException);
}

[ewoutkramer]

Ha! I should have thought of that, but now I am really glad I added that callback ;-)

(by the way, it's interesting that the Java library accepts these strings too - it must mean they are operating in lenient mode - which then in turns means there must be databases full of incorrect FHIR data). I'll ask James about it next week at the WGM.

We got in an interesting discussion on what things should be in the validation space and what should be in the parsing space. Turns out Java considers the syntax of decimals to not be the business of the parser, and expect the profile validator to catch this. I personally disagree - this is syntax and should be caught by the parser.

Anyway, I am happy this callback can be used to fix this issue and many similar ones.