diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 1aafbf78a..0294a268d 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -5332,34 +5332,139 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { - // first pass: traverse this object's elements + // first pass: record, for every source key, whether it is + // common to both objects (in source's iteration order) or + // was deleted (i.e., in source but not in target) -- this is + // a by-product of the target.find() call already needed to + // tell the two cases apart, so it adds no extra lookups. The + // "remove" ops themselves are emitted later, interleaved + // with the recursive per-key diffs in the fast path below, + // to match source's original iteration order (as the + // original, pre-reordering-aware implementation did) instead + // of grouping all removes before all recursive diffs. + std::vector common_keys_source_order; 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()) { - // 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()); + common_keys_source_order.push_back(it.key()); + } + } + + // second pass: find keys that were added (i.e., in target but + // not in source), and record the keys common to both, in + // target's iteration order -- again a by-product of the + // source.find() call already needed to detect added keys. At + // the same time, determine whether every added key comes + // after every common key in target's order (a precondition + // for the fast path below, which only ever appends new keys + // at the very end): 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), the order + // check further below is always true and this whole + // mechanism is effectively a no-op; it only matters for a + // reorderable object_t such as the one backing `ordered_json`. + // patch ops for keys that were added (i.e., in target but not + // in source); built here so the fast path below can reuse + // them without a second source.find() per target key. Only + // used by the fast path -- the slow (reordering) path + // rebuilds "add" ops for every key itself. + std::vector common_keys_target_order; + basic_json added_ops(value_t::array); + 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; + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); + added_ops.push_back( + { + {"op", "add"}, {"path", path_key}, + {"value", it.value()} + }); } else { - // found a key that is not in o -> remove it + common_keys_target_order.push_back(it.key()); + if (seen_new_key) + { + new_keys_form_suffix = false; + } + } + } + + if (common_keys_source_order == common_keys_target_order && 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. common_keys_source_order + // is, by construction, the subsequence of source's keys + // that are common to both objects, in source's iteration + // order -- so it can be walked in lockstep with `source` + // using a cheap key comparison instead of another lookup. + // Deleted keys (those source keys not in common_keys_source_order) + // are interleaved here too, in source's original order, to + // match the historical (pre-reordering-aware) output order. + auto common_it = common_keys_source_order.cbegin(); + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + if (common_it != common_keys_source_order.cend() && it.key() == *common_it) + { + 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()); + ++common_it; + } + else + { + // 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} + })); + } + } + + // append the "add" ops for brand-new keys collected above + // during the pass over target -- no second source.find() + // per target key needed + result.insert(result.end(), added_ops.begin(), added_ops.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 source key -- both + // deleted keys (which must be removed regardless) and + // common keys (removed so they can be re-added in + // target's order) -- and re-add every key that should + // remain, with its final target value, in target's + // order. 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. + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back(object( { {"op", "remove"}, {"path", path_key} })); } - } - // second pass: traverse other object's elements - for (auto it = target.cbegin(); it != target.cend(); ++it) - { - if (source.find(it.key()) == source.end()) + // 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( { diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 213236b51..7222dbb93 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -29257,34 +29257,139 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { - // first pass: traverse this object's elements + // first pass: record, for every source key, whether it is + // common to both objects (in source's iteration order) or + // was deleted (i.e., in source but not in target) -- this is + // a by-product of the target.find() call already needed to + // tell the two cases apart, so it adds no extra lookups. The + // "remove" ops themselves are emitted later, interleaved + // with the recursive per-key diffs in the fast path below, + // to match source's original iteration order (as the + // original, pre-reordering-aware implementation did) instead + // of grouping all removes before all recursive diffs. + std::vector common_keys_source_order; 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()) { - // 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()); + common_keys_source_order.push_back(it.key()); + } + } + + // second pass: find keys that were added (i.e., in target but + // not in source), and record the keys common to both, in + // target's iteration order -- again a by-product of the + // source.find() call already needed to detect added keys. At + // the same time, determine whether every added key comes + // after every common key in target's order (a precondition + // for the fast path below, which only ever appends new keys + // at the very end): 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), the order + // check further below is always true and this whole + // mechanism is effectively a no-op; it only matters for a + // reorderable object_t such as the one backing `ordered_json`. + // patch ops for keys that were added (i.e., in target but not + // in source); built here so the fast path below can reuse + // them without a second source.find() per target key. Only + // used by the fast path -- the slow (reordering) path + // rebuilds "add" ops for every key itself. + std::vector common_keys_target_order; + basic_json added_ops(value_t::array); + 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; + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); + added_ops.push_back( + { + {"op", "add"}, {"path", path_key}, + {"value", it.value()} + }); } else { - // found a key that is not in o -> remove it + common_keys_target_order.push_back(it.key()); + if (seen_new_key) + { + new_keys_form_suffix = false; + } + } + } + + if (common_keys_source_order == common_keys_target_order && 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. common_keys_source_order + // is, by construction, the subsequence of source's keys + // that are common to both objects, in source's iteration + // order -- so it can be walked in lockstep with `source` + // using a cheap key comparison instead of another lookup. + // Deleted keys (those source keys not in common_keys_source_order) + // are interleaved here too, in source's original order, to + // match the historical (pre-reordering-aware) output order. + auto common_it = common_keys_source_order.cbegin(); + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + if (common_it != common_keys_source_order.cend() && it.key() == *common_it) + { + 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()); + ++common_it; + } + else + { + // 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} + })); + } + } + + // append the "add" ops for brand-new keys collected above + // during the pass over target -- no second source.find() + // per target key needed + result.insert(result.end(), added_ops.begin(), added_ops.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 source key -- both + // deleted keys (which must be removed regardless) and + // common keys (removed so they can be re-added in + // target's order) -- and re-add every key that should + // remain, with its final target value, in target's + // order. 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. + for (auto it = source.cbegin(); it != source.cend(); ++it) + { + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back(object( { {"op", "remove"}, {"path", path_key} })); } - } - // second pass: traverse other object's elements - for (auto it = target.cbegin(); it != target.cend(); ++it) - { - if (source.find(it.key()) == source.end()) + // 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( { 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); + } +}