Skip to content

Support System.Text.Json #664

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

Closed
bjornharrtell opened this issue Jan 13, 2020 · 4 comments · Fixed by #1075
Closed

Support System.Text.Json #664

bjornharrtell opened this issue Jan 13, 2020 · 4 comments · Fixed by #1075

Comments

@bjornharrtell
Copy link
Contributor

With System.Text.Json gaining popularity it would be good if JsonApiDotNetCore can natively work with both it and Newtonsoft.Json. One important case is that the former is used in npgsql efcore (see http://www.npgsql.org/efcore/mapping/json.html#jsondocument-dom-mapping).

@maurei
Copy link
Member

maurei commented Feb 12, 2020

Right now the serialization sort of heavily relies on Newtonsoft.Json annotation internally. We could investigate what it takes to decouple this. Apart from the performance improvements, what is a usecase for using System.Text.Json rather than Newtonsoft?

One important case is that the former is used in npgsql efcore

Would one of the usecases be relying on just one json library instead of both ?

@bart-degreed
Copy link
Contributor

To aid investigation: The limitations of and behavioral differences between System.Text.Json and Newtonsoft.Json are listed here.

@bart-degreed
Copy link
Contributor

@bart-degreed
Copy link
Contributor

bart-degreed commented Jul 15, 2021

I've done a thorough investigation on the feasibility of replacing internal usage of Newtonsoft.Json with System.Text.Json.

Summary

  • On .NET 5 or higher this is possible, but comes with considerable trade-offs due to the immaturity of STJ (see below).
  • Supporting both libraries at the same time is a no-go.
  • STJ is roughly 50% faster with the right tweaks in place, which results in 5% faster response times on average (not counting database access).

Measurements

Measurement results for GET http://localhost:14140/api/v1/todoItems?include=owner,assignee,tags:
  Newtonsoft.Serialize ...........  0:00:00:00.0003357
  System.Text.Json.Serialize .....  0:00:00:00.0001612

Measurement results for POST http://localhost:14140/api/v1/todoItems:
  Newtonsoft.Deserialize .........  0:00:00:00.0001317
  System.Text.Json.Deserialize ...  0:00:00:00.0000536
  Newtonsoft.Serialize ...........  0:00:00:00.0000561
  System.Text.Json.Serialize .....  0:00:00:00.0000220

Observations and learnings

  • Started out on .NET Core 3.1, but it is just missing too many fundamental building blocks.
  • When deserializing to object (value in attribute/relationship/meta dictionaries), Newtonsoft performs a best guess by looking at the input string, while STJ does not even try. This means we need help from the resource graph to provide target types. It's unfortunate that we must take this dependency, but by doing that we solve some oddities around floating-point numbers and DateTime/DateTimeOffset. We can make this work by injecting extra converters in the serializer options after building the resource graph. However, for meta there's not much we can offer, so its dictionary values become JsonElement, which means API developers will need to convert these values themselves from incoming requests. But given we have the resource graph at our hands, we can potentially fail earlier on invalid data, as opposed to the current post-processing.
  • Because we need the resource graph during serialization, the API client must always send "type" before the attributes, so we can resolve. It might be possible to clone the forward-only reader implementation of STJ so we can re-read and catch up later, but I haven't tried that.
  • In STJ there is no built-in support to control the order of serialized members (added in .NET 6). What's worse, base members end up at the end. This requires us to spell out the order in custom converters. This is especially unfortunate because by default it emits JSON:API attributes before type/id (declared in base class), which breaks our serialization because we need type first.
  • The dynamic nature of data in JSON:API, which can be null, a single item, or an array, is challenging to read, but solvable using custom converters.
  • Newtonsoft allows choosing when to serialize at runtime using ShouldSerializeXXX() methods. STJ requires a custom converter for that.
  • In STJ, custom converters cannot be read-only or write-only. In various cases, we need to customize one but delegate the other to built-in handling. This results in an endless cycle, so I came up with the idea to clone serializer options, take ourselves out of the converter list and then delegate. At this point, I thought I solved this, but later I found that performance was terrible: 10-100x slower than Newtonsoft. It turns out that serializer options build a private cache on the first usage. In Newtonsoft this happens too, but it preserves its cache on clone, so only the first usage is slow. See https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-configure-options?pivots=dotnet-5-0#reuse-jsonserializeroptions-instances. So I ended up writing all the extra boilerplate conversion logic myself because there's no good way to delegate. While this solved the problem at hand, it is brittle: any changes in object models or its member types require updates to various converters too. This approach also means all the [JsonProperty] annotations (required, nullability, casing, ordering) become useless and we need to spell everything out in procedural code after all.
  • We need to set JavaScriptEncoder.UnsafeRelaxedJsonEscaping in options, to prevent a string like "some 'quoted' value" to become Unicode-escaped as "some \u0027quoted\u0027 value".
  • Only camel-case and pascal-case conventions (JsonNamingPolicy) are built-in. This is unfortunate, though there are custom implementations online for kebab/snake casing.
  • STJ is better at applying casing convention in dictionary keys, this enables us to remove Newtonsoft-specific trickery.
  • We should add tests for using pascal casing convention. This means setting PropertyNamingPolicy/DictionaryKeyPolicy to null and it should not crash.
  • The serialization of DateTime/DateTimeOffset in STJ differs from Newtonsoft, where the latter has higher milliseconds precision.
  • There is no built-in support to handle TimeSpan. Though there are custom implementations online, this means the format is non-standardized. In our tests, we need to choose between compact notation or backwards compatibility with Newtonsoft. Update: This is added in .NET 6.
  • STJ has good async support built-in, which enables us to remove Newtonsoft-specific workarounds that copy streams.
  • Newtonsoft can deserialize into derived types, but STJ does not because they consider it insecure. The workaround to declare as object is not feasible for us, so this means writing custom converters again. This may complicate resource inheritance in JADNC, which is poorly supported already, so I haven't looked into that further.
  • In the end, I was able to make nearly all integration tests pass. The few that still fail are caused by changed error messages.
  • Newtonsoft provides clear errors, where STJ usually fails with an unhelpful "too much or too few has been read". This makes troubleshooting (for us, as well as API developers) take a lot of time.

While working on this, several improvements came to mind:

  • We should provide an option to fail on unknown attributes/relationships and turn that on by default. The power of JSON is that readers can easily skip over unknown elements, which is great for resiliency in versioning. But during debugging, this often bites us wondering why something does not work, only to realize a typo later.
  • The query string ?nulls=true&defaults=false (which means: include null values but hide default values in response) is impossible in STJ. Where Newtonsoft had separate settings for Null and Default, STJ unifies them into one choice between IgnoreAlways, IgnoreNever, IgnoreNull, IgnoreDefault. Where IgnoreDefault implies IgnoreNull, because the default value of a nullable/reference type is null. We could solve this by replacing ?nulls and ?defaults with a single parameter, or keep them but throw on an unsupported combination. Given these parameters are undocumented, I think we should drop them entirely. Based on feedback, we can bring them back later in the same or a different form.
  • This is an opportunity to reorder JSON elements in response bodies, to optimize interop between API and clients.
    • Put "jsonapi" at the top, so clients can quickly terminate or take the version/extensions into account when reading the rest of the payload.
    • Put "meta" above "data", so clients get the total resource count early.
    • Rename "total-resources" meta key to "total" because thats what Ember.js expects it to be named.
    • Put "links" after "data", because it is follow-up information.
    • JSON:API does not specify order, but we should look at the examples carefully and see what patterns we can infer.
  • Consolidate object model differences between error- and request/operation responses (meta, don't create default instances) and add few missing members from JSON:API v1.1.
  • We should consider to rename Document to something more specific, to reduce the chance of name clashes with other libraries
  • I haven't changed existing logic that converts from IIdentifiable to ResourceObject, but I suspect there's room for improvement in that area. Both in performance as well as in code clarity and reducing dependencies.

Conclusion

Aside from bad error messages, the pain is mostly on our (that is, the JADNC library developers) side. We must trade a lot of declarative annotations (required, nullability, ordering etc.) for hand-written procedural code. For this investigation, I've done the minimal effort to make things work. But in addition to that, a lot of extra code is needed to validate all required elements are provided, their data types are valid, etc. in the various custom converters. But because the JSON:API structure does not change often, we should be able to manage that.

This means we're going to drop support for Newtonsoft.Json in the next major version in favor of STJ, unless we receive substantial feedback to not make these changes and wait for improved support in future versions of .NET.

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

Successfully merging a pull request may close this issue.

3 participants