Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
NicBright opened this issue Aug 11, 2021 · 2 comments
Closed

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

NicBright opened this issue Aug 11, 2021 · 2 comments

Comments

@NicBright
Copy link

NicBright commented Aug 11, 2021

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
Copy link
Owner

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants