Setting a property to a undefined value results in a patch without value argument

[malx122]

lets say "test" is undefined and "MyData" is a JSon object tat is coming from the server.

 MyData.WriteableProperty$ = test;

This will result in following:

{"op":"replace","path":"/MyData/WriteableProperty$"}

The value argument of the patch is missing. (And at least SC server will complain)

[miyconst]

As pointed in #47 the undefined is not a valid json value. undefined should be replaced with null.

Can this issue be closed?

[warpech]

My goal was to add client-side checks before sending of generated JSON Patch that would give the developer a clear information what is wrong. We know that undefined is not a valid JSON type but how can we save an hour of frustrating debugging for an inexperienced programmer?

There is patch validation functionality in JSON-Patch (Starcounter-Jack/JSON-Patch#29). PuppetJS is using it (Puppet.prototype.validateAndApplySequence, Puppet.prototype.validateSequence) so this case should appear as an error for the client. Could you verify that?

[miyconst]

Sure. Assigning to myself.

[miyconst]

If an outgoing patch has value = undefined or no value at all, then puppet throws an exception: OPERATION_VALUE_REQUIRED, but if the value is an object with an undefined property, then such patch goes through validation.

I have added three new tests, but not yet pushed, as one of them fails.

[warpech]

Good catch! Can you propose a solution that passes this test?

[miyconst]

Looks like we have to check the outgoing changes recursively, remove private properties and check values, if there is an undefined value then replace it with null or throw an exception.

[warpech]

Let's do the first part of the above sentence. Do the same validation it does now, only recursively.

[warpech]

Actually I think we have to reconsider that to do in this case.

Let's assume the following code:

//puppet.obj.person$ == {"FirstName": "Marcin", "LastName": "W"}
puppet.obj.person$ = {"FirstName": "Marcin", "LastName": undefined}

What I am sure is:

• We cannot implicitly replace undefined with null. They are different things.
• We cannot use the current error OPERATION_VALUE_REQUIRED, because there is a value. The problem is that it has undefined inside of it.

What options are we left with?

1. Create a copy of the value object for the patch, without the properties of undefined value. This would be similar to running the value object through JSON.parse(JSON.stringify())
2. Run a deep check in the validator and if undefined value is encountered show a new error VALUE_OBJECT_CANNOT_CONTAIN_UNDEFINED

Anyway, the solution would be applied to JSON-Patch not to PuppetJs.

@tomalec WDYT?

[miyconst]

Current validation is implemented here: https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.js#L632

And it is throwing OPERATION_VALUE_REQUIRED for undefined values, which should be replaced with VALUE_OBJECT_CANNOT_CONTAIN_UNDEFINED.

[tomalec]

I would say, that

1. JSON Patch performance should not get hit if someone uses it for something else than JSON
2. If user wants to check whether JSON Patch that came from outside is valid, he/she may use on-demand validation, that may not be a bit slower,
3. PuppetJS, and Starcounter uses JSON as communication format, not JS,
4. If user knows what he's doing, and tries to use Puppet for non JSON data: puppet.obj.person$ = {"FirstName": "Marcin", "LastName": undefined}, we may transfer it to the server (another question is what would server do with such data), but when asked our validators will say that this is not valid.

So my suggested behavior is described here: https://github.com/Starcounter-Jack/JSON-Patch#undefineds-json-to-js-extension

To sum up. For valid JSONs we will generate, and communicate via valid JSON Patches.
For JSONs with undefined (invalid in terms of RFC), we will produce JSON Patches (invalid as well) with undefined as any other value in JS.
For other types of invalid JSON - JS objects with functions, etc. we will not bother at all, we could check that in validator, but we do not try to gaurantee that PuppetJS and JSON Patch will do something specific with it.

[warpech]

Fair enough! PuppetJs in debug mode (currently ON by default) validates outgoing patches:

   if(this.debug) {
      this.validateSequence(this.remoteObj, patches);
    }

in which case the error should be reported. I am glad that we both agree :)

@miyconst: Current validation is implemented here: https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.js#L632

And it is throwing OPERATION_VALUE_REQUIRED for undefined values, which should be replaced with VALUE_OBJECT_CANNOT_CONTAIN_UNDEFINED.

It should not be mixed. The current check for undefined value (OPERATION_VALUE_REQUIRED) should be kept. A new check for undefined property of the value (if the value is an object) should be added after it (VALUE_OBJECT_CANNOT_CONTAIN_UNDEFINED).

@miyconst could you prepare a solution for this on a separate branch in https://github.com/Starcounter-Jack/JSON-Patch? Create a new issue for it, publish code on a separate branch for review. The changes should include test and update to README.md

[warpech]

I meant that the issue should be created in JSON-Patch, because it is to be solved in JSON-Patch

[miyconst]

The Starcounter-Jack/JSON-Patch#73 issue is closed and merged, can this one also be closed?

[warpech]

To wrap up... What will be the current behavior in case described in the first message of this thread?

[miyconst]

The patch

{"op":"replace","path":"/MyData/WriteableProperty$"}

Won't pass validation and the OPERATION_VALUE_REQUIRED exception will be thrown.

[warpech]

Thanks!

[miyconst]

I have added some automated tests for OPERATION_VALUE_REQUIRED and OPERATION_VALUE_CANNOT_CONTAIN_UNDEFINED exceptions.