Not testable: A.13 Invalid JSON Patch Document

[warpech]

There is one test in tests.js that cannot be parsed as a JSON document:

{
    "comment": "A.13 Invalid JSON Patch Document",
    "doc": {
     "foo": "bar"
    },
    "patch": [
  { "op": "add", "path": "/baz", "value": "qux", "op": "remove" }
],
    "error": "operation has two 'op' members"
  }

First value assignment ("op": "add") is simply ignored by a standard JSON parser, so this condition is really untestable. I suggest to remove this test from the spec.

[almost]

The JSON spec actually doesn't specify what parses should do with duplicate keys. A lot of parsers will simply discard the first of the duplicate keys making it not possible to raise an error under this condition.

So specifying a JSON Patch implementation this must show an error is going to make it not possible for a lot of implementations to confirm and you really don't want different implementations acting differently if at all possible. So I would suggest the correct behavior here would be to do what most JSON parsers force you to do anyway.

[warpech]

Relevant discussion about duplicate keys in JSON: http://esdiscuss.org/topic/json-duplicate-keys

Thing is, that the tests are stored in JSON themselves, so a JSON parser is affecting the way the test is read. In my implementation of this test I decided to remove the test A.13 from the test suite: https://github.com/Starcounter-Jack/JSON-Patch/blob/master/test/spec/json-patch-tests/spec_tests.json

[mikemccabe]

I also agree that this shouldn't have been made a spec requirement. However, it's there!

I have the same issue with my implementation.

It's easy to add flags (such as "disabled":true) or ("dont_bother_with_meta_issues":true). An implementation could then check that flag and skip the test.

Would that be an acceptable solution?

Mike

On 9/16/14, 4:26 AM, Marcin Warpechowski wrote:

Relevant discussion about duplicate keys in JSON: http://esdiscuss.org/topic/json-duplicate-keys

Thing is, that the tests are stored in JSON themselves, so a JSON parser is affecting the way the test is read. In my implementation of this test I decided to remove the test A.13 from the test suite: https://github.com/Starcounter-Jack/JSON-Patch/blob/master/test/spec/json-patch-tests/spec_tests.json

— Reply to this email directly or view it on GitHub https://github.com/json-patch/json-patch-tests/issues/15#issuecomment-55728887.

[mnot]

A.13 isn't a normative requirement; dropping this from the test suite (or flagging as per Mike) makes sense to me...

[mikemccabe]

I added "disabled": true.

[warpech]

Perfect, thanks! (Sorry for not answering on Sep 16th)