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 <mail@nlohmann.me>
2026-09-24 06:10:22 +01:00 · 2026-09-23 22:45:53 +02:00
parent f751547a81
commit 57ed43b060
3 changed files with 134 additions and 16 deletions
@@ -896,6 +896,27 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
        return j;
    }

+    /// @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<object_t>::value)
+        {
+            set_parents();
+        }
+#ifdef JSON_HEDLEY_MSVC_VERSION
+#pragma warning( pop )
+#endif
+#endif
+    }
+
  public:
    //////////////////////////
    // JSON parser callback //
@@ -2629,6 +2650,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;
            }

@@ -2701,6 +2723,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;
            }

@@ -2731,7 +2754,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<KeyType>(key));
+        const auto erased = m_data.m_value.object->erase(std::forward<KeyType>(key));
+        set_parents_after_object_erase();
+        return erased;
    }

    template < typename KeyType, detail::enable_if_t <
@@ -2748,6 +2773,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;
@@ -3622,16 +3648,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(it.value(), true);
-#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());
        }
    }

@@ -25126,6 +25126,27 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
        return j;
    }

+    /// @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<object_t>::value)
+        {
+            set_parents();
+        }
+#ifdef JSON_HEDLEY_MSVC_VERSION
+#pragma warning( pop )
+#endif
+#endif
+    }
+
  public:
    //////////////////////////
    // JSON parser callback //
@@ -26859,6 +26880,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;
            }

@@ -26931,6 +26953,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;
            }

@@ -26961,7 +26984,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<KeyType>(key));
+        const auto erased = m_data.m_value.object->erase(std::forward<KeyType>(key));
+        set_parents_after_object_erase();
+        return erased;
    }

    template < typename KeyType, detail::enable_if_t <
@@ -26978,6 +27003,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;
@@ -27852,16 +27878,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(it.value(), true);
-#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());
        }
    }

@@ -304,5 +304,79 @@ 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);
+        }
+    }
 }