Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

json patch: adding to a subfield of a non-object now fails as expected #251

Merged
merged 1 commit into from
Mar 22, 2018
Merged

json patch: adding to a subfield of a non-object now fails as expected #251

merged 1 commit into from
Mar 22, 2018

Conversation

bobkocisko
Copy link
Contributor

This fixes a non-compliance with the json patch standard

@bobkocisko
Copy link
Contributor Author

Without this patch the library fails to apply the requested patch but reports success

@FSMaxB FSMaxB added the bug label Mar 21, 2018
FSMaxB
FSMaxB reviewed Mar 21, 2018
View reviewed changes
tests/json-patch-tests/tests.json Outdated
{ "comment": "adding to subfield of a non-object should fail",
"doc": {"foo": {"bar": 1}},
"patch": [{"op": "add", "path": "/foo/bar/baz", "value": "5"}],
"error": "attempting to add to subfield of non-object"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but please add the test to cjson-utils-tests.json instead. You can also make a pull request with the same test here, then I will pull the change into cJSON.

FSMaxB
FSMaxB requested changes Mar 21, 2018
View reviewed changes
Copy link
Collaborator

@FSMaxB FSMaxB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the test to cjson-utils-tests.json and this is ready to merge.

@FSMaxB
Copy link
Collaborator

FSMaxB commented Mar 21, 2018

Thanks a lot for spotting and fixing this error. cJSON_Utils has received much less attention than the code in main cJSON, so I'm really happy to see it improved upon.

@bobkocisko
Copy link
Contributor Author

You're welcome. I'm glad it was such a simple fix!

I moved the test as you requested, but for what it's worth I'm a little confused why there are 2-3 different places with tests for json patch?

@FSMaxB
Copy link
Collaborator

FSMaxB commented Mar 22, 2018

but for what it's worth I'm a little confused why there are 2-3 different places with tests for json patch?

That's because the json-patch-tests is a git subtree of https://github.com/json-patch/json-patch-tests and that already contains 2 files. One of them being the tests from the text of RFC6902 which defines the JSON Patch format and the other one being the additional tests.

cjson-utils-tests.json contains additional tests that originally came from a C file that cJSON Utils used for testing. I don't want to put them in the other files, because I want to keep them in sync with the upstream json-patch-tests.

@FSMaxB FSMaxB merged commit 8abf110 into DaveGamble:master Mar 22, 2018
@bobkocisko
Copy link
Contributor Author

Ok, thanks for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FSMaxB FSMaxB FSMaxB requested changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobkocisko @FSMaxB