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 <mail@nlohmann.me>

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
+                // first pass: record, for every source key, whether it is
-                // not in target), and record the keys common to both, in
+                // common to both objects (in source's iteration order) or
-                // source's iteration order -- this is a by-product of the
+                // was deleted (i.e., in source but not in target) -- this is
-                // target.find() call already needed to detect removed keys,
+                // a by-product of the target.find() call already needed to
-                // so it adds no extra lookups.
+                // 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<typename object_t::key_type> common_keys_source_order;
                 for (auto it = source.cbegin(); it != source.cend(); ++it)
                 {
-                    if (target.find(it.key()) == target.end())
+                    if (target.find(it.key()) != target.end())
-                    {
-                        // found a key that is not in target -> remove it
-                        const auto path_key = detail::concat<string_t>(path, '/', detail::escape(it.key()));
-                        result.push_back(object(
-                        {
-                            {"op", "remove"}, {"path", path_key}
-                        }));
-                    }
-                    else
                     {
                         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<string_t>(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<string_t>(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
+                    // problem; instead, remove every source key -- both
-                    // (with its final target value) in target's order, which
+                    // deleted keys (which must be removed regardless) and
-                    // is enough to guarantee source.patch(diff(source,
+                    // common keys (removed so they can be re-added in
-                    // target)) == target. basic_json::patch()'s "add"
+                    // target's order) -- and re-add every key that should
-                    // operation on an object uses operator[], which appends
+                    // remain, with its final target value, in target's
-                    // at the end for a vector-backed insertion-ordered map
+                    // order. basic_json::patch()'s "add" operation on an
-                    // when the key does not already exist -- so removing a
+                    // object uses operator[], which appends at the end for a
-                    // key and then adding it moves it to the end, fixing its
+                    // vector-backed insertion-ordered map when the key does
-                    // position.
+                    // not already exist -- so removing a key and then adding
-                    for (const auto& key : common_keys_source_order)
+                    // it moves it to the end, fixing its position.
+                    for (auto it = source.cbegin(); it != source.cend(); ++it)
                     {
-                        const auto path_key = detail::concat<string_t>(path, '/', detail::escape(key));
+                        const auto path_key = detail::concat<string_t>(path, '/', detail::escape(it.key()));
                         result.push_back(object(
                         {
                             {"op", "remove"}, {"path", path_key}

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
+                // first pass: record, for every source key, whether it is
-                // not in target), and record the keys common to both, in
+                // common to both objects (in source's iteration order) or
-                // source's iteration order -- this is a by-product of the
+                // was deleted (i.e., in source but not in target) -- this is
-                // target.find() call already needed to detect removed keys,
+                // a by-product of the target.find() call already needed to
-                // so it adds no extra lookups.
+                // 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<typename object_t::key_type> common_keys_source_order;
                 for (auto it = source.cbegin(); it != source.cend(); ++it)
                 {
-                    if (target.find(it.key()) == target.end())
+                    if (target.find(it.key()) != target.end())
-                    {
-                        // found a key that is not in target -> remove it
-                        const auto path_key = detail::concat<string_t>(path, '/', detail::escape(it.key()));
-                        result.push_back(object(
-                        {
-                            {"op", "remove"}, {"path", path_key}
-                        }));
-                    }
-                    else
                     {
                         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<string_t>(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<string_t>(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
+                    // problem; instead, remove every source key -- both
-                    // (with its final target value) in target's order, which
+                    // deleted keys (which must be removed regardless) and
-                    // is enough to guarantee source.patch(diff(source,
+                    // common keys (removed so they can be re-added in
-                    // target)) == target. basic_json::patch()'s "add"
+                    // target's order) -- and re-add every key that should
-                    // operation on an object uses operator[], which appends
+                    // remain, with its final target value, in target's
-                    // at the end for a vector-backed insertion-ordered map
+                    // order. basic_json::patch()'s "add" operation on an
-                    // when the key does not already exist -- so removing a
+                    // object uses operator[], which appends at the end for a
-                    // key and then adding it moves it to the end, fixing its
+                    // vector-backed insertion-ordered map when the key does
-                    // position.
+                    // not already exist -- so removing a key and then adding
-                    for (const auto& key : common_keys_source_order)
+                    // it moves it to the end, fixing its position.
+                    for (auto it = source.cbegin(); it != source.cend(); ++it)
                     {
-                        const auto path_key = detail::concat<string_t>(path, '/', detail::escape(key));
+                        const auto path_key = detail::concat<string_t>(path, '/', detail::escape(it.key()));
                         result.push_back(object(
                         {
                             {"op", "remove"}, {"path", path_key}