fix notifications on list of primitive mixed (#4770)

* fix notifications on list of primitive mixed

* fix handling of unresolved links in dict/set when checking deep changes

* fix table cache, optimize and reuse more code

CHANGELOG.md

@@ -5,8 +5,10 @@
 
 ### Fixed
 * <How to hit and notice issue? what was the impact?> ([#????](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)
+* 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.
 

src/realm/obj.cpp

@@ -2150,12 +2150,26 @@ void Obj::assign_pk_and_backlinks(const Obj& other)
                 REALM_ASSERT(!linking_obj.get<ObjKey>(c) || linking_obj.get<ObjKey>(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<Mixed>(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<ObjKey>(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<ObjKey>(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;

src/realm/object-store/impl/deep_change_checker.cpp

@@ -108,57 +108,62 @@ DeepChangeChecker::DeepChangeChecker(TransactionChangeInfo const& info, Table co
 {
 }
 
+template <typename T>
+bool DeepChangeChecker::do_check_mixed_for_link(T* coll, TableRef& cached_linked_table, Mixed value, size_t depth)
+{
+    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;
+}
+
+template <typename T>
+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);
+    });
+}
+
 bool DeepChangeChecker::do_check_for_collection_modifications(std::unique_ptr<CollectionBase> coll, size_t depth)
 {
     REALM_ASSERT(coll);
     if (auto lst = dynamic_cast<LnkLst*>(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<Lst<Mixed>*>(coll.get())) {
+        return do_check_for_collection_of_mixed(list, depth);
+    }
     else if (auto dict = dynamic_cast<Dictionary*>(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();
-                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<ObjKey>().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<LnkSet*>(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);
+        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<Set<Mixed>*>(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();

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 {
@@ -81,6 +83,10 @@ 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<CollectionBase> coll, size_t depth);
+    template <typename T>
+    bool do_check_for_collection_of_mixed(T* coll, size_t depth);
+    template <typename T>
+    bool do_check_mixed_for_link(T* coll, TableRef& cached_linked_table, Mixed value, size_t depth);
 };
 
 } // namespace _impl

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