From 5cbb8e6d6c107d4a257c84090d74bb54102da19a Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Fri, 9 Oct 2026 17:30:30 +0200 Subject: [PATCH] Assign the member that lookups find when set() meets duplicate keys An editable document reads the last member of a repeated key (as the read-only document does), but set(object, key, value) assigned the first one: a view taken from object[key] before the call did not show the new value. It now assigns the last member's value, keeps the key at the position of its first occurrence (where materialize() puts it), and drops the other members. The seeded differential test now also edits documents whose objects repeat keys and compares every lookup (operator[], at, find, value, contains, count, and JSON pointers) with the parsed basic_json value. A targeted test covers duplicates before and after edits that move the object, in large objects with an index, in moved arrays, and in values copied from other documents. Signed-off-by: Niels Lohmann --- .../docs/api/basic_json_document/erase.md | 4 +- .../docs/api/basic_json_document/set.md | 16 +- include/nlohmann/detail/view/edit.hpp | 45 +-- tests/src/unit-json_view_edit.cpp | 281 +++++++++++++++++- 4 files changed, 315 insertions(+), 31 deletions(-) diff --git a/docs/mkdocs/docs/api/basic_json_document/erase.md b/docs/mkdocs/docs/api/basic_json_document/erase.md index cbdc79765..25249fa73 100644 --- a/docs/mkdocs/docs/api/basic_json_document/erase.md +++ b/docs/mkdocs/docs/api/basic_json_document/erase.md @@ -86,8 +86,8 @@ view of a *different* document (overloads 1-2 only; overload 3 always starts fro !!! info "Duplicate keys" - Overload 1. removes *every* member with `key`, not just the first -- unlike [`set`](set.md), which assigns the - first occurrence and drops the rest. This is why it returns a count rather than a single view: there may be + Overload 1. removes *every* member with `key`, not just the last one that lookups find -- unlike [`set`](set.md), + which assigns that member and drops the rest. This is why it returns a count rather than a single view: there may be more than one member removed, or none. Like [`set`](set.md) and [`push_back`](push_back.md), `erase` never moves an element's *value*: a view still diff --git a/docs/mkdocs/docs/api/basic_json_document/set.md b/docs/mkdocs/docs/api/basic_json_document/set.md index ae26ed516..3f29a3f6f 100644 --- a/docs/mkdocs/docs/api/basic_json_document/set.md +++ b/docs/mkdocs/docs/api/basic_json_document/set.md @@ -23,7 +23,8 @@ has `set`; calling it on a read-only `basic_json_document` fails to compile (`#! 1. Replaces the value `target` refers to with `value`. 2. Sets the member `key` of the object `object` to `value`: assigns it if `object` already has a member with this - key -- the first one, should the key occur more than once, and the later duplicates are then dropped (see the + key -- the last one, should the key occur more than once (the member + [`operator[]`](../basic_json_view/operator%5B%5D.md) returns), and the other duplicates are then dropped (see the [Notes](#notes) below) -- or appends a new member at the end otherwise. A [null](../basic_json_view/is_null.md) `object` first becomes an empty object. 3. Assigns `value` to the element at index `idx` of the array `array`, which must already exist (`#!cpp idx < @@ -146,12 +147,13 @@ document") if `target`/`object`/`array` is a [discarded](../basic_json_view/is_d !!! info "Duplicate keys" - If `object` already has more than one member with `key` (2.), the *first* one is assigned `value` and every - later member with the same key is removed -- so that a lookup, an iteration, and - [`materialize()`](../basic_json_view/materialize.md) of `object` afterward all agree on a single value for - `key`, the same way [`operator[]`](../basic_json_view/operator%5B%5D.md) already picks the first occurrence of a - duplicate key for reading. See the [Notes on duplicate keys](../basic_json_view/operator%5B%5D.md#notes) of - `operator[]`. + If `object` already has more than one member with `key` (2.), `value` is assigned to the *last* one -- the member + [`operator[]`](../basic_json_view/operator%5B%5D.md), [`at`](../basic_json_view/at.md), and + [`find`](../basic_json_view/find.md) return for reading, so that a view taken from `object["key"]` before the call + shows `value` afterward -- and every other member with the same key is removed. The key stays at the + position of its *first* occurrence, where [`materialize()`](../basic_json_view/materialize.md) puts it as well. A + lookup, an iteration, and `materialize()` of `object` afterward therefore all agree on a single member for `key`. See the + [Notes on duplicate keys](../basic_json_view/operator%5B%5D.md#notes) of `operator[]`. Setting a member (2.) or an element (3., through 4.) of an array or object whose elements have not been edited before switches it from its parsed layout to a growable block holding links to its elements; a later diff --git a/include/nlohmann/detail/view/edit.hpp b/include/nlohmann/detail/view/edit.hpp index 7a9daa6bc..57a0b0826 100644 --- a/include/nlohmann/detail/view/edit.hpp +++ b/include/nlohmann/detail/view/edit.hpp @@ -152,25 +152,23 @@ class editor { become_empty(o, value_t::object); } - // an existing member: assign it (and drop later duplicates, so that - // lookups, iteration, and materialize() agree) + // an existing member: assign the one that lookups find (the last + // one, should the key occur more than once), and drop the others, so + // that lookups, iteration, and materialize() agree. The key stays at + // the position of its first occurrence, as materialize() puts it. node* slot = nullptr; - bool duplicates = false; + std::size_t matches = 0; for (const node* k = nav::first(m_doc, o), *end = nav::end(m_doc, o); k != end; k = document_data::after(k + 1)) { if (key_equals(*k, key)) { - if (slot != nullptr) - { - duplicates = true; - break; - } slot = const_cast(nav::value(k + 1)); // NOLINT(cppcoreguidelines-pro-type-const-cast): the nodes belong to this document + ++matches; } } if (slot != nullptr) { - if (duplicates) + if (matches > 1) { erase_members(o, key, true); } @@ -316,27 +314,38 @@ class editor return k.len == key.size() && (key.size() == 0 || std::memcmp(m_doc.str(k), key.data(), key.size()) == 0); } - /// remove the members with this key (all, or all but the first) from an object - std::size_t erase_members(node* o, string_view_t key, bool keep_first) + /// Remove the members with this key from an object: all of them, or all + /// but one. That one stays where the first occurrence is, but holds the + /// value of the last (the one that lookups find, which views may refer to). + std::size_t erase_members(node* o, string_view_t key, bool keep_one) { node* const h = block_of(m_doc, o, 0); + node last_value{}; // the entry of the value of the last member + node* const end = h + h->next; + if (keep_one) + { + for (node* r = h + 1; r != end; r += 2) + { + if (key_equals(*r, key)) + { + last_value = r[1]; + } + } + } node* w = h + 1; std::size_t erased = 0; bool kept = false; - for (node* r = h + 1, *end = h + h->next; r != end; r += 2) + for (node* r = h + 1; r != end; r += 2) { const bool match = key_equals(*r, key); - if (match && (kept || !keep_first)) + if (match && (kept || !keep_one)) { ++erased; continue; } + w[0] = r[0]; + w[1] = match ? last_value : r[1]; kept = kept || match; - if (w != r) - { - w[0] = r[0]; - w[1] = r[1]; - } w += 2; } h->next = static_cast(w - h); diff --git a/tests/src/unit-json_view_edit.cpp b/tests/src/unit-json_view_edit.cpp index d58d4b3ca..3502b0d07 100644 --- a/tests/src/unit-json_view_edit.cpp +++ b/tests/src/unit-json_view_edit.cpp @@ -180,6 +180,104 @@ void compare(const ordered_json_editable_view& v, const ordered_json& j) } } +// the text of j, in which some objects repeat a key of theirs: before their +// members (the real member is then the last), or after (the repeat is) +std::string text_with_duplicates(const ordered_json& j) +{ + if (j.is_object()) + { + std::vector keys; + for (const auto& kv : j.items()) + { + keys.push_back(kv.key()); + } + const auto repeated = [&keys]() + { + return ordered_json(keys[static_cast(r(static_cast(keys.size())))]).dump() + ":" + random_value(2).dump(); + }; + std::string text = "{"; + if (!keys.empty() && r(4) == 0) + { + text += repeated() + ","; + } + bool first = true; + for (const auto& kv : j.items()) + { + text += (first ? "" : ",") + ordered_json(kv.key()).dump() + ":" + text_with_duplicates(kv.value()); + first = false; + } + for (int i = keys.empty() ? 0 : r(3); i > 0; --i) + { + text += "," + repeated(); + } + return text + "}"; + } + if (j.is_array()) + { + std::string text = "["; + for (std::size_t i = 0; i < j.size(); ++i) + { + text += (i != 0 ? "," : "") + text_with_duplicates(j[i]); + } + return text + "]"; + } + return j.dump(); +} + +// every lookup of the edited view finds what j holds, although the view may +// have several members for a key (j has the last value, at the position of the +// first member: what parse() and materialize() make of it) +void check_lookups(const ordered_json_editable_view& v, const ordered_json& j) +{ + REQUIRE(v.type() == j.type()); + if (j.is_object()) + { + for (const auto& kv : j.items()) + { + const std::string& key = kv.key(); + const ptr_t ptr = ptr_t() / key; + CAPTURE(key) + const ordered_json_editable_view m = v[key]; + REQUIRE(!m.is_discarded()); + CHECK(m.materialize() == kv.value()); + CHECK(v.at(key).materialize() == kv.value()); + CHECK(v[ptr].materialize() == kv.value()); + CHECK(v.at(ptr).materialize() == kv.value()); + CHECK(v.contains(key)); + CHECK(v.contains(ptr)); + CHECK(v.count(key) == 1); + const auto it = v.find(key); + REQUIRE(it != v.end()); + CHECK((*it).materialize() == kv.value()); + CHECK(v.value(ptr, ordered_json(nullptr)) == kv.value()); + if (kv.value().is_string()) + { + CHECK(v.value(key, std::string("-")) == kv.value().get()); + } + check_lookups(m, kv.value()); + } + CHECK(v["missing#key"].is_discarded()); + CHECK(!v.contains("missing#key")); + CHECK(v.find("missing#key") == v.end()); + } + else if (j.is_array()) + { + for (std::size_t i = 0; i < j.size(); ++i) + { + check_lookups(v[i], j[i]); + check_lookups(v.at(i), j[i]); + } + } +} + +// (for documents whose objects repeat keys: dump(), size(), and iteration +// list all members, so only the lookups and materialize() are compared) +void check_duplicates(const ordered_json_editable_document& d, const ordered_json& j) +{ + CHECK(d.root().materialize() == j); + check_lookups(d.root(), j); +} + void check_all(const ordered_json_editable_document& d, const ordered_json& j, bool deep) { const std::string text = d.root().dump(); @@ -203,14 +301,16 @@ TEST_CASE("json_view edits: differential") // random edits are applied to an ordered_json_editable_document and to the // ordered_json parse() produces; after every edit both must serialize, // materialize, and read back the same - for (int n = 0; n < 150; ++n) + for (int n = 0; n < 250; ++n) { + // the last 100 documents repeat keys in some of their objects + const bool duplicates = n >= 150; ordered_json j = random_value(0); if (r(4) == 0) { j = ordered_json::object({{"a", random_value(1)}, {"b", random_value(1)}}); } - const std::string text = j.dump(r(2) == 0 ? -1 : 2); + const std::string text = duplicates ? text_with_duplicates(j) : j.dump(r(2) == 0 ? -1 : 2); CAPTURE(text) ordered_json_editable_document d = ordered_json_editable_document::parse(text); j = ordered_json::parse(text); @@ -339,7 +439,14 @@ TEST_CASE("json_view edits: differential") } CAPTURE(p.to_string()) CAPTURE(op) - check_all(d, j, e % 8 == 7 || e == edits - 1); + if (duplicates) + { + check_duplicates(d, j); + } + else + { + check_all(d, j, e % 8 == 7 || e == edits - 1); + } } } } @@ -503,7 +610,7 @@ TEST_CASE("json_view edits: views and values") SECTION("duplicate keys") { json_editable_document d = json_editable_document::parse(R"({"a": 1, "b": 2, "a": 3})"); - d.set(d.root(), "a", 4); // the first member is assigned, the others dropped + d.set(d.root(), "a", 4); // the last member is assigned (at the position of the first), the others dropped CHECK(d.root().dump() == R"({"a":4,"b":2})"); d = json_editable_document::parse(R"({"a": 1, "b": 2, "a": 3})"); CHECK(d.erase(d.root(), "a") == 2); @@ -787,3 +894,169 @@ TEST_CASE("json_view edits: replacing arrays and objects by scalars") CHECK(d.root().dump() == expected.dump()); CHECK(d.root().materialize() == expected); } + +namespace +{ +// reads of "a" in an object that repeats it: the last member wins, however the +// object is stored +template +void check_last_wins(const View& o, int expected) +{ + CAPTURE(expected) + CHECK(o["a"].template get() == expected); + CHECK(o.at("a").template get() == expected); + CHECK(o.find("a")->template get() == expected); + CHECK(o.value("a", -1) == expected); + CHECK(o[json::json_pointer("/a")].template get() == expected); + CHECK(o.at(json::json_pointer("/a")).template get() == expected); + CHECK(o.value(json::json_pointer("/a"), -1) == expected); + CHECK(o.contains("a")); + CHECK(o.contains(json::json_pointer("/a"))); + CHECK(o.count("a") == 1); +} +} // namespace + +TEST_CASE("json_view edits: duplicate keys") +{ + SECTION("an object in its parsed layout, then moved by edits") + { + json_editable_document d = json_editable_document::parse(R"({"a": 1, "b": 2, "a": 3})"); + check_last_wins(d.root(), 3); + CHECK(d.root().size() == 3); + CHECK(d.root().dump() == R"({"a":1,"b":2,"a":3})"); + + // an edit of a value does not move the object + d.set(d.root()["b"], 5); + check_last_wins(d.root(), 3); + d.set(d.root()["a"], 4); // the member that reads find + check_last_wins(d.root(), 4); + CHECK(d.root().dump() == R"({"a":1,"b":5,"a":4})"); + + // an appended member moves it + d.set(d.root(), "c", true); + check_last_wins(d.root(), 4); + CHECK(d.root().size() == 4); + CHECK(d.root().dump() == R"({"a":1,"b":5,"a":4,"c":true})"); + d.set(d.root()["a"], 6); + check_last_wins(d.root(), 6); + CHECK(d.root().dump() == R"({"a":1,"b":5,"a":6,"c":true})"); + d.set(json::json_pointer("/a"), 7); + check_last_wins(d.root(), 7); + } + + SECTION("set assigns the member that reads find") + { + // the key keeps the position of its first occurrence (as in materialize()), the later members are dropped + ordered_json_editable_document d = ordered_json_editable_document::parse(R"({"a": 1, "b": 2, "a": 3, "c": 4, "a": 5})"); + const ordered_json_editable_view held = d.root()["a"]; + CHECK(held.get() == 5); + const ordered_json_editable_view assigned = d.set(d.root(), "a", "x"); + CHECK(held.get() == "x"); + CHECK(assigned.get() == "x"); + CHECK(d.root().dump() == R"({"a":"x","b":2,"c":4})"); + CHECK(d.root().materialize() == ordered_json::parse(R"({"a": "x", "b": 2, "c": 4})")); + CHECK(d.root().size() == 3); + + // as for the object that parse() makes of the text + ordered_json j = ordered_json::parse(R"({"a": 1, "b": 2, "a": 3, "c": 4, "a": 5})"); + j["a"] = "x"; + CHECK(d.root().materialize().dump() == j.dump()); + + // through a pointer, below a duplicate + d = ordered_json_editable_document::parse(R"({"a": {"x": 1}, "b": 2, "a": {"x": 3}})"); + d.set(ptr_t("/a/x"), 4); + CHECK(d.root().dump() == R"({"a":{"x":1},"b":2,"a":{"x":4}})"); + d.set(ptr_t("/a"), 0); + CHECK(d.root().dump() == R"({"a":0,"b":2})"); + } + + SECTION("erase removes every member") + { + json_editable_document d = json_editable_document::parse(R"({"a": 1, "b": 2, "a": 3})"); + d.set(d.root(), "c", 4); + check_last_wins(d.root(), 3); + CHECK(d.erase(d.root(), "a") == 2); + CHECK(d.root().dump() == R"({"b":2,"c":4})"); + CHECK(!d.root().contains("a")); + CHECK(d.root()["a"].is_discarded()); + CHECK(d.erase(d.root(), "a") == 0); + d = json_editable_document::parse(R"({"a": 1, "b": 2, "a": 3})"); + CHECK(d.erase(json::json_pointer("/a")) == 2); + CHECK(d.root().dump() == R"({"b":2})"); + } + + SECTION("a large object with an index") + { + std::string text = R"({"a":0,"k7":"first")"; + for (int i = 0; i < 200; ++i) + { + text += ",\"k" + std::to_string(i) + "\":" + std::to_string(i); + } + text += R"(,"a":1,"k7":"last","a":2})"; + json_editable_document d = json_editable_document::parse(text); + check_last_wins(d.root(), 2); + CHECK(d.root()["k7"].get_string() == "last"); + CHECK(d.root()["k199"].get() == 199); + + // a value assigned in place: the index stays in use + d.set(d.root()["a"], 3); + check_last_wins(d.root(), 3); + d.set(d.root()["k7"], "changed"); + CHECK(d.root()["k7"].get_string() == "changed"); + CHECK(d.root()["k199"].get() == 199); + + // an appended member moves the members: the lookup scans them + d.set(d.root(), "new", 1); + check_last_wins(d.root(), 3); + CHECK(d.root()["k7"].get_string() == "changed"); + CHECK(d.root()["k7"].get_string() == d.root().at("k7").get_string()); + CHECK(d.root()["k199"].get() == 199); + CHECK(d.root()["new"].get() == 1); + + // a value replaced by a container, in an object that was not moved + d = json_editable_document::parse(text); + d.set(d.root()["a"], json{{"x", 1}}); + CHECK(d.root()["a"]["x"].get() == 1); + CHECK(d.root()["k7"].get_string() == "last"); + CHECK(d.erase(d.root(), "k7") == 3); // (the key occurs three times) + CHECK(d.root()["k7"].is_discarded()); + CHECK(d.root()["a"]["x"].get() == 1); + } + + SECTION("objects in moved arrays and in new values") + { + json_editable_document d = json_editable_document::parse(R"([{"a": 1, "a": 2}, {"a": 3, "b": 0, "a": 4}])"); + check_last_wins(d.root()[0], 2); + check_last_wins(d.root()[1], 4); + d.insert(d.root(), 0, json::parse(R"({"a": 0})")); + d.push_back(d.root(), json::parse(R"({"a": 5, "a": 6})")); // (a basic_json value has one member) + check_last_wins(d.root()[1], 2); + check_last_wins(d.root()[2], 4); + CHECK(d.root()[3]["a"].get() == 6); + d.set(d.root()[1], "a", 8); + check_last_wins(d.root()[1], 8); + CHECK(d.root()[1].dump() == R"({"a":8})"); + d.erase(d.root(), 0); + check_last_wins(d.root()[1], 4); + CHECK(d.root().dump() == R"([{"a":8},{"a":3,"b":0,"a":4},{"a":6}])"); + } + + SECTION("values of other documents") + { + const json_document source = json_document::parse(R"({"list": [{"a": 1, "a": 2}], "o": {"b": {"a": 3, "a": 4}, "a": 5, "a": 6}})"); + json_editable_document d = json_editable_document::parse("{}"); + d.set(d.root(), "copy", source.root()["list"]); + d.set(d.root(), "o", source.root()["o"]); + // (the copies keep the repeated members) + CHECK(d.root().dump() == R"({"copy":[{"a":1,"a":2}],"o":{"b":{"a":3,"a":4},"a":5,"a":6}})"); + check_last_wins(d.root()["copy"][0], 2); + check_last_wins(d.root()["o"]["b"], 4); + check_last_wins(d.root()["o"], 6); + d.set(d.root()["o"]["b"], "c", 0); + check_last_wins(d.root()["o"]["b"], 4); + d.set(d.root()["o"], "d", 0); + check_last_wins(d.root()["o"], 6); + CHECK(d.root()["o"]["b"]["a"].get() == 4); + CHECK(d.root()["o"]["d"].get() == 0); + } +}