Response when updating attributes of resource not conform spec

[maurei]

The specs for updating a resource state:

200 OK
If a server accepts an update but also changes the resource(s) in ways other than those specified by the request (for example, updating the updated-at attribute or a computed sha), it MUST return a 200 OK response. The response document MUST include a representation of the updated resource(s) as if a GET request was made to the request URL.

A server MUST return a 200 OK status code if an update is successful, the client’s current attributes remain up to date, and the server responds only with top-level meta data. In this case the server MUST NOT include a representation of the updated resource(s).

JsonApiDotNetCore does not comply with the piece in bold text. Currently, after an update the response for the update pipeline always returns a full representation of the resource on the server.

In the default implementation of the pipelines, how a resource will change will always be fully specified by the request, in which case the server must never return an updated representation.

Users can easily add business logic that introduces additional changes, and only in this case the server must return an updated representation.

Implementing this part of the spec is challenging because it is nearly impossible for JADNC to detect if the business logic as introduced by the developer does anything like this. Hence it would probably be a more feasible approach to create a service that is responsible for signalling this to the internals of the framework. Such service would then explicitly be called by the developer.

Perhaps this issue can be tackled together with #536.
Eg we could introduce a helper service to take care of updates other than those specified by the request

updatedArticle.updatedAt = DateTime.Now();
_updateHelper.MarkAsUpdated(a => a.updatedAt);

the _updateHelper can then signal this to the repository layer which would solve #536, as well as to the serialization layer which would solve the current issue. This way we wont have to clutter the interface of the repository layer and could custom serialization layer opt out on such signal.

[bart-degreed]

One way to deal with this in a generic way could be the next flow:

1. Fetch full resource from database (version A)
2. Apply changes (partial field set) from patch request on A and save (version B)
3. Re-fetch the full resource from database (version C)
4. diff between versions A and C, after converting both to JSON (string-based equality comparison)

[maurei]

I've considered that. It would only work if additional changes apart from those that are encoded in the PATCH request are executed directly on DbContext by the dev. This wouldn't work if we were to do a simple update of a property within a custom service layer like this:

public override Task<Dummy> UpdateAsync(int id, Dummy resource)
{
    resource.Prop2 = null
    return base.UpdateAsync(id, resource);
}

This scenario isn't covered because the repository layer will not be able to distinguish that additional null value from the default value of a string (which results from instantiation during deserialization) versus a devs manual update of that property

[bart-degreed]

I think your concern is that derived classes should be able to execute additional changes on the entity that were not in the patch request. We can solve that like this:

// in DefaultResourceRepository
public virtual void BeforeUpdate(TResource entityToUpdate)
{
}

public virtual async Task<TResource> UpdateAsync(TResource entity)
{
    var databaseEntity = await Get(entity.Id).FirstOrDefaultAsync();
    if (databaseEntity == null)
    {
        throw new Exception("Resource not found...");
    }

    string versionA = JsonConvert.SerializeObject(databaseEntity);

    BeforeUpdate(entity);

    foreach (var attribute in _targetedFields.Attributes)
        attribute.SetValue(databaseEntity, attribute.GetValue(databaseEntity));

    // Relations update skipped for brevity

    await _context.SaveChangesAsync();

    databaseEntity = await Get(entity.Id).FirstOrDefaultAsync();

    string versionC = JsonConvert.SerializeObject(databaseEntity);

    return versionA == versionC ? null : databaseEntity;
}

// in DummyRepository
public override void BeforeUpdate(Dummy entityToUpdate)
{
    entityToUpdate.Name = "Jill";
}

[maurei]

That would indeed work from a technical point of view because this bit would be implemented in the repository layer:

// in DummyRepository
public override void BeforeUpdate(Dummy entityToUpdate)
{
  entityToUpdate.Name = "Jill";
}

But I think there is an architectural issue with that. I believe that in the service-repository pattern, this type of business logic does not belong in the repository layer but in the service layer instead. Mind experiment to support that: if one were to migrate from SQL to NoSQL, i.e. switch-out the repository layer, business logic like entityToUpdate.Name = "Jill"; should remain invariant under that. Such assignment shouldn't depend on eg. SQL vs NoSQL.

