Skip to content
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

Merged
merged 19 commits into from
Apr 4, 2022
Merged

Resource inheritance #1142

merged 19 commits into from
Apr 4, 2022

Conversation

bart-degreed
Copy link
Contributor

@bart-degreed bart-degreed commented Mar 16, 2022

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:

public abstract class ShoppingBasket : Identifiable<long>
{
}

public sealed class SilverShoppingBasket : ShoppingBasket
{
    [HasMany]
    public ISet<Article> Items { get; get; }
}

public sealed class PlatinumShoppingBasket : ShoppingBasket
{
    [HasMany]
    public ISet<Product> Items { get; get; }
}

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:

/blogs/1/contributors?filter=isType(,men) -- returns only male contributors
/blogs/1/contributors?filter=isType(,men,equals(hasBeard,'true')) -- returns only male contributors with beards
/blogs?filter=isType(author,men) -- returns only blogs whose author is male
/blogs?filter=isType(author,men,equals(hasBeard,'true')) -- returns only blogs whose author is male and has a beard
/blogs?filter=has(contributors,isType(,men)) -- returns only blogs that contain at least one male contributor

There is no direct syntax to perform an exact type check, however, that can be accomplished by combining operators:

/vehicles?filter=and(or(isType(,bikes),isType(,motorVehicles)),not(isType(,trucks))) -- bikes and cars, but no trucks

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 and as 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:

public abstract class ShoppingBasket : Identifiable<long>
{
}

public sealed class SilverShoppingBasket : ShoppingBasket
{
    [Attr]
    public DateTime? LastUpdatedAt { get; set; }

    [HasOne]
    public Person Owner { get; get; }

    [HasMany]
    public ISet<Article> Items { get; get; }
}

public sealed class PlatinumShoppingBasket : ShoppingBasket
{
    [Attr]
    public DateTimeOffset? LastUpdatedAt { get; set; }

    [HasOne]
    public Person Owner { get; get; }

    [HasMany]
    public ISet<Product> Items { get; get; }
}

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:

GET /shoppingBaskets?sort=lastUpdatedAt
GET /shoppingBaskets?sort=owner.firstName
GET /shoppingBaskets?sort=count(items)

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:

  • The endpoints: create/update resource-with-relationships, delete resource, set/add-to/remove-from relationship, operations
  • Resource types: abstract class, concrete base class, concrete derived class
  • Type usage: endpoint type, request body type, and the resource type stored in the database
  • Left-side and right-side resource types
  • The TResource type parameter used in the request pipeline and resource definitions

First 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 in JsonApiController, JsonApiResourceService and EntityFrameworkCoreRepository.
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 into CarsController or VehiclesController.

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, where TResource 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:

public sealed class BikeDefinition : JsonApiResourceDefinition<Bike, long>
{
    public BikeDefinition(IResourceGraph resourceGraph)
        : base(resourceGraph)
    {
    }

    public override Task<IIdentifiable?> OnSetToOneRelationshipAsync(Bike leftResource,
        HasOneAttribute hasOneRelationship, IIdentifiable? rightResourceId, WriteOperationKind writeOperation,
        CancellationToken cancellationToken)
    {
        // ...
    }
}

public sealed class TandemDefinition : JsonApiResourceDefinition<Tandem, long>
{
    private readonly IResourceDefinition<Bike, long> _baseDefinition;

    public TandemDefinition(IResourceGraph resourceGraph, IResourceDefinition<Bike, long> baseDefinition)
        : base(resourceGraph)
    {
        _baseDefinition = baseDefinition;
    }

    public override Task<IIdentifiable?> OnSetToOneRelationshipAsync(Tandem leftResource,
        HasOneAttribute hasOneRelationship, IIdentifiable? rightResourceId, WriteOperationKind writeOperation,
        CancellationToken cancellationToken)
    {
        if (ResourceType.BaseType!.FindRelationshipByPublicName(hasOneRelationship.PublicName) != null)
        {
            // Delegate to ResourceDefinition for base type Bike.
            return _baseDefinition.OnSetToOneRelationshipAsync(leftResource, hasOneRelationship, rightResourceId,
                writeOperation, cancellationToken);
        }

        // Handle here.
        return Task.FromResult(rightResourceId);
    }
}

So when trying to update a resource that is stored as a Tandem, the following requests will all be routed to TandemDefinition:

  • 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:

if (leftResource is Bike && rightResourceId is CarbonWheel)
{
    throw new Exception("Bikes can only have chrome wheels.");
}

To make the above possible, the following extra queries are executed:

  • delete resource: fetch the left-side resource if it is part of a type hierarchy
  • add to to-many relationship: fetch the left-side resource if it is part of a type hierarchy (if not already done because it concerns a many-to-many relationship); fetch the right-side resources if the relationship right type is part of a type hierarchy
  • create resource: fetch the right-side resources in the subset of relationships whose right type is part of a type hierarchy (this would normally only execute on failure to produce a proper error message)
  • update resource: same as create resource
  • update relationship: fetch the right-side resources if the relationship right type is part of a type hierarchy

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 inject IJsonApiRequest 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 and Engine are abstract types.

image

Closes #844.

QUALITY CHECKLIST

@bart-degreed bart-degreed force-pushed the resource-inheritance branch 4 times, most recently from b0e733d to 446e6d3 Compare March 17, 2022 11:35
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #1142 (364e641) into master (978a311) will increase coverage by 3.15%.
The diff coverage is 93.44%.

❗ Current head 364e641 differs from pull request most recent head 3a1665c. Consider uploading reports for the commit 3a1665c to get more accurate results

@@            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     
Impacted Files Coverage Δ
...etCore/Queries/Expressions/ComparisonExpression.cs 85.00% <0.00%> (-4.48%) ⬇️
...tCore/Queries/Expressions/IncludeChainConverter.cs 100.00% <ø> (ø)
...re/Queries/Expressions/IncludeElementExpression.cs 67.74% <0.00%> (+5.67%) ⬆️
...e/Queries/Expressions/LiteralConstantExpression.cs 68.75% <0.00%> (-4.59%) ⬇️
...ApiDotNetCore/Queries/Expressions/NotExpression.cs 80.00% <0.00%> (-5.72%) ⬇️
...Core/Queries/Expressions/NullConstantExpression.cs 58.33% <0.00%> (-5.31%) ⬇️
...ons/PaginationElementQueryStringValueExpression.cs 75.00% <0.00%> (+35.00%) ⬆️
...etCore/Queries/Expressions/PaginationExpression.cs 52.94% <0.00%> (-3.31%) ⬇️
...xpressions/PaginationQueryStringValueExpression.cs 55.55% <0.00%> (+26.14%) ⬆️
...Core/Queries/Expressions/QueryExpressionVisitor.cs 4.16% <0.00%> (-0.19%) ⬇️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 978a311...3a1665c. Read the comment docs.

@bart-degreed bart-degreed force-pushed the resource-inheritance branch 4 times, most recently from 5b9a64d to b092f2d Compare March 21, 2022 10:10
@bart-degreed bart-degreed marked this pull request as ready for review March 31, 2022 15:26
@bart-degreed bart-degreed requested a review from maurei March 31, 2022 15:26
bart-degreed pushed a commit that referenced this pull request Apr 1, 2022
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Support for inheritance among resources
2 participants