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); + } +}