PATCH operations don't return updated copy of resource if resource was changed by custom business logic

[DumpsterDoofus]

Description

Not sure if this is a bug or expected behavior, but figured I'd ask. I have a custom IResourceService that modifies some data when a PATCH request comes through. When I send a PATCH request, the updated resource does not come back in the response, even though the updated resource has data that the client has never seen before.

I sort of expected that during a PATCH, if a resource has been updated with data that the client has never seen before, then the updated resource would be sent in the response so that the client was aware that something had changed.

Example code:

public class ThingResourceService : IResourceService<Thing, Guid>
{
    private readonly IResourceService<Thing, Guid> _resourceService;
    private readonly ITargetedFields _targetedFields;
    private readonly IResourceGraph _resourceGraph;

    public ThingResourceService(
        IResourceService<Thing, Guid> resourceService,
        ITargetedFields targetedFields,
        IResourceGraph resourceGraph)
    {
        _resourceService = resourceService;
        _targetedFields = targetedFields;
        _resourceGraph = resourceGraph;
    }

    public Task<Thing> UpdateAsync(Guid id, Thing thing)
    {
        HandleUpdate(thing);
        return _resourceService.UpdateAsync(id, thing);
    }

    // Other interface methods

    private void HandleUpdate(Thing thing)
    {
        thing.LastModifiedOn = DateTimeOffset.UtcNow;

        // TODO: This is a workaround for https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/536. If you change data on an entity in an extensibility point (like an IResourceService or a JsonApiController), the changes are only saved if the framework knows the field was changed. The framework knows a field is changed if the field was sent in the request, but if the field was not sent in the request, then the framework does not track changes to it, and any changes made to the field here will be silently lost. The workaround is to explicitly tell the framework that the fields were changed.
        // Once this is fixed, delete the below code.
        var attrAttribute = _resourceGraph.GetAttributes<Thing>(g => new { g.LastModifiedOn })
            .Single();
        if (!_targetedFields.Attributes.Contains(attrAttribute))
        {
            _targetedFields.Attributes.Add(attrAttribute);
        }
    }
}

The response to a PATCH operation:

{
    "links": {
        "self": "https://localhost/..."
    },
    "data": null
}

An empty response is returned regardless of whether the client did not send lastModifiedOn in the request, or did send it in the request and the client value was overwritten by the custom logic.

As a workaround, you can do a GET after each PATCH.

Environment

• JsonApiDotNetCore Version: v4.0.0-alpha5

[bart-degreed]

Hi @DumpsterDoofus , thanks for bringing this up.

In all honestly: we can (and should) provide a better way to set modification dates.
What is holding us back is a lack of use cases to properly design the feature in broad sense. We'd like to get it right from the start.

What we offer today:

• Override ResourceHooksDefinition.BeforeUpdate
  Resource hooks were added in v4.x before I joined the project. Personally I believe they are a big mistake for various reasons and I would not recommend anyone using them. They are marked experimental and disabled by default.
• Override ResourceService.UpdateAsync
  There is currently no clean way to intercept at the right point so that change tracking works properly (even when setting targetted fields). You need to copy/paste the full method body, which is problematic due to usage of internal types.
• Override ResourceRepository.UpdateAsync
  This works by applying your changes on the resourceFromDatabase parameter, before calling the base implementation. Except for fields you are changing which are also in the request body: they overrule your changes.

How it works today:

1. Service registers the subset of attributes coming from request body (called the 'request version' in change tracking)
2. Service fetches the stored resource via repository (the 'initial version' in change tracking)
3. Service calls repository Update method, passing the stored version and request version
4. Repository copies the attributes listed in targetted fields from request version into stored version (relationships omitted here for brevity)
5. Repository persists changes in stored version
6. Service re-fetches the resource via repository (the 'final version' in change tracking)
7. Changes are detected: When an attribute in the 'final' version differs from the 'request' version -or- an attribute in the 'final' version was not in the request and differs from the 'initial' version, then side effects have occured and we return the resource.

The best place to intercept would be between 4 and 5: By doing the changes on the stored version, they get persisted and the change tracker will properly detect them. And optionally, code could inspect targetted fields and choose not to overwrite values from the request.

Solutions:

• Provide a protected virtual method in repository that is invoked between 4 and 5. This may be counter-intuitive, as currently users expect to put their 'business logic' in the service layer, agnostic of persistence technology. On the other hand, one could argue that the service layer only concerns json:api concepts (attributes and relationships), so updating non-exposed entity properties on CLR objects should be done at the repository layer.
• Split the repository Update into pre/post methods, so we can have a protected virtual method at the service layer.
• Extend IResourceDefinition with pre/post Create/Update/Delete methods that are invoked by service and/or repository, depending on use case.

We've recently added the same change tracking mechanism for POST requests, resulting in similar challenges.

Eventually we'd like to implement the last option, so we are hesitant to add things now we'll be removing later. On the other hand, we feel your pain and would like to provide something today that somehow enables your use case.

Suggestions are welcome!

[bart-degreed]

Today we merged changes that make it at least possible to accomplish what you need, though we may provide better ways in the future.

Using the example below, change tracking works properly, you don't need special handling for targeted fields and can change both exposed and non-exposed properties. The downside is you need to copy/paste the contents of the repository UpdateAsync method and put your own changes in-between. Example:

public class ArticleRepository : EntityFrameworkCoreRepository<Article>
{
    private readonly ITargetedFields _targetedFields;
    private readonly IResourceFactory _resourceFactory;
    private readonly DbContext _dbContext;

    public ArticleRepository(ITargetedFields targetedFields, IDbContextResolver contextResolver,
        IResourceGraph resourceGraph, IResourceFactory resourceFactory,
        IEnumerable<IQueryConstraintProvider> constraintProviders, ILoggerFactory loggerFactory)
        : base(targetedFields, contextResolver, resourceGraph, resourceFactory, constraintProviders, loggerFactory)
    {
        _targetedFields = targetedFields;
        _resourceFactory = resourceFactory;
        _dbContext = contextResolver.GetContext();
    }

    public override async Task UpdateAsync(Article resourceFromRequest, Article resourceFromDatabase)
    {
        using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);

        foreach (var relationship in _targetedFields.Relationships)
        {
            var rightResources = relationship.GetValue(resourceFromRequest);

            AssertIsNotClearingRequiredRelationship(relationship, resourceFromDatabase, rightResources);

            await UpdateRelationshipAsync(relationship, resourceFromDatabase, rightResources, collector);
        }

        // Start: Custom business logic (overwritten by request data)
        resourceFromDatabase.IsInStock = true;
        // End: Custom business logic (overwritten by request data)

        foreach (var attribute in _targetedFields.Attributes)
        {
            attribute.SetValue(resourceFromDatabase, attribute.GetValue(resourceFromRequest));
        }

        // Start: Custom business logic (overwrites request data)
        resourceFromDatabase.LastModifiedAt = DateTimeOffset.UtcNow;
        // End: Custom business logic (overwrites request data)

        await SaveChangesAsync();
    }
}

Hope this helps.

[bart-degreed]

Closed in favor of #934.