-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
Resource inheritance #1142
Resource inheritance #1142
Conversation
b0e733d
to
446e6d3
Compare
Codecov Report
@@ Coverage Diff @@
## master #1142 +/- ##
==========================================
+ Coverage 89.46% 92.62% +3.15%
==========================================
Files 242 241 -1
Lines 7213 7686 +473
==========================================
+ Hits 6453 7119 +666
+ Misses 760 567 -193
Continue to review full report at Codecov.
|
5b9a64d
to
b092f2d
Compare
…ullString() to provide improved debug info
b092f2d
to
1b2c75e
Compare
…econdary/relationship endpoints
…t on derived type
…l need this later)
68063dc
to
d12eec7
Compare
… new implementation in #1142
… merge Removed existing resource inheritance tests Resource inheritance: return derived types Resource inheritance: sparse fieldsets Changed the expression tokenizer to recognize dots in field chains Resource inheritance: derived includes Resource inheritance: derived filters Added ResourceFieldAttribute.Type and use it from QueryExpression.ToFullString() to provide improved debug info Resource inheritance: sorting on derived fields Added missing tests for GET abstract/concrete base primary types at secondary/relationship endpoints Resource graph validation: fail if field from base type does not exist on derived type Clarified error message on type mismatch in request body Rename inheritance tests Added extension method to obtain ClrType of a resource instance (we'll need this later) Resource inheritance: write endpoints Updated documentation Added rewriter unit tests to improve code coverage Exclude example project from code coverage
This PR adds full support for resource inheritance. While earlier versions did allow that in some sense, the experience was limited, inconsistent and largely untested. Tests are provided in this PR for both Inheritance per Hierarchy and Inheritance per Type (see https://docs.microsoft.com/en-us/ef/core/modeling/inheritance for details).
Discovery of the type hierarchy is out of scope for this PR. The API client not being able to programmatically retrieve that information falls in the same category as not being able to retrieve which resources and relationships exist. This should be solved as part of adding OpenAPI support (#1046). Until then, such information should be exposed through manual documentation.
While implementing this, I came to realize it doesn't make sense to return only base types on non-abstract base endpoints. The stored objects are a mix of base and derived types, so that's the most appropriate to return, just like EF Core does when querying for base types. Callers knowing the type hierarchy can use sparse fieldsets to retrieve only the fields defined in the base type and convert/downcast as desired. It wouldn't be right to force API developers to choose between base or derived types returned by making base types abstract or not. Furthermore, while API callers could choose the base endpoint (to indicate whether or not derived types are returned), they would be unable to indicate whether to return base or derived types in included resources. Therefore, any endpoint should always return the actual stored types, independent of whether the endpoint affects an abstract resource type or a concrete (base or derived) type.
Sparse Fieldsets
QueryLayer.Projection
used to store a set of fields (attributes and relationships). This PR renames that to.Selection
(because we never project into other shapes) and its data structure is expanded to store a set of fields per resource type.SelectClauseBuilder
uses that to generate switches per resource type. The resource graph has been expanded to provide base/derived resource types for this.Includes
EF Core is tolerant when writing
.Include("...")
by allowing navigations that only exist on some of the derived types.IncludeParser
is adapted in this PR to allow that too, so it accepts relationships on derived types. This works without the need for adding upcast syntax to the?include=
query string parameter. However, there's an interesting case:For the above model, a request to
GET /shoppingBaskets?include=items
yields two matching relationships. So any subsequent relationships in the chain need to take both into account when looking for relationships on derived types. The advantage of this unfolding is we don't require callers to upcast in relationship chains. The downside is that there's currently no way to include Products without Articles. We could add such optional upcast syntax in the future, if desired.Filters
The new
isType
filter operator can be used to verify whether the value of a resource field is convertible to the specified derived type. A chain of to-one relationships can be optionally specified to perform the type check on a nested relationship. The last parameter is an optional nested filter, which executes if the conversion succeeds. In the nested filter, derived fields are accessible.Examples:
There is no direct syntax to perform an exact type check, however, that can be accomplished by combining operators:
The comma syntax was chosen instead of an optional parameter to keep the parser simple (no need for backtracking or knowledge of nested terms).
Sorting
Similar to includes, this PR extends the
?sort=
parser to accept attributes and relationships on derived types, without requiring upcast syntax. Likewise,IResourceDefinition<,>.CreateSortExpressionFromLambda()
has been updated to tolerate casts andas
syntax. It now supports.Count()
as well, has improved validation with clearer error messages and is fully covered by unit tests.The interesting case from parsing includes affects parsing sorts too, which may result in multiple matches:
Because the ordering depends on which attribute/relationship we choose, we cannot just pick one, so the next examples are ambiguous and fail with an error:
We can make it work in the future by adding optional upcast syntax (which would be required for the above cases).
Write endpoints
This was quite a challenge. I've gone back and forth over various designs to find the best possible experience.
Considering support for type hierarchies, the following aspects come into play:
TResource
type parameter used in the request pipeline and resource definitionsFirst of all, endpoint types are allowed to be abstract, concrete-base or concrete-derived types. This applies to all endpoints. It matches the
TResource
type parameter inJsonApiController
,JsonApiResourceService
andEntityFrameworkCoreRepository
.For example, you can create a
Car
at the/cars
endpoint, but also at the/vehicles
endpoint. Depending on the endpoint type, it would call intoCarsController
orVehiclesController
.Request bodies can also contain these three kinds of types, except that abstract resource types are not allowed in the request bodies for create/update resource. The reason for create resource is obvious, but for update resource this is problematic because the ASP.NET controller binder requires an object instance that is assignable to
TResource
. But we can't create an instance of an abstract type. At the remaining places, we use a trick internally:AbstractResourceWrapper<TId>
stores the abstract type to use, which is special-cased for at various places.Resource definitions are the place to implement custom business logic, which may vary depending on the resource type. In such cases, the endpoint type or request body type is irrelevant. What really matters is the type as it is stored in the database. To make this possible, JADNC will perform extra queries when a resource type is part of a type hierarchy, to ensure that
IResourceDefinition<TResource>
is called, whereTResource
is the stored type. Even better: this applies to both left-side and right-side resources.You can make resource definitions for derived types depend on resource definitions for base types, to ensure logic is in a single place. For example:
So when trying to update a resource that is stored as a
Tandem
, the following requests will all be routed toTandemDefinition
:PATCH /vehicles/1 { "data": { type: "bikes" ... } }
PATCH /vehicles/1 { "data": { type: "tandems" ... } }
PATCH /bikes/1 { "data": { type: "bikes" ... } }
PATCH /bikes/1 { "data": { type: "tandems" ... } }
PATCH /tandems/1 { "data": { type: "tandems" ... } }
Because the same applies for the right-side type, you can safely write business logic in resource definitions, such as:
To make the above possible, the following extra queries are executed:
For consistency,
IJsonApiRequest.PrimaryResourceType
and.Relationship
are actualized after fetch, to refer to the stored types instead of the endpoint types. This means you can injectIJsonApiRequest
anywhere to obtain the stored left type. So keep in mind that this may be different from the endpoint type when using type hierarchies.Along the lines, this PR fixes the bug where we forgot to call into
IResourceDefinition.OnPrepareWrite
from a delete relationship request.Tests
Integration tests use the model below, where
Vehicle
,MotorVehicle
,Wheel
andEngine
are abstract types.Closes #844.
QUALITY CHECKLIST