task: make [Attr] and RequestDeserializer.SetAttributes open to extension

[alastairtree]

QUALITY CHECKLIST

• Changes implemented in code
• Adapted tests
• Documentation updated

Hello!

This is a very minor change to open a couple of types to extension, which allows me to implement some custom deserialisation logic without needing to duplicate large chunks of code. Would you be willing to accept? Please let me know if you need further work to justify such as docs etc.

Unsealing Attr allows me to create my own variant of the Attr attribute that has custom metadata about how that attr should be deserialised. Making SetAttributes virtual allows me to inherit from RequestDeserializer and override SetAttributes so that is can observer my custom attribute and implement the custom deserialisation at the correct moment so that _targetedFields gets populated. Also open to alternative implementations if opening these up is unfavourable.

(My requirement is that my custom Attr attribute triggers the property to be populated from route data rather than from the JSON Body and although that is outside the scope of this PR it might help explain why these are required)

Thanks for the great project.

[bart-degreed]

Hi @alastairtree,

I've discussed your PR with @maurei. Unsealing Attr has several downsides and we believe there are better ways to achieve what you need.

We suggest to create a custom attribute and put that on your resource property, next to Attr. Then in the serialization process, you can intercept and check for the existence of your custom attribute to set the property value. AfterProcessField is an existing extensibility point you can use for that. Attr.Property points to the PropertyInfo of your resource property, which you can query for additional attributes.

[alastairtree]

Yeah, a separate attribute should work fine, so that's fine. However, AfterProcessField does not fire if the attribute is not in the attribute values dictionary as seen below. In my scenario I am trying to populate that Attr property with data that does not originate from the request body, so I need an extensibility point that grants access to all attributes, not just those in the request. Have added a couple of suggestions below - moving AfterProcessField or adding BeforeProcessField as alternatives to exposing SetAttributes s a whole. Let me know what you think.

// in BaseDeserializer.SetAttributes
foreach (var attr in attributes)
{
    // Add BeforeProcessField() extensibility point here?
    if (attributeValues.TryGetValue(attr.PublicName, out object newValue))
    {
        if (attr.Property.SetMethod == null)
        {
            throw new JsonApiSerializationException("Attribute is read-only.", $"Attribute '{attr.PublicName}' is read-only.");
        }

        var convertedValue = ConvertAttrValue(newValue, attr.Property.PropertyType);
        attr.SetValue(resource, convertedValue);

        // we never arrive here if the property is not in the request payload so extension point cannot be used
        AfterProcessField(resource, attr);
    }
    // move AfterProcessField() here?
}

return resource;

[bart-degreed]

It is not clear to us what you're trying to accomplish. For instance, are you trying to save route-data into the database, or are you trying to return extra data in the response that is based on the route?

Can you elaborate on what your use case is? An example with code and request/response would probably help.

[alastairtree]

In my specific case I want to persist an attribute whose value is calculated from the context of the request rather than from the HTTP Body.

For example, say I have a multi-tenant book store app, with 2 customers, alice and bob, where my books API has a route like /api/{tenantSlug}/books. Alice uses the API at /api/alices-store/books and bob uses /api/bobs-boutique/books but in reality these are the same json-api-dotnet controller with some routing.

Then to create a book I could POST like this:

POST /api/alices-store/books HTTP/1.1
Host: localhost:5001
Accept: application/vnd.api+json
Content-Type: application/vnd.api+json

{
    "data": {
        "type": "book",
        "attributes": {
            "name": "1984"
        }
    }
}

Given this request I want to store an attribute, say TenantId with value alices-store that gets populated from the route data dictionary using key tenantSlug and saved into the database with the rest of the book. The resource class might look like

    public class  Book : IIdentifiable<string>
    {
        [Attr(Capabilities = AttrCapabilities.All & ~(AttrCapabilities.AllowChange))]
        public virtual string Id { get; set; }

        [Attr] public string Name { get; set; }

        [RouteDataAttr("tenantSlug")]
        [Attr]
        public string TenantId { get; set; }

        public string StringId { get => Id; set => Id = value; }
    }

I would then be returned from the POST something like:

{
    "links": {
        "self": "/books"
    },
    "data": {
        "type": "books",
        "id": "602b243b5b5aadc6be2e664b",
        "attributes": {
            "name": "1984",
            "tenantId": "alices-store"
        },
        "links": {
            "self":  "/books/602b243b5b5aadc6be2e664b"
        }
    }
}