This is also why the ResourceHooks have been implemented to work on the service layer rather than the repository layer. Also, I think it would be problematic to have a hook-like method like BeforeUpdate in the repository layer which one would have to implement to achieve this functionality, in parallel to having all the ResourceHooks functionality out there. Rather one should be able to plug this kind business logic right in the latter (recurring statement: assuming here that ResourceHooks has been improved to work in high-performance environments, which we should work on).

[bart-degreed]

But I think there is an architectural issue with that.

That makes perfect sense to me. I actually expected this logic to be in the service layer, so was a bit surprised to find it in the repository. The reason may be the relationship loading, attaching etc. that I skipped over in my example. Do you think that can be moved to the service layer in a good way?

This is also why the ResourceHooks have been implemented...

Yes that sounds like a good place to put such logic, once hooks are fixed. To prevent making this issue dependent on resource hooks, would you be okay with implementing the diffing in the repository for now? Alternatively, can you explain how to move this to the service layer?

[maurei]

so was a bit surprised to find it in the repository.

Very interesting: I have never considered if out-of-the-box assignment of properties and relationships ((i.e. as indicated by POST/PATCH request) ) should be part of the core service implementation rather than the repository layer. That would get rid of a lot of complexity in this problem for sure!

I need to look into if that makes sense from an architectural point of view, but also from a technical stance considering that, indeed, loading related data is definitely a responsibility of the repository. That could complicate things significantly with things like implicit removal of relationships. But perhaps such complexity would be still easier to deal with than the one we're facing right now.

Alternatively, can you explain how to move this to the service layer?

With respect to this, if the approach above doesn't turn out to be feasible: I think we would end up with something like

updatedArticle.updatedAt = DateTime.Now();
_updateHelper.MarkAsUpdated(a => a.updatedAt);

as described in the top post of this thread

would you be okay with implementing the diffing in the repository for now

I am a little hesitant baking such functionality into the public API knowing that we'll remove it once hooks are improved. Perhaps as a dirty approach forward we could consider adding it as private method so it can be called with reflection and staying away from a breaking change once the hooks are fixed.

[bart-degreed]

Update: we discussed to move all logic except:

    foreach (var attribute in _targetedFields.Attributes)
        attribute.SetValue(databaseEntity, attribute.GetValue(databaseEntity));

    // Relations update skipped for brevity

    await _context.SaveChangesAsync();

from the repository to the service layer (including the diffing).

The question on how to inject custom business logic is something we're going to resolve at a later time. Either by providing an extensibility point in the service layer, or using resource hooks, or both.

[maurei]

A caveat: the logic in the repo that takes care of the implicit removal of relationships:

/// loads databasePerson.todoItems
LoadCurrentRelationships(databaseEntity, relationshipAttr);
/// trackedRelationshipValue is either equal to updatedPerson.todoItems,
/// or replaced with the same set (same ids) of todoItems from the EF Core change tracker,
/// which is the case if they were already tracked
object trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, updatedEntity, out _);
/// loads into the db context any persons currently related
/// to the todoItems in trackedRelationshipValue
LoadInverseRelationships(trackedRelationshipValue, relationshipAttr);

needs to be applied before assigning new relationship value to the entity. Something to keep in mind when moving the logic over.

[bart-degreed]

Yes, that is what I had in mind. My comment:

// Relations update skipped for brevity

represents that block of code, including the foreach statement around it.

[bart-degreed]

I just realized this is not going to work. Because aside from database triggers, calculated properties may also update additional fields. For example:

public class Person : Identifiable
{
    private string _name;

    [Attr]
    public string Name
    {
        get => _name;
        set
        {
            if (value != _name)
            {
                LastModifiedAt = DateTime.Now;
                _name = value;
            }
        }
    }

    [Attr] 
    public DateTime LastModifiedAt { get; set; }
}

So we need to verify that all exposed properties that are not in _targetedFields remain unchanged. Would that be a top-level scan only, or can it be nested?