JsonApiDotNetCore icon indicating copy to clipboard operation
JsonApiDotNetCore copied to clipboard

Add equivalent of AttrCapabilities for relationships

Open bkoelman opened this issue 2 years ago • 0 comments

Is your feature request related to a problem?

This design is based on the community request at https://gitter.im/json-api-dotnet-core/Lobby?at=63199f777ccf6b6d45c52943.

Describe the solution you'd like

Currently, developers can control which operations are allowed declaratively per JSON:API attribute in the following way:

public class Customer : Identifiable<long>
{
    [Attr(Capabilities = AttrCapabilities.AllowFilter | AttrCapabilities.AllowSort)]
    public string Name { get; set; }
}

If no capabilities are set in [Attr], what's set in IJsonApiOptions.DefaultAttrCapabilities (which defaults to AttrCapabilities.All) is used.

The proposal here is to add a similar feature for relationships. It would enable the next example:

public class Customer : Identifiable<long>
{
    [Attr(Capabilities = AttrCapabilities.AllowFilter | AttrCapabilities.AllowSort)]
    public string Name { get; set; }

    [HasOne(Capabilities = HasOneCapabilities.AllowView | HasOneCapabilities.AllowSet)]
    public Address? Address { get; set; }

    [HasMany(Capabilities = HasManyCapabilities.AllowView | HasManyCapabilities.AllowAdd)]
    public ISet<Order> Orders = new HashSet<Order>();
}

The effective relationship capabilities would likewise be determined by falling back to what's set on IJsonApiOptions, defaulting to All. When an incoming request violates these constraints, an appropriate HTTP error is returned for one of the violations (the order is undefined and we won't aggregate all violations; I'm assuming that's how AttrCapabilities works today). Similarly to how AttrCapabilities is implemented, custom logic in resource definitions is not constrained by relationship capabilities. In case AllowView is blocked, we need to take precautions to never return the relationship in any response bodies.

Proposed capabilities to add:

/// <summary>
/// Indicates capabilities that can be performed on a <see cref="HasOneAttribute" />.
/// </summary>
[Flags]
public enum HasOneCapabilities
{
    None = 0,

    /// <summary>
    /// Whether or not GET requests can retrieve the to-one relationship. Attempts to retrieve when disabled
    /// will return an HTTP 400 response.
    /// </summary>
    AllowView = 1,

    /// <summary>
    /// Whether or not POST and PATCH requests can assign or clear the to-one relationship. Attempts to assign
    /// or clear when disabled will return an HTTP 400 response from relationship endpoints and an HTTP 422
    /// response from resource endpoints.
    /// </summary>
    AllowSet = 2,

    All = AllowView | AllowSet
}

/// <summary>
/// Indicates capabilities that can be performed on a <see cref="HasManyAttribute" />.
/// </summary>
[Flags]
public enum HasManyCapabilities
{
    None = 0,

    /// <summary>
    /// Whether or not GET requests can retrieve the to-many relationship. Attempts to retrieve when disabled
    /// will return an HTTP 400 response.
    /// </summary>
    AllowView = 1,

    /// <summary>
    /// Whether or not POST and PATCH requests can replace the to-many relationship. Attempts to replace
    /// when disabled will return an HTTP 400 response from relationship endpoints and an HTTP 422 response
    /// from resource endpoints.
    /// </summary>
    AllowSet = 2,

    /// <summary>
    /// Whether or not POST requests can add to the to-many relationship. Attempts to add when disabled
    /// will return an HTTP 400 response.
    /// </summary>
    AllowAdd = 4,

    /// <summary>
    /// Whether or not DELETE requests can remove from the to-many relationship. Attempts to remove when
    /// disabled will return an HTTP 400 response.
    /// </summary>
    AllowRemove = 8,

    All = AllowView | AllowSet | AllowAdd | AllowRemove
}

Alternative solutions and open questions

  1. List of capabilities We could split Has[One|Many]Capabilities.AllowSet into .Clear and .Assign, or add these while keeping Set (which implies both).

    • .Clear for a to-one relationship means assigning null and for a to-many relationship it means assigning an empty array.
    • .Assign for a to-one relationship means assigning a non-null value and for a to-many relationship it means assigning a non-empty array.

    We can't further distinguish between "assign from empty" vs "replace the existing value" for a to-one relationship, because it would require fetching the currently stored value first, which we're trying to avoid for performance reasons.

    The split could be done at a later time by adding the Clear/Assign members next to the existing Set member. On the other hand, there's no good reason to postpone it if it's considered useful to have them today. I can't think of a reason why the need for them would increase in the future.

  2. HTTP status code Today JsonApiDotNetCore returns 400 for bad requests, but 422 when the problem is in the request body. Because setting a relationship could be done from:

    • POST /customers
    • PATCH /customers/1
    • PATCH /customers/1/relationships/orders

    adhering to the existing rule means to return 422 in the first two cases, but 400 in the last one. The alternative is to break with the rule and always return HTTP 400 on capabilities violations. I'm not sure what the right choice is here.

  3. Meaning of AllowView When blocked, this could either mean hiding the relationship data (type + id), or the relationship itself (data + links). And this raises the question of how it interacts with includes from query strings. Today we have the boolean CanInclude property on HasOne/HasMany relationships, which determines whether ?include= can contain the relationship. Should blocking AllowView implicitly mean to also block ?include=, or should we leave this up to the user? The latter means more flexibility, but at the cost of unintentionally forgetting to set CanInclude = false in addition to blocking AllowView, resulting in the wrong outcome. And should we deprecate CanInclude in favor of adding .AllowInclude to the enumerations?

bkoelman avatar Sep 09 '22 02:09 bkoelman