docs: state merge_patch complexity and RFC 7396 limitations; test non-object, discarded, binary cases

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

docs/mkdocs/docs/api/basic_json/merge_patch.md

@@ -39,13 +39,33 @@ Basic guarantee: if an exception is thrown during the operation, the JSON value
 
 ## Complexity
 
-Linear in the lengths of `apply_patch`.
+Linear in the total number of values (including nested ones) in `apply_patch`, times the cost of a key-based object
+operation (looking up, adding, or erasing a member): logarithmic for [`json`](../json.md), and linear for
+[`ordered_json`](../ordered_json.md) (see [`ordered_map` complexity](../ordered_map.md#complexity)). For
+`ordered_json`, patching an object that has `n` members with a patch that has `m` members therefore takes O(n·m). The
+cost also includes one copy of `apply_patch` (see the notes below).
 
 ## Notes
 
-`apply_patch` may be `#!cpp *this` itself or refer to a value contained in `#!cpp *this` (for example, a subobject
-returned by `#!cpp (*this)[key]`); it is read as it was when `merge_patch()` was called, before any modification of
-`#!cpp *this`.
+- `apply_patch` may be `#!cpp *this` itself or refer to a value contained in `#!cpp *this` (for example, a subobject
+  returned by `#!cpp (*this)[key]`); it is read as it was when `merge_patch()` was called, before any modification of
+  `#!cpp *this`. To make this possible, the function copies `apply_patch` before it modifies `#!cpp *this`, so the cost
+  includes one copy of the patch.
+- A merge patch uses `#!json null` to remove a member, so it cannot set a member to `#!json null`: applying
+  `#!json {"a":null}` to `#!json {"a":1}` gives `#!json {}`.
+- Arrays are not merged: an array in the patch replaces the target value as a whole, and a `#!json null` inside an
+  array is kept as an element. `#!json null` members of the target that the patch does not mention are preserved.
+- A `#!json null` in the patch only removes a member; it does not prevent the creation of the enclosing objects.
+  Applying `#!json {"a":{"b":null}}` to `#!json {}` gives `#!json {"a":{}}` (see example 15 in Appendix A of the RFC).
+- If `apply_patch` is not an object, it replaces the target as a whole. In particular, a `#!json null` patch turns the
+  target into `#!json null`. If `apply_patch` is an object and the target is not, the target is first replaced by an
+  empty object, so applying `#!json {}` to `#!json 1` gives `#!json {}`.
+- A merge patch cannot express every change; for example, it cannot reorder array elements or remove a specific array
+  element. Use [JSON Patch](../../features/json_patch.md) ([RFC 6902](https://tools.ietf.org/html/rfc6902)) for that.
+  [`merge_diff`](merge_diff.md) has a matching limitation, see its notes.
+- A [discarded](is_discarded.md) value is treated like any other non-object value: a discarded
+  `apply_patch` turns the target into a discarded value, and an object patch replaces a discarded target by an object
+  as described above.
 
 ## Examples
 

docs/mkdocs/docs/api/basic_json/patch_inplace.md

@@ -62,16 +62,6 @@ Unlike [`patch`](patch.md), `patch_inplace` applies the operation "in place" and
 That makes it faster for large documents by avoiding the copy. However, the JSON value might be corrupted if the
 function throws an exception.
 
-The JSON patch may be the JSON value itself (`#!cpp j.patch_inplace(j)`): the operations are then read from a copy of
-the patch made before the first operation is applied, so the result is the same as that of
-`#!cpp j = j.patch(j)`, and the exception safety is the same as above.
-
-!!! danger "Undefined behavior"
-
-    The JSON patch must not be a value stored inside the JSON value (e.g., `#!cpp j.patch_inplace(j["patch"])`), as the
-    operations may change or destroy it while it is read. Pass a copy (`#!cpp j.patch_inplace(json(j["patch"]))`) or use
-    [`patch`](patch.md) instead.
-
 ## Examples
 
 ??? example "Example: apply a JSON patch in place"
@@ -123,4 +113,3 @@ the patch made before the first operation is applied, so the result is the same
 - Throws [`parse_error.109`](../../home/exceptions.md#jsonexceptionparse_error109) instead of
   [`out_of_range.404`](../../home/exceptions.md#jsonexceptionout_of_range404) for a one-character array index that is
   not a digit (e.g., `/x`) in version 3.13.0, as it already did for longer ones.
-- Applying a JSON value as patch to itself no longer results in undefined behavior in version 3.13.0.

docs/mkdocs/docs/api/json_pointer/operator_slasheq.md

@@ -18,8 +18,7 @@ json_pointer& operator/=(std::size_t array_idx)
 ## Parameters
 
 `ptr` (in)
-:    JSON pointer to append; may be the JSON pointer itself (`#!cpp p /= p`), which then appends a copy of its
-     reference tokens
+:    JSON pointer to append
 
 `token` (in)
 :    reference token to append
@@ -69,7 +68,6 @@ json_pointer& operator/=(std::size_t array_idx)
 
 ## Version history
 
-1. Added in version 3.6.0. Appending a JSON pointer to itself no longer results in undefined behavior in version
-   3.13.0.
+1. Added in version 3.6.0.
 2. Added in version 3.6.0. Changed type of `token` to `string_t` in version 3.11.0.
 3. Added in version 3.6.0.

docs/mkdocs/docs/features/merge_patch.md

@@ -2,7 +2,7 @@
 
 The library supports JSON Merge Patch ([RFC 7396](https://tools.ietf.org/html/rfc7396)) as a patch format.
 The merge patch format is primarily intended for use with the HTTP PATCH method as a means of describing a set of
-modifications to a target resource's content. This function applies a merge patch to the current JSON value.
+modifications to a target resource's content. Applying a merge patch changes the JSON value the patch is applied to.
 
 Instead of using [JSON Pointer](json_pointer.md) to specify values to be manipulated, it describes the changes using a
 syntax that closely mimics the document being modified. Unlike [JSON Patch](json_patch.md), a JSON Merge Patch cannot
@@ -19,6 +19,18 @@ another.
     [`update`](../api/basic_json/update.md) with `merge_objects` set to `#!cpp true`; see
     [Merging objects](modifying_values.md#merging-objects).
 
+!!! note "Limitations of RFC 7396"
+
+    - A patch cannot set a member to `#!json null`, because `#!json null` removes the member:
+      `#!json {"a":1}` patched with `#!json {"a":null}` gives `#!json {}`.
+    - Arrays are replaced as a whole, never merged, and a `#!json null` inside an array is kept.
+    - `#!json null` members of the target that the patch does not mention are preserved.
+    - A `#!json null` that would remove a member of a not yet existing object still creates that object:
+      `#!json {}` patched with `#!json {"a":{"b":null}}` gives `#!json {"a":{}}`.
+    - A patch that is not an object replaces the whole target (a `#!json null` patch makes the target
+      `#!json null`); an object patch applied to a non-object target starts from an empty object.
+    - [`merge_diff`](../api/basic_json/merge_diff.md#notes) has a matching limitation for `#!json null` members.
+
 ??? example
 
     The following code shows how a JSON Merge Patch is applied to a JSON document.

include/nlohmann/detail/json_pointer.hpp

@@ -243,13 +243,6 @@ class json_pointer
     /// @sa https://json.nlohmann.me/api/json_pointer/operator_slasheq/
     json_pointer& operator/=(const json_pointer& ptr)
     {
-        // insert() must not be given iterators into the vector it inserts
-        // into: append a copy when a pointer is appended to itself
-        if (JSON_HEDLEY_UNLIKELY(&ptr == this))
-        {
-            return *this /= json_pointer(ptr);
-        }
-
         reference_tokens.insert(reference_tokens.end(),
                                 ptr.reference_tokens.begin(),
                                 ptr.reference_tokens.end());

include/nlohmann/json.hpp

@@ -6950,15 +6950,6 @@ public:
     /// @sa https://json.nlohmann.me/api/basic_json/patch/
     void patch_inplace(const basic_json& json_patch)
     {
-        // the operations below change *this while iterating json_patch: apply
-        // a copy when a value is applied as patch to itself
-        if (JSON_HEDLEY_UNLIKELY(&json_patch == this))
-        {
-            const basic_json json_patch_copy = json_patch;
-            patch_inplace(json_patch_copy);
-            return;
-        }
-
         basic_json& result = *this;
         // the valid JSON Patch operations
         enum class patch_operations {add, remove, replace, move, copy, test, invalid};

single_include/nlohmann/json.hpp

@@ -23828,13 +23828,6 @@ class json_pointer
     /// @sa https://json.nlohmann.me/api/json_pointer/operator_slasheq/
     json_pointer& operator/=(const json_pointer& ptr)
     {
-        // insert() must not be given iterators into the vector it inserts
-        // into: append a copy when a pointer is appended to itself
-        if (JSON_HEDLEY_UNLIKELY(&ptr == this))
-        {
-            return *this /= json_pointer(ptr);
-        }
-
         reference_tokens.insert(reference_tokens.end(),
                                 ptr.reference_tokens.begin(),
                                 ptr.reference_tokens.end());
@@ -42747,15 +42740,6 @@ public:
     /// @sa https://json.nlohmann.me/api/basic_json/patch/
     void patch_inplace(const basic_json& json_patch)
     {
-        // the operations below change *this while iterating json_patch: apply
-        // a copy when a value is applied as patch to itself
-        if (JSON_HEDLEY_UNLIKELY(&json_patch == this))
-        {
-            const basic_json json_patch_copy = json_patch;
-            patch_inplace(json_patch_copy);
-            return;
-        }
-
         basic_json& result = *this;
         // the valid JSON Patch operations
         enum class patch_operations {add, remove, replace, move, copy, test, invalid};

tests/src/unit-json_patch.cpp

@@ -823,109 +823,6 @@ TEST_CASE("JSON patch")
             CHECK(doc.at("foo") == "bar");
         }
 #endif // !defined(JSON_NOEXCEPTION)
-
-        SECTION("applying a value as patch to itself")
-        {
-            // a JSON patch is an array, so an array of operations can be
-            // applied to itself; the operations are read from the patch as
-            // it was before the first operation, as patch() does it
-            const json copy_op = {{"op", "copy"}, {"from", "/0"}, {"path", "/-"}};
-            json copy_ops = json::array();
-            for (int i = 0; i < 5; ++i)
-            {
-                copy_ops.push_back(copy_op);
-            }
-
-            json expected_copies = copy_ops;
-            for (int i = 0; i < 5; ++i)
-            {
-                expected_copies.push_back(copy_op);
-            }
-
-            SECTION("copy operations without spare capacity")
-            {
-                json doc = copy_ops;
-                doc.get_ref<json::array_t&>().shrink_to_fit();
-                CHECK(doc.patch(doc) == expected_copies);
-
-                doc.patch_inplace(doc);
-                CHECK(doc == expected_copies);
-            }
-
-            SECTION("copy operations with spare capacity")
-            {
-                json doc = copy_ops;
-                doc.get_ref<json::array_t&>().reserve(100);
-                CHECK(doc.patch(doc) == expected_copies);
-
-                doc.patch_inplace(doc);
-                CHECK(doc == expected_copies);
-            }
-
-            SECTION("remove operations")
-            {
-                json doc = R"([
-                        {"op": "remove", "path": "/0"},
-                        {"op": "remove", "path": "/0"},
-                        {"op": "remove", "path": "/0"}
-                    ])"_json;
-                CHECK(doc.patch(doc) == json::array());
-
-                doc.patch_inplace(doc);
-                CHECK(doc == json::array());
-            }
-
-            SECTION("operations that read a changed element")
-            {
-                // the second operation is still a "test" operation, although
-                // the first one replaced it in the document
-                json doc = R"([
-                        {"op": "replace", "path": "/1", "value": 1},
-                        {"op": "test", "path": "/1", "value": 1},
-                        {"op": "add", "path": "/-", "value": 2}
-                    ])"_json;
-                const json expected = R"([
-                        {"op": "replace", "path": "/1", "value": 1},
-                        1,
-                        {"op": "add", "path": "/-", "value": 2},
-                        2
-                    ])"_json;
-                CHECK(doc.patch(doc) == expected);
-
-                doc.patch_inplace(doc);
-                CHECK(doc == expected);
-            }
-
-            SECTION("replacing the root")
-            {
-                json doc = R"([
-                        {"op": "add", "path": "", "value": [1, 2]},
-                        {"op": "add", "path": "/-", "value": 3},
-                        {"op": "replace", "path": "", "value": {"a": 4}}
-                    ])"_json;
-                const json expected = {{"a", 4}};
-                CHECK(doc.patch(doc) == expected);
-
-                doc.patch_inplace(doc);
-                CHECK(doc == expected);
-            }
-
-#if !defined(JSON_NOEXCEPTION)
-            SECTION("failing operation")
-            {
-                // the operations before the failing one remain applied
-                json doc = R"([
-                        {"op": "add", "path": "/-", "value": 1},
-                        {"op": "test", "path": "/0/op", "value": "remove"}
-                    ])"_json;
-                CHECK_THROWS_AS(doc.patch(doc), json::other_error&);
-
-                CHECK_THROWS_AS(doc.patch_inplace(doc), json::other_error&);
-                CHECK(doc.size() == 3);
-                CHECK(doc.back() == 1);
-            }
-#endif // !defined(JSON_NOEXCEPTION)
-        }
     }
 
     SECTION("errors")

tests/src/unit-json_pointer.cpp

@@ -779,44 +779,6 @@ TEST_CASE("JSON pointers")
         CHECK(ptr.to_string() == "/object/~1");
     }
 
-    SECTION("appending a pointer to itself")
-    {
-        // operator/= must not pass iterators into its own token vector to
-        // insert(); each pointer is appended to itself and must yield the
-        // pointer whose string representation is the original one twice
-        const std::vector<std::string> pointers =
-        {
-            "",
-            "/",
-            "/a",
-            "/a/b",
-            "/foo/0/-",
-            "/a~1b/~0c/~01",
-            "/1/2/3/4/5/6/7/8/9/10/11/12/13/14/15/16/17"
-        };
-
-        for (const auto& s : pointers)
-        {
-            CAPTURE(s)
-            json::json_pointer ptr(s);
-            const json::json_pointer expected(s + s);
-
-            json::json_pointer& ret = (ptr /= ptr);
-            CHECK(&ret == &ptr);
-            CHECK(ptr == expected);
-            CHECK(ptr.to_string() == s + s);
-
-            // appending to itself again doubles the pointer once more
-            ptr /= ptr;
-            CHECK(ptr.to_string() == s + s + s + s);
-
-            // operator/ appends to a copy and leaves the operand unchanged
-            const json::json_pointer original(s);
-            CHECK((original / original) == expected);
-            CHECK(original.to_string() == s);
-        }
-    }
-
     SECTION("equality comparison")
     {
         std::string ptr_string{"/foo/bar"};

tests/src/unit-merge_patch.cpp

@@ -423,3 +423,86 @@ TEST_CASE("merge_patch() with an argument that aliases *this (#5641)")
         }
     }
 }
