Skip to content

Optimize IIdentifiable to ResourceObject conversion #1028

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
bart-degreed opened this issue Jul 16, 2021 · 5 comments · Fixed by #1091
Closed

Optimize IIdentifiable to ResourceObject conversion #1028

bart-degreed opened this issue Jul 16, 2021 · 5 comments · Fixed by #1091

Comments

@bart-degreed
Copy link
Contributor

In JADNC, what we call the "serialization layer" actually consists of two steps: converting between IIdentifiable and ResourceObject and converting between ResourceObject and JSON. This issue concerns refactoring/redesigning/optimizing the first part, as follow-up work to #664.

Goals are to optimize performance, as well as improve code clarity and reducing dependencies.

We'll need to decide what to do with the JsonApiDotNetCore.Serialization.Client.Internal namespace, which contains facilities to serialize a request body and deserialize a response body using the resource graph. We used this internally in the past for writing integration tests, but have stepped away from it because it hides the actual JSON data structure that goes over the wire. And it does not support all JSON:API features (such as links and meta, simply because an IIdentitiable does not provide storage for this information).

Zooming out a bit, what are the use cases for such a client library?

  • Writing integration tests
    For integration tests, we don't need/want it anymore, because we prefer a resource-graph-agnostic approach that enables us to use the full JSON:API data structure. For example, it enables to compose invalid requests and assert the right error message is returned. Also if we break our serialization layer, we want existing tests to fail instead of silently hiding the change.
  • Developing a JSON:API client in JavaScript/Ember/Angular/React/Go/Python/Rust/...
    N/A. There are existing libraries at https://jsonapi.org/implementations/#client-libraries. We're investigating to add support for OpenAPI that produces a typed model based on the resource graph, but it is complicated and is going to take a while before we can deliver that.
  • Developing a JSON:API client in .NET Core/5, when the server source is unavailable (third party).
    This means the resource graph is unavailable, so we cannot provide any better experience than what existing serialization libraries such as this one already provide.
  • For server-to-server communication (same project), having a single resource graph with models that can be shared between API server and API client would be nice. We found in practice this does not always work out so nicely, for example when resource properties are read-only or [EagerLoad] is used. Another thing is that we take dependencies on the request execution context to make the right translation, such as HttpContext for resolving ASP.NET routes, query strings usage, targeted fields in the JSON request body, and various other subtle details. In the API Server context, we have all these available, which enables JADNC to "understand" what is going on and produce helpful error messages, such as "ID cannot be specified in POST, unless AllowClientGeneratedIds is turned on". But in the API Client context, we'll need to add various pluggable extensibility points so that callers can feed in all the details. This is a lot of work to implement, but also a cumbersome experience when using it.

Therefore we'd like to delete the JsonApiDotNetCore.Serialization.Client.Internal namespace as part of this work.

@bart-degreed
Copy link
Contributor Author

Should delete ./wiki/v4/content/serialization.md as part of this work.

@bart-degreed
Copy link
Contributor Author

Should also remove ServiceCollectionExtensions.AddClientSerialization if removing JsonApiDotNetCore.Serialization.Client.Internal namespace.

@bart-degreed
Copy link
Contributor Author

The order of resources in included[] response body is currently influenced by the order of relationships in ?include= query string. We should break that dependency and instead use the declaration order of relationships in de resource class.

After that change, IncludeExpression.Elements and IncludeElementExpression.Children can become sets instead of lists.

@bart-degreed
Copy link
Contributor Author

bart-degreed commented Sep 14, 2021

Additional changes as part of these refactorings:

  • Currently, we append the contents of the request body to the detail line of an error message (but not for atomic:operations requests because they are usually very long). We should move the request body to top-level meta, and make it configurable whether this information should be included (off by default).
  • In tests, add assertion on error.source.pointer everywhere and make the post-processor fill this.
  • Add configuration option (turned on by default) to fail on unknown attribute/relationship names in request bodies. This is helpful for first-time users but may need to be turned off in versioned APIs.

@bart-degreed bart-degreed mentioned this issue Sep 14, 2021
5 tasks
@bart-degreed
Copy link
Contributor Author

As part of this, should fix the broken JsonApiDeserializerBenchmarks, which cannot work with an attribute due to a hard dependency on HttpContext, which should be factored out.

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.

1 participant