Update openapi branch #1105
Merged
Update openapi branch #1105
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
* 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
* Removed single-parameter controllers * Removed base class: I can't think of a reason why we should have an intermediate base class for these tests * Removed single-parameter resource services * Removed single-parameter resource repositories * Removed single-parameter resource definitions * Removed non-generic Identifiable and throw for resource classes that only implement IIdentifiable (without ID) when building the resource graph * Updated documentation
* Renamed TryXXX resource graph lookup methods to FindXXX, throwing on incoming null (not empty). Made non-public TryXXX methods tolerant against incoming nulls. * Added directive to disable NRT in all source files * Turn on NRT globally * Annotated JsonApiDotNetCore library * Use alternate public name for attribute, to improve test coverage * Annotated example projects * Annotated benchmark project - Removed LinkBuilderGetNamespaceFromPathBenchmarks, we don't have such code anymore - Cleanup query string benchmark * Enhanced rendering of error.source.pointer on ModelState validation errors * Respect configured MaxModelValidationErrors in operations * Added docs for nullable reference types * Automated cleanup code * Fixed: do not fail when clearing a required to-many relationship * Annotated controllers in integration tests * Annotated DbContexts in integration tests * Annotated fakers in integration tests * Annotated TestBuildingBlocks * Use Should() replacements that flow nullability * Re-align similar testsets * Annotated test projects, use fakers for non-trivial models in integration tests, FluentAssertions everywhere * Fixed: make faker dates deterministic See https://giters.com/bchavez/Bogus/issues/247 * Enable ASP.NET ModelState validation by default Due to recent enhancements, this produces better error messages for missing required relationships and avoids 500 errors due to foreign key constraint violations. * Check off roadmap entry * Documented required one-to-one relationships mapping * Added missing HEAD routes * Prefer IDictionary<,>.TryGetValue() over .Contains() followed by indexer lookup * Optimized response rendering time when no sparse fieldset is requested. This relies on the fact that the resource model is frozen after building the resource graph. | Method | Mean | Error | StdDev | | ------------------------------------ | -------- | ------- | ------- | | SerializeOperationsResponse (before) | 168.6 us | 1.74 us | 1.63 us | | SerializeOperationsResponse (after) | 148.5 us | 1.06 us | 0.99 us | | SerializeResourceResponse (before) | 123.7 us | 1.12 us | 1.05 us | | SerializeResourceResponse (after) | 119.6 us | 1.05 us | 0.98 us | This makes SerializeOperationsResponse 12% faster and SerializeResourceResponse 3% faster. What the benchmark does not show is the performance improvement on subsequent requests, so in practice the gain in higher. * Fixed: handle nulls in request body * Workaround for crash in cibuild * Update to latest JetBrains tools In the past, [cibuild hung after update to the latest version](#1045). Since then, the codebase has changed and a new minor version was released. These result in the cibuild no longer hanging. Stats from my laptop (old=v2021.1.4, new=v2021.2.2): ``` master, inspectcode (old): 2:59 master, inspectcode (new): 2:00 nrt, inspectcode (old): 3:04 nrt, inspectcode (new): 2:33 master, cleanupcode (old): 11:06 master, cleanupcode (new): 5:39 nrt, cleanupcode (old): 11:59 nrt, cleanupcode (new): 6:34 ``` So the newer versions got faster. And cleanupcode still takes the most time. In this PR, 722 of 1020 files have changed, which is 70% of the total codebase. Because during PR cibuild, we run cleanupcode only on changed files (in chunks), this PR takes very long and sometimes times out. This is an exceptional case, future PRs shouldn't touch so many files. So in the end, I believe this update is the best way forward. * Package updates * Review non-public TryXXX methods * Addressed review feedback
* JSON:API spec compliance: do not unescape brackets in response From https://jsonapi.org/format/1.1/#appendix-query-details-square-brackets: > According to the query parameter serialization rules above, a compliant implementation will percent-encode these square brackets. * Updated existing IResourceDefinition tests to capture all relevant callbacks * Retrieve total resource count on secondary/relationship endpoints using inverse relationship Bugfix: links.next was not set on full page at relationship endpoint * Rename flags enum to plural * Clarified documentation; fixed broken link * Check off roadmap entry
maurei
force-pushed
the
master-into-openapi
branch
from
22e6f7f to
f1d0bcd
Compare
* Added extra validations of the resource graph and unified unit tests * Fail at startup on controller that derives from BaseJsonApiController, but uses a resource type that is not registered in the resource graph * Check off roadmap entry
…e.ClrType - Applied NRT - Renamed methods that used TryXXX pattern - For files in JsonApiObjects folder: instead of adding null! to every property assignment, I decided to disable the warning. This seems reasonable because these types have been introduced to only pass static type information to the ApiExplorer. They are guaranteed to never be instantiated.
maurei
force-pushed
the
master-into-openapi
branch
from
f1d0bcd to
7b78e99
Compare
bart-degreed
suggested changes
Nov 9, 2021
src/JsonApiDotNetCore.OpenApi/JsonApiMetadata/JsonApiEndpointMetadataProvider.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiMetadata/JsonApiEndpointMetadataContainer.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiMetadata/RelationshipRequestMetadata.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiObjects/Documents/PrimaryResourceResponseDocument.cs
Outdated
Show resolved
Hide resolved
bart-degreed
suggested changes
Nov 17, 2021
src/JsonApiDotNetCore.OpenApi/JsonApiObjects/Documents/PrimaryResourceResponseDocument.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiActionDescriptorCollectionProvider.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/SwaggerComponents/ResourceFieldObjectSchemaBuilder.cs
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/SwaggerComponents/ResourceFieldObjectSchemaBuilder.cs
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/SwaggerComponents/ResourceFieldObjectSchemaBuilder.cs
Outdated
Show resolved
Hide resolved
* Disable GlobalPerProduct setting layer in cleanupcode/inspectcode * Temporary code change to verify cleanupcode still fails in cibuild * Revert "Temporary code change to verify cleanupcode still fails in cibuild" This reverts commit 037b58f. * Adapted to latest version of ReGitLint
bart-degreed
suggested changes
Nov 18, 2021
src/JsonApiDotNetCore.OpenApi/JsonApiObjects/Documents/PrimaryResourceResponseDocument.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiActionDescriptorCollectionProvider.cs
Outdated
Show resolved
Hide resolved
maurei
force-pushed
the
master-into-openapi
branch
from
c6e8542 to
df25522
Compare
maurei
force-pushed
the
master-into-openapi
branch
from
df25522 to
a1b0a7b
Compare
added 2 commits
November 22, 2021 12:10
* Fixed inconsistencies in naming of resource type * Fixed: Query to determine initial state is sent to the read-only database on POST many-to-many and DELETE to-many requests * Make command/query controllers inherit from JsonApiController, passing null for missing services This produces HTTP 405 (Method Not Allowed) instead of 404 (Not Found), which better reveals the intent * Added overload on TypeExtensions.IsOrImplementsInterface to take a type parameter, used for non-generic or generic constructed interfaces * Use ResourceType instead of public name in local-id tracker * Change IQueryStringParameterReader.AllowEmptyValue into normal interface member * Simplified startup in example project * Removed left-over overload with single type parameter in ResourceGraphBuilder * Various corrections in documentation, added #nullable where it makes a difference * Revert DocFx workaround * Updated version compatibility table * Added release notes and icon to NuGet package * Fix nullability warnings produced by .NET 6 with EF Core 6 * Fixed: Error when using EagerLoad on a relationship * Use VS2022 image in AppVeyor on Windows * Fixed broken tests on EF Core 6 Apparently EF Core 5 did not always fail on missing required relationships, which was fixed in EF Core 6. I tried to update existing tests leaving the models intact, but that poluted lots of tests so I made them optional instead. * Fixed redacted data when running tests * Breaking: Removed access-control action filter attributes such as HttpReadOnly, NoHttpPost etc. because they interfere with relationship endpoints. For example, blocking POST would block creating resources, as well as adding to to-many relationships, which is not very useful. The replacement is to inject just the subset of exposed services, or simply use the Command/Query controllers. When an endpoint is not exposed, we now return HTTP 403 Forbidden instead of 404 or 405. * Increase version number, use branch name in suffix * Pass the full resource to LinkBuilder, instead of just its ID. This allows for more intelligence in the link builder, such as handling versioning or inheritance. * Optimization: Only save when there are changes in a remove-from-to-many relationship request * Clarifications in doc-comments * Extract constant * Extract method * Cleanup SelectClauseBuilder.ToPropertySelectors * Since EF Core 5, SaveChanges automatically creates a savepoint and rolls back to it on failure. This produces more correct error responses in operations, compared to rolling back the entire transaction. For example, when an operations request creates a resource and the next operation fails, the resource service may incorrectly conclude that the resource from the first operation does not exist. * Improved unittests for populating IJsonApiRequest in middleware * Clarify intent in AtomicOperationObjectAdapter * Review feedback: use `ResourceType resourceType` instead of `ResourceType type` everywhere
- Made constructors of document type factories private and exposed a static instance
bart-degreed
suggested changes
Nov 22, 2021
src/JsonApiDotNetCore.OpenApi/JsonApiObjects/Documents/PrimaryResourceResponseDocument.cs
Outdated
Show resolved
Hide resolved
test/OpenApiTests/LegacyOpenApiIntegration/LegacyIntegrationDbContext.cs
Outdated
Show resolved
Hide resolved
...ApiDotNetCore.OpenApi/JsonApiObjects/Documents/NullableResourceIdentifierResponseDocument.cs
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiObjects/NonPrimaryDocumentTypeFactory.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/SwaggerComponents/JsonApiSchemaGenerator.cs
Outdated
Show resolved
Hide resolved
src/JsonApiDotNetCore.OpenApi/JsonApiActionDescriptorCollectionProvider.cs
Outdated
Show resolved
Hide resolved
bart-degreed
approved these changes
Nov 23, 2021
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.
Updates the
openapibranch with the latest changes from master (among which the introduction of NRT).QUALITY CHECKLIST