Fallback to dynamic/guessed types on unknown data #3080

andrzejskowronski · 2025-03-17T08:42:57Z

Description

Targets poco branch for ease of viewing just the relevant changes, and will require the other pr.

Related issues

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

ewoutkramer · 2025-03-19T13:30:37Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

+            if(choiceMapping is not null)
+                return (choiceMapping, null);
+
+            return (null, ERR.CHOICE_ELEMENT_HAS_UNKOWN_TYPE(ref r, path.GetInstancePath(), propertyMapping.Name, typeSuffix));


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.

ewoutkramer · 2025-03-19T13:47:54Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

        FhirJsonPocoDeserializerState state,
        bool stayOnLastToken = false) where T : Base
    {
        if (reader.TokenType != JsonTokenType.StartObject)


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.

ewoutkramer · 2025-03-19T13:51:11Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

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


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.

ewoutkramer · 2025-03-19T13:52:30Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

        // 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);


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

…into feature/parsers-overflow-unknown-data # Conflicts: # src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoDeserializer.cs

ewoutkramer · 2025-03-24T16:23:38Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

+        {
+            state.Path.EnterElement(name, null, choiceType.IsPrimitive);
+
+            result = DeserializeFhirPrimitive(value as PrimitiveType, name, choiceType, null, ref reader, new(), state);


This does not need to be a primitive, right? Choice types can be non-primitives too,

ewoutkramer · 2025-03-24T16:28:42Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

+        {
+            state.Path.EnterElement(name, null, choiceType.IsPrimitive);
+
+            result = DeserializeFhirPrimitive(value as PrimitiveType, name, choiceType, null, ref reader, new(), state);


What if it is a list?

ewoutkramer · 2025-03-24T16:30:20Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoDeserializer.cs

                {
-                    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);


Maybe we should try to merge deserializeUnknownPropertiesInto and deserializePropertyValueInto into a single function? (it needs to do the same thing, with "the only difference" that we don't have type information)

Maybe by making sure we always have metadata, but then the metadata for DynamicPrimitive/DynamicType/DynaimcResource etc.

…erflow-unknown-data

src/Hl7.Fhir.Base/Model/Base.Extensions.cs

ewoutkramer · 2025-04-01T08:48:35Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonSerializer.cs

            writer.WritePropertyName(elementName);
            SerializePrimitiveValue(value.ObjectValue, writer, requiredType);
        }
+        // Since the object is not null, try overflow, but only if no extension data defined


I dont really understand whats going on, but it looks like we're serializing the contents of the overflow into the primitive element. This is invalid fhir json, so I'd like to understand better which problem we are solving here. It seems the overflow should be put under the _element?

ewoutkramer · 2025-04-02T09:44:33Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonDeserializer.cs

+            if(!string.IsNullOrEmpty(typeSuffix))
+                choiceMapping = inspector.FindClassMapping(typeSuffix);
+
+            choiceMapping ??= inspector.FindClassMapping(nameof(DynamicDataType));


We should introduce a non-null ClassMapping.DynamicDataType property (and use that in the NewPocoBuilder too), so the last line of this function (which will and should never be reached) can be removed.

ewoutkramer · 2025-04-02T09:45:00Z

src/Hl7.Fhir.Base/Serialization/BaseFhirXmlDeserializer.cs

+            if(!string.IsNullOrEmpty(typeSuffix))
+                choiceMapping = inspector.FindClassMapping(typeSuffix);
+
+            choiceMapping ??= inspector.FindClassMapping(nameof(DynamicDataType));


We should introduce a non-null ClassMapping.DynamicDataType property (and use that in the NewPocoBuilder too), so the last line of this function (which will and should never be reached) can be removed.

ewoutkramer · 2025-04-02T10:12:46Z

src/Hl7.Fhir.Base/Serialization/BaseFhirJsonDeserializer.cs

+        }
+        else if (reader.TokenType == JsonTokenType.StartArray)
+        {
+            var peeked = peekTypeNested(ref reader);


Is there a way to avoid these peeks (since they do cost a bit of time) for the case where we do have a mapping?

ewoutkramer · 2025-04-02T11:49:57Z

src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirJsonDeserializationTests.cs

            [
                new { resourceType = nameof(OperationOutcome), crap = 5 }, JsonTokenType.EndObject,
-                ERR.UNKNOWN_PROPERTY_FOUND_CODE
+                COVE.INCORRECT_CARDINALITY_MIN_CODE


This will be different again when we integrate Kas's changes.

ewoutkramer · 2025-04-02T11:51:49Z

src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirJsonDeserializationTests.cs

            new { name = "Ewout", _telecom = new object[] { new { system = "phone" }, new { systemX = "b" } } },
-            checkData, ERR.USE_OF_UNDERSCORE_ILLEGAL_CODE, ERR.UNKNOWN_PROPERTY_FOUND_CODE);
-        yield return data<ContactDetail>(new { name = new[] { "Ewout" } }, ERR.EXPECTED_PRIMITIVE_NOT_ARRAY_CODE);
+            checkData);


Why is this use of underscore illegal gone?

ewoutkramer · 2025-04-02T11:53:29Z

src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirJsonDeserializationTests.cs

            Console.WriteLine(recoveredActual);

            assertErrors(dfe.Exceptions, [
                COVE.LITERAL_INVALID_CODE,


I think we should review this test after Kas's work has been merged, then we can verify the original tests versus the completed one with both your PRs

ewoutkramer · 2025-04-02T11:56:29Z

src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirSerializationEngineTests.cs

    [DataRow(PERMISSIVEXML, null, POCO_EMPTY_VALUE, POCO_EMPTY_VALUE, DisplayName = "Permissive XML")]
-    [DataRow(BWCOMPATIBLEXML, POCO_UNKNOWN_ELEMENT, POCO_UNKNOWN_ELEMENT, null, DisplayName = "Backwards-compatible XML")]
-    [DataRow(POCO_WRONGXML, POCO_EXPECTED_OBJECT, POCO_EXPECTED_OBJECT, POCO_EXPECTED_OBJECT, DisplayName = "Wrong XML")]
+    [DataRow(BWCOMPATIBLEXML, null, null, null, DisplayName = "Backwards-compatible XML")]


This isn't testing much anymore, it was supposed to test the differences between CORREECt/permissive/bw compat etc, but it no longer functions as a good test, we will need to write another one. I will take this up as part of #2878

ewoutkramer · 2025-04-02T14:37:03Z

src/Hl7.Fhir.Support.Poco.Tests/TestData/fp-test-patient-errors-recovered.json

@@ -1,16 +1,87 @@
 {
  "resourceType": "Patient",


We should manually review the new output (recovered + dumped errors) to verify this after pulling the PR.

ewoutkramer

We're working on this already....

ewoutkramer · 2025-04-07T15:32:32Z

Superceded by #3094.

andrzejskowronski added 2 commits March 17, 2025 09:33

Fallback to dynamic/guessed types on unknown data

4f36d43

Cleanup

baa5f3d

ewoutkramer marked this pull request as ready for review March 19, 2025 13:59

ewoutkramer requested changes Mar 19, 2025

View reviewed changes

Merge remote-tracking branch 'origin/spike/overflow-errors-on-pocos' …

c074a2e

…into feature/parsers-overflow-unknown-data # Conflicts: # src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoDeserializer.cs

ewoutkramer requested changes Mar 24, 2025

View reviewed changes

andrzejskowronski added 3 commits March 25, 2025 11:21

Merge branch 'spike/overflow-errors-on-pocos' into feature/parsers-ov…

4d96003

…erflow-unknown-data

Fix new tests

99c78c9

XML tests

bc90a97

Base automatically changed from spike/overflow-errors-on-pocos to develop-6.0 March 25, 2025 17:11

andrzejskowronski added 8 commits March 26, 2025 16:51

Simplify json functionality, cleanup xml

751bf28

Throw away unneeded param

f54979a

Fix most test cases

38125e2

Cleanup

cb2f8a8

Nested arrays support

6ab077e

TestRecovery tweaks

14d3763

Annotate attributes

a4a0a2b

Fix roundtriping from incorrect list type

745cadb

andrzejskowronski requested a review from ewoutkramer April 1, 2025 08:13

andrzejskowronski and others added 2 commits April 1, 2025 11:52

Xml ordering

f351320

Merge branch 'develop-6.0' into feature/parsers-overflow-unknown-data

1e7625d

ewoutkramer requested changes Apr 2, 2025

View reviewed changes

ewoutkramer requested changes Apr 3, 2025

View reviewed changes

ewoutkramer closed this Apr 7, 2025

ewoutkramer deleted the feature/parsers-overflow-unknown-data branch April 23, 2025 21:40

Fallback to dynamic/guessed types on unknown data #3080

Fallback to dynamic/guessed types on unknown data #3080

Uh oh!

Conversation

andrzejskowronski commented Mar 17, 2025

Description

Related issues

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewoutkramer left a comment

Choose a reason for hiding this comment

Uh oh!

ewoutkramer commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants