From 450fc8dce77ea2b90f26356cf754d80381ac9996 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Sat, 5 Sep 2026 18:42:17 +0200 Subject: [PATCH] Preserve diff()'s original op ordering and fix a slow-path deletion gap Splitting removed-key detection and common-key recursion into separate passes (for the earlier lookup-count fix) changed the emitted patch's op order: all "remove" ops now came before all recursive per-key diffs, instead of interleaved in source's iteration order as the original implementation did. This broke docs/mkdocs/docs/examples/diff.output's exact-match CI check (ci_test_examples) even though the patch was still semantically correct. Defer "remove" emission into the same walk that does the recursive diffs, so common keys and deleted keys are interleaved in source order again, matching historical output. While restructuring that walk, the reordering ("slow path") branch was only emitting "remove" for keys common to both objects, never for keys present in source but genuinely absent from target -- a key deleted alongside an actual reorder would silently survive the patch. Fixed by removing every source key in the slow path (both deleted and common keys need removing there; common keys are then re-added in target's order). Verified with a targeted reorder+deletion case and a fresh 20,000-case round-trip fuzz run (0 failures). Signed-off-by: Niels Lohmann --- include/nlohmann/json.hpp | 65 ++++++++++++++++++-------------- single_include/nlohmann/json.hpp | 65 ++++++++++++++++++-------------- 2 files changed, 74 insertions(+), 56 deletions(-) diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index 38090da22..250d3c9a4 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -5159,24 +5159,20 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { - // first pass: find keys that were deleted (i.e., in source but - // not in target), and record the keys common to both, in - // source's iteration order -- this is a by-product of the - // target.find() call already needed to detect removed keys, - // so it adds no extra lookups. + // 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) { - if (target.find(it.key()) == target.end()) - { - // 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} - })); - } - else + if (target.find(it.key()) != target.end()) { common_keys_source_order.push_back(it.key()); } @@ -5236,16 +5232,28 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // 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() && common_it != common_keys_source_order.cend(); ++it) + for (auto it = source.cbegin(); it != source.cend(); ++it) { - if (it.key() == *common_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 @@ -5259,18 +5267,19 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // 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. - for (const auto& key : common_keys_source_order) + // 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(key)); + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back(object( { {"op", "remove"}, {"path", path_key} diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index d8233fb70..66360671c 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -26587,24 +26587,20 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec case value_t::object: { - // first pass: find keys that were deleted (i.e., in source but - // not in target), and record the keys common to both, in - // source's iteration order -- this is a by-product of the - // target.find() call already needed to detect removed keys, - // so it adds no extra lookups. + // 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) { - if (target.find(it.key()) == target.end()) - { - // 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} - })); - } - else + if (target.find(it.key()) != target.end()) { common_keys_source_order.push_back(it.key()); } @@ -26664,16 +26660,28 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // 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() && common_it != common_keys_source_order.cend(); ++it) + for (auto it = source.cbegin(); it != source.cend(); ++it) { - if (it.key() == *common_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 @@ -26687,18 +26695,19 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec // 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. - for (const auto& key : common_keys_source_order) + // 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(key)); + const auto path_key = detail::concat(path, '/', detail::escape(it.key())); result.push_back(object( { {"op", "remove"}, {"path", path_key}