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.83k stars 215 forks source link

Add new validation rule: VALUE_OBJECT_CANNOT_CONTAIN_UNDEFINED #73

Closed warpech closed 9 years ago

warpech commented 9 years ago

(Continued from https://github.com/PuppetJs/PuppetJs/issues/21)

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? Publish code on a separate branch for review. The changes should include test and update to README.md

miyconst commented 9 years ago

Here is the new branch: https://github.com/Starcounter-Jack/JSON-Patch/tree/issues/73

The exception name is OPERATION_VALUE_CANNOT_CONTAIN_UNDEFINED, I added OPERATION_ to match all other exceptions and removed _OBJECT to shorten it somehow.

Three new validation specs added:

should return an error if an "add" operation "value" contains "undefined"
should return an error if a "replace" operation "value" contains "undefined"
should return an error if a "test" operation "value" contains "undefined"

with the following operation values:

{ foo: undefined }
{ foos: [undefined] }
{ foo: { bars: [undefined] }} }
warpech commented 9 years ago

@tomalec can you review?

tomalec commented 9 years ago

Check itself LGTM, but we need that change to be made in .ts files as well.

miyconst commented 9 years ago

Updated .ts files. How do you test TypeScript? There is nothing about it in the project.

tomalec commented 9 years ago

U should use tsc in command-line to generate js out of ts, then test js ;)

miyconst commented 9 years ago

@tomalec thanks, tested.

tomalec commented 9 years ago

cool, can you merge it to the master, and close the issue?

miyconst commented 9 years ago

Done. Released 0.5.3 version.