Only track the target key order in diff() for insertion-ordered objects

common_keys_target_order and new_keys_form_suffix are only read when object_t keeps its members in insertion order; skip building them otherwise. Addresses review comment by @gregmarr.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

include/nlohmann/json.hpp

@@ -6150,12 +6150,10 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
                 // 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`.
+                // at the very end). Both are only needed for an object_t that
+                // keeps its members in insertion order, such as the one
+                // backing `ordered_json`; for any other object_t, the fast
+                // path is always taken and they are not computed.
                 // 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
@@ -6179,15 +6177,32 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
                     }
                     else
                     {
-                        common_keys_target_order.push_back(it.key());
-                        if (seen_new_key)
+#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)
                         {
-                            new_keys_form_suffix = false;
+                            common_keys_target_order.push_back(it.key());
+                            if (seen_new_key)
+                            {
+                                new_keys_form_suffix = false;
+                            }
                         }
+#ifdef JSON_HEDLEY_MSVC_VERSION
+#pragma warning( pop )
+#endif
                     }
                 }
 
-                if (common_keys_source_order == common_keys_target_order && new_keys_form_suffix)
+                // Only an object type that keeps its members in insertion
+                // order, such as nlohmann::ordered_map, can need reordering:
+                // patch() appends a new member at the end of such an object.
+                // Any other object type places its members itself - std::map
+                // in key order, a hash map in an order its operator== ignores -
+                // so a member-by-member diff always reproduces target there.
+                if (!detail::is_ordered_map<object_t>::value
+                        || (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

single_include/nlohmann/json.hpp

@@ -32231,12 +32231,10 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
                 // 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`.
+                // at the very end). Both are only needed for an object_t that
+                // keeps its members in insertion order, such as the one
+                // backing `ordered_json`; for any other object_t, the fast
+                // path is always taken and they are not computed.
                 // 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
@@ -32260,15 +32258,32 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
                     }
                     else
                     {
-                        common_keys_target_order.push_back(it.key());
-                        if (seen_new_key)
+#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)
                         {
-                            new_keys_form_suffix = false;
+                            common_keys_target_order.push_back(it.key());
+                            if (seen_new_key)
+                            {
+                                new_keys_form_suffix = false;
+                            }
                         }
+#ifdef JSON_HEDLEY_MSVC_VERSION
+#pragma warning( pop )
+#endif
                     }
                 }
 
-                if (common_keys_source_order == common_keys_target_order && new_keys_form_suffix)
+                // Only an object type that keeps its members in insertion
+                // order, such as nlohmann::ordered_map, can need reordering:
+                // patch() appends a new member at the end of such an object.
+                // Any other object type places its members itself - std::map
+                // in key order, a hash map in an order its operator== ignores -
+                // so a member-by-member diff always reproduces target there.
+                if (!detail::is_ordered_map<object_t>::value
+                        || (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

tests/src/unit-json_patch.cpp

@@ -1752,6 +1752,58 @@ TEST_CASE("JSON patch - diff emits array removals in descending index order")
     }
 }
 
+TEST_CASE("JSON patch - diff() takes the fast path for non-reorderable object types (regression #5639)")
+{
+    // #5465 added an order check to diff()'s object handling so a
+    // member-by-member diff is only used when it would also reproduce
+    // target's member *order* -- needed for ordered_json, whose object_t
+    // keeps insertion order and whose patch() "add" op appends a new
+    // member at the end. For json's default object_t (std::map, which
+    // orders members by key regardless of insertion history), that check
+    // could still fail: a new key that sorts before an existing common key
+    // makes target's iteration interleave the new key between common keys,
+    // even though nothing else about the object changed. That sent the
+    // whole object through the slow (remove-every-member,
+    // re-add-every-member) path instead of the minimal one.
+    SECTION("json: added key sorts before an existing common key")
+    {
+        const json source = {{"a", 1}, {"c", {{"x", 1}, {"y", 2}}}};
+        const json target = {{"a", 1}, {"b", 0}, {"c", {{"x", 1}, {"y", 2}}}};
+
+        const json patch = json::diff(source, target);
+
+        // only the new key is added; "a" and "c" are left alone instead of
+        // being removed and re-added
+        const json expected = R"([{"op": "add", "path": "/b", "value": 0}])"_json;
+        CHECK(patch == expected);
+        CHECK(source.patch(patch) == target);
+    }
+
+    SECTION("ordered_json: reordering behavior from #5465 is unchanged")
+    {
+        using nlohmann::ordered_json;
+
+        // same key/value shape as the json case above, but for ordered_json
+        // the *target*'s member order must be reproduced, so the slow path
+        // is still required here.
+        ordered_json source;
+        source["a"] = 1;
+        source["c"] = ordered_json{{"x", 1}, {"y", 2}};
+
+        ordered_json target;
+        target["a"] = 1;
+        target["b"] = 0;
+        target["c"] = ordered_json{{"x", 1}, {"y", 2}};
+
+        const ordered_json patch = ordered_json::diff(source, target);
+
+        // unlike the json case: every member is still removed and re-added
+        // so the result ends up in target's order (2 removes + 3 adds)
+        CHECK(patch.size() == 5);
+        CHECK(source.patch(patch) == target);
+    }
+}
+
 TEST_CASE("JSON patch - every operation on ordered_json")
 {
     using nlohmann::ordered_json;