nlohmann / json

JSON for Modern C++
https://json.nlohmann.me
MIT License
42.93k stars 6.71k forks source link

Invalid RFC6902 copy operation succeeds #894

Closed cmannett85 closed 6 years ago

cmannett85 commented 6 years ago
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 commented 6 years ago

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 commented 6 years ago

Thanks for the rapid response!

nlohmann commented 6 years ago

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 commented 6 years ago

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

cmannett85 commented 6 years ago

Using my trivial example above, I now get:

Move: 403
Copy: 403

Which is expected - thank you!