+
+TEST_CASE("JSON Merge Patch on non-object, discarded, and binary values")
+{
+    SECTION("an empty object patch turns a non-object target into an empty object")
+    {
+        json target = 1;
+        target.merge_patch(json::object());
+        CHECK(target == json::object());
+
+        json from_null;
+        from_null.merge_patch(json::object());
+        CHECK(from_null == json::object());
+    }
+
+    SECTION("a non-object patch replaces a deeply nested target")
+    {
+        // checked by type, since == recurses
+        SECTION("number")
+        {
+            json target = json::parse(nested_objects(1000, 0));
+            target.merge_patch(json(42));
+            CHECK(target.is_number_integer());
+            CHECK(target.get<int>() == 42);
+        }
+
+        SECTION("array")
+        {
+            json target = json::parse(nested_objects(1000, 0));
+            target.merge_patch(json({1, nullptr}));
+            CHECK(target.is_array());
+            CHECK(target.size() == 2);
+            CHECK(target[1].is_null());
+        }
+
+        SECTION("null")
+        {
+            json target = json::parse(nested_objects(1000, 0));
+            target.merge_patch(json(nullptr));
+            CHECK(target.is_null());
+        }
+    }
+
+    SECTION("discarded values")
+    {
+        const json discarded(json::value_t::discarded);
+
+        // a discarded patch is not an object and replaces the target
+        json target = {{"a", 1}};
+        target.merge_patch(discarded);
+        CHECK(target.is_discarded());
+
+        // a discarded target is replaced by an empty object first
+        json discarded_target(json::value_t::discarded);
+        discarded_target.merge_patch(json({{"a", 1}}));
+        CHECK(discarded_target == json({{"a", 1}}));
+
+        json both(json::value_t::discarded);
+        both.merge_patch(discarded);
+        CHECK(both.is_discarded());
+    }
+
+    SECTION("binary values")
+    {
+        const json bin = json::binary({1, 2, 3}, 7);
+
+        // a binary patch is not an object and replaces the target
+        json target = {{"a", 1}};
+        target.merge_patch(bin);
+        CHECK(target.is_binary());
+        CHECK(target.get_binary() == bin.get_binary());
+
+        // a binary target is replaced by an object
+        json binary_target = bin;
+        binary_target.merge_patch(json({{"a", 1}}));
+        CHECK(binary_target == json({{"a", 1}}));
+
+        // binary values nested in the patch are kept
+        json nested = json::object();
+        nested.merge_patch(json({{"a", bin}}));
+        CHECK(nested["a"].is_binary());
+        CHECK(nested["a"].get_binary() == bin.get_binary());
+    }
+}