To be able to achieve the above I need to be able to hook into json-api-dotnet somewhere where I can

• access DI/HttpContext
• ignore/block any http body data POSTed as tenantId
• assign the attribute from routing
• also mark that attribute as modified in ITargetedFields.Attributes so that it gets both persisted and returned from the POST.

AFAIK that only becomes possible by making RequestDeserializer.SetAttributes overridable, or by providing another hook somewhere else granting access during deserialisation, but maybe there is a better way? I tried setting the value in the controller but because ITargetedFields does not get populated the value is not persisted.

(Lets ignore for now the fact that the links generated are imperfect - I am working on a fork of LinkBuilder that uses the MVC route data and LinkGenerator to build correct URLs - would you be interested in a PR if/when I am done?)

[maurei]

Thanks for the elaboration. AfterProcessField isn't the place for this extension because it is a hook that fires for all json:api fields in the request body, so I think it makes sense your route data can't be processed there.

Setting SetAttributes virtual would solve your issue, but I think it can be achieved without too:

public class ExampleCustomRequestDeserializer : IJsonApiDeserializer
{
    private readonly RequestDeserializer _defaultDeserializer;

    public CustomRequestDeserializer(RequestDeserializer defaultDeserializer)
    {
        _defaultDeserializer = defaultDeserializer;

        // Also inject a mechanism here to resolve `TenantId` from route data.
    }

    public object Deserialize(string body)
    {
        var instance = _defaultDeserializer.Deserialize(body);

        // Additional post processing here
        // If it's just book, you could just check for this type here, otherwise consider checking 
        // for an interface like `IHasTenantId` and then perform the additional attribute setting here.


        return instance;
    }
}


// In startup:
services.AddScoped<RequestDeserializer>();
services.AddScoped<IJsonApiDeserializer, ExampleCustomRequestDeserializer>();

I'm interested to know more about your link generator. What problem does your implementation solve that is posing an obstacle with the current implementation? Feel free to open up an issue for this so we can discuss it further there.

Thanks

[bart-degreed]

An alternative (simpler) way to solve this: set Book.TenantId from IResourceWriteRepository.GetForCreateAsync, then you don't need a custom attribute.

Anyway, this sounds like a good use case for adding "Pre-processing incoming resources (POST/PATCH)" to #934.

[alastairtree]

@maurei I had a go at wrapping RequestDeserializer with a custom implementation as you suggested but it gets really awkward because you still need to put the Attr into _targetedFields to mark it as modified which means you need access to the ResourceObject or the Document, neither of which are available to the outer code, so you end up either deserialising it twice, or wrapping and inheriting RequestDeserializer to get access to the inner workings. Both are really hacky and no better than just rewriting the whole class.

@bart-degreed implementing in IResourceWriteRepository looks like it would work, but putting custom http de-serialisation logic into the repository feels like the wrong place to me, and a violation of the SRP?

I have copied and forked the whole of RequestDeserializer and BaseSerialiser(and TypeHelper) into a new class/project just to override the small part I need and it works. Not ideal but will tide me over until a supported extension point is added in #934.

Thanks for the advice, will close for now and make do without a dedicated extension point.

[bart-degreed]

What I suggested is simpler because you don't need to look at ITargetedFields at all. And you don't even need to expose "tenantId" as a resource attribute, which seems preferable to me because it is just an implementation detail that clients should not be concerned with.

In the end, your goal seems to be making the TenantId database column part of the functional key for books. So you'll need to ensure it gets assigned properly on creation. And it should not be modifiable by clients, once set. You'll also need to ensure the TenantId filter criteria is added to all book queries, otherwise alices-store can get/modify/delete books belonging to bobs-boutique. The filter criteria can be done using Global Query Filters. But because JADNC does not fetch the resource on delete, you'll need to do that yourself to prevent cross-tenant deletions.

This is not that hard to accomplish with SOLID principles in mind:

public interface ITenantProvider
{
    string GetTenantId();
}

public class RoutingTenantProvider : ITenantProvider
{
    private readonly IHttpContextAccessor _httpContextAccessor;

    public RoutingTenantProvider(IHttpContextAccessor httpContextAccessor)
    {
        _httpContextAccessor = httpContextAccessor;
    }

