Do not resurrect objects that have been deleted by

the server when copying local lists with links in them.
realm · Nov 2, 2023 · 7268d03 · 7268d03
1 parent 2203fdd
commit 7268d03
Show file tree

Hide file tree

Showing 4 changed files with 136 additions and 36 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
 * A new design around using a scheduler in C API has enabled the proper release of the user data (See "Breaking Changes") ([#7094](https://github.com/realm/realm-core/issues/7094), since v10.4.0)
 * Potential stack-use-after-scope issue on changesets integration with msvc-2019 and mpack code ([PR #6911](https://github.com/realm/realm-core/pull/6911))
+* During a client reset with recovery when recovering a move or set operation on a `LnkLst` or `Lst<Mixed>` that operated on indices that were not also added in the recovery, links to an object which had been deleted by another client while offline would be recreated by the recovering client. But the objects of these links would only have the primary key populated and all other fields would be default values. Now, instead of creating these zombie objects, the lists being recovered skip such deleted links. ([#7112](https://github.com/realm/realm-core/issues/7112) since the beginning of client reset with recovery in v11.16.0)
 * Fix compilation with non-beta Xcode 15. Building for visionOS now requires explicitly specifying `-DCMAKE_XCODE_ATTRIBUTE_SDKROOT=xros` (PR [#7055](https://github.com/realm/realm-core/pull/7055)).
 * Fixed FLX subscriptions not being sent to the server if the session was interrupted during bootstrapping. ([#7077](https://github.com/realm/realm-core/issues/7077), since v11.8.0)
 * Fixed FLX subscriptions not being sent to the server if an upload message was sent immediately after a subscription was committed but before the sync client checks for new subscriptions via `SubscriptionStore::get_next_pending_version()`. ([#7076](https://github.com/realm/realm-core/issues/7076), since v13.23.1)

diff --git a/src/realm/object_converter.cpp b/src/realm/object_converter.cpp
@@ -41,9 +41,9 @@ void InterRealmValueConverter::copy_list(const Obj& src_obj, Obj& dst_obj, bool*
     LstBasePtr dst = dst_obj.get_listbase_ptr(m_dst_col);
 
     bool updated = false;
-    size_t len_src = src->size();
-    size_t len_dst = dst->size();
-    size_t len_min = std::min(len_src, len_dst);
+    const size_t len_src = src->size();
+    const size_t len_dst_orig = dst->size();
+    size_t len_min = std::min(len_src, len_dst_orig);
 
     size_t ndx = 0;
     size_t suffix_len = 0;
@@ -52,25 +52,47 @@ void InterRealmValueConverter::copy_list(const Obj& src_obj, Obj& dst_obj, bool*
         ndx++;
     }
 
+    if (ndx == len_src && len_src == len_dst_orig) {
+        // all are equal, early out
+        if (update_out) {
+            *update_out = false;
+        }
+        return;
+    }
+
     size_t suffix_len_max = len_min - ndx;
 
     while (suffix_len < suffix_len_max &&
-           cmp_src_to_dst(src->get_any(len_src - 1 - suffix_len), dst->get_any(len_dst - 1 - suffix_len), nullptr,
-                          update_out) == 0) {
+           cmp_src_to_dst(src->get_any(len_src - 1 - suffix_len), dst->get_any(len_dst_orig - 1 - suffix_len),
+                          nullptr, update_out) == 0) {
         suffix_len++;
     }
 
     len_min -= (ndx + suffix_len);
 
+    auto dst_as_link_list = dynamic_cast<LnkLst*>(dst.get());
+    auto dst_as_lst_mixed = dynamic_cast<Lst<Mixed>*>(dst.get());
+    auto is_link_to_deleted_object = [&](const Mixed& src_value, const Mixed& converted_value) -> bool {
+        return (dst_as_link_list && converted_value.is_null()) ||
+               (dst_as_lst_mixed && converted_value.is_null() && src_value.is_type(type_TypedLink));
+    };
+
+    std::vector<size_t> dst_to_erase;
     for (size_t i = 0; i < len_min; i++) {
         InterRealmValueConverter::ConversionResult converted_src;
-        if (cmp_src_to_dst(src->get_any(ndx), dst->get_any(ndx), &converted_src, update_out)) {
+        const Mixed src_value = src->get_any(ndx);
+        if (cmp_src_to_dst(src_value, dst->get_any(ndx), &converted_src, update_out)) {
             if (converted_src.requires_new_embedded_object) {
-                auto lnklist = dynamic_cast<LnkLst*>(dst.get());
-                REALM_ASSERT(lnklist); // this is the only type of list that supports embedded objects
-                Obj embedded = lnklist->create_and_set_linked_object(ndx);
+                REALM_ASSERT(dst_as_link_list); // this is the only type of list that supports embedded objects
+                Obj embedded = dst_as_link_list->create_and_set_linked_object(ndx);
                 track_new_embedded(converted_src.src_embedded_to_check, embedded);
             }
+            else if (is_link_to_deleted_object(src_value, converted_src.converted_value)) {
+                // this can happen when the source linked list points to an object
+                // which has been deleted in the dest Realm. Lists do not support
+                // setting an element to null, so it must be deleted later.
+                dst_to_erase.push_back(ndx);
+            }
             else {
                 dst->set_any(ndx, converted_src.converted_value);
             }
@@ -80,29 +102,36 @@ void InterRealmValueConverter::copy_list(const Obj& src_obj, Obj& dst_obj, bool*
     }
 
     // New elements must be inserted in dst.
-    while (len_dst < len_src) {
+    while (ndx < len_src - suffix_len) {
         InterRealmValueConverter::ConversionResult converted_src;
-        cmp_src_to_dst(src->get_any(ndx), Mixed{}, &converted_src, update_out);
+        const Mixed src_value = src->get_any(ndx);
+        cmp_src_to_dst(src_value, Mixed{}, &converted_src, update_out);
+        size_t dst_ndx_to_insert = dst->size() - suffix_len;
         if (converted_src.requires_new_embedded_object) {
-            auto lnklist = dynamic_cast<LnkLst*>(dst.get());
-            REALM_ASSERT(lnklist); // this is the only type of list that supports embedded objects
-            Obj embedded = lnklist->create_and_insert_linked_object(ndx);
+            REALM_ASSERT(dst_as_link_list); // this is the only type of list that supports embedded objects
+            Obj embedded = dst_as_link_list->create_and_insert_linked_object(dst_ndx_to_insert);
             track_new_embedded(converted_src.src_embedded_to_check, embedded);
         }
+        else if (is_link_to_deleted_object(src_value, converted_src.converted_value)) {
+            // ignore trying to insert a link to a object which no longer exists
+        }
         else {
-            dst->insert_any(ndx, converted_src.converted_value);
+            dst->insert_any(dst_ndx_to_insert, converted_src.converted_value);
         }
-        len_dst++;
         ndx++;
         updated = true;
     }
     // Excess elements must be removed from ll_dst.
-    if (len_dst > len_src) {
-        dst->remove(len_src - suffix_len, len_dst - suffix_len);
+    if (dst->size() > len_src) {
+        dst->remove(len_src - suffix_len, dst->size() - suffix_len);
         updated = true;
     }
 
-    REALM_ASSERT(dst->size() == len_src);
+    while (dst_to_erase.size()) {
+        size_t ndx_to_remove = dst_to_erase.back();
+        dst_as_link_list ? dst_as_link_list->remove(ndx_to_remove) : dst_as_lst_mixed->remove(ndx_to_remove);
+        dst_to_erase.pop_back();
+    }
     if (updated && update_out) {
         *update_out = updated;
     }
@@ -373,7 +402,8 @@ int InterRealmValueConverter::cmp_src_to_dst(Mixed src, Mixed dst, ConversionRes
                 // in different Realms we create a new object
                 if (m_opposite_of_src->get_primary_key_column()) {
                     Mixed src_link_pk = m_opposite_of_src->get_primary_key(src_link_key);
-                    dst_link = m_opposite_of_dst->create_object_with_primary_key(src_link_pk, did_update_out);
+                    dst_link =
+                        m_opposite_of_dst->get_object_with_primary_key(src_link_pk); // returns ObjKey{} if not found
                 }
                 else {
                     dst_link = m_opposite_of_dst->create_object();
@@ -404,8 +434,9 @@ int InterRealmValueConverter::cmp_src_to_dst(Mixed src, Mixed dst, ConversionRes
             // regular table, convert by pk
             if (src_link_table->get_primary_key_column()) {
                 Mixed src_pk = src_link_table->get_primary_key(src_link.get_obj_key());
-                Obj dst_link = dst_link_table->create_object_with_primary_key(src_pk, did_update_out);
-                converted_src = ObjLink{dst_link_table->get_key(), dst_link.get_key()};
+                if (Obj dst_link = dst_link_table->get_object_with_primary_key(src_pk)) {
+                    converted_src = ObjLink{dst_link_table->get_key(), dst_link.get_key()};
+                }
             }
             else if (src_link_table == dst_link_table) {
                 // no pk, but this is the same Realm, so convert by ObjKey

diff --git a/src/realm/sync/noinst/client_reset.cpp b/src/realm/sync/noinst/client_reset.cpp
@@ -366,9 +366,9 @@ void transfer_group(const Transaction& group_src, Transaction& group_dst, util::
     }
 
     converters::EmbeddedObjectConverter embedded_tracker;
-    // Now src and dst have identical schemas and no extraneous objects from dst.
-    // There may be missing object from src and the values of existing objects may
-    // still differ. Diff all the values and create missing objects on the fly.
+    // Now src and dst have identical schemas and all the top level objects are created.
+    // What is left to do is to diff all properties of the existing objects.
+    // Embedded objects are created on the fly.
     for (auto table_key : group_src.get_table_keys()) {
         if (should_skip_table(group_src, table_key))
             continue;

diff --git a/test/object-store/sync/client_reset.cpp b/test/object-store/sync/client_reset.cpp
@@ -401,6 +401,16 @@ TEST_CASE("sync: client reset", "[sync][pbs][client reset][baas]") {
         REQUIRE(after->config().path == local_config.path);
         REQUIRE(after->current_transaction_version() > before->current_transaction_version());
     };
+    auto get_key_for_object_with_value = [&](TableRef table, int64_t value) -> ObjKey {
+        REQUIRE(table);
+        auto target = std::find_if(table->begin(), table->end(), [&](auto& it) -> bool {
+            return it.template get<Int>("value") == value;
+        });
+        if (target == table->end()) {
+            return {};
+        }
+        return target->get_key();
+    };
 
     Results results;
     Object object;
@@ -739,6 +749,59 @@ TEST_CASE("sync: client reset", "[sync][pbs][client reset][baas]") {
             REQUIRE(before_callback_invocations == 1);
             REQUIRE(after_callback_invocations == 0);
         }
+
+        SECTION("add remotely deleted object to list") {
+            test_reset
+                ->setup([&](SharedRealm realm) {
+                    ObjKey k1 =
+                        create_object(*realm, "link target", ObjectId::gen(), partition).set("value", 1).get_key();
+                    ObjKey k2 =
+                        create_object(*realm, "link target", ObjectId::gen(), partition).set("value", 2).get_key();
+                    ObjKey k3 =
+                        create_object(*realm, "link target", ObjectId::gen(), partition).set("value", 3).get_key();
+                    Obj o = create_object(*realm, "link origin", ObjectId::gen(), partition);
+                    auto list = o.get_linklist("list");
+                    list.add(k1);
+                    list.add(k2);
+                    list.add(k3);
+                    // 1, 2, 3
+                })
+                ->make_local_changes([&](SharedRealm local) {
+                    auto key1 = get_key_for_object_with_value(get_table(*local, "link target"), 1);
+                    auto key2 = get_key_for_object_with_value(get_table(*local, "link target"), 2);
+                    auto key3 = get_key_for_object_with_value(get_table(*local, "link target"), 3);
+                    auto table = get_table(*local, "link origin");
+                    auto list = table->begin()->get_linklist("list");
+                    REQUIRE(list.size() == 3);
+                    list.insert(1, key2);
+                    list.add(key2);
+                    list.add(key3); // common suffix of key3
+                    list.set(0,
+                             key1); // this set operation triggers the list copy because the index becomes ambiguious
+                    // 1, 2, 2, 3, 2, 3
+                })
+                ->make_remote_changes([&](SharedRealm remote) {
+                    auto table = get_table(*remote, "link target");
+                    auto key = get_key_for_object_with_value(table, 2);
+                    REQUIRE(key);
+                    table->remove_object(key);
+                })
+                ->on_post_reset([&](SharedRealm realm) {
+                    REQUIRE_NOTHROW(realm->refresh());
+                    auto table = get_table(*realm, "link origin");
+                    auto target_table = get_table(*realm, "link target");
+                    REQUIRE(table->size() == 1);
+                    REQUIRE(target_table->size() == 2);
+                    REQUIRE(get_key_for_object_with_value(target_table, 1));
+                    REQUIRE(get_key_for_object_with_value(target_table, 3));
+                    auto list = table->begin()->get_linklist("list");
+                    REQUIRE(list.size() == 3);
+                    REQUIRE(list.get_object(0).get<Int>("value") == 1);
+                    REQUIRE(list.get_object(1).get<Int>("value") == 3);
+                    REQUIRE(list.get_object(2).get<Int>("value") == 3);
+                })
+                ->run();
+        }
     } // end recovery section
 
     SECTION("discard local") {
@@ -1484,17 +1547,6 @@ TEST_CASE("sync: client reset", "[sync][pbs][client reset][baas]") {
                 ->run();
         }
 
-        auto get_key_for_object_with_value = [&](TableRef table, int64_t value) -> ObjKey {
-            REQUIRE(table);
-            auto target = std::find_if(table->begin(), table->end(), [&](auto& it) -> bool {
-                return it.template get<Int>("value") == value;
-            });
-            if (target == table->end()) {
-                return {};
-            }
-            return target->get_key();
-        };
-
         SECTION("link to remotely deleted object") {
             test_reset
                 ->setup([&](SharedRealm realm) {
@@ -2885,6 +2937,22 @@ TEMPLATE_TEST_CASE("client reset collections of links", "[sync][pbs][client rese
             reset_collection({Move{0, 1}, Add{dest_pk_5}}, {Add{dest_pk_4}},
                              {dest_pk_2, dest_pk_1, dest_pk_3, dest_pk_5});
         }
+        SECTION("local moves on non-added elements with server dest obj removal") {
+            reset_collection(
+                {Move{0, 1}, Add{dest_pk_5}}, {Add{dest_pk_4}, RemoveObject("dest", dest_pk_1)},
+                {dest_pk_2, dest_pk_3,
+                 dest_pk_5}); // copy over local list, but without the dest_pk_1 link because that object was deleted
+        }
+        SECTION("local moves on non-added elements with all server dest objs removed") {
+            reset_collection({Move{0, 1}, Add{dest_pk_5}},
+                             {Add{dest_pk_4}, RemoveObject("dest", dest_pk_1), RemoveObject("dest", dest_pk_2),
+                              RemoveObject("dest", dest_pk_3), RemoveObject("dest", dest_pk_5)},
+                             {}); // copy over local list, but all links have been removed
+        }
+        SECTION("local moves on non-added elements when server creates a new object and adds it to the list") {
+            reset_collection({Move{0, 1}, Add{dest_pk_5}}, {CreateObject("dest", 6), Add{6}},
+                             {dest_pk_2, dest_pk_1, dest_pk_3, dest_pk_5});
+        }
         SECTION("local moves on added elements can be merged with remote moves") {
             reset_collection({Add{dest_pk_4}, Add{dest_pk_5}, Move{3, 4}}, {Move{0, 1}},
                              {dest_pk_2, dest_pk_1, dest_pk_3, dest_pk_5, dest_pk_4});