From c97928080d39b704c5ae4b5518ab55a5d7c93c5e Mon Sep 17 00:00:00 2001 From: James Stone Date: Thu, 17 Jun 2021 12:53:46 -0700 Subject: [PATCH 1/3] fix notifications on list of primitive mixed --- CHANGELOG.md | 2 +- .../object-store/impl/deep_change_checker.cpp | 35 +++-- .../object-store/impl/deep_change_checker.hpp | 2 + test/object-store/primitive_list.cpp | 137 ++++++++++++++++++ 4 files changed, 162 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89ca52eb6d9..2abeea8c443 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* Fixed an assertion failure when listening for changes to a list of primitive Mixed which contains links. ([#4767](https://github.com/realm/realm-core/issues/4767), since the beginning of Mixed v11.0.0) ### Breaking changes * None. diff --git a/src/realm/object-store/impl/deep_change_checker.cpp b/src/realm/object-store/impl/deep_change_checker.cpp index ef7ef0b8c1b..61ae4c8a99f 100644 --- a/src/realm/object-store/impl/deep_change_checker.cpp +++ b/src/realm/object-store/impl/deep_change_checker.cpp @@ -108,6 +108,24 @@ DeepChangeChecker::DeepChangeChecker(TransactionChangeInfo const& info, Table co { } +template +bool DeepChangeChecker::do_check_for_collection_of_mixed(T* coll, size_t depth) +{ + TableRef cached_linked_table; + return std::any_of(coll->begin(), coll->end(), [&, this](auto value) { + if (value.is_type(type_TypedLink)) { + auto link = value.get_link(); + REALM_ASSERT(link); + if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { + cached_linked_table = coll->get_table()->get_parent_group()->get_table(link.get_table_key()); + REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); + } + return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); + } + return false; + }); +} + bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptr coll, size_t depth) { REALM_ASSERT(coll); @@ -117,6 +135,9 @@ bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptrcheck_row(*target, key.value, depth + 1); }); } + else if (auto list = dynamic_cast*>(coll.get())) { + return do_check_for_collection_of_mixed(list, depth); + } else if (auto dict = dynamic_cast(coll.get())) { auto target = dict->get_target_table(); TableRef cached_linked_table; @@ -146,19 +167,7 @@ bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptr*>(coll.get())) { - TableRef cached_linked_table; - return std::any_of(set->begin(), set->end(), [&, this](auto value) { - if (value.is_type(type_TypedLink)) { - auto link = value.get_link(); - REALM_ASSERT(link); - if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { - cached_linked_table = set->get_table()->get_parent_group()->get_table(link.get_table_key()); - REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); - } - return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); - } - return false; - }); + return do_check_for_collection_of_mixed(set, depth); } // at this point, we have not handled all datatypes REALM_UNREACHABLE(); diff --git a/src/realm/object-store/impl/deep_change_checker.hpp b/src/realm/object-store/impl/deep_change_checker.hpp index 3d83fcaaeb6..82086089a5f 100644 --- a/src/realm/object-store/impl/deep_change_checker.hpp +++ b/src/realm/object-store/impl/deep_change_checker.hpp @@ -81,6 +81,8 @@ class DeepChangeChecker { bool check_row(Table const& table, ObjKeyType obj_key, size_t depth = 0); bool check_outgoing_links(TableKey table_key, Table const& table, ObjKey obj_key, size_t depth = 0); bool do_check_for_collection_modifications(std::unique_ptr coll, size_t depth); + template + bool do_check_for_collection_of_mixed(T* coll, size_t depth); }; } // namespace _impl diff --git a/test/object-store/primitive_list.cpp b/test/object-store/primitive_list.cpp index ca91f1b1402..19382198641 100644 --- a/test/object-store/primitive_list.cpp +++ b/test/object-store/primitive_list.cpp @@ -798,3 +798,140 @@ TEMPLATE_TEST_CASE("primitive list", "[primitives]", cf::MixedVal, cf::Int, cf:: } #endif } + +TEST_CASE("list of mixed links", "[primitives]") { + InMemoryTestFile config; + config.cache = false; + config.automatic_change_notifications = false; + config.schema = Schema{ + {"object", {{"value", PropertyType::Array | PropertyType::Mixed | PropertyType::Nullable}}}, + {"target1", + {{"value1", PropertyType::Int}, {"link1", PropertyType::Object | PropertyType::Nullable, "target1"}}}, + {"target2", + {{"value2", PropertyType::Int}, {"link2", PropertyType::Object | PropertyType::Nullable, "target2"}}}}; + + auto r = Realm::get_shared_realm(config); + + auto table = r->read_group().get_table("class_object"); + auto target1 = r->read_group().get_table("class_target1"); + auto target2 = r->read_group().get_table("class_target2"); + ColKey col_value1 = target1->get_column_key("value1"); + ColKey col_value2 = target2->get_column_key("value2"); + ColKey col_link1 = target1->get_column_key("link1"); + r->begin_transaction(); + Obj obj = table->create_object(); + Obj obj1 = table->create_object(); // empty dictionary + Obj target1_obj = target1->create_object().set(col_value1, 100); + Obj target2_obj = target2->create_object().set(col_value2, 200); + ColKey col = table->get_column_key("value"); + + List list(r, obj, col); + CppContext ctx(r); + + list.add(Mixed{ObjLink(target1->get_key(), target1_obj.get_key())}); + list.add(Mixed{}); + list.add(Mixed{}); + list.add(Mixed{int64_t{42}}); + r->commit_transaction(); + + Results all_objects(r, table->where()); + REQUIRE(all_objects.size() == 2); + CollectionChangeSet local_changes; + auto x = all_objects.add_notification_callback([&local_changes](CollectionChangeSet c, std::exception_ptr) { + local_changes = c; + }); + advance_and_notify(*r); + local_changes = {}; + + SECTION("insertion") { + r->begin_transaction(); + table->create_object(); + r->commit_transaction(); + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 1); + REQUIRE(local_changes.modifications.count() == 0); + REQUIRE(local_changes.deletions.count() == 0); + } + SECTION("add a normal item is a modification") { + r->begin_transaction(); + list.add(Mixed{"hello"}); + r->commit_transaction(); + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 1); + REQUIRE(local_changes.deletions.count() == 0); + } + SECTION("modify an existing item is a modification") { + r->begin_transaction(); + list.set(0, Mixed{}); + r->commit_transaction(); + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 1); + REQUIRE(local_changes.deletions.count() == 0); + } + SECTION("modify a linked object is a modification") { + r->begin_transaction(); + target1_obj.set(col_value1, 1000); + r->commit_transaction(); + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 1); + REQUIRE(local_changes.deletions.count() == 0); + } + SECTION("modify a linked object once removed is a modification") { + r->begin_transaction(); + auto target1_obj2 = target1->create_object().set(col_value1, 1000); + target1_obj.set(col_link1, target1_obj2.get_key()); + r->commit_transaction(); + advance_and_notify(*r); + r->begin_transaction(); + target1_obj2.set(col_value1, 2000); + r->commit_transaction(); + local_changes = {}; + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 1); + REQUIRE(local_changes.deletions.count() == 0); + } + SECTION("adding a link to a new table is a modification") { + r->begin_transaction(); + list.add(Mixed{ObjLink(target2->get_key(), target2_obj.get_key())}); + r->commit_transaction(); + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 1); + REQUIRE(local_changes.deletions.count() == 0); + + SECTION("changing a property from the newly linked table is a modification") { + r->begin_transaction(); + target2_obj.set(col_value2, 42); + r->commit_transaction(); + local_changes = {}; + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 1); + REQUIRE(local_changes.deletions.count() == 0); + } + } + SECTION("adding a link to a new table and rolling back is not a modification") { + r->begin_transaction(); + list.add(Mixed{ObjLink(target2->get_key(), target2_obj.get_key())}); + r->cancel_transaction(); + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 0); + REQUIRE(local_changes.deletions.count() == 0); + + SECTION("changing a property from rollback linked table is not a modification") { + r->begin_transaction(); + target2_obj.set(col_value2, 42); + r->commit_transaction(); + local_changes = {}; + advance_and_notify(*r); + REQUIRE(local_changes.insertions.count() == 0); + REQUIRE(local_changes.modifications.count() == 0); + REQUIRE(local_changes.deletions.count() == 0); + } + } +} From 8b67efdf3c1bbaa11157b80628730ac2cdfb09a9 Mon Sep 17 00:00:00 2001 From: James Stone Date: Thu, 17 Jun 2021 16:49:10 -0700 Subject: [PATCH 2/3] fix handling of unresolved links in dict/set when checking deep changes --- CHANGELOG.md | 1 + src/realm/obj.cpp | 24 +- .../object-store/impl/deep_change_checker.cpp | 25 +- test/object-store/transaction_log_parsing.cpp | 227 ++++++++++++++++-- 4 files changed, 244 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2abeea8c443..0df1900ac1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) * Fixed an assertion failure when listening for changes to a list of primitive Mixed which contains links. ([#4767](https://github.com/realm/realm-core/issues/4767), since the beginning of Mixed v11.0.0) +* Fixed an assertion failure when listening for changes to a dictionary or set which contains an invalidated link. ([#4770](https://github.com/realm/realm-core/pull/4770), since the beginning of v11) ### Breaking changes * None. diff --git a/src/realm/obj.cpp b/src/realm/obj.cpp index 1b38edc2944..77787464b3a 100644 --- a/src/realm/obj.cpp +++ b/src/realm/obj.cpp @@ -2150,12 +2150,26 @@ void Obj::assign_pk_and_backlinks(const Obj& other) REALM_ASSERT(!linking_obj.get(c) || linking_obj.get(c) == other.get_key()); linking_obj.set(c, get_key()); } + else if (c.is_list()) { + if (c.get_type() == col_type_Mixed) { + auto l = linking_obj.get_list(c); + auto n = l.find_first(ObjLink{m_table->get_key(), other.get_key()}); + REALM_ASSERT(n != realm::npos); + l.set(n, ObjLink{m_table->get_key(), get_key()}); + } + else if (c.get_type() == col_type_LinkList) { + // Link list + auto l = linking_obj.get_list(c); + auto n = l.find_first(other.get_key()); + REALM_ASSERT(n != realm::npos); + l.set(n, get_key()); + } + else { + REALM_UNREACHABLE(); // missing type handling + } + } else { - // Link list - auto l = linking_obj.get_list(c); - auto n = l.find_first(other.get_key()); - REALM_ASSERT(n != realm::npos); - l.set(n, get_key()); + REALM_UNREACHABLE(); // missing type handling } } return false; diff --git a/src/realm/object-store/impl/deep_change_checker.cpp b/src/realm/object-store/impl/deep_change_checker.cpp index 61ae4c8a99f..931443c474f 100644 --- a/src/realm/object-store/impl/deep_change_checker.cpp +++ b/src/realm/object-store/impl/deep_change_checker.cpp @@ -115,12 +115,13 @@ bool DeepChangeChecker::do_check_for_collection_of_mixed(T* coll, size_t depth) return std::any_of(coll->begin(), coll->end(), [&, this](auto value) { if (value.is_type(type_TypedLink)) { auto link = value.get_link(); - REALM_ASSERT(link); - if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { - cached_linked_table = coll->get_table()->get_parent_group()->get_table(link.get_table_key()); - REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); + if (!link.is_unresolved()) { + if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { + cached_linked_table = coll->get_table()->get_parent_group()->get_table(link.get_table_key()); + REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); + } + return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); } - return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); } return false; }); @@ -145,12 +146,14 @@ bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptrget_key() != link.get_table_key()) { - cached_linked_table = dict->get_table()->get_parent_group()->get_table(link.get_table_key()); - REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); + if (!link.is_unresolved()) { + REALM_ASSERT(link); + if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { + cached_linked_table = dict->get_table()->get_parent_group()->get_table(link.get_table_key()); + REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); + } + return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); } - return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); } else if (value.is_type(type_Link)) { REALM_ASSERT(target); @@ -163,7 +166,7 @@ bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptrget_target_table(); REALM_ASSERT(target); return std::any_of(set->begin(), set->end(), [&, this](auto obj_key) { - return obj_key && this->check_row(*target, obj_key.value); + return obj_key && this->check_row(*target, obj_key.value, depth + 1); }); } else if (auto set = dynamic_cast*>(coll.get())) { diff --git a/test/object-store/transaction_log_parsing.cpp b/test/object-store/transaction_log_parsing.cpp index 3e007ac275a..3c99152e49b 100644 --- a/test/object-store/transaction_log_parsing.cpp +++ b/test/object-store/transaction_log_parsing.cpp @@ -1577,7 +1577,185 @@ TEST_CASE("Transaction log parsing: changeset calcuation") { } } -TEST_CASE("DeepChangeChecker") { +struct ListOfObjects { + const Property property = {"array", PropertyType::Array | PropertyType::Object, "table"}; + void add_link(Obj from, ColKey col, ObjLink to) + { + from.get_linklist(col).add(to.get_obj_key()); + } + size_t size_of_collection(Obj obj, ColKey col) + { + return obj.get_linklist(col).size(); + } + void set_relation_updater(std::function) {} + size_t count_unresolved_links(Obj, ColKey) + { + return 0; + } + constexpr static bool allows_storing_nulls = false; +}; + +struct ListOfMixedLinks { + const Property property = {"array", PropertyType::Array | PropertyType::Mixed | PropertyType::Nullable}; + void add_link(Obj from, ColKey col, ObjLink to) + { + from.get_list(col).add(to); + // When adding dynamic links through a mixed value, the relationship map needs to be dynamically updated. + // In practice, this is triggered by the addition of backlink columns to any table. + if (m_relation_updater) { + m_relation_updater(); + } + } + size_t size_of_collection(Obj obj, ColKey col) + { + return obj.get_list(col).size(); + } + void set_relation_updater(std::function updater) + { + m_relation_updater = updater; + } + size_t count_unresolved_links(Obj obj, ColKey col) + { + Lst list = obj.get_list(col); + size_t num_unresolved = 0; + for (auto value : list) { + if (value.is_unresolved_link()) { + ++num_unresolved; + } + } + return num_unresolved; + } + std::function m_relation_updater; + constexpr static bool allows_storing_nulls = true; +}; + +struct SetOfObjects { + const Property property = {"array", PropertyType::Set | PropertyType::Object, "table"}; + void add_link(Obj from, ColKey col, ObjLink to) + { + from.get_linkset(col).insert(to.get_obj_key()); + } + size_t size_of_collection(Obj obj, ColKey col) + { + return obj.get_linkset(col).size(); + } + void set_relation_updater(std::function) {} + size_t count_unresolved_links(Obj, ColKey) + { + return 0; + } + constexpr static bool allows_storing_nulls = false; +}; + +struct SetOfMixedLinks { + const Property property = {"array", PropertyType::Set | PropertyType::Mixed | PropertyType::Nullable}; + void add_link(Obj from, ColKey col, ObjLink to) + { + from.get_set(col).insert(to); + // When adding dynamic links through a mixed value, the relationship map needs to be dynamically updated. + // In practice, this is triggered by the addition of backlink columns to any table. + if (m_relation_updater) { + m_relation_updater(); + } + } + size_t size_of_collection(Obj obj, ColKey col) + { + return obj.get_set(col).size(); + } + void set_relation_updater(std::function updater) + { + m_relation_updater = updater; + } + size_t count_unresolved_links(Obj obj, ColKey col) + { + Set set = obj.get_set(col); + size_t num_unresolved = 0; + for (auto value : set) { + if (value.is_unresolved_link()) { + ++num_unresolved; + } + } + return num_unresolved; + } + std::function m_relation_updater; + constexpr static bool allows_storing_nulls = true; +}; + +struct DictionaryOfObjects { + const Property property = {"array", PropertyType::Dictionary | PropertyType::Object | PropertyType::Nullable, + "table"}; + void add_link(Obj from, ColKey col, ObjLink to) + { + from.get_dictionary(col).insert(util::format("key_%1", key_counter++), to); + // When adding dynamic links through a mixed value, the relationship map needs to be dynamically updated. + // In practice, this is triggered by the addition of backlink columns to any table. + if (m_relation_updater) { + m_relation_updater(); + } + } + size_t size_of_collection(Obj obj, ColKey col) + { + return obj.get_dictionary(col).size(); + } + void set_relation_updater(std::function updater) + { + m_relation_updater = updater; + } + size_t count_unresolved_links(Obj obj, ColKey col) + { + Dictionary dict = obj.get_dictionary(col); + size_t num_unresolved = 0; + for (auto value : dict) { + if (value.second.is_unresolved_link()) { + ++num_unresolved; + } + } + return num_unresolved; + } + size_t key_counter = 0; + std::function m_relation_updater; + constexpr static bool allows_storing_nulls = true; +}; + +struct DictionaryOfMixedLinks { + const Property property = {"array", PropertyType::Dictionary | PropertyType::Mixed | PropertyType::Nullable}; + void add_link(Obj from, ColKey col, ObjLink to) + { + from.get_dictionary(col).insert(util::format("key_%1", key_counter++), to); + // When adding dynamic links through a mixed value, the relationship map needs to be dynamically updated. + // In practice, this is triggered by the addition of backlink columns to any table. + if (m_relation_updater) { + m_relation_updater(); + } + } + size_t size_of_collection(Obj obj, ColKey col) + { + return obj.get_dictionary(col).size(); + } + void set_relation_updater(std::function updater) + { + m_relation_updater = updater; + } + size_t count_unresolved_links(Obj obj, ColKey col) + { + Dictionary dict = obj.get_dictionary(col); + size_t num_unresolved = 0; + for (auto value : dict) { + if (value.second.is_unresolved_link()) { + ++num_unresolved; + } + } + return num_unresolved; + } + size_t key_counter = 0; + std::function m_relation_updater; + constexpr static bool allows_storing_nulls = true; +}; + +TEMPLATE_TEST_CASE("DeepChangeChecker", "[notifications]", ListOfObjects, ListOfMixedLinks, SetOfObjects, + SetOfMixedLinks, DictionaryOfObjects, DictionaryOfMixedLinks) +{ + TestType test_type; InMemoryTestFile config; config.automatic_change_notifications = false; auto r = Realm::get_shared_realm(config); @@ -1586,9 +1764,10 @@ TEST_CASE("DeepChangeChecker") { {{"int", PropertyType::Int}, {"link1", PropertyType::Object | PropertyType::Nullable, "table"}, {"link2", PropertyType::Object | PropertyType::Nullable, "table"}, - {"array", PropertyType::Array | PropertyType::Object, "table"}}}, + test_type.property}}, }); auto table = r->read_group().get_table("class_table"); + TableKey dst_table_key = table->get_key(); std::vector objects; r->begin_transaction(); @@ -1613,7 +1792,12 @@ TEST_CASE("DeepChangeChecker") { }; _impl::DeepChangeChecker::RelatedTables tables; + auto relation_updater = [&]() { + _impl::DeepChangeChecker::find_related_tables(tables, *table); + }; + relation_updater(); _impl::DeepChangeChecker::find_related_tables(tables, *table); + test_type.set_relation_updater(relation_updater); auto cols = table->get_column_keys(); SECTION("direct changes are tracked") { @@ -1684,12 +1868,12 @@ TEST_CASE("DeepChangeChecker") { }); } - SECTION("changes over linklists are tracked") { + SECTION("changes over collections are tracked") { r->begin_transaction(); for (int i = 0; i < 3; ++i) { - objects[i].get_linklist(cols[3]).add(objects[i].get_key()); - objects[i].get_linklist(cols[3]).add(objects[i].get_key()); - objects[i].get_linklist(cols[3]).add(objects[i + 1 + (i == 2)].get_key()); + test_type.add_link(objects[i], cols[3], {dst_table_key, objects[i].get_key()}); + test_type.add_link(objects[i], cols[3], {dst_table_key, objects[i].get_key()}); + test_type.add_link(objects[i], cols[3], {dst_table_key, objects[i + 1 + (i == 2)].get_key()}); } r->commit_transaction(); @@ -1705,16 +1889,25 @@ TEST_CASE("DeepChangeChecker") { r->begin_transaction(); size_t obj_ndx_to_invalidate = 6; for (int i = 0; i < 3; ++i) { - objects[i].get_linklist(cols[3]).add(objects[i].get_key()); - objects[i].get_linklist(cols[3]).add(objects[obj_ndx_to_invalidate].get_key()); - objects[i].get_linklist(cols[3]).add(objects[i + 1 + (i == 2)].get_key()); + test_type.add_link(objects[i], cols[3], {dst_table_key, objects[i].get_key()}); + test_type.add_link(objects[i], cols[3], {dst_table_key, objects[obj_ndx_to_invalidate].get_key()}); + test_type.add_link(objects[i], cols[3], {dst_table_key, objects[i + 1 + (i == 2)].get_key()}); } // Object invalidation can only happen if another sync client has deleted the object // we simulate this by calling it directly here. The consequence is that links to this // object are changed to the invalidated key, but not removed. This tests that the abstraction // that core has built to hide invalidated links inside LnkLst is not leaking at this level. - objects[6].invalidate(); - REQUIRE(objects[0].get_linklist(cols[3]).size() == 2); // LnkLst actually has 3 entries but hides one + objects[obj_ndx_to_invalidate].invalidate(); + if (TestType::allows_storing_nulls) { + REQUIRE(test_type.size_of_collection(objects[0], cols[3]) == 3); + REQUIRE(test_type.count_unresolved_links(objects[0], cols[3]) == 1); + REQUIRE(test_type.count_unresolved_links(objects[1], cols[3]) == 1); + REQUIRE(test_type.count_unresolved_links(objects[2], cols[3]) == 1); + } + else { + REQUIRE(test_type.size_of_collection(objects[0], cols[3]) == + 2); // LnkLst actually has 3 entries but hides one + } r->commit_transaction(); auto info = track_changes([&] { @@ -1767,9 +1960,9 @@ TEST_CASE("DeepChangeChecker") { REQUIRE_FALSE(_impl::DeepChangeChecker(info, *table, tables)(0)); } - SECTION("cycles over linklists do not loop forever") { + SECTION("cycles over collections do not loop forever") { r->begin_transaction(); - objects[0].get_linklist(cols[3]).add(objects[0].get_key()); + test_type.add_link(objects[0], cols[3], {dst_table_key, objects[0].get_key()}); r->commit_transaction(); auto info = track_changes([&] { @@ -1813,9 +2006,9 @@ TEST_CASE("DeepChangeChecker") { SECTION("changes made in the 3rd elements in the link list") { r->begin_transaction(); - objects[0].get_linklist(cols[3]).add(objects[1].get_key()); - objects[0].get_linklist(cols[3]).add(objects[2].get_key()); - objects[0].get_linklist(cols[3]).add(objects[3].get_key()); + test_type.add_link(objects[0], cols[3], {dst_table_key, objects[1].get_key()}); + test_type.add_link(objects[0], cols[3], {dst_table_key, objects[2].get_key()}); + test_type.add_link(objects[0], cols[3], {dst_table_key, objects[3].get_key()}); objects[1].set(cols[1], objects[0].get_key()); objects[2].set(cols[1], objects[0].get_key()); objects[3].set(cols[1], objects[0].get_key()); @@ -1832,7 +2025,7 @@ TEST_CASE("DeepChangeChecker") { SECTION("changes made to lists mark the containing row as modified") { auto info = track_changes([&] { - objects[0].get_linklist(cols[3]).add(objects[1].get_key()); + test_type.add_link(objects[0], cols[3], {dst_table_key, objects[1].get_key()}); }); _impl::DeepChangeChecker checker(info, *table, tables); REQUIRE(checker(0)); From 53bee90f05ba92f35d308d362f9643b198085805 Mon Sep 17 00:00:00 2001 From: James Stone Date: Fri, 18 Jun 2021 12:55:34 -0700 Subject: [PATCH 3/3] fix table cache, optimize and reuse more code --- CHANGELOG.md | 3 +- .../object-store/impl/deep_change_checker.cpp | 61 ++++++++----------- .../object-store/impl/deep_change_checker.hpp | 6 +- test/object-store/transaction_log_parsing.cpp | 13 +--- 4 files changed, 36 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0df1900ac1c..4f2f22168e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,8 @@ * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) * Fixed an assertion failure when listening for changes to a list of primitive Mixed which contains links. ([#4767](https://github.com/realm/realm-core/issues/4767), since the beginning of Mixed v11.0.0) * Fixed an assertion failure when listening for changes to a dictionary or set which contains an invalidated link. ([#4770](https://github.com/realm/realm-core/pull/4770), since the beginning of v11) - +* Fixed an endless recursive loop that could cause a stack overflow when computing changes on a set of objects which contained cycles. ([#4770](https://github.com/realm/realm-core/pull/4770), since the beginning of v11) + ### Breaking changes * None. diff --git a/src/realm/object-store/impl/deep_change_checker.cpp b/src/realm/object-store/impl/deep_change_checker.cpp index 931443c474f..03ffdfacd2d 100644 --- a/src/realm/object-store/impl/deep_change_checker.cpp +++ b/src/realm/object-store/impl/deep_change_checker.cpp @@ -109,21 +109,27 @@ DeepChangeChecker::DeepChangeChecker(TransactionChangeInfo const& info, Table co } template -bool DeepChangeChecker::do_check_for_collection_of_mixed(T* coll, size_t depth) +bool DeepChangeChecker::do_check_mixed_for_link(T* coll, TableRef& cached_linked_table, Mixed value, size_t depth) { - TableRef cached_linked_table; - return std::any_of(coll->begin(), coll->end(), [&, this](auto value) { - if (value.is_type(type_TypedLink)) { - auto link = value.get_link(); - if (!link.is_unresolved()) { - if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { - cached_linked_table = coll->get_table()->get_parent_group()->get_table(link.get_table_key()); - REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); - } - return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); + if (value.is_type(type_TypedLink)) { + auto link = value.get_link(); + if (!link.is_unresolved()) { + if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { + cached_linked_table = coll->get_table()->get_parent_group()->get_table(link.get_table_key()); + REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); } + return check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); } - return false; + } + return false; +} + +template +bool DeepChangeChecker::do_check_for_collection_of_mixed(T* coll, size_t depth) +{ + TableRef cached_linked_table; + return std::any_of(coll->begin(), coll->end(), [&](auto value) { + return do_check_mixed_for_link(coll, cached_linked_table, value, depth); }); } @@ -132,41 +138,28 @@ bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptr(coll.get())) { TableRef target = lst->get_target_table(); - return std::any_of(lst->begin(), lst->end(), [&, this](auto key) { - return this->check_row(*target, key.value, depth + 1); + return std::any_of(lst->begin(), lst->end(), [&](auto key) { + return check_row(*target, key.value, depth + 1); }); } else if (auto list = dynamic_cast*>(coll.get())) { return do_check_for_collection_of_mixed(list, depth); } else if (auto dict = dynamic_cast(coll.get())) { - auto target = dict->get_target_table(); TableRef cached_linked_table; - return std::any_of(dict->begin(), dict->end(), [&, this](auto key_value_pair) { + return std::any_of(dict->begin(), dict->end(), [&](auto key_value_pair) { Mixed value = key_value_pair.second; - if (value.is_type(type_TypedLink)) { - auto link = value.get_link(); - if (!link.is_unresolved()) { - REALM_ASSERT(link); - if (!cached_linked_table || cached_linked_table->get_key() != link.get_table_key()) { - cached_linked_table = dict->get_table()->get_parent_group()->get_table(link.get_table_key()); - REALM_ASSERT_EX(cached_linked_table, link.get_table_key().value); - } - return this->check_row(*cached_linked_table, link.get_obj_key().value, depth + 1); - } - } - else if (value.is_type(type_Link)) { - REALM_ASSERT(target); - return this->check_row(*target, value.get().value, depth + 1); - } - return false; + // Here we rely on Dictionaries storing all links as a TypedLink + // even if the dictionary is set to a single object type. + REALM_ASSERT(!value.is_type(type_Link)); + return do_check_mixed_for_link(dict, cached_linked_table, value, depth); }); } else if (auto set = dynamic_cast(coll.get())) { auto target = set->get_target_table(); REALM_ASSERT(target); - return std::any_of(set->begin(), set->end(), [&, this](auto obj_key) { - return obj_key && this->check_row(*target, obj_key.value, depth + 1); + return std::any_of(set->begin(), set->end(), [&](auto obj_key) { + return obj_key && check_row(*target, obj_key.value, depth + 1); }); } else if (auto set = dynamic_cast*>(coll.get())) { diff --git a/src/realm/object-store/impl/deep_change_checker.hpp b/src/realm/object-store/impl/deep_change_checker.hpp index 82086089a5f..ec16dbdf3c0 100644 --- a/src/realm/object-store/impl/deep_change_checker.hpp +++ b/src/realm/object-store/impl/deep_change_checker.hpp @@ -26,8 +26,10 @@ namespace realm { class CollectionBase; -class Table; +class Mixed; class Realm; +class Table; +class TableRef; class Transaction; namespace _impl { @@ -83,6 +85,8 @@ class DeepChangeChecker { bool do_check_for_collection_modifications(std::unique_ptr coll, size_t depth); template bool do_check_for_collection_of_mixed(T* coll, size_t depth); + template + bool do_check_mixed_for_link(T* coll, TableRef& cached_linked_table, Mixed value, size_t depth); }; } // namespace _impl diff --git a/test/object-store/transaction_log_parsing.cpp b/test/object-store/transaction_log_parsing.cpp index 3c99152e49b..17dfd57ff5c 100644 --- a/test/object-store/transaction_log_parsing.cpp +++ b/test/object-store/transaction_log_parsing.cpp @@ -1686,21 +1686,13 @@ struct DictionaryOfObjects { "table"}; void add_link(Obj from, ColKey col, ObjLink to) { - from.get_dictionary(col).insert(util::format("key_%1", key_counter++), to); - // When adding dynamic links through a mixed value, the relationship map needs to be dynamically updated. - // In practice, this is triggered by the addition of backlink columns to any table. - if (m_relation_updater) { - m_relation_updater(); - } + from.get_dictionary(col).insert(util::format("key_%1", key_counter++), to.get_obj_key()); } size_t size_of_collection(Obj obj, ColKey col) { return obj.get_dictionary(col).size(); } - void set_relation_updater(std::function updater) - { - m_relation_updater = updater; - } + void set_relation_updater(std::function) {} size_t count_unresolved_links(Obj obj, ColKey col) { Dictionary dict = obj.get_dictionary(col); @@ -1796,7 +1788,6 @@ TEMPLATE_TEST_CASE("DeepChangeChecker", "[notifications]", ListOfObjects, ListOf _impl::DeepChangeChecker::find_related_tables(tables, *table); }; relation_updater(); - _impl::DeepChangeChecker::find_related_tables(tables, *table); test_type.set_relation_updater(relation_updater); auto cols = table->get_column_keys();