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

Temporary rewrite of entire json implementation #21

Closed
warriordog opened this issue Jul 5, 2023 · 1 comment · Fixed by #22
Closed

Temporary rewrite of entire json implementation #21

warriordog opened this issue Jul 5, 2023 · 1 comment · Fixed by #22
Assignees
Labels
area:code Affects or applies to the library code

Comments

@warriordog
Copy link
Owner

warriordog commented Jul 5, 2023

The JSON-LD model, as currently designed and implemented, will not and can not work due to this open issue in System.Text.Json: dotnet/runtime#63791. Until that feature gap is resolved, we will need to fall back to a lower-level implementation. This is the proposed fallback:

  • Simple types (not polymorphic, no special AS logic or encoding) will be handled by System.Text.Json as usual.
  • Complex types will implement an interface exposing static virtual Serialize and Deserialize methods, which will work with JsonNode/JsonElement respectively.
  • Complex types will also implement protected static virtual methods Read and Write. Read will populate an existing object from a reader, and write will write properties into a pre-initialized JsonObject. These will allow serialization logic to be "inherited" by calling into the base class members.
  • Top-level code will create readers and writers manually. We should wrap this up in some kind of helper class, ideally with DI so that we can easily remove it later.

This rewrite will be completed in the sample-app branch. Work will be considered done when the sample app is able to query a simple object from a remote instance.

@warriordog
Copy link
Owner Author

This is actually a ton of effort, so here's a revised approach:

  • We start with the previous, broken solution. Everything runs through system.text.json manually, as usual.
  • Introduce a new [CustomJsonDeserializer] attribute that can be placed on a method to perform custom deserialization logic.
    • It should have this signature: static bool TryDeserialize(JsonElement element, JsonSerializerOptions options, out TThis? obj);
    • If false is returned, then use the default logic.
  • Introduce a new [CustomJsonSerializer] attribute that can be placed on a method to perform custom serialization logic.
    • It should have this signature: static bool TrySerialize(TThis obj, JsonSerializerOptions options, out JsonNode? node);
  • ASTypeConverter is modified to no longer be re-entrant. Instead, it works like this:
    • For deserialization:
      1. Convert the input into JsonElement
      2. Determine the real type to construct based on typeToConvert and the JSON data
      3. If the type has a CustomJsonDeserializer, then call it.
      4. If there is no converter, or the converter returns false, then manually deserialize using reflection.
    • For serialization:
      1. If the type has a CustomJsonSerializer, then call it.
      2. If there is no converter, or the converter returns false, then manually serialize using reflection.
      3. Write the JsonNode to the output stream
  • ASTypeRegistry will be updated to store metadata for all of the above attributes and properties.

This approach has several downsides:

  • It will be very slow due to double-parsing and use of reflection
  • We'll have to manually re-implement all JSON attributes, or at least the ones used in ASType
  • Other JsonConverters wont work with ASType without additional work

But it has several upsides, too:

  • No recursion / re-entry - we won't risk blowing the stack anymore
  • We have full control over parsing for ASType
  • No need to manually implement JSON logic for every type
  • Types can have custom parsing OR serialization, without both
  • Types can safely "fall back" to the default logic. This will be helpful in types like ASLink.
  • It bypasses the underlying issue

Open questions:

  • ASCollection requires a custom converter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:code Affects or applies to the library code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant