From b4fbac0f4c61a211f21d32c026319b1a60f65f78 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Wed, 23 Sep 2026 22:45:53 +0200 Subject: [PATCH] Keep JSON_DIAGNOSTICS parent pointers of ordered_json members after erase() and update() ordered_json stores its members in a vector, and two operations moved members without restoring their parent pointers afterwards: - ordered_map::erase() re-constructs every member after the erased one in place. The basic_json move constructor leaves m_parent at nullptr, and none of the object branches of basic_json::erase() (by key, iterator, or iterator range) called set_parents(). This also affected merge_patch() with a null member and patch() with a remove operation. - update() only set the parent pointer of the inserted member. Adding a key can reallocate the vector, which copies all other members and leaves their m_parent at nullptr. The set_parents() call added for #4813 only repaired this for the nested object of a merge, not for the target. The next assert_invariant() on such an object (for instance, when copying it) aborted, and diagnostic messages lost the path prefix above the moved member. std::map-based json was not affected, because its nodes do not move. Erasing from an ordered_map object now calls set_parents(), and update() uses set_parent(), which already refreshes all members for vector-based objects. This makes the #4813 workaround redundant. Signed-off-by: Niels Lohmann --- include/nlohmann/json.hpp | 48 ++++++++++++++------- single_include/nlohmann/json.hpp | 48 ++++++++++++++------- tests/src/unit-diagnostics.cpp | 74 ++++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 30 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index e40a2aa27..09dca4694 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -1243,6 +1243,27 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } + /// @brief restore the parent pointers after erasing from an object + /// ordered_json keeps its members in a vector, and erasing a member + /// re-constructs every member after it in place, which resets their + /// parent pointers + void set_parents_after_object_erase() + { +#if JSON_DIAGNOSTICS +#ifdef JSON_HEDLEY_MSVC_VERSION +#pragma warning(push ) +#pragma warning(disable : 4127) // ignore warning to replace if with if constexpr +#endif + if (detail::is_ordered_map::value) + { + set_parents(); + } +#ifdef JSON_HEDLEY_MSVC_VERSION +#pragma warning( pop ) +#endif +#endif + } + public: ////////////////////////// // JSON parser callback // @@ -2931,6 +2952,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { result.m_it.object_iterator = erase_from_object(pos.m_it.object_iterator); + set_parents_after_object_erase(); break; } @@ -3003,6 +3025,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec { result.m_it.object_iterator = m_data.m_value.object->erase(first.m_it.object_iterator, last.m_it.object_iterator); + set_parents_after_object_erase(); break; } @@ -3033,7 +3056,9 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec JSON_THROW(type_error::create(307, detail::concat("cannot use erase() with ", type_name()), this)); } - return m_data.m_value.object->erase(std::forward(key)); + const auto erased = m_data.m_value.object->erase(std::forward(key)); + set_parents_after_object_erase(); + return erased; } template < typename KeyType, detail::enable_if_t < @@ -3050,6 +3075,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec if (it != m_data.m_value.object->end()) { m_data.m_value.object->erase(it); + set_parents_after_object_erase(); return 1; } return 0; @@ -3961,16 +3987,12 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec if (it2 != m_data.m_value.object->end() && it2->second.is_object()) { it2->second.update_members(it.value().cbegin(), it.value().cend(), true, depth + 1); -#if JSON_DIAGNOSTICS - it2->second.set_parents(); -#endif continue; } } - m_data.m_value.object->operator[](it.key()) = it.value(); -#if JSON_DIAGNOSTICS - m_data.m_value.object->operator[](it.key()).m_parent = this; -#endif + // set_parent() also repairs the other members, which ordered_json + // relocates when adding a key makes its vector grow + set_parent(m_data.m_value.object->operator[](it.key()) = it.value()); } } @@ -3999,9 +4021,6 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } // a nested object is merged: continue with its parent -#if JSON_DIAGNOSTICS - target->set_parents(); -#endif target = stack.back().target; first = stack.back().position; last = stack.back().last; @@ -4023,10 +4042,9 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec continue; } } - target->m_data.m_value.object->operator[](first.key()) = first.value(); -#if JSON_DIAGNOSTICS - target->m_data.m_value.object->operator[](first.key()).m_parent = target; -#endif + // set_parent() also repairs the other members, which ordered_json + // relocates when adding a key makes its vector grow + target->set_parent(target->m_data.m_value.object->operator[](first.key()) = first.value()); ++first; } } diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 3c5821d92..9091c1198 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -25701,6 +25701,27 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } + /// @brief restore the parent pointers after erasing from an object + /// ordered_json keeps its members in a vector, and erasing a member + /// re-constructs every member after it in place, which resets their + /// parent pointers + void set_parents_after_object_erase() + { +#if JSON_DIAGNOSTICS +#ifdef JSON_HEDLEY_MSVC_VERSION +#pragma warning(push ) +#pragma warning(disable : 4127) // ignore warning to replace if with if constexpr +#endif + if (detail::is_ordered_map::value) + { + set_parents(); + } +#ifdef JSON_HEDLEY_MSVC_VERSION +#pragma warning( pop ) +#endif +#endif + } + public: ////////////////////////// // JSON parser callback // @@ -27389,6 +27410,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { result.m_it.object_iterator = erase_from_object(pos.m_it.object_iterator); + set_parents_after_object_erase(); break; } @@ -27461,6 +27483,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec { result.m_it.object_iterator = m_data.m_value.object->erase(first.m_it.object_iterator, last.m_it.object_iterator); + set_parents_after_object_erase(); break; } @@ -27491,7 +27514,9 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec JSON_THROW(type_error::create(307, detail::concat("cannot use erase() with ", type_name()), this)); } - return m_data.m_value.object->erase(std::forward(key)); + const auto erased = m_data.m_value.object->erase(std::forward(key)); + set_parents_after_object_erase(); + return erased; } template < typename KeyType, detail::enable_if_t < @@ -27508,6 +27533,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec if (it != m_data.m_value.object->end()) { m_data.m_value.object->erase(it); + set_parents_after_object_erase(); return 1; } return 0; @@ -28419,16 +28445,12 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec if (it2 != m_data.m_value.object->end() && it2->second.is_object()) { it2->second.update_members(it.value().cbegin(), it.value().cend(), true, depth + 1); -#if JSON_DIAGNOSTICS - it2->second.set_parents(); -#endif continue; } } - m_data.m_value.object->operator[](it.key()) = it.value(); -#if JSON_DIAGNOSTICS - m_data.m_value.object->operator[](it.key()).m_parent = this; -#endif + // set_parent() also repairs the other members, which ordered_json + // relocates when adding a key makes its vector grow + set_parent(m_data.m_value.object->operator[](it.key()) = it.value()); } } @@ -28457,9 +28479,6 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } // a nested object is merged: continue with its parent -#if JSON_DIAGNOSTICS - target->set_parents(); -#endif target = stack.back().target; first = stack.back().position; last = stack.back().last; @@ -28481,10 +28500,9 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec continue; } } - target->m_data.m_value.object->operator[](first.key()) = first.value(); -#if JSON_DIAGNOSTICS - target->m_data.m_value.object->operator[](first.key()).m_parent = target; -#endif + // set_parent() also repairs the other members, which ordered_json + // relocates when adding a key makes its vector grow + target->set_parent(target->m_data.m_value.object->operator[](first.key()) = first.value()); ++first; } } diff --git a/tests/src/unit-diagnostics.cpp b/tests/src/unit-diagnostics.cpp index cdb6185b3..2326c2f2a 100644 --- a/tests/src/unit-diagnostics.cpp +++ b/tests/src/unit-diagnostics.cpp @@ -361,6 +361,80 @@ TEST_CASE("Regression tests for extended diagnostics") CHECK(p == o); } } + + SECTION("Regression test - erase() and update() must keep JSON_DIAGNOSTICS parent pointers of ordered_json members") + { + // ordered_json keeps its members in a vector: erasing a member + // re-constructs all members after it in place, and adding a key may + // reallocate the vector; both reset the parent pointers of the members + // that were moved + using nlohmann::ordered_json; + + const auto check_parents = [](const ordered_json & j) + { + // const access, so operator[] cannot repair the parent pointers + CHECK_THROWS_WITH_AS(j["z"]["x"].at(0), "[json.exception.type_error.304] (/z/x) cannot use at() with number", ordered_json::type_error); + + // must not trigger assert_invariant() in a debug/assert-enabled build + ordered_json const copy = j; + CHECK(copy == j); + }; + + // erase(key) + { + ordered_json j = {{"a", 1}, {"z", {{"x", 1}}}}; + CHECK(j.erase("a") == 1); + check_parents(j); + } + + // erase(iterator) + { + ordered_json j = {{"a", 1}, {"z", {{"x", 1}}}}; + j.erase(j.begin()); + check_parents(j); + } + + // erase(iterator, iterator) + { + ordered_json j = {{"a", 1}, {"b", 2}, {"z", {{"x", 1}}}}; + j.erase(j.begin(), j.find("z")); + check_parents(j); + } + + // patch() removes via erase(iterator) + { + ordered_json j = {{"a", 1}, {"z", {{"x", 1}}}}; + j.patch_inplace(ordered_json::parse(R"([{"op": "remove", "path": "/a"}])")); + check_parents(j); + } + + // update(j) + { + ordered_json j = {{"z", {{"x", 1}}}}; + j.update({{"a", 1}, {"b", 2}}); + check_parents(j); + } + + // update(j, true), the outer and the nested vector both grow + { + ordered_json j = {{"z", {{"x", 1}}}}; + j.update({{"z", {{"y", 2}}}, {"a", 1}}, true); + check_parents(j); + } + + // merge_patch() inserts "c" and removes "d" at /a/c, then inserts "e" + // at /a, which copies /a/c + { + auto j = ordered_json::parse(R"({"a": {"c": {"d": {}}}})"); + j.merge_patch(ordered_json::parse(R"({"a": {"c": {"c": "s", "d": null}, "e": "s"}})")); + CHECK(j.dump() == R"({"a":{"c":{"c":"s"},"e":"s"}})"); + + auto const& constJ = j; + CHECK_THROWS_WITH_AS(constJ["a"]["c"]["c"].at(0), "[json.exception.type_error.304] (/a/c/c) cannot use at() with string", ordered_json::type_error); + ordered_json const copy = j; + CHECK(copy == j); + } + } } TEST_CASE("Better diagnostics past the descent bound of update() and merge_patch()")