    public string GetTenantId()
    {
        var httpContext = _httpContextAccessor.HttpContext;
        return (string)httpContext.GetRouteValue("tenantSlug");
    }
}

public interface IHasTenant
{
    string TenantId { get; set; }
}

public class Book : Identifiable, IHasTenant
{
    public string TenantId { get; set; }
}

public class AppDbContext : DbContext
{
    public AppDbContext(DbContextOptions<AppDbContext> options, ITenantProvider tenantProvider)
        : base(options)
    {
        _tenantProvider = tenantProvider;
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Book>()
            .HasQueryFilter(book => book.TenantId == _tenantProvider.GetTenantId());
    }
}

public class TenantAwareRepository<TResource> : EntityFrameworkCoreRepository<TResource>
    where TResource : class, IIdentifiable<int>
{
    private readonly ITenantProvider _tenantProvider;
    private readonly IJsonApiRequest _request;
    private readonly DbContext _dbContext;

    public TenantAwareRepository(ITenantProvider tenantProvider, IJsonApiRequest request,
        IQueryLayerComposer queryLayerComposer, ITargetedFields targetedFields, IDbContextResolver contextResolver,
        IResourceGraph resourceGraph, IResourceFactory resourceFactory,
        IEnumerable<IQueryConstraintProvider> constraintProviders, ILoggerFactory loggerFactory)
        : base(targetedFields, contextResolver, resourceGraph, resourceFactory, constraintProviders, loggerFactory)
    {
        _tenantProvider = tenantProvider;
        _request = request;
        _dbContext = contextResolver.GetContext();
    }

    public override async Task<TResource> GetForCreateAsync(int id, CancellationToken cancellationToken)
    {
        var resource = await base.GetForCreateAsync(id, cancellationToken);

        if (typeof(TResource).IsAssignableFrom(typeof(IHasTenant)))
        {
            string tenantId = _tenantProvider.GetTenantId();

            var resourceWithTenant = (IHasTenant) resource;
            resourceWithTenant.TenantId = tenantId;
        }

        return resource;
    }

    public override async Task DeleteAsync(int id, CancellationToken cancellationToken)
    {
        if (typeof(TResource).IsAssignableFrom(typeof(IHasTenant)))
        {
            string tenantId = _tenantProvider.GetTenantId();

            var resourceWithTenant = await _dbContext.Set<TResource>()
                .Where(resource => resource.Id == id && ((IHasTenant)resource).TenantId == tenantId)
                .FirstOrDefaultAsync(cancellationToken);

            if (resourceWithTenant == null)
            {
                throw new ResourceNotFoundException(_request.PrimaryId, _request.PrimaryResource.PublicName);
            }

            _dbContext.Remove(resourceWithTenant);
            await base.SaveChangesAsync(cancellationToken);
        }
        else
        {
            await base.DeleteAsync(id, cancellationToken);
        }
    }
}

and then register from Startup.ConfigureServices, after the call to .AddJsonApi():

services.AddSingleton<ITenantProvider, RoutingTenantProvider>();

services.AddScoped(typeof(IResourceRepository<>), typeof(TenantAwareRepository<>));
services.AddScoped(typeof(IResourceReadRepository<>), typeof(TenantAwareRepository<>));
services.AddScoped(typeof(IResourceWriteRepository<>), typeof(TenantAwareRepository<>));

Note I wrote this code without running it, you it may contain some bugs. But it should demonstrate the general idea. In my opinion this is a lot cleaner than messing with the serialization layers.

[alastairtree]

That's really kind of you to give such a detailed suggestion and example code and now I think I understand much better what you meant. Agree it looks like a workable sensible solution. Will give it a go.

The customised delete logic had come up for me before as well, where I had to put a base query to enforce permissions on all queries, so u know what you mean about having to do some extra work to enforce it.
It might make for a good optional extension point or change to accommodate these sorry of use cases. The non query check on delete is a good performant default setting, but if a user is applying some kind of global query filter then maybe the delete logic should also enforce it?

[bart-degreed]

As an extension point I don't see how that would work. How would JADNC know the user is applying 'some kind of global query filter'? But more importantly, how would JADNC know that the global filter concerns tenant filtering, as opposed to soft-deletion or something else entirely?

If you write up the code to be added to JADNC, then perhaps I better understand what you mean.

[bart-degreed]

Let's continue this conversation at #952.