[RFC] Simple custom implementation of UpdateAsync in service layer not possible due to tight coupling with model Attributes

[maurei]

The issue

• Tight coupling between the repository layer and the the model attributes (Attr, HasOne, etc ... ) makes it hard to add very straight forward business logic in a custom service layer

Why?

The following internal snippet from DefaultEntityRepository shows how updating resources is currently reliant on JsonApiContext and the Attr attribute. The JsonApiContext property AttributesToUpdate contains a list of the AttrAttributes of updated properties of the model, which reflects update request that was sent to the application. These are then used to update values on the entity loaded from the database:

foreach (AttrAttribute attr in _jsonApiContext.AttributesToUpdate.Keys)
    attr.SetValue(databaseEntity, attr.GetValue(updatedEntity));

Note that the reliance here on JsonApiContext is fine, because reflectively inspecting the updatedEntity object and checking for any updated properties would not work. This is because it is not possible to distinguish between a null-value (of eg a string property) that

• was set to null as a result of an update request,
• is just null because it was never instantiated by the deserializer, which means the property wasn't targeted by the request

The problem is the reliance on AttrAttribute here. To see why, consider the following model

public class Company : Identifiable<int>
{
    // This exposed property will be targeted by our example request
    [Attr("company-name")]
    public string Name { get; set; }
    // This exposed property will NOT be targeted by our example request,
    // but we will update in our custom service layer containing business logic
    [Attr("foobar")]
    public string FoobarExposed { get; set; }
    // This property is not exposed and as such can never be targeted by a 
    // request, but we still want to update it using business logic
    public string FoobarInternal { get; set; }
}

Assume a request is updating only the exposed company-name attribute, and we'll set FoobarExposed and FoorbarInternal as a part of the business logic in our custom service layer:

public class CompanyService : EntityResourceService<Company>
{
    private readonly IJsonApiContext _jsonApiContext;
    private readonly ResourceGraph _graph;
    public CompanyService( ... ) : base( ... ) { }
    public override Task<Company> UpdateAsync(int id, Company resource)
    {
        // This alone will not work: we need to add 
        // FoobarExposed to _jsonApiContext.AttributesToUpdate
        // and for that we need to access the `[Attr("foobar")]`
        resource.FoobarExposed = "foobar"

        // So we get `[Attr("foobar")]` from the resource graph. This is inconvenient!
        var foobarExposedAttrAttribute = _graph.GetContextEntity<Company>().Attributes.Single( ... );
        _jsonApiContext.AttributesToUpdate.Add(foobarExposedAttrAttribute, ...);


        // This alone will again not work, but now there isn't even a `[Attr]`
        // that we can access from the resource graph because it isn't set in the model
        resource.FoobarInternal = "internal foobar"

        // so we need to instantiate it manually and set a bunch of internal properties using 
        // reflection. This is NOT ok!
        var foobarInternalDummyAttrAttribute = DoABunchOfSmellyReflection( ... );
        _jsonApiContext.AttributesToUpdate.Add(foobarInternalDummyAttrAttribute, ...);

        return base.UpdateAsync(resource);
    }
}

My comments in this example should illustrate how hard it is for a developer to do such a seemingly straight forward thing. My main concern with it is that the developer needs to know about the internals of JsonApiDotNetCore to be able to custom update a property, because the dev needs to know:

• about the internal usage of jsonApiContext.AttributesToUpdate
• about how to instantiate a AttrAttribute and which internal properties to set.

Solution

We need to get rid of the reliance of the model attributes in the repository layer. These attributes should only be used to configure which properties are exposed externally, and therefore the DeSerializer and Serializer should care about them, but they shouldn't be required in the service/repository layer. To that end, I believe we need to consider two things

• Replace the AttrAttribute in jsonApiContext.AttributesToUpdate with either
  • native PropertyInfos to internally communicate which properties are targeted
    • this will significantly complicate things to support entity-resource separation. But I am really wondering what the usecase is for this feature anyway? Do we need this fluff?
  • a new Attribute class that is required on EVERY property that is used by JADNC, so that we can easily communicate internally
    • feels very verbose and feels like it shouldn't be strictly necessary
• Create a helper method that allows the developer to mark properties as updated without exposing the internal workings of jsonApiContext.AttributesToUpdate, something like jsonApiContext.MarkAsUpdated( company => company.FoobarInternal )

[wisepotato]

So one of the things we should note here that could be a solution is a " entity core"esque implementation of the dbcontext for our resourcegraph. An example:

contextGraph.Build<Company>(resource => {
  resource.HasManyThrough(c => c.Users)
    .Through(c => c.CompanyUsers);
  resource.HasAttribute(c => c.Name).IsImmutable();
});

[bart-degreed]

Meanwhile, a solution is described at #876 (comment). There's still room for improvement, but at least it is not so painful anymore.

@maurei would you be okay with closing this? We can keep the linked issue open to track building a better experience.