Make `add` operations check array bounds

[grncdr]

From section 4.1 of the RFC:

The specified index MUST NOT be greater than the number of elements in the array.

This caught me out as doing get("/foo/1", patch([{"op": "add", "path": "/foo/1", "value": "bar"}], {foo: []})) would fail in the get instead of the patch.

[briancavalier]

Hey @grncdr, thanks! I'll have time to look at this more closely tomorrow.

[briancavalier]

Took a quick look at section 4.1 the RFC, and you're absolutely right. It's a shame that sentence is buried right in the middle of one of the bullet points--makes it hard to see.

Could you please also add a unit test that fails without your change, and passes with it? Thanks!

[grncdr]

Hi @briancavalier, I ended up opening https://github.com/json-patch/json-patch-tests/pull/24 so that all implementations would benefit, I hope that's appropriate! (p.s. have you considered using a submodule for those tests? That makes it much easier to tell how up-to-date the test suite is).

[briancavalier]

I ended up opening json-patch/json-patch-tests#24 so that all implementations would benefit, I hope that's appropriate!

:+1: awesome

have you considered using a submodule for those tests?

Finding a better way to keep the tests updated would be great. Personally, I'm not a fan of submodules, but it'd probably work just fine for this. Another interesting route might be to petition the json-patch test authors to publish the tests on npm (e.g. like Promises/A+). Then, it's just a matter of adding them a dependency in package.json. What do you think?

[sonnyp]

@briancavalier https://github.com/json-patch/json-patch-tests/pull/25 it's published on npm as json-patch-test-suite it was updated with @grncdr array bound PR.

[briancavalier]

@sonnyp Nice, thanks! In that case, I'm ok with merging this now. I'll add json-patch-test-suite in another commit.

[grncdr]

:tada: