Fix quadratic parent updates for ordered_json with JSON_DIAGNOSTICS

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

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
+    /// would invalidate the parent pointers. Containers that do not have a
-    /// capacity() member function report unknown_size(), which is treated as
+    /// capacity() member function report unknown_size() if their elements may
-    /// "the elements may have moved".
+    /// 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<A>::value, int > = 0 >
+    template < typename C, detail::enable_if_t < detail::has_capacity<C>::value, int > = 0 >
-    std::size_t array_capacity() const noexcept
+    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<A>::value, int > = 0 >
+    template < typename C, detail::enable_if_t < !detail::has_capacity<C>::value, int > = 0 >
-    std::size_t array_capacity() const noexcept
+    static std::size_t capacity_of(const C& /*container*/) noexcept
     {
-        return detail::unknown_size();
+        return std::is_same<C, array_t>::value || detail::is_ordered_map<C>::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
+    /// @brief set the parent of a value that has just been added to an array or object
-    /// @param j the added value
+    /// @param j the added (or looked-up) value
-    /// @param old_capacity the value @ref array_capacity() returned before the
+    /// @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 capacity has changed, or the container type does not let us
-            // the elements may have moved, so update all parents
+            // tell: the elements may have moved, so update all parents
             set_parents();
             return j;
         }
 #else
         static_cast<void>(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<basic_json_t, KeyType>::value, int> = 0>
     reference at(KeyType && key)
     {
-        return set_parent(object_at(*this, std::forward<KeyType>(key)));
+        return set_parent_at(object_at(*this, std::forward<KeyType>(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<KeyType>(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>(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>(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
+            // operator[] repairs the other members if ordered_json relocates
-            // relocates when adding a key makes its vector grow
+            // them; assigning through it keeps this O(1) per key
-            set_parent(m_data.m_value.object->operator[](it.key()) = std::move(it.value()));
+            operator[](it.key()) = std::move(it.value());
         }
     }
 
@@ -5014,9 +5030,8 @@ public:
                     continue;
                 }
             }
-            // set_parent() also repairs the other members, which ordered_json
+            // operator[] repairs the other members if ordered_json relocates them
-            // relocates when adding a key makes its vector grow
+            target->operator[](first.key()) = std::move(first.value());
-            target->set_parent(target->m_data.m_value.object->operator[](first.key()) = std::move(first.value()));
             ++first;
         }
     }

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
+    /// would invalidate the parent pointers. Containers that do not have a
-    /// capacity() member function report unknown_size(), which is treated as
+    /// capacity() member function report unknown_size() if their elements may
-    /// "the elements may have moved".
+    /// 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<A>::value, int > = 0 >
+    template < typename C, detail::enable_if_t < detail::has_capacity<C>::value, int > = 0 >
-    std::size_t array_capacity() const noexcept
+    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<A>::value, int > = 0 >
+    template < typename C, detail::enable_if_t < !detail::has_capacity<C>::value, int > = 0 >
-    std::size_t array_capacity() const noexcept
+    static std::size_t capacity_of(const C& /*container*/) noexcept
     {
-        return detail::unknown_size();
+        return std::is_same<C, array_t>::value || detail::is_ordered_map<C>::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
+    /// @brief set the parent of a value that has just been added to an array or object
-    /// @param j the added value
+    /// @param j the added (or looked-up) value
-    /// @param old_capacity the value @ref array_capacity() returned before the
+    /// @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 capacity has changed, or the container type does not let us
-            // the elements may have moved, so update all parents
+            // tell: the elements may have moved, so update all parents
             set_parents();
             return j;
         }
 #else
         static_cast<void>(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<basic_json_t, KeyType>::value, int> = 0>
     reference at(KeyType && key)
     {
-        return set_parent(object_at(*this, std::forward<KeyType>(key)));
+        return set_parent_at(object_at(*this, std::forward<KeyType>(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<KeyType>(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>(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>(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
+            // operator[] repairs the other members if ordered_json relocates
-            // relocates when adding a key makes its vector grow
+            // them; assigning through it keeps this O(1) per key
-            set_parent(m_data.m_value.object->operator[](it.key()) = std::move(it.value()));
+            operator[](it.key()) = std::move(it.value());
         }
     }
 
@@ -34754,9 +34770,8 @@ public:
                     continue;
                 }
             }
-            // set_parent() also repairs the other members, which ordered_json
+            // operator[] repairs the other members if ordered_json relocates them
-            // relocates when adding a key makes its vector grow
+            target->operator[](first.key()) = std::move(first.value());
-            target->set_parent(target->m_data.m_value.object->operator[](first.key()) = std::move(first.value()));
             ++first;
         }
     }

tests/src/unit-diagnostics.cpp

@@ -13,9 +13,11 @@
 #endif
 
 #define JSON_DIAGNOSTICS 1
+#define JSON_TESTS_PRIVATE
 
 #include <nlohmann/json.hpp>
 using nlohmann::json;
+using nlohmann::ordered_json;
 
 #include <map>
 #include <unordered_map>
@@ -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<typename BasicJsonType>
+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<size_t>(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");
+    }
+}