From d82ab217247242d6c1b56e1d8b2910556d7c5f66 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Sat, 5 Sep 2026 16:42:28 +0200 Subject: [PATCH] Make diff() account for member order in ordered_json objects diff() compared source/target objects purely by key set, ignoring relative member order. For ordered_json (insertion-ordered, vector- backed object_t), two objects that differ only in member order are unequal via operator==, but diff() never emitted any patch operation to fix the order, so source.patch(diff(source, target)) == target could fail to hold. Fix by detecting when common keys appear in a different relative order in source vs. target (or when a new key would need to land somewhere other than the end), and in that case removing and re-adding the affected keys in target's order, which relies on patch()'s "add" op appending new keys at the end of an ordered_map. For plain json (std::map-backed, always key-sorted iteration) this is a no-op and the original minimal per-key diff path is unchanged. Signed-off-by: Niels Lohmann --- include/nlohmann/json.hpp | 138 +++++++++++++++++++++++++++---- single_include/nlohmann/json.hpp | 138 +++++++++++++++++++++++++++---- tests/src/unit-ordered_json.cpp | 81 ++++++++++++++++++ 3 files changed, 327 insertions(+), 30 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index f90999c22..5ad522395 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -5159,21 +5159,13 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { - // first pass: traverse this object's elements + // first pass: find keys that were deleted (i.e., in source but not in target) for (auto it = source.cbegin(); it != source.cend(); ++it) { - // escape the key name to be used in a JSON patch - const auto path_key = detail::concat(path, '/', detail::escape(it.key())); - - if (target.find(it.key()) != target.end()) + if (target.find(it.key()) == target.end()) { - // recursive call to compare object values at key it - auto temp_diff = diff(it.value(), target[it.key()], path_key); - result.insert(result.end(), temp_diff.begin(), temp_diff.end()); - } - else - { - // found a key that is not in o -> remove it + // found a key that is not in target -> remove it + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back(object( { {"op", "remove"}, {"path", path_key} @@ -5181,12 +5173,108 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } } - // second pass: traverse other object's elements + // determine whether the relative order of the keys common to + // source and target already matches. For an object_t whose + // iteration order is a pure function of the key set (e.g., + // the default std::map, which always iterates in sorted key + // order), this is always true, so this check (and the "else" + // branch below) is effectively a no-op for a regular `json` + // object; it only matters for a reorderable object_t such as + // the one backing `ordered_json`. + std::vector order_in_source; + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + if (target.find(it.key()) != target.end()) + { + order_in_source.push_back(it.key()); + } + } + + std::vector order_in_target; for (auto it = target.cbegin(); it != target.cend(); ++it) { - if (source.find(it.key()) == source.end()) + if (source.find(it.key()) != source.end()) + { + order_in_target.push_back(it.key()); + } + } + + // The fast path below (like the original implementation) + // only ever recurses into common keys -- without touching + // their relative position -- and appends brand-new keys at + // the very end. That reproduces target's order only if the + // common keys already appear in target in the same relative + // order as in source, AND every brand-new key comes after + // every common key in target (i.e., the new keys form a + // suffix of target's key order); otherwise a new key would + // need to be inserted somewhere other than the end. + bool new_keys_form_suffix = true; + { + bool seen_new_key = false; + for (auto it = target.cbegin(); it != target.cend(); ++it) + { + if (source.find(it.key()) == source.end()) + { + seen_new_key = true; + } + else if (seen_new_key) + { + new_keys_form_suffix = false; + break; + } + } + } + + bool reordered = false; + + if (order_in_source == order_in_target && new_keys_form_suffix) + { + // fast path: order of common keys already matches (or the + // object_t's iteration order does not depend on + // insertion history), so a plain per-key recursive diff + // is correct and minimal, as before + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + if (target.find(it.key()) != target.end()) + { + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); + auto temp_diff = diff(it.value(), target[it.key()], path_key); + result.insert(result.end(), temp_diff.begin(), temp_diff.end()); + } + } + } + else + { + // slow path: the common keys are in a different relative + // order in source and target (only possible for a + // reorderable object_t like ordered_map). Building a + // minimal reordering patch is a nontrivial (LCS-like) + // problem; instead, remove every common key and re-add it + // (with its final target value) in target's order, which + // is enough to guarantee source.patch(diff(source, + // target)) == target. basic_json::patch()'s "add" + // operation on an object uses operator[], which appends + // at the end for a vector-backed insertion-ordered map + // when the key does not already exist -- so removing a + // key and then adding it moves it to the end, fixing its + // position. + reordered = true; + + for (const auto& key : order_in_source) + { + const auto path_key = detail::concat(path, '/', detail::escape(key)); + result.push_back(object( + { + {"op", "remove"}, {"path", path_key} + })); + } + + // add every key that is either common (just removed + // above) or brand new, in target's iteration order, so + // that the final order after applying the patch matches + // target exactly + for (auto it = target.cbegin(); it != target.cend(); ++it) { - // found a key that is not in this -> add it const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back( { @@ -5196,6 +5284,26 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } } + // second pass: find keys that were added (i.e., in target but + // not in source); already handled above when reordering, to + // keep them correctly interleaved with the moved keys + if (!reordered) + { + for (auto it = target.cbegin(); it != target.cend(); ++it) + { + if (source.find(it.key()) == source.end()) + { + // found a key that is not in source -> add it + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); + result.push_back( + { + {"op", "add"}, {"path", path_key}, + {"value", it.value()} + }); + } + } + } + break; } diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 6e38fbfa0..989735440 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -26587,21 +26587,13 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { - // first pass: traverse this object's elements + // first pass: find keys that were deleted (i.e., in source but not in target) for (auto it = source.cbegin(); it != source.cend(); ++it) { - // escape the key name to be used in a JSON patch - const auto path_key = detail::concat(path, '/', detail::escape(it.key())); - - if (target.find(it.key()) != target.end()) + if (target.find(it.key()) == target.end()) { - // recursive call to compare object values at key it - auto temp_diff = diff(it.value(), target[it.key()], path_key); - result.insert(result.end(), temp_diff.begin(), temp_diff.end()); - } - else - { - // found a key that is not in o -> remove it + // found a key that is not in target -> remove it + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back(object( { {"op", "remove"}, {"path", path_key} @@ -26609,12 +26601,108 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } } - // second pass: traverse other object's elements + // determine whether the relative order of the keys common to + // source and target already matches. For an object_t whose + // iteration order is a pure function of the key set (e.g., + // the default std::map, which always iterates in sorted key + // order), this is always true, so this check (and the "else" + // branch below) is effectively a no-op for a regular `json` + // object; it only matters for a reorderable object_t such as + // the one backing `ordered_json`. + std::vector order_in_source; + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + if (target.find(it.key()) != target.end()) + { + order_in_source.push_back(it.key()); + } + } + + std::vector order_in_target; for (auto it = target.cbegin(); it != target.cend(); ++it) { - if (source.find(it.key()) == source.end()) + if (source.find(it.key()) != source.end()) + { + order_in_target.push_back(it.key()); + } + } + + // The fast path below (like the original implementation) + // only ever recurses into common keys -- without touching + // their relative position -- and appends brand-new keys at + // the very end. That reproduces target's order only if the + // common keys already appear in target in the same relative + // order as in source, AND every brand-new key comes after + // every common key in target (i.e., the new keys form a + // suffix of target's key order); otherwise a new key would + // need to be inserted somewhere other than the end. + bool new_keys_form_suffix = true; + { + bool seen_new_key = false; + for (auto it = target.cbegin(); it != target.cend(); ++it) + { + if (source.find(it.key()) == source.end()) + { + seen_new_key = true; + } + else if (seen_new_key) + { + new_keys_form_suffix = false; + break; + } + } + } + + bool reordered = false; + + if (order_in_source == order_in_target && new_keys_form_suffix) + { + // fast path: order of common keys already matches (or the + // object_t's iteration order does not depend on + // insertion history), so a plain per-key recursive diff + // is correct and minimal, as before + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + if (target.find(it.key()) != target.end()) + { + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); + auto temp_diff = diff(it.value(), target[it.key()], path_key); + result.insert(result.end(), temp_diff.begin(), temp_diff.end()); + } + } + } + else + { + // slow path: the common keys are in a different relative + // order in source and target (only possible for a + // reorderable object_t like ordered_map). Building a + // minimal reordering patch is a nontrivial (LCS-like) + // problem; instead, remove every common key and re-add it + // (with its final target value) in target's order, which + // is enough to guarantee source.patch(diff(source, + // target)) == target. basic_json::patch()'s "add" + // operation on an object uses operator[], which appends + // at the end for a vector-backed insertion-ordered map + // when the key does not already exist -- so removing a + // key and then adding it moves it to the end, fixing its + // position. + reordered = true; + + for (const auto& key : order_in_source) + { + const auto path_key = detail::concat(path, '/', detail::escape(key)); + result.push_back(object( + { + {"op", "remove"}, {"path", path_key} + })); + } + + // add every key that is either common (just removed + // above) or brand new, in target's iteration order, so + // that the final order after applying the patch matches + // target exactly + for (auto it = target.cbegin(); it != target.cend(); ++it) { - // found a key that is not in this -> add it const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back( { @@ -26624,6 +26712,26 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec } } + // second pass: find keys that were added (i.e., in target but + // not in source); already handled above when reordering, to + // keep them correctly interleaved with the moved keys + if (!reordered) + { + for (auto it = target.cbegin(); it != target.cend(); ++it) + { + if (source.find(it.key()) == source.end()) + { + // found a key that is not in source -> add it + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); + result.push_back( + { + {"op", "add"}, {"path", path_key}, + {"value", it.value()} + }); + } + } + } + break; } diff --git a/tests/src/unit-ordered_json.cpp b/tests/src/unit-ordered_json.cpp index a38a1a2b8..62a949a7f 100644 --- a/tests/src/unit-ordered_json.cpp +++ b/tests/src/unit-ordered_json.cpp @@ -81,3 +81,84 @@ TEST_CASE("regression test for issue #3732 - iteration_proxy_value(fn); } + +TEST_CASE("regression test - diff() must account for ordered_json member order") +{ + SECTION("pure reorder, no value changes") + { + ordered_json a = {{"a", 1}, {"b", 2}}; + ordered_json b = {{"b", 2}, {"a", 1}}; + CHECK(a != b); // order-sensitive equality + CHECK(a.patch(ordered_json::diff(a, b)) == b); + } + + SECTION("new key must land at the front") + { + ordered_json c = {{"b", 2}}; + ordered_json e = {{"a", 1}, {"b", 2}}; + CHECK(c.patch(ordered_json::diff(c, e)) == e); + } + + SECTION("reorder plus a value change on one of the reordered keys") + { + ordered_json a = {{"a", 1}, {"b", 2}}; + ordered_json b = {{"b", 20}, {"a", 1}}; + CHECK(a != b); + CHECK(a.patch(ordered_json::diff(a, b)) == b); + } + + SECTION("reorder plus a deleted key") + { + ordered_json a = {{"a", 1}, {"b", 2}, {"c", 3}}; + ordered_json b = {{"b", 2}, {"a", 1}}; + CHECK(a != b); + CHECK(a.patch(ordered_json::diff(a, b)) == b); + } + + SECTION("reorder plus a nested value that itself needs a recursive diff") + { + ordered_json a = {{"a", {{"x", 1}, {"y", 2}}}, {"b", 2}}; + ordered_json b = {{"b", 2}, {"a", {{"x", 1}, {"y", 99}}}}; + CHECK(a != b); + CHECK(a.patch(ordered_json::diff(a, b)) == b); + } + + SECTION("three or more keys shuffled into a different order") + { + ordered_json a = {{"a", 1}, {"b", 2}, {"c", 3}, {"d", 4}}; + ordered_json b = {{"d", 4}, {"b", 2}, {"a", 1}, {"c", 3}}; + CHECK(a != b); + CHECK(a.patch(ordered_json::diff(a, b)) == b); + } + + SECTION("matching order still produces a minimal patch (fast path unaffected)") + { + ordered_json a = {{"a", 1}, {"b", 2}, {"c", 3}}; + ordered_json b = {{"a", 1}, {"b", 20}, {"c", 3}}; + auto p = ordered_json::diff(a, b); + // only the changed value should be touched, not a wholesale remove+add + CHECK(p.size() == 1); + CHECK(p[0]["op"] == "replace"); + CHECK(p[0]["path"] == "/b"); + CHECK(a.patch(p) == b); + } + + SECTION("plain json (std::map-backed) is unaffected by same-key-different-insertion-order") + { + json a; + a["b"] = 2; + a["a"] = 1; + + json b; + b["a"] = 1; + b["b"] = 2; + + // std::map iteration is always sorted by key, so a == b regardless of + // insertion order, and diff() must still produce the same minimal + // (empty) result as before this fix + CHECK(a == b); + auto p = json::diff(a, b); + CHECK(p.empty()); + CHECK(a.patch(p) == b); + } +}