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.

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

Invalid RFC6902 copy operation succeeds #894

Closed
cmannett85 opened this issue Dec 26, 2017 · 5 comments
Closed

Invalid RFC6902 copy operation succeeds #894

cmannett85 opened this issue Dec 26, 2017 · 5 comments
Assignees
Labels
confirmed kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@cmannett85
Copy link

cmannett85 commented Dec 26, 2017

using nlohmann::json;

int main()
{
    auto model = R"({
        "one": {
            "two": {
                "three": "hello",
                "four": 42
            }
        }
    })"_json;

    try {
        model.patch(R"([{"op": "move",
                         "from": "/one/two/three",
                         "path": "/a/b/c"}])"_json);
    } catch (json::exception& e) {
        std::cout << "Move: " << e.id << std::endl;
    }

    try {
        model = model.patch(R"([{"op": "copy",
                                 "from": "/one/two/three",
                                 "path": "/a/b/c"}])"_json);
        std::cout << model.at("/a/b/c"_json_pointer) << std::endl;
    } catch (json::exception& e) {
        std::cout << "Copy: " << e.id << std::endl;
    }
}

// Results in:
Move: 403
"hello"

The above "move" and "copy" JSON patch operations are both invalid for the same reason: /a/b/c is an invalid path because b does not already exist - however, the "copy" succeeds.

Legalise:
RFC6902 4.5:

This operation is functionally identical to an "add" operation at the target location using the value specified in the "from" member.

RFC6092 4.1:

Because this operation is designed to add to existing objects and arrays, its target location will often not exist. Although the pointer's error handling algorithm will thus be invoked, this specification defines the error handling behavior for "add" pointers to ignore that error and add the value as specified.
However, the object itself or an array containing it does need to exist, and it remains an error for that not to be the case.

@nlohmann
Copy link
Owner

Thanks for noting! I could fix this issue by implementing the copy operation similar to the move operation:

const std::string from_path = get_value("copy", "from", true);
const json_pointer from_ptr(from_path);

// the "from" location must exist - use at()
basic_json v = result.at(from_ptr);

// The copy is functionally identical to an "add"
// operation at the target location using the value
// specified in the "from" member.
operation_add(ptr, v);

I was just about to commit the change when I realized the library currently fails some tests from https://github.com/json-patch/json-patch-tests - some even trigger assertions. I shall have a look at these failing tests and see if I can fix these problems as well.

@cmannett85
Copy link
Author

Thanks for the rapid response!

@nlohmann
Copy link
Owner

The test suite revealed another problem: we did not check if we completely parsed an array index. For instance, /array/1 and /array/1e were treated the same way.

@nlohmann nlohmann added this to the Release 3.0.1 milestone Dec 28, 2017
nlohmann added a commit that referenced this issue Dec 28, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
- Implemented "copy" in terms of "add".
- Added check for JSON Pointer array indices to make sure the complete reference token was processed.
- Added test suite from https://github.com/json-patch/json-patch-tests
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Dec 28, 2017
@nlohmann
Copy link
Owner

@cmannett85 Could you check whether the develop branch works for you?

@cmannett85
Copy link
Author

Using my trivial example above, I now get:

Move: 403
Copy: 403

Which is expected - thank you!

nlohmann added a commit that referenced this issue Dec 29, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants