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.

Sign up for GitHub

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

Fallback to dynamic/guessed types on unknown data #3080

Open
wants to merge 3 commits into
base: spike/overflow-errors-on-pocos
Choose a base branch
from
Open

Fallback to dynamic/guessed types on unknown data #3080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f36d43
Fallback to dynamic/guessed types on unknown data
andrzejskowronski Mar 17, 2025
baa5f3d
Cleanup
andrzejskowronski Mar 17, 2025
c074a2e
Merge remote-tracking branch 'origin/spike/overflow-errors-on-pocos' …
ewoutkramer Mar 19, 2025
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
233 changes: 206 additions & 27 deletions src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Hl7.Fhir.Introspection;
using Hl7.Fhir.Model;
using Hl7.Fhir.Utility;
using Hl7.Fhir.Validation;
using System;
using System.Collections;
using System.Collections.Generic;
Expand Down Expand Up @@ -162,9 +163,13 @@ public bool TryDeserializeObject(Type targetType, ref Utf8JsonReader reader, [No
{
if (reader.TokenType != JsonTokenType.StartObject)
{
state.Errors.Add(ERR.EXPECTED_START_OF_OBJECT(ref reader, state.Path.GetInstancePath(), reader.TokenType));
reader.Recover(); // skip to the end of the construct encountered (value or array)
return null;
var result = deserializeUnexpectedJsonValue(state.Path.GetLastPart(), ref reader, state, stayOnLastToken);

var target = _inspector.FindClassMapping(typeof(DynamicResource))?.Factory() as Resource;

target!.SetValue("value", result);

return target;
}

(ClassMapping? resourceMapping, FhirJsonException? error) = DetermineClassMappingFromInstance(ref reader, _inspector, state.Path);
Expand Down Expand Up @@ -261,8 +266,10 @@ private void deserializeObjectInto<T>(
{
if (reader.TokenType != JsonTokenType.StartObject)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be more flexible in our thinking: we will only call this function IF we are at an object. I know at this moment we are doing it if we are expecting an object (because of classmappings etc), but since those are just hints now, we should probably never end up in this function if we don't actually see an object on the wire.

{
state.Errors.Add(ERR.EXPECTED_START_OF_OBJECT(ref reader, state.Path.GetInstancePath(), reader.TokenType));
reader.Recover(); // skip to the end of the construct encountered (value or array)
var result = deserializeUnexpectedJsonValue(state.Path.GetLastPart(), ref reader, state, stayOnLastToken);

target.SetValue("value", result);

return;
}

Expand Down Expand Up @@ -295,8 +302,8 @@ private void deserializeObjectInto<T>(
{
state.Errors.Add(error);

// try to recover by skipping to the next property.
reader.SkipTo(JsonTokenType.PropertyName);
// no mapping found, make guess for type and insert into overflow
deserializeUnknownPropertiesInto(target, currentPropertyName, ref reader, state, stayOnLastToken);
}
else
{
Expand All @@ -305,7 +312,7 @@ private void deserializeObjectInto<T>(

try
{
state.Path.EnterElement(propMapping!.Name, !propMapping.IsCollection ? null : 0, propMapping.IsPrimitive);
state.Path.EnterElement(propMapping!.Name, isEnteringJsonArray(ref reader) ? 0 : null, propMapping.IsPrimitive);
deserializePropertyValueInto(target, currentPropertyName, propMapping, propValueMapping!, ref reader, objectParsingState, state);
}
finally
Expand Down Expand Up @@ -335,6 +342,142 @@ private void deserializeObjectInto<T>(
PocoDeserializationHelper.RunInstanceValidation(target, Settings.Validator, context, state.Errors);
}
}

private void deserializeUnknownPropertiesInto<T>(
T target,
string propertyName,
ref Utf8JsonReader reader,
FhirJsonPocoDeserializerState state,
bool stayOnLastToken = false) where T : Base
{
target.TryGetValue(propertyName, out var value);

var (name, choiceType) = tryDetectChoiceTypeFromName(propertyName);

// move past property name
reader.Read();

object? result;
if(choiceType is not null)
{
state.Path.EnterElement(name, null, choiceType.IsPrimitive);

result = DeserializeFhirPrimitive(value as PrimitiveType, name, choiceType, null, ref reader, new(), state);

state.Path.ExitElement();
}
else
{
state.Path.EnterElement(name, reader.TokenType == JsonTokenType.StartArray ? 0 : null, isJsonPrimitive(ref reader));

result = deserializeUnexpectedJsonValue(propertyName, ref reader, state, stayOnLastToken);

state.Path.ExitElement();
}

target.SetValue(name, result);
(string name, ClassMapping? choiceType) tryDetectChoiceTypeFromName(string propertyName)
{
var span = propertyName.AsSpan();
for(int i = 0; i < span.Length; i++)
{
if (!char.IsUpper(span[i]))
continue;

var subSpan = span.Slice(i);
if (subSpan.IsEmpty)
break;

var choiceMapping = _inspector.FindClassMapping(subSpan.ToString());
if (choiceMapping is not null)
return (span[..i].ToString(), choiceMapping);
}
return (propertyName, null);
}
}

private object? deserializeUnexpectedJsonValue(string propertyName, ref Utf8JsonReader reader, FhirJsonPocoDeserializerState state, bool stayOnLastToken, ClassMapping? propertySuggestion = null)
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I think this is going to be the main workhorse, in fact we could call this straight from the main TryDeserializeResouirce and TryDeserializeObject functions - we're only expecting something, but we're no longer bound to it, so this dynamic function is really nice.

{
if(reader.TokenType == JsonTokenType.StartObject)
{
var propMapping = propertySuggestion ?? _inspector.FindClassMapping(typeof(DynamicDataType))!;

var primitive = (propMapping.Factory() as Base)!;

deserializeObjectInto(primitive, propMapping, ref reader, DeserializedObjectKind.FhirPrimitive, state, stayOnLastToken);

return primitive;
}
else if (reader.TokenType == JsonTokenType.StartArray)
{
var primitiveType = guessFhirPrimitiveType(peekType(ref reader));

var propMapping = propertySuggestion ?? _inspector.FindClassMapping(primitiveType!)!;

var primitiveList = propMapping.ListFactory();

var objectState = new ObjectParsingState();

deserializeFhirPrimitiveList(primitiveList, propertyName, propMapping, primitiveType, ref reader, objectState, state);

return primitiveList;
}
else if (reader.TokenType switch
{
JsonTokenType.String => typeof(FhirString),
JsonTokenType.Number => typeof(FhirDecimal),
JsonTokenType.True or JsonTokenType.False => typeof(FhirBoolean),
_ => null
} is { } type)
{
var (primitive, error) = DeserializePrimitiveValue(ref reader, type, state.Path);

if(error is not null)
state.Errors.Add(error);

return new DynamicPrimitive { ObjectValue = primitive };
}

return null;
}

private static JsonTokenType peekType(ref Utf8JsonReader reader)
{
if (reader.TokenType == JsonTokenType.StartArray)
{
var peekCopy = reader;

peekCopy.Read();

return peekCopy.TokenType;
}

return reader.TokenType;
}

private static Type? guessFhirPrimitiveType(JsonTokenType tokenType)
{
return tokenType switch
{
JsonTokenType.String => typeof(FhirString),
JsonTokenType.Number => typeof(FhirDecimal),
JsonTokenType.True or JsonTokenType.False => typeof(FhirBoolean),
JsonTokenType.StartObject => typeof(DynamicPrimitive),
_ => null
};
}

private static bool isEnteringJsonArray(ref Utf8JsonReader reader)
{
return reader.TokenType == JsonTokenType.StartArray;
}

private static bool isJsonPrimitive(ref Utf8JsonReader reader)
{
return reader.TokenType
is JsonTokenType.String or JsonTokenType.Number
or JsonTokenType.False or JsonTokenType.True;
}

/// <summary>
/// Reads the value of a json property.
Expand Down Expand Up @@ -367,7 +510,7 @@ FhirJsonPocoDeserializerState state

// There might be an existing value, since FhirPrimitives may be spread out over two properties
// (one with, and one without the '_')
var existingValue = propertyMapping.GetValue(target);
target.TryGetValue(propertyMapping.Name, out var existingValue);
Copy link
Member

Choose a reason for hiding this comment

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

Note that we've moved from using IL (in GetValue) to our generated function now, which might be a bit slower.


if (propertyValueMapping.IsFhirPrimitive)
{
Expand All @@ -378,8 +521,8 @@ FhirJsonPocoDeserializerState state

// Note that the POCO model will always allocate a new list if the property had not been set before,
// so there is always an existingValue for IList
result = propertyMapping.IsCollection ?
deserializeFhirPrimitiveList((IList)existingValue!, propertyName, propertyValueMapping, fhirType, ref reader, delayedValidations, state) :
result = isEnteringJsonArray(ref reader) ?
deserializeFhirPrimitiveList((IList)(existingValue ?? propertyValueMapping.ListFactory()), propertyName, propertyValueMapping, fhirType, ref reader, delayedValidations, state) :
DeserializeFhirPrimitive(existingValue as PrimitiveType, propertyName, propertyValueMapping, fhirType, ref reader, delayedValidations, state);
}
else
Expand All @@ -388,10 +531,15 @@ FhirJsonPocoDeserializerState state
if (propertyName[0] == '_')
state.Errors.Add(ERR.USE_OF_UNDERSCORE_ILLEGAL(ref reader, state.Path.GetInstancePath(), propertyMapping.Name, propertyName));

// handle case where we detect array where it shouldn't be, or primitive where there should be array
if (isEnteringJsonArray(ref reader) != propertyMapping.IsCollection)
{
result = deserializeUnexpectedJsonValue(propertyName, ref reader, state, false, propertyValueMapping);
}
// Note that repeating simple elements (like Extension.url) do not currently exist in the FHIR serialization
if (propertyMapping.IsCollection)
else if(propertyMapping.IsCollection)
{
var l = (IList)existingValue!;
var l = (IList)(existingValue ?? propertyValueMapping.ListFactory());
// if the list is already populated, a property with an identical key was encountered earlier
if (l.Count > 0)
{
Expand Down Expand Up @@ -444,7 +592,15 @@ FhirJsonPocoDeserializerState state
PocoDeserializationHelper.RunPropertyValidation(result, Settings.Validator!, deserializationContext, state.Errors);
}

propertyMapping.SetValue(target, result);
target.SetValue(propertyMapping.Name, result);
try
{
_ = propertyMapping.GetValue(target);
}
catch (CodedValidationException ex)
{
state.Errors.Add(new CodedValidationException(ex.ErrorCode, ex.Message, state.Path.GetInstancePath(), line, pos, ex.IssueSeverity, ex.IssueType));
}
}

/// <summary>
Expand Down Expand Up @@ -641,7 +797,16 @@ FhirJsonPocoDeserializerState state
{
state.Path.EnterElement("value", 0, true);

var (result, error) = DeserializePrimitiveValue(ref reader, primitiveValueProperty.ImplementingType, state.Path);
object? result;
FhirJsonException? error = null;
if (reader.TokenType is JsonTokenType.StartObject or JsonTokenType.StartArray)
{
result = deserializeUnexpectedJsonValue(propertyName, ref reader, state, false);
}
else
{
(result, error) = DeserializePrimitiveValue(ref reader, primitiveValueProperty.ImplementingType, state.Path);
}

if (error is not null)
state.Errors.Add(error);
Expand Down Expand Up @@ -791,13 +956,21 @@ internal static (ClassMapping?, FhirJsonException?) DetermineClassMappingFromIns
{
var (resourceType, error) = determineResourceType(ref reader);

if (resourceType is null) return (null, error);

var resourceMapping = inspector.FindClassMapping(resourceType);

return resourceMapping is not null ?
(new(resourceMapping, null)) :
(new(null, ERR.UNKNOWN_RESOURCE_TYPE(ref reader, path.GetInstancePath(), resourceType)));
ClassMapping? resourceMapping = null;
if (resourceType is not null)
resourceMapping = inspector.FindClassMapping(resourceType);

// fall back to DynamicResource, if we can't find the resource requested
resourceMapping ??= inspector.FindClassMapping(nameof(DynamicResource));

if(resourceMapping is not null)
return (resourceMapping, null);

if(resourceType is null)
return (null, error);


return (null, ERR.UNKNOWN_RESOURCE_TYPE(ref reader, path.GetInstancePath(), resourceType));
}

private static (string?, FhirJsonException?) determineResourceType(ref Utf8JsonReader reader)
Expand Down Expand Up @@ -854,6 +1027,7 @@ private static (PropertyMapping? propMapping, ClassMapping? propValueMapping, Fh
var propertyMapping = parentMapping.FindMappedElementByName(elementName)
?? parentMapping.FindMappedElementByChoiceName(elementName);

// handled by the unknown type deserialization
if (propertyMapping is null)
return (null, null, ERR.UNKNOWN_PROPERTY_FOUND(ref reader, path.GetInstancePath(), propertyName));

Expand All @@ -873,11 +1047,16 @@ private static (PropertyMapping? propMapping, ClassMapping? propValueMapping, Fh
{
string typeSuffix = elementName[propertyMapping.Name.Length..];

return string.IsNullOrEmpty(typeSuffix)
? (null, ERR.CHOICE_ELEMENT_HAS_NO_TYPE(ref r, path.GetInstancePath(), propertyMapping.Name))
: inspector.FindClassMapping(typeSuffix) is { } cm
? (cm, null)
: (null, ERR.CHOICE_ELEMENT_HAS_UNKOWN_TYPE(ref r, path.GetInstancePath(), propertyMapping.Name, typeSuffix));
ClassMapping? choiceMapping = null;
if(!string.IsNullOrEmpty(typeSuffix))
choiceMapping = inspector.FindClassMapping(typeSuffix);

choiceMapping ??= inspector.FindClassMapping(nameof(DynamicDataType));
Copy link
Member

Choose a reason for hiding this comment

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

But this could also be a DynamicPrimitive - which we could heuristically determine from the fact that there is a _ or a primitive value. But we don't know at this point, so maybe we need a method ToDynamicPrimitive() that switches to a primitive if we discover that later on.


if(choiceMapping is not null)
return (choiceMapping, null);

return (null, ERR.CHOICE_ELEMENT_HAS_UNKOWN_TYPE(ref r, path.GetInstancePath(), propertyMapping.Name, typeSuffix));
Copy link
Member

Choose a reason for hiding this comment

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

This should never happen. In fact, the NewPocoBuilder has constants for the classmappings for DynamicDataType, and this will never be null when we use those.

}
}
}
Expand Down
Loading
Loading