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

Use System.Text.Json for JSON serialization and deserialization #870

Open
wants to merge 170 commits into
base: master
Choose a base branch
from

Conversation

ikeough
Copy link
Contributor

@ikeough ikeough commented Aug 22, 2022

BACKGROUND

We would like to use Elements in Hypar's web application, specifically for geometry creation. This requires being able to deserialize a model and generate geometry quickly. Previous testing with the existing Newtonsoft.Json serialization methods revealed poor performance when used through web assembly. Microsoft's recommendation is to move to System.Text.Json.

DESCRIPTION

  • This PR introduces the ElementConverter<T>: JsonConverter<T> which takes the place of the JsonInheritanceConverter to handle element reference and inheritance serialization.
  • This PR replaces JsonProperty with JsonPropertyName.
  • This PR removes all references to Newtonsoft.Json throughout the core Elements project.
  • This PR creates a minimal implementation of a wrapper API around Elements and a "bridge" API in javascript.
  • As the purpose of this change was to support faster serialization and deserialization in wasm, this PR introduces Elements.Wasm, a minimal blazor test application, to test model creation, serialization, deserialization, and visualization.

Trade-offs
Moving to System.Text.Json is not a 1:1 port. We have had to make several trade-offs.

  • JsonProperty is now JsonPropertyName and doesn't support the Required or NullValueHandling parameters. This makes it impossible for us to specify, at the property level how null values should be handled. These parameters are removed which means if we receive a null value for a property that used to specify that the value must not be null, instead of getting a deserialization validation exception, we get a type mismatch exception. The end result of the two is the same however, an element is not added to the model. If we find this is problematic, we will need to handle it using converters.
  • There is no error handler in the deserializer. The out errors parameter that previously recorded deserialization errors has been removed. The ModelConverter now plays the part of the handling element deserialization errors gracefully, but we cannot at this time return those errors to the caller.
  • We can't use private fields as properties in json by attaching the JsonProperty attribute. We've added the IncludeFields flag to the serializer and made all of these fields follow Pascal case standards, which is going to break serialization for the two types where this pattern was used (CellComplex, and AdaptiveGrid).
  • We can't use internal constructors, even if they're marked with JsonConstructor. These constructors must be public.
  • The largest change of this PR is the requirement to add attributes to sub-properties that are elements which specify the generic converter to be used: ex: JsonConverter(typeof(ElementConverter<Curve>)) We got away without doing this previously because (I think), json.net's converter's did not need to be typed. We will need to ensure that all built-in element types have the correct attributes applied. And, we will need to make sure that generated elements code applies an attribute to properties whose type inherits from Element.

TESTING

cd Elements.Wasm
dotnet watch run

FUTURE WORK

  • The move to System.Text.Json facilitates updating our element addition code to not require recursive addition of sub-elements as those elements will be "read ahead" during deserialization.

REQUIRED

  • All changes are up to date in CHANGELOG.md.

This test, creates a model, serializes it, deserializes it, and writes it to glTF.

Release build without AOT.
image

Release build with AOT.
image

TODO:

  • Convert PositionConverter
  • Convert GeometryConverter
  • Convert SolidConverter
  • Convert MeshConverter
  • Update Model.ToJson
  • Update Model.FromJson
  • Remove JsonInheritanceConverter

This change is Reviewable

@ikeough ikeough changed the title Stj Use System.Text.Json for JSON serialization and deserialization Aug 22, 2022
@ikeough ikeough marked this pull request as ready for review August 22, 2022 05:33
@ikeough ikeough self-assigned this Aug 22, 2022
Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @mattc)


Elements/src/Model.cs line 340 at r8 (raw file):

                            // Qualify element as a geometric element by seeing 
                            // whether it has a representation.
                            if (element.Value.TryGetProperty("Representation", out var repProperty))

let's also verify that repProperty does not have a null value. some GeometricElements (MeshElement, for instance) may have a null Representation. Here's an easy failing test:

var model = new Model();
var meshSphere = Mesh.Sphere(5, 10);
var meshElement = new MeshElement(meshSphere);
model.AddElement(meshElement);
var json2 = model.ToJson();
var newModel2 = Model.GeometricElementModelFromJson(json2);

Elements/src/Model.cs line 355 at r8 (raw file):

                                    {
                                        case "Elements.Geometry.Solids.Extrude":
                                            var profile = (Profile)resolver.ResolveReference(solidOp.GetProperty("Profile").GetString());

Our current deserialization logic handles inline references as well as id-based references for other elements, like Profiles. This probably doesn't occur much in normal function execution, where the Id replacement happens at AddElements time, but other pathways might use it (I ran into it w/ typescript functions where I was hand-generating model json, but I can imagine it popping up in revit, content workflows, etc). Your call — perhaps we want to be strict that the "Profile" property can always be assumed to be a string — but we could make this a bit more tolerant with something like this:

       private static T ResolveOrReturn<T>(JsonElement jsonElement, string propertyName, ElementReferenceResolver resolver) where T : Element
        {
            if (jsonElement.TryGetProperty(propertyName, out var property))
            {
                if (property.ValueKind == JsonValueKind.String)
                {
                    return resolver.ResolveReference(property.GetString()) as T;
                }
                else if (property.ValueKind == JsonValueKind.Object)
                {
                    return JsonSerializer.Deserialize<T>(property);
                }
            }
            return null;
        }

so the call could just be var profile = ResolveOrReturn<Profile>(solidOp, "Profile", resolver);. I could also see this being folded into ResolveReference by making it take a JsonElement instead of a string.

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Haven't done a full review yet, just marking some stuff I noticed over the weekend.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @mattc)

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @mattc)


Elements/src/Serialization/JSON/ElementConverter.cs line 112 at r9 (raw file):

                            int end = discriminator.IndexOf(">", start);
                            string elementType = discriminator.Substring(start, end - start);
                            if (!resolver.TypeCache.TryGetValue(elementType, out var genericType))

It's quite common to come across a type you don't have a class for. We need to be able to fall back to Element or GeometricElement if appropriate.

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @mattc)


Elements/src/Serialization/JSON/ElementConverter.cs line 112 at r9 (raw file):

Previously, andrewheumann wrote…

It's quite common to come across a type you don't have a class for. We need to be able to fall back to Element or GeometricElement if appropriate.

(actually, since this is just the proxy code, it's probably safe to just fallback to "Element.")

@ikeough ikeough added this to the 2.0 milestone Apr 15, 2023
@ikeough ikeough modified the milestones: 2.0, 2.1 Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants