Skip to content

Commit

Permalink
Redesign conversion between JSON objects and ASP.NET models (#1091)
Browse files Browse the repository at this point in the history
* Replaced suppression with fix

* Fixed: do not duplicate the list of JSON:API errors in meta stack trace, but do write them when logging

* Breaking: Added option to write request body in meta when unable to read it (false by default)

* Breaking: Added option to allow unknown attribute/relationship keys in request body (false by default)

* Fixed invalid test

* Rewrite of reading the request body and converting it into models.

The existing validation logic inside controllers, JsonApiReader and RequestDeserializer/BaseDeserializer has been consolidated into a set of adapters that delegate to each other, passing along the current state. The dependency on `HttpContext` has been removed and similar errors are now grouped, in preparation to unify them. Because in this commit we always track the location of errors and write them to `error.source.pointer`, the error messages can be unified and simplified in future commits.

In this commit, I've gone through great lenghts to preserve the existing error messages in our tests as much as possible, while keeping all tests green. All error tests in ReadWrite and Operations now have assertions on the source pointer. Added tests for missing/invalid 'data' in resource requests. Removed outdated unit tests and added new one for handling attributes of various CLR types.

Updated benchmark code. The synthetic BenchmarkDotNet reports it has become 3-7% slower (we're talking microseconds here) compared to the old code. But when running full requests with our pipeline-instrumentation turned on, the difference falls in the range of background noise and is thus unmeasurable.

```
BEFORE:
          Read request body ...............................  0:00:00:00.0000167 ...   1.08%  } 167 + 434 = 601
            JsonSerializer.Deserialize ....................  0:00:00:00.0000560 ...   3.62%  +
            Deserializer.Build (single) ...................  0:00:00:00.0000434 ...   2.80%  }

AFTER:
          Read request body ...............................  0:00:00:00.0000511 ...   3.43%
            JsonSerializer.Deserialize ....................  0:00:00:00.0000537 ...   3.61%
```

* Updated error message for unknown attribute/relationship

* Updated error message for unknown resource type

* Unified error messages about the absense or presense of 'id' and 'lid'

* Unified error messages about the absense of 'type'

* Unified error messages about incompatible types

* Revert the use of different exception, because this way the request body does not get added to the error response meta

* Unified error messages about mismatches in 'id' and 'lid' values

* Additional unification of error messages

* Unified error messages for failed type conversion

* Fix cibuild

* Unified error messages about data presense and its value: null/object/array

* Unified remaining error messages

* Sealed types and reduced dependencies

* Fixed broken test on linux

* Adapter renames:
- IOperationsDocumentAdapter -> IDocumentInOperationsRequestAdapter
- IResourceDocumentAdapter -> IDocumentInResourceOrRelationshipRequestAdapter
- IOperationResourceDataAdapter -> IResourceDataInOperationsRequestAdapter

* Added missing assertions on request body in error meta

* Refactorings:
- Changed deserializer to accept Error(s) instead of Document (used to be ErrorDocument, but they were merged)
- Write request body in error meta instead of top-level meta
- Fixed: Log at Error instead of Info when processing an operation throws unknown error

* Enhanced existing tests: Assert on resource type when `included` contains mixed types

* Fixed the number of resource definition callbacks for sparse fieldsets

* Refactor: removed OperationContainer.Kind because IJsonApiRequest.WriteOperation is now populated consistently; applied c# pattern usage

* Added test to capture the current behavior to return data:null for void operations. The spec allows an empty object too, but I don't consider this important enough to add yet another JSON converter with all its overhead.

* Improved tests for includes

* Rewrite of rendering the response body from models

- Added temporary bridge class to toggle between old/new code
- Fixed: Missing top-level link in error- and operations response
- Fixed: `ILinkBuilder.GetResourceLinks` was also used to render relationship links
- IJsonApiRequest management: restore backup after processing operations
- Refreshed serialization benchmarks

Measurement results for GET http://localhost:14140/api/v1/todoItems?include=owner,assignee,tags:
    Write response body ............................  0:00:00:00.0010385 ->  0:00:00:00.0013343 = 130%

Measurement results for GET http://localhost:14140/api/v1/todoItems?filter=and(startsWith(description,'T'),equals(priority,'Low'),not(equals(owner,null)),not(equals(assignee,null))):
    Write response body ............................  0:00:00:00.0006601 -> 0:00:00:00.0004624 = 70%

Measurement results for POST http://localhost:14140/api/v1/operations (10x add-resource):
    Write response body ............................  0:00:00:00.0003432 -> 0:00:00:00.0003289 = 95%

|                            Method |     Mean |   Error |  StdDev |
|---------------------------------- |---------:|--------:|--------:|
|       SerializeOperationsResponse | 153.8 us | 0.72 us | 0.60 us |
| LegacySerializeOperationsResponse | 239.0 us | 3.17 us | 2.81 us |

|                          Method |     Mean |   Error |  StdDev |
|-------------------------------- |---------:|--------:|--------:|
|       SerializeResourceResponse | 101.3 us | 0.31 us | 0.29 us |
| LegacySerializeResourceResponse | 177.6 us | 0.56 us | 0.50 us |

* Avoid closure in hot code path to reduce allocations

* Cleanup reader and writer

* Removed old code

* Fixed: crash in test serializer on assertion failure

* Removed RequestScopedServiceProvider

* Use sets for include expressions

* Fixed: return Content-Length header in HEAD response
https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

* Reorganized JADNC.Serialization namespace

* Created custom exception for remaining errors

* Fixed: call ResourceDefinition.OnApplyIncludes for all children, even when empty

* Renamed ResourceContext to ResourceType and exposed it through relationship left/right. This enabled to reduce many resource graph lookups from the codebase.

* Moved logic to build resource graph from DbContext into ResourceGraphBuilder for easier reuse.

* Opened up ResponseModelAdapter for extensibility

* Check off roadmap entry

* Review feedback

* Simplified existing tests

* Added extra test for data:null in relationship

* Added test for broken resource linkage

* Ported existing unit tests and changed how included[] is built.

It now always emits related resources in relationship declaration order, even in deeply nested circular chains where a subsequent inclusion chain is deeper and adds more relationships to an already converted resource.

Performance impact, summary:
- In the endpoint test, this commit improves performance for rendering includes, while slightly decreasing it for the other two scenarios. All are still faster or the same compared to the master branch.
- In BenchmarkDotNet, this commit slightly increases rendering time, compared to earlier commits in this PR, but it is still faster than the master branch.

Measurement results for GET http://localhost:14140/api/v1/todoItems?include=owner,assignee,tags:
    Write response body .............................  0:00:00:00.0010385 -> 0:00:00:00.0008060 ...  77% (was: 130%)

Measurement results for GET http://localhost:14140/api/v1/todoItems?filter=and(startsWith(description,'T'),equals(priority,'Low'),not(equals(owner,null)),not(equals(assignee,null))):
    Write response body .............................  0:00:00:00.0006601 -> 0:00:00:00.0005629 ...  85% (was: 70%)

Measurement results for POST http://localhost:14140/api/v1/operations (10x add-resource):
    Write response body .............................  0:00:00:00.0003432 -> 0:00:00:00.0003411 ...  99% (was: 95%)

|                            Method |     Mean |   Error |  StdDev |
|---------------------------------- |---------:|--------:|--------:|
| LegacySerializeOperationsResponse | 239.0 us | 3.17 us | 2.81 us |
|       SerializeOperationsResponse | 153.8 us | 0.72 us | 0.60 us |
| (new) SerializeOperationsResponse | 168.6 us | 1.74 us | 1.63 us |

|                          Method |     Mean |   Error |  StdDev |
|-------------------------------- |---------:|--------:|--------:|
| LegacySerializeResourceResponse | 177.6 us | 0.56 us | 0.50 us |
|       SerializeResourceResponse | 101.3 us | 0.31 us | 0.29 us |
| (new) SerializeResourceResponse | 123.7 us | 1.12 us | 1.05 us |

* Fixed cibuild
  • Loading branch information
Bart Koelman committed Oct 27, 2021
1 parent fb0c5fc commit 758a191
Show file tree
Hide file tree
Showing 305 changed files with 9,573 additions and 7,588 deletions.
2 changes: 1 addition & 1 deletion ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ The need for breaking changes has blocked several efforts in the v4.x release, s
- [x] Instrumentation [#1032](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1032)
- [x] Optimized delete to-many [#1030](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1030)
- [x] Support System.Text.Json [#664](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/664) [#999](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/999) [1077](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1077) [1078](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1078)
- [ ] Optimize IIdentifiable to ResourceObject conversion [#1028](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1028) [#1024](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1024) [#233](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/233)
- [x] Optimize IIdentifiable to ResourceObject conversion [#1028](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1028) [#1024](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1024) [#233](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/233)
- [ ] Nullable reference types [#1029](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1029)

Aside from the list above, we have interest in the following topics. It's too soon yet to decide whether they'll make it into v5.x or in a later major version.
Expand Down
122 changes: 122 additions & 0 deletions benchmarks/Deserialization/DeserializationBenchmarkBase.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
using System;
using System.Collections.Generic;
using System.ComponentModel.Design;
using System.Text.Json;
using JetBrains.Annotations;
using JsonApiDotNetCore.Configuration;
using JsonApiDotNetCore.Middleware;
using JsonApiDotNetCore.Resources;
using JsonApiDotNetCore.Resources.Annotations;
using JsonApiDotNetCore.Serialization.JsonConverters;
using JsonApiDotNetCore.Serialization.Request.Adapters;
using Microsoft.Extensions.Logging.Abstractions;

namespace Benchmarks.Deserialization
{
public abstract class DeserializationBenchmarkBase
{
protected readonly JsonSerializerOptions SerializerReadOptions;
protected readonly DocumentAdapter DocumentAdapter;

protected DeserializationBenchmarkBase()
{
var options = new JsonApiOptions();
IResourceGraph resourceGraph = new ResourceGraphBuilder(options, NullLoggerFactory.Instance).Add<ResourceA>().Build();
options.SerializerOptions.Converters.Add(new ResourceObjectConverter(resourceGraph));
SerializerReadOptions = ((IJsonApiOptions)options).SerializerReadOptions;

var serviceContainer = new ServiceContainer();
var resourceFactory = new ResourceFactory(serviceContainer);
var resourceDefinitionAccessor = new ResourceDefinitionAccessor(resourceGraph, serviceContainer);

serviceContainer.AddService(typeof(IResourceDefinitionAccessor), resourceDefinitionAccessor);
serviceContainer.AddService(typeof(IResourceDefinition<ResourceA>), new JsonApiResourceDefinition<ResourceA>(resourceGraph));

// ReSharper disable once VirtualMemberCallInConstructor
JsonApiRequest request = CreateJsonApiRequest(resourceGraph);
var targetedFields = new TargetedFields();

var resourceIdentifierObjectAdapter = new ResourceIdentifierObjectAdapter(resourceGraph, resourceFactory);
var relationshipDataAdapter = new RelationshipDataAdapter(resourceIdentifierObjectAdapter);
var resourceObjectAdapter = new ResourceObjectAdapter(resourceGraph, resourceFactory, options, relationshipDataAdapter);
var resourceDataAdapter = new ResourceDataAdapter(resourceDefinitionAccessor, resourceObjectAdapter);

var atomicReferenceAdapter = new AtomicReferenceAdapter(resourceGraph, resourceFactory);
var atomicOperationResourceDataAdapter = new ResourceDataInOperationsRequestAdapter(resourceDefinitionAccessor, resourceObjectAdapter);

var atomicOperationObjectAdapter = new AtomicOperationObjectAdapter(options, atomicReferenceAdapter,
atomicOperationResourceDataAdapter, relationshipDataAdapter);

var resourceDocumentAdapter = new DocumentInResourceOrRelationshipRequestAdapter(options, resourceDataAdapter, relationshipDataAdapter);
var operationsDocumentAdapter = new DocumentInOperationsRequestAdapter(options, atomicOperationObjectAdapter);

DocumentAdapter = new DocumentAdapter(request, targetedFields, resourceDocumentAdapter, operationsDocumentAdapter);
}

protected abstract JsonApiRequest CreateJsonApiRequest(IResourceGraph resourceGraph);

[UsedImplicitly(ImplicitUseTargetFlags.Members)]
public sealed class ResourceA : Identifiable
{
[Attr]
public bool Attribute01 { get; set; }

[Attr]
public char Attribute02 { get; set; }

[Attr]
public ulong? Attribute03 { get; set; }

[Attr]
public decimal Attribute04 { get; set; }

[Attr]
public float? Attribute05 { get; set; }

[Attr]
public string Attribute06 { get; set; }

[Attr]
public DateTime? Attribute07 { get; set; }

[Attr]
public DateTimeOffset? Attribute08 { get; set; }

[Attr]
public TimeSpan? Attribute09 { get; set; }

[Attr]
public DayOfWeek Attribute10 { get; set; }

[HasOne]
public ResourceA Single1 { get; set; }

[HasOne]
public ResourceA Single2 { get; set; }

[HasOne]
public ResourceA Single3 { get; set; }

[HasOne]
public ResourceA Single4 { get; set; }

[HasOne]
public ResourceA Single5 { get; set; }

[HasMany]
public ISet<ResourceA> Multi1 { get; set; }

[HasMany]
public ISet<ResourceA> Multi2 { get; set; }

[HasMany]
public ISet<ResourceA> Multi3 { get; set; }

[HasMany]
public ISet<ResourceA> Multi4 { get; set; }

[HasMany]
public ISet<ResourceA> Multi5 { get; set; }
}
}
}
Loading

0 comments on commit 758a191

Please sign in to comment.