"Replace" operations should remove the existing value before adding the new one

[TAGC]

Summary

I'm facing an issue that relates to a subtle technicality of the JSON Patch "replace" operation. In one of my model classes, I'm trying to enforce a requirement that a client does not try to register two "definitions" (instances of another model class) with the same name.

However, it should be entirely possible for a client to replace an existing definition with another one using the same name, and JSON Patch does not currently allow this, at least for properties being handled by DynamicObjects.

Expected Behaviour

I expect that submitting JSON Patch operations to add two values with the same path will result in an error (when trying to enforce uniqueness).

This means, for example, that the following is considered invalid, and will cause an error to be returned to the client:

{"op":"add", "path":"candb.foo", "value": {"..."}}
{"op":"add", "path":"candb.foo", "value": {"..."}}

However, I expect that if the client is simply trying to replace an existing definition, that should be considered okay:

{"op":"add", "path":"candb.foo", "value": {"..."}}
{"op":"replace", "path":"candb.foo", "value": {"..."}}

Actual Behaviour

I've noticed that my application doesn't permit this either. An error will occur even if the client is trying to replace the value at a particular path.

Further Details

I created an integration test to isolate this bug. I've narrowed the issue down to the TryReplace method in DynamicObjectAdapter:

public bool TryReplace(
    object target,
    string segment,
    IContractResolver contractResolver,
    object value,
    out string errorMessage)
{
    if (!TryGetDynamicObjectProperty(target, contractResolver, segment, out var property, out errorMessage))
    {
        return false;
    }

    if (!TryConvertValue(value, property.GetType(), out var convertedValue))
    {
        errorMessage = Resources.FormatInvalidValueForProperty(value);
        return false;
    }

    if (!TrySetDynamicObjectProperty(target, contractResolver, segment, convertedValue, out errorMessage))
    {
        return false;
    }

    errorMessage = null;
    return true;
}

Compare this against the implementation of TryAdd:

public bool TryAdd(
    object target,
    string segment,
    IContractResolver contractResolver,
    object value,
    out string errorMessage)
{
    if (!TrySetDynamicObjectProperty(target, contractResolver, segment, value, out errorMessage))
    {
        return false;
    }

    errorMessage = null;
    return true;
}

In both cases, the only mutating method that the object adapter invokes is TrySetDynamicObjectProperty. This means that there is no way to distinguish between "add" operations and "replace" operations in the object being adapted itself.

I use a class called DynamicDeserializationStore to handle JSON patch operations relating to adding/removing/changing definitions. This class extends DynamicObject and so gets handled by this adapter. Currently, both "add" and "replace" operations will call into public void Add(string key, object value), which through a small call chain will lead to AddDefinition:

private void AddDefinition(string name, ICanMessageDefinition definition)
{
    Raise.ArgumentException.If(
        TypedDefinitions.ContainsKey(name),
        nameof(name),
        "Cannot add definition to CAN database as a definition with that name already exists");
    // ...
}

This is where an exception is raised and the problem becomes apparent.

Proposed Solution

According to the JSON Patch specification (RFC 6902):

The "replace" operation replaces the value at the target location with a new value. The operation object MUST contain a "value" member whose content specifies the replacement value. ... This operation is functionally identical to a "remove" operation for a value, followed immediately by an "add" operation at the same location with the replacement value.

To be consistent with the specification, I think TryReplace should attempt to remove the existing value and then add the new value:

public bool TryReplace(
    object target,
    string segment,
    IContractResolver contractResolver,
    object value,
    out string errorMessage)
{
    if (!TryRemove(target, segment, contractResolver, out errorMessage))
    {
        return false;
    }

    if (!TryConvertValue(value, property.GetType(), out var convertedValue))
    {
        errorMessage = Resources.FormatInvalidValueForProperty(value);
        return false;
    }

    if (!TrySetDynamicObjectProperty(target, contractResolver, segment, convertedValue, out errorMessage))
    {
        return false;
    }

    errorMessage = null;
    return true;
}

The way that TryRemove is currently implemented, it will try to set the dynamic object property to null. This would be great for me, as DynamicDeserializationStore will treat this as a request to remove the value:

public bool TryUpdateValue(string name, object value)
{
    switch (value)
    {
        case JObject jObject:
            _storeValue(name, jObject.ToObject<T>());
            return true;

        case T tObject:
            _storeValue(name, tObject);
            return true;

        case null:
            _removeValue(name);
            return true;

        default:
            return false;
    }
}

This should then allow the subsequent operation to add a new definition at the same path to succeed without error.

[TAGC]

By the way @jbagga, your implementation of JSON patch support for dynamic objects has been working out really well (apart from this minor corner case). I noticed that the new implementation respects the JSON contract resolver too. Thanks again 👍

[TAGC]

Also, this is how it was originally implemented (treating "replace" as a "remove" followed by an "add").

[tannergooding]

@Eilon, @jbagga. I believe this should be re-opened.

The spec does not specify how the replace function should be implemented, but it does specify how the replace function should behave and it looks like @TAGC has clearly demonstrated a case where the current replace implementation is no longer functionally equivalent to a "remove", followed immediately by an "add".

From RFC6902:

The "replace" operation replaces the value at the target location with a new value. The operation object MUST contain a "value" member whose content specifies the replacement value.

The target location MUST exist for the operation to be successful.

For example:

{ "op": "replace", "path": "/a/b/c", "value": 42 }

This operation is functionally identical to a "remove" operation for a value, followed immediately by an "add" operation at the same location with the replacement value.

[Eilon]

@mkArtakMSFT can you take a look?

[mkArtakMSFT]

Sure, this seems like a reasonable ask. While it doesn't align with our current priorities, we're marking it with up-for-grabs label so community can submit a PR for this. We'll happily review it.

[TAGC]

@mkArtakMSFT For what it's worth, I did submit a PR for this already. https://github.com/aspnet/JsonPatch/pull/111

[mkArtakMSFT]

@TAGC, can you please submit a new PR, as that one is closed and I can't reopen it?

[TAGC]

@mkArtakMSFT Just re-submit the original commit I made, or rebase my changes first? If the latter, should it be on master? "dev" seems to have gone.

[mkArtakMSFT]

@TAGC , please submit a PR targeting the master branch. We'll do the integrations from there as necessary.

[mkArtakMSFT]

This issue was moved to aspnet/AspNetCore#3623