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

{"op":"add"} has no value if the value is undefined. #60

Closed
wolfgang42 opened this issue Dec 8, 2014 · 9 comments
Closed

{"op":"add"} has no value if the value is undefined. #60

wolfgang42 opened this issue Dec 8, 2014 · 9 comments

Comments

@wolfgang42
Copy link

RFC 6902, section 4.1 ("add") says: "The operation object MUST contain a "value" member whose content specifies the value to be added."

jsonpatch.compare() can, however, produce an add operation with no value when the value is undefined.

Test case:

original = {foo: 1};
changed = {foo: 1, bar: undefined};
patch = jsonpatch.compare(original, changed);
jsonpatch.apply(original, patch);

Result: Error: 'value' MUST be defined

@warpech
Copy link
Collaborator

warpech commented Jan 12, 2015

Please note that undefined is not a valid value in JSON (ECMA-404).

Theoretically we could ditch checking via undefined for hasOwnProperty, but that would have performance implications: http://jsperf.com/hasownproperty-vs-undefined so I prefer to stick to the standard

@warpech
Copy link
Collaborator

warpech commented Jan 12, 2015

I think that actually the right solution is that jsonpatch.compare should create:

["op": "remove", "path": "/bar"]

WDYT?

@warpech
Copy link
Collaborator

warpech commented Jan 12, 2015

Related: #32

@sonnyp
Copy link
Contributor

sonnyp commented Jan 12, 2015

See #32 (comment)

@wolfgang42
Copy link
Author

Indeed, this is a duplicate of #36; I missed this because I was looking specifically for {"op": "add"}. Personally, I think that it should not be possible for fast-json-patch to emit an invalid json-patch; in this case, it ought to raise an error informing the programmer that undefined is invalid JSON instead.

@sonnyp
Copy link
Contributor

sonnyp commented Jan 13, 2015

@wolfgang42 👍

@tomalec
Copy link
Collaborator

tomalec commented Apr 6, 2015

I've just added few test cases for it, so it should no longer result in error. Behavior in case of undefined is described at README.md, and at #32

@miyconst
Copy link
Collaborator

miyconst commented Nov 4, 2015

The test cases added by @tomalec does not work without native Object.observe support.
As stated in the issue #82, the Object.observe is no longer supported, so we can't rely on it.

Current implementation of dirty checking creates mirror of object with the following code:

mirror.value = JSON.parse(JSON.stringify(obj));

As mentioned above, undefined is not a valid JSON value, and JSON.stringify ignores such properties.

This leads to:

  • Replacing value of an undefined property generates add patch instead of replace.
  • Removing property with undefined value does not generate any patches.

@miyconst miyconst reopened this Nov 4, 2015
warpech added a commit that referenced this issue Jun 3, 2016
this is a fix for issues #105, #90, #60, #32 and a part of #76 that describes a problem with undefined
all tests pass after this commit
@warpech
Copy link
Collaborator

warpech commented Jun 21, 2016

Fixed in 1.0.0

@warpech warpech closed this as completed Jun 21, 2016
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

5 participants