Undefined properties cause an exception to be thrown

[davidpolberger]

Consider the following code:

var object = {
  test: undefined
};

var observer = jsonpatch.observe(object);
var result = jsonpatch.generate(observer);

JSON-Patch throws a SyntaxError exception when trying to generate the patch at line 421 of json-patch-duplex.js (in the function _generate):

mirror[key] = JSON.parse(JSON.stringify(obj[key]));

obj[key] is undefined, causing JSON.stringify() to return undefined, which JSON.parse() cannot handle ("Unexpected token u").

The expected behavior is that no patches should be generated, as nothing has changed.

[davidpolberger]

We are using 1c5ef73 internally. This commit fixes the bug trivially, by simply not reporting on properties that are undefined. This may or may not be the appropriate long-term fix, though it does work for our needs. If it is desirable to track the addition of undefined properties in some context, a more thorough solution should be sought. I am thus not adding a pull request for this fix.

[jamesplease]

Something like this should do the trick:

var clone = obj[key];
mirror[key] = clone ? JSON.parse(JSON.stringify(obj[key])) : clone;

We only need to create the clone if it's an object; otherwise it's passed by value and we're fine. And objects are always truthy.

//cc @Starcounter-Jack @warpech

[warpech]

We could handle it somehow in generate step. My idea is that when a change to undefined is detected, jsonpatch.generate reports it as a remove operation. What do you think?

[jamesplease]

@sonnyp has a convincing argument that we should not handle invalid JSON values. I've been persuaded. What do you think, @warpech?

[sonnyp]

@jmeas We shouldn't handle undefined in a JSON Patch document (see #32), however not sure what to do about undefined in a JavaScript object. I think best option here is to let the user configure behavior. throw error, ignore, remove, accept, ... with default of throwing an error.

[jamesplease]

Ah, okay. Sounds good 👍

[tomalec]

I've just added a description at README.md, and bunch of tests.

Basically:

1. For valid JSON document, we will generate only valid JSON Patches,
2. For invalid JSONs (ones with undefined), we will handle it gently. .apply, .observe, .compare, .generate will use and produce JSON Patches with undefined as with any other falsy value,
3. We will not validate entire JSON document in question,
4. If you are not sure if your JSON is valid, and you'd like to make sure that produced JSON Patch is, use .validate.

[garthk]

Is it too late to reconsider?

• The results don't conform to RFC6902 after being run through JSON.stringify
• The results are least astonishing to the wrong community: people who never touch JSON, despite the job of the module being to help users PATCH their JSON

Users can JSON.stringify even if they used undefined. The results simply lack the undefined values and their keys, making JSON.stringify least astonishing and robust. We can do the same.

Detail:

Like @wolfgang42 in #60, I believe

it should not be possible for fast-json-patch to emit an invalid json-patch

RFC6902 says, over and over:

The operation object MUST contain a "value" member

If value is undefined before we stringify the patch, we violate the spec. That's not strictly our fault: undefined literally doesn't exist in EMCA-404. I don't think users considering hiring us to help them switch from POST to PATCH much care for that distinction: they just need the job done. If we can't help them, they'll hire someone else.

Users can JSON.stringify despite their undefined values, making it least astonishing and robust. As @warpech observed in #60, we can do the same by treating the operation as if the before and after pictures had been run through JSON.stringify and JSON.parse.

Consider setting a value to undefined:

var myobj = { };
var observer = jsonpatch.observe( myobj );
myobj.firstName = undefined;
var patch = jsonpatch.generate( observer );

A patch of [] might be astonishing to anyone using jsonpatch purely in memory, but — again — our job is to help users PATCH their JSON. The JSON encodings of myobj before and after the mutation are identical. From the JSON point of view, the current behaviour is astonishing and, once you stringify it, non-compliant:

[ { op: 'add', path: '/firstName', value: undefined } ]

[ { "op": "add", "path": "/firstName" } ]

In short, for any mutation:

• any patch should remain valid after encoding it with JSON.stringify
• the patch from jsonpatch.compare should be consistent whether you run its inputs through a stringify and parse round-trip or not
• the patch from jsonpatch.observe should match the patch from jsonpatch.compare
• all the above should be true even if undefined is present

Edits:

• Don't bury the compare-to-JSON lede
• More consistently apply POLA terminology

[sonnyp]

There are many other edge cases.

JSON.stringify discard undefined values in objects, not in arrays where they are turned into null.

JSON.stringify({foo: undefined}) returns {}
JSON.stringify([true, undefined, true]) returns [true,null,true]

Same thing for ES6 symbols
Also NaN, Infinity, -Infinity, +Infinity but in an object contrary to undefined and symbols the key is kept and the value converted to null)

And there is the special case where the value is an object and replaced with the result of its toJSON method if defined.

also
JSON.stringify([,,]) returns [null, null]

and pretty much the same hings if an object key isn't JSON valid.

I think we should provide a sanitize function that runs the patch and/or the target document through JSON.stringify/JSON.parse and recommend using it before evaluation.

[warpech]

Labelling this as "question". This lib will always prioritise performance over sanity-checking. But perhaps something can be done to improve the developer UX.

Currently the explanation in README.md is not very helping (written in strikethrough):

[warpech]

Fixed in 1.0.0