Starcounter-Jack / JSON-Patch

Lean and mean Javascript implementation of the JSON-Patch standard (RFC 6902). Update JSON documents using delta patches.
MIT License
1.78k stars 215 forks source link

Replace mistreats properties that are set to "undefined" #279

Closed NicBright closed 3 years ago

NicBright commented 3 years ago

From the JSON patch spec (Section 4.3): https://datatracker.ietf.org/doc/html/rfc6902#section-4.3

Here it says: "The target location MUST exist for the operation to be successful."

Now I've run into the following issue which is best explained with a screenshot from my debugging session: image

As a consequence, when the code enters validation (line 198) I then get an OPERATION_PATH_UNRESOLVABLE error: image

I think the check on line 191 should rather be done using Object.hasOwnProperty() ... what do you think?

Starcounter-Jack commented 3 years ago

Tricky. A property set to undefined falls outside the spec as undefined is not a valid JSON value (https://datatracker.ietf.org/doc/html/rfc4627). I.e. the "shaden" value is not really well defined with respect to RFC6902 or RFC4627 .

The fact that a property is not inherited is not really relevant with respect to JSON Patch, so I don't believe that it is the appropriate way. It might risk properties that do exist (due to inheritance) to be wrongly identified as non existing.

If you believe that the arguing is lacking or can provide further insight about the proper way to treat a property set to 'undefined', I am willing to reassess.

Thanks

Starcounter-Jack commented 3 years ago

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

I do agree that it could be argued that the target do exist, but a target does also exist if the property is defined by means rendering hasOwnProperty() to return false (such as for example getters and setters).

There are basically four ways of checking if a property exists. You could use hasOwnProperty, you could use in, you could see if the typeof yields undefined and you could evaluate the property and compare it to the undefined value.

If you stick to properties existing in Javascript, regardless of inheritance or if the property is defined using setters and getters, you should use the in keyword or check of the undefined type or value.

The problem with using in is that it would require a property set to undefined to be treated as defined, which has two problems:

  1. It is clearly a semantic streach to view undefined properties to be defined properties. If undefined is to mean that the property exists but the value of it is "nothing" (the empty set mathematically speaking), the JSON spec defines null for this. In fact, this is the only such value allowed in JSON. Thus, it can be argued that setting a property to undefined means that you wish it to be treated as such.
  2. Changing this behaviour would be a breaking change to a lot of libraries and code bases currently using fast JSON patch. (this could be mitigated by configuration however).

Closing, but can be reopened