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.

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

Fix Primitive Collection serialization #6157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Suchiman
Copy link
Contributor

@Suchiman Suchiman commented Nov 28, 2024

This was broken by both #5871 and #5682:

  1. When a List<Guid> was serialized, it was recognized as a primitive collection and thus plainly written to the JSON without any type information: ["d4d8404c-4357-47ff-a343-649a116539f5"]
  2. When this JSON was deserialized, due to lack of type info, it was deserialized as List<object>, containing strings. This is already not good.
  3. When this List<object> gets serialized again (e.g. due to multiple workflow suspends causing WorkflowState serialization), this time it fails the primitive collection recognition, because object is not a primitive type. It now gets serialized as {"_items": ["d4d8404c-4357-47ff-a343-649a116539f5"], "_type": "Object[]"}
  4. When that JSON gets deserialized, it tries to ReadType(...) but ReadType fails to parse Object[] since it lacks the logic from TypeJsonConverter to throw away the [] before looking up Object in the WellKnownTypeRegistry, so it returns null as a type. Without a type but being faced with a json object { ... } it now deserializes into an ExpandoObject
  5. Any further serialization / deserializations will now cause the expando object to get nested deeper and deeper every time.

What is the motivation behind the WellKnownTypeRegistry? It seems to hugely complicate the codebase for little benefit and is handled inconsistently, it is being used in many places without the special logic around recognizing collections, which sometimes works on ICollection, sometimes on Lists, attaching [] or not, using the word "array" when a List is meant, etc.

Also is there a reason for reinventing the polymorphic serialization wheel, when STJ supports it out of the box?

Additionally i've adjusted the Write method to write the _type field first so that the Read method does not have to skip over all the content while searching for _type.


This change is Reviewable

@Suchiman
Copy link
Contributor Author

I'll have a look at the test failures later, i need to sleep x_x

This was broken by both elsa-workflows#5871 and elsa-workflows#5682:
1. When a List<Guid> was serialized, it was recognized as a primitive collection and thus plainly written to the JSON without any type information: ["d4d8404c-4357-47ff-a343-649a116539f5"]
2. When this JSON was deserialized, due to lack of type info, it was deserialized as List<object>, containing strings. This is already not good.
3. When this List<object> gets serialized again (e.g. due to multiple workflow suspends causing WorkflowState serialization), this time it fails the primitive collection recognition, because object is not a primitive type. It now gets serialized as {"_items": ["d4d8404c-4357-47ff-a343-649a116539f5"], "_type": "Object[]"}
4. When that JSON gets deserialized, it tries to ReadType() but ReadType() fails to parse Object[] since it lacks the logic from TypeJsonConverter to throw away the [] before looking up Object in the WellKnownTypeRegistry, so it returns null as a type. Without a type but being faced with a json object { ... } it now deserializes into an ExpandoObject
5. Any further serialization / deserializations will now cause the expando object to get nested deeper and deeper every time.
@Suchiman
Copy link
Contributor Author

@sfmskywalker good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant