From ebab8a11cc416bc93af24b714edc2d31817ccc16 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Thu, 15 Oct 2026 08:17:28 +0200 Subject: [PATCH] Fix quadratic parent updates for ordered_json with JSON_DIAGNOSTICS Signed-off-by: Niels Lohmann --- include/nlohmann/json.hpp | 99 ++++++++++-------- single_include/nlohmann/json.hpp | 99 ++++++++++-------- tests/src/unit-diagnostics.cpp | 174 +++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 84 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 665ebb8e1..aed3938af 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -1079,51 +1079,64 @@ public: return next; } - /// @brief the capacity of the stored array, or unknown_size() + /// @brief the capacity of the stored array or object, or unknown_size() /// Only JSON_DIAGNOSTICS uses the value, to detect a reallocation that - /// would invalidate the parent pointers. Array types that do not have a - /// capacity() member function report unknown_size(), which is treated as - /// "the elements may have moved". + /// would invalidate the parent pointers. Containers that do not have a + /// capacity() member function report unknown_size() if their elements may + /// move (arrays, ordered_map) and 0 if they never do (std::map). #if JSON_DIAGNOSTICS - template < typename A = array_t, detail::enable_if_t < detail::has_capacity::value, int > = 0 > - std::size_t array_capacity() const noexcept + template < typename C, detail::enable_if_t < detail::has_capacity::value, int > = 0 > + static std::size_t capacity_of(const C& container) noexcept { - return m_data.m_value.array->capacity(); + return container.capacity(); } - template < typename A = array_t, detail::enable_if_t < !detail::has_capacity::value, int > = 0 > - std::size_t array_capacity() const noexcept + template < typename C, detail::enable_if_t < !detail::has_capacity::value, int > = 0 > + static std::size_t capacity_of(const C& /*container*/) noexcept { - return detail::unknown_size(); + return std::is_same::value || detail::is_ordered_map::value ? detail::unknown_size() : 0; + } + + std::size_t container_capacity() const noexcept + { + return is_array() ? capacity_of(*m_data.m_value.array) : capacity_of(*m_data.m_value.object); } #else - static constexpr std::size_t array_capacity() noexcept + static constexpr std::size_t container_capacity() noexcept { return detail::unknown_size(); } #endif - /// @brief set the parent of a value that has just been added to an array - /// @param j the added value - /// @param old_capacity the value @ref array_capacity() returned before the + /// @brief set the parent of a value that has just been added to an array or object + /// @param j the added (or looked-up) value + /// @param old_capacity the value @ref container_capacity() returned before the /// insertion - reference set_parent_after_array_insert(reference j, std::size_t old_capacity) + reference set_parent_after_insert(reference j, std::size_t old_capacity) { #if JSON_DIAGNOSTICS // see https://github.com/nlohmann/json/issues/2838 - JSON_ASSERT(type() == value_t::array); if (JSON_HEDLEY_UNLIKELY(old_capacity == detail::unknown_size() - || array_capacity() != old_capacity)) + || container_capacity() != old_capacity)) { - // the capacity has changed, or the array type does not let us tell: - // the elements may have moved, so update all parents + // the capacity has changed, or the container type does not let us + // tell: the elements may have moved, so update all parents set_parents(); return j; } #else static_cast(old_capacity); #endif - return set_parent(j); + return set_parent_at(j); + } + + /// @brief set the parent of an element accessed via at(); never moves elements + reference set_parent_at(reference j) noexcept + { +#if JSON_DIAGNOSTICS + j.m_parent = this; +#endif + return j; } reference set_parent(reference j) @@ -2557,7 +2570,6 @@ public: copy_leaf_value(other, *this); } - set_parents(); assert_invariant(); } @@ -3445,7 +3457,7 @@ public: /// @sa https://json.nlohmann.me/api/basic_json/at/ reference at(size_type idx) { - return set_parent(array_at(*this, idx)); + return set_parent_at(array_at(*this, idx)); } /// @brief access specified array element with bounds checking @@ -3459,7 +3471,7 @@ public: /// @sa https://json.nlohmann.me/api/basic_json/at/ reference at(const typename object_t::key_type& key) { - return set_parent(object_at(*this, key)); + return set_parent_at(object_at(*this, key)); } /// @brief access specified object element with bounds checking @@ -3468,7 +3480,7 @@ public: detail::is_usable_as_basic_json_key_type::value, int> = 0> reference at(KeyType && key) { - return set_parent(object_at(*this, std::forward(key))); + return set_parent_at(object_at(*this, std::forward(key))); } /// @brief access specified object element with bounds checking @@ -3513,13 +3525,13 @@ public: #if JSON_DIAGNOSTICS // remember array size & capacity before resizing const auto old_size = m_data.m_value.array->size(); - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); #endif m_data.m_value.array->resize(idx + 1); #if JSON_DIAGNOSTICS if (JSON_HEDLEY_UNLIKELY(old_capacity == detail::unknown_size() - || array_capacity() != old_capacity)) + || container_capacity() != old_capacity)) { // capacity has changed: update all parents set_parents(); @@ -3566,8 +3578,9 @@ public: // operator[] only works for objects if (JSON_HEDLEY_LIKELY(is_object())) { + const auto old_capacity = container_capacity(); auto result = m_data.m_value.object->emplace(std::move(key), nullptr); - return set_parent(result.first->second); + return set_parent_after_insert(result.first->second, old_capacity); } throw_subscript_wrong_type("string"); @@ -3617,8 +3630,9 @@ public: // operator[] only works for objects if (JSON_HEDLEY_LIKELY(is_object())) { + const auto old_capacity = container_capacity(); auto result = m_data.m_value.object->emplace(lookup_key(std::forward(key)), nullptr); - return set_parent(result.first->second); + return set_parent_after_insert(result.first->second, old_capacity); } throw_subscript_wrong_type("string"); @@ -4524,9 +4538,9 @@ public: } // add the element to the array (move semantics) - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); m_data.m_value.array->push_back(std::move(val)); - set_parent_after_array_insert(m_data.m_value.array->back(), old_capacity); + set_parent_after_insert(m_data.m_value.array->back(), old_capacity); // if val is moved from, basic_json move constructor marks it null, so we do not call the destructor } @@ -4555,9 +4569,9 @@ public: } // add the element to the array - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); m_data.m_value.array->push_back(val); - set_parent_after_array_insert(m_data.m_value.array->back(), old_capacity); + set_parent_after_insert(m_data.m_value.array->back(), old_capacity); } /// @brief add an object to an array @@ -4585,8 +4599,9 @@ public: } // add the element to the object + const auto old_capacity = container_capacity(); auto res = m_data.m_value.object->insert(val); - set_parent(res.first->second); + set_parent_after_insert(res.first->second, old_capacity); } /// @brief add an object to an object @@ -4639,9 +4654,9 @@ public: } // add the element to the array (perfect forwarding) - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); m_data.m_value.array->emplace_back(std::forward(args)...); - return set_parent_after_array_insert(m_data.m_value.array->back(), old_capacity); + return set_parent_after_insert(m_data.m_value.array->back(), old_capacity); } /// @brief add an object to an object if key does not exist @@ -4662,8 +4677,9 @@ public: } // add the element to the array (perfect forwarding) + const auto old_capacity = container_capacity(); auto res = m_data.m_value.object->emplace(std::forward(args)...); - set_parent(res.first->second); + set_parent_after_insert(res.first->second, old_capacity); // create a result iterator and set iterator to the result of emplace auto it = begin(); @@ -4962,9 +4978,9 @@ public: continue; } } - // 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()) = std::move(it.value())); + // operator[] repairs the other members if ordered_json relocates + // them; assigning through it keeps this O(1) per key + operator[](it.key()) = std::move(it.value()); } } @@ -5014,9 +5030,8 @@ public: continue; } } - // 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()) = std::move(first.value())); + // operator[] repairs the other members if ordered_json relocates them + target->operator[](first.key()) = std::move(first.value()); ++first; } } diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 1e6ce1175..2f4fa38a2 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -30819,51 +30819,64 @@ public: return next; } - /// @brief the capacity of the stored array, or unknown_size() + /// @brief the capacity of the stored array or object, or unknown_size() /// Only JSON_DIAGNOSTICS uses the value, to detect a reallocation that - /// would invalidate the parent pointers. Array types that do not have a - /// capacity() member function report unknown_size(), which is treated as - /// "the elements may have moved". + /// would invalidate the parent pointers. Containers that do not have a + /// capacity() member function report unknown_size() if their elements may + /// move (arrays, ordered_map) and 0 if they never do (std::map). #if JSON_DIAGNOSTICS - template < typename A = array_t, detail::enable_if_t < detail::has_capacity::value, int > = 0 > - std::size_t array_capacity() const noexcept + template < typename C, detail::enable_if_t < detail::has_capacity::value, int > = 0 > + static std::size_t capacity_of(const C& container) noexcept { - return m_data.m_value.array->capacity(); + return container.capacity(); } - template < typename A = array_t, detail::enable_if_t < !detail::has_capacity::value, int > = 0 > - std::size_t array_capacity() const noexcept + template < typename C, detail::enable_if_t < !detail::has_capacity::value, int > = 0 > + static std::size_t capacity_of(const C& /*container*/) noexcept { - return detail::unknown_size(); + return std::is_same::value || detail::is_ordered_map::value ? detail::unknown_size() : 0; + } + + std::size_t container_capacity() const noexcept + { + return is_array() ? capacity_of(*m_data.m_value.array) : capacity_of(*m_data.m_value.object); } #else - static constexpr std::size_t array_capacity() noexcept + static constexpr std::size_t container_capacity() noexcept { return detail::unknown_size(); } #endif - /// @brief set the parent of a value that has just been added to an array - /// @param j the added value - /// @param old_capacity the value @ref array_capacity() returned before the + /// @brief set the parent of a value that has just been added to an array or object + /// @param j the added (or looked-up) value + /// @param old_capacity the value @ref container_capacity() returned before the /// insertion - reference set_parent_after_array_insert(reference j, std::size_t old_capacity) + reference set_parent_after_insert(reference j, std::size_t old_capacity) { #if JSON_DIAGNOSTICS // see https://github.com/nlohmann/json/issues/2838 - JSON_ASSERT(type() == value_t::array); if (JSON_HEDLEY_UNLIKELY(old_capacity == detail::unknown_size() - || array_capacity() != old_capacity)) + || container_capacity() != old_capacity)) { - // the capacity has changed, or the array type does not let us tell: - // the elements may have moved, so update all parents + // the capacity has changed, or the container type does not let us + // tell: the elements may have moved, so update all parents set_parents(); return j; } #else static_cast(old_capacity); #endif - return set_parent(j); + return set_parent_at(j); + } + + /// @brief set the parent of an element accessed via at(); never moves elements + reference set_parent_at(reference j) noexcept + { +#if JSON_DIAGNOSTICS + j.m_parent = this; +#endif + return j; } reference set_parent(reference j) @@ -32297,7 +32310,6 @@ public: copy_leaf_value(other, *this); } - set_parents(); assert_invariant(); } @@ -33185,7 +33197,7 @@ public: /// @sa https://json.nlohmann.me/api/basic_json/at/ reference at(size_type idx) { - return set_parent(array_at(*this, idx)); + return set_parent_at(array_at(*this, idx)); } /// @brief access specified array element with bounds checking @@ -33199,7 +33211,7 @@ public: /// @sa https://json.nlohmann.me/api/basic_json/at/ reference at(const typename object_t::key_type& key) { - return set_parent(object_at(*this, key)); + return set_parent_at(object_at(*this, key)); } /// @brief access specified object element with bounds checking @@ -33208,7 +33220,7 @@ public: detail::is_usable_as_basic_json_key_type::value, int> = 0> reference at(KeyType && key) { - return set_parent(object_at(*this, std::forward(key))); + return set_parent_at(object_at(*this, std::forward(key))); } /// @brief access specified object element with bounds checking @@ -33253,13 +33265,13 @@ public: #if JSON_DIAGNOSTICS // remember array size & capacity before resizing const auto old_size = m_data.m_value.array->size(); - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); #endif m_data.m_value.array->resize(idx + 1); #if JSON_DIAGNOSTICS if (JSON_HEDLEY_UNLIKELY(old_capacity == detail::unknown_size() - || array_capacity() != old_capacity)) + || container_capacity() != old_capacity)) { // capacity has changed: update all parents set_parents(); @@ -33306,8 +33318,9 @@ public: // operator[] only works for objects if (JSON_HEDLEY_LIKELY(is_object())) { + const auto old_capacity = container_capacity(); auto result = m_data.m_value.object->emplace(std::move(key), nullptr); - return set_parent(result.first->second); + return set_parent_after_insert(result.first->second, old_capacity); } throw_subscript_wrong_type("string"); @@ -33357,8 +33370,9 @@ public: // operator[] only works for objects if (JSON_HEDLEY_LIKELY(is_object())) { + const auto old_capacity = container_capacity(); auto result = m_data.m_value.object->emplace(lookup_key(std::forward(key)), nullptr); - return set_parent(result.first->second); + return set_parent_after_insert(result.first->second, old_capacity); } throw_subscript_wrong_type("string"); @@ -34264,9 +34278,9 @@ public: } // add the element to the array (move semantics) - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); m_data.m_value.array->push_back(std::move(val)); - set_parent_after_array_insert(m_data.m_value.array->back(), old_capacity); + set_parent_after_insert(m_data.m_value.array->back(), old_capacity); // if val is moved from, basic_json move constructor marks it null, so we do not call the destructor } @@ -34295,9 +34309,9 @@ public: } // add the element to the array - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); m_data.m_value.array->push_back(val); - set_parent_after_array_insert(m_data.m_value.array->back(), old_capacity); + set_parent_after_insert(m_data.m_value.array->back(), old_capacity); } /// @brief add an object to an array @@ -34325,8 +34339,9 @@ public: } // add the element to the object + const auto old_capacity = container_capacity(); auto res = m_data.m_value.object->insert(val); - set_parent(res.first->second); + set_parent_after_insert(res.first->second, old_capacity); } /// @brief add an object to an object @@ -34379,9 +34394,9 @@ public: } // add the element to the array (perfect forwarding) - const auto old_capacity = array_capacity(); + const auto old_capacity = container_capacity(); m_data.m_value.array->emplace_back(std::forward(args)...); - return set_parent_after_array_insert(m_data.m_value.array->back(), old_capacity); + return set_parent_after_insert(m_data.m_value.array->back(), old_capacity); } /// @brief add an object to an object if key does not exist @@ -34402,8 +34417,9 @@ public: } // add the element to the array (perfect forwarding) + const auto old_capacity = container_capacity(); auto res = m_data.m_value.object->emplace(std::forward(args)...); - set_parent(res.first->second); + set_parent_after_insert(res.first->second, old_capacity); // create a result iterator and set iterator to the result of emplace auto it = begin(); @@ -34702,9 +34718,9 @@ public: continue; } } - // 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()) = std::move(it.value())); + // operator[] repairs the other members if ordered_json relocates + // them; assigning through it keeps this O(1) per key + operator[](it.key()) = std::move(it.value()); } } @@ -34754,9 +34770,8 @@ public: continue; } } - // 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()) = std::move(first.value())); + // operator[] repairs the other members if ordered_json relocates them + target->operator[](first.key()) = std::move(first.value()); ++first; } } diff --git a/tests/src/unit-diagnostics.cpp b/tests/src/unit-diagnostics.cpp index 37427587c..e075fa9ba 100644 --- a/tests/src/unit-diagnostics.cpp +++ b/tests/src/unit-diagnostics.cpp @@ -13,9 +13,11 @@ #endif #define JSON_DIAGNOSTICS 1 +#define JSON_TESTS_PRIVATE #include using nlohmann::json; +using nlohmann::ordered_json; #include #include @@ -631,3 +633,175 @@ TEST_CASE("Better diagnostics past the descent bound of update(), merge_patch() CHECK_THROWS_WITH_AS(p->at("y").at(0), expected_y.c_str(), json::type_error); } } + +namespace +{ +// check that every child's parent pointer points to the containing value +template +bool parents_consistent(const BasicJsonType& j) +{ + bool ok = true; + if (j.is_array() || j.is_object()) + { + for (const auto& child : j) + { + ok = ok && child.m_parent == &j && parents_consistent(child); + } + } + return ok; +} +} // namespace + +TEST_CASE_TEMPLATE("Parent pointers stay consistent", J, json, ordered_json) +{ + SECTION("parse, copy, move, assign, swap") + { + J j = J::parse(R"({"a":[1,{"b":[2,3]},[4]],"c":{"d":{"e":[]}}})"); + CHECK(parents_consistent(j)); + + J copy(j); + CHECK(parents_consistent(copy)); + + J moved(std::move(copy)); + CHECK(parents_consistent(moved)); + + J assigned; + assigned = j; + CHECK(parents_consistent(assigned)); + + J other = {1, 2, {{"k", J::array({1, 2})}}}; + other.swap(assigned); + CHECK(parents_consistent(other)); + CHECK(parents_consistent(assigned)); + } + + SECTION("push_back growth") + { + J arr = J::array(); + for (int i = 0; i < 500; ++i) + { + arr.push_back(J::array({i})); + } + CHECK(parents_consistent(arr)); + + J obj = J::object(); + for (int i = 0; i < 500; ++i) + { + obj.push_back({std::to_string(i), J::array({i})}); + } + CHECK(parents_consistent(obj)); + } + + SECTION("emplace_back and emplace growth") + { + J arr = J::array(); + J obj = J::object(); + for (int i = 0; i < 500; ++i) + { + arr.emplace_back(J::array({i})); + obj.emplace(std::to_string(i), J::array({i})); + } + CHECK(parents_consistent(arr)); + CHECK(parents_consistent(obj)); + } + + SECTION("operator[] growth and at()") + { + J obj = J::object(); + for (int i = 0; i < 500; ++i) + { + obj[std::to_string(i)] = J::array({i}); + CHECK(obj[std::to_string(i / 2)].m_parent == &obj); + } + CHECK(parents_consistent(obj)); + + J arr = J::array(); + for (int i = 0; i < 500; ++i) + { + arr[static_cast(i)] = J::object({{"x", i}}); + } + for (size_t i = 0; i < 500; ++i) + { + CHECK(arr.at(i).m_parent == &arr); + } + CHECK(obj.at("3").m_parent == &obj); + CHECK(parents_consistent(arr)); + CHECK(parents_consistent(obj)); + } +} + +TEST_CASE_TEMPLATE("update() and merge_patch() keep parent pointers and diagnostic paths", J, json, ordered_json) +{ + // enough keys to make the vector behind ordered_json reallocate several times + const auto make_object = [](int first, int last) + { + J o = J::object(); + for (int i = first; i < last; ++i) + { + o[std::to_string(i)] = J::object({{"x", J::array({i})}}); + } + return o; + }; + + // const access, so operator[] cannot repair the parent pointers + const auto check_path = [](const J & j, const std::string & key) + { + const std::string expected = "[json.exception.type_error.304] (/" + key + "/x) cannot use at() with array"; + CHECK_THROWS_WITH_AS(j[key]["x"].at("y"), expected.c_str(), json::type_error); + }; + + SECTION("update(const_reference)") + { + J j = make_object(0, 200); + j.update(make_object(100, 400)); + CHECK(j.size() == 400); + CHECK(parents_consistent(j)); + check_path(j, "0"); + check_path(j, "150"); + check_path(j, "399"); + } + + SECTION("update(const_reference, merge_objects)") + { + J j = make_object(0, 200); + j["50"]["y"] = 1; + J other = make_object(100, 400); + other["50"] = J::object({{"z", J::array({1})}}); + j.update(other, true); + CHECK(j.size() == 400); + CHECK(j["50"].size() == 3); + CHECK(parents_consistent(j)); + check_path(j, "0"); + check_path(j, "399"); + } + + SECTION("update(first, last)") + { + J j = make_object(0, 200); + J other = make_object(100, 400); + j.update(other.begin(), other.end()); + CHECK(j.size() == 400); + CHECK(parents_consistent(j)); + check_path(j, "0"); + check_path(j, "399"); + + J k = make_object(0, 200); + k.update(other.begin(), other.end(), true); + CHECK(k.size() == 400); + CHECK(parents_consistent(k)); + check_path(k, "399"); + } + + SECTION("merge_patch()") + { + J j = make_object(0, 200); + J patch = make_object(100, 400); + patch["0"] = nullptr; + j.merge_patch(patch); + CHECK(j.size() == 399); + CHECK_FALSE(j.contains("0")); + CHECK(parents_consistent(j)); + check_path(j, "1"); + check_path(j, "399"); + } +}