Redesign conversion between JSON objects and ASP.NET models #1091
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c4a6a53 to
525dac7
Compare
added 4 commits
September 29, 2021 15:19
…ce, but do write them when logging
…ead it (false by default)
…n request body (false by default)
8d3dfce to
5f618d2
Compare
added 10 commits
September 30, 2021 12:37
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%
```
…ody does not get added to the error response meta
f39c18b to
6244ef7
Compare
c0d7915 to
804464d
Compare
added 7 commits
October 8, 2021 11:22
…onship left/right. This enabled to reduce many resource graph lookups from the codebase.
9d057f1 to
0efde1b
Compare
added 2 commits
October 8, 2021 12:42
|
@maurei I wasn't sure if we need to port some unit tests from https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/c933e8b56849c6cbe6791243668e3735d04491b0/test/UnitTests/Serialization/Server/IncludedResourceObjectBuilderTests.cs. |
bart-degreed
pushed a commit
that referenced
this pull request
Oct 8, 2021
added 5 commits
October 20, 2021 11:48
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 |
88e5552 to
0f41f85
Compare
|
For review and future reference, I'm adding the data and steps produced by ResponseModelAdapterTests below. resource-model Resources_in_deeply_nested_circular_chain_are_written_in_relationship_declaration_order Resources_in_overlapping_deeply_nested_circular_chains_are_written_in_relationship_declaration_order |
maurei
approved these changes
Oct 27, 2021
bart-degreed
pushed a commit
that referenced
this pull request
Dec 3, 2021
* 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
bart-degreed
pushed a commit
that referenced
this pull request
Dec 3, 2021
* 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
bart-degreed
pushed a commit
that referenced
this pull request
Dec 3, 2021
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR rewrites the conversion between JSON objects (
ResourceObject,RelationshipObject) to theIIdentifiableentity classes which are used in ASP.NET model binding, ie the parameters on controller action methods.Fixes #1028.
Fixes #1024.
Fixes #1089.
Closes #661.
Note: Client serialization support was removed in #1075.
JsonApiOptions.IncludeRequestBodyInErrors(falseby default) to write request body in error meta when unable to read it. Before, we'd always append it to detail message, except for atomic:operations requests. This is off by default to hide sensitive data in production logs.JsonApiOptions.AllowUnknownFieldsInRequestBody(falseby default) to fail on unknown attributes/relationships in the request body. Before, we'd just ignore them, which sometimes makes debugging harder. Turn this on for the old behavior, typically if your API needs to support multiple endpoint versions.error.source.pointerwhen unable to read the request body. Before, we'd only point to the affected atomic:operation index, but now we provide deep paths like/atomic:operations[2]/data/relationships/performers/data[3]/type(and also on resource/relationship endpoints).ILinkBuilder.GetResourceLinkswas also used to render relationship links.IJsonApiRequestmanagement: restore backup after processing operations, so you'll get the correct injected top-level state when not processing an operation, for example inIResourceDefinition.OnSerialize.RequestScopedServiceProviderandIFieldsToSerialize(no longer used)includeexpressions. Theincludedorder in response body is now driven by the declaration order in theIIdentifiableclass and no longer based on the order in?include=query string value. Note though that this is an implementation detail: the order is undefined in JSON:API spec and we may change it over time. Also note there are corner cases, such as circular references and multiple paths, where we don't preserve this ordering for performance.Content-Lengthheader in HEAD response.JsonApiDotNetCore.Serializationnamespace. To customize response serialization, inherit fromJsonApiDotNetCore.Serialization.Response.ResponseModelAdapter.ResourceDefinition.OnApplyIncludesfor all nested children, even when empty. This allows adding extra nested includes from code.ResourceContexttoResourceTypeand exposed it through relationship left/right. This enabled to significantly reduce resource graph lookups from the codebase and reduce the dependency on the resource graph from various types. Code that used to beType type = relationship.RightTypeshould be changed to:Type type = relationship.RightType.ClrType.LinkBuilder, the (de)serialization flow no longer depends onHttpContext, which makes it easier to use in our tests and benchmark code. Instead, we heavily rely on the information inIJsonApiRequest, which is easy and cheap to instantiate.QUALITY CHECKLIST