fix sort/distinct on LnkSet with unresolved links

CHANGELOG.md

@@ -9,6 +9,8 @@
 * `Set::assign_intersection()` on `Set<StringData>`, `Set<BinaryData>`, and `Set<Mixed>` containing string or binary would cause a use-after-free if a set was intersected with itself ([PR #7144](https://github.com/realm/realm-core/pull/7144), since v10.0.0).
 * Set algebra on `Set<StringData>` and `Set<BinaryData>` gave incorrect results when used on platforms where `char` is signed ([#7135](https://github.com/realm/realm-core/issues/7135), since v13.23.3).
 * 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)
+* During a client reset recovery a Set of links could be missing items, or an exception could be thrown that prevents recovery ex: "Requested index 1 calling get() on set 'source.collection' when max is 0" ([#7112](https://github.com/realm/realm-core/issues/7112), since the beginning of client reset with recovery in v11.16.0)
+* Calling `sort()` or `distinct()` on a `LnkSet` that had unresolved links in it would produce duplicate indices.
 
 ### Breaking changes
 * None.

src/realm/collection.cpp

@@ -44,6 +44,8 @@ size_t real2virtual(const std::vector<size_t>& vec, size_t ndx) noexcept
 {
     // Subtract the number of tombstones below ndx.
     auto it = std::lower_bound(vec.begin(), vec.end(), ndx);
+    // A tombstone index has no virtual mapping. This is an error.
+    REALM_ASSERT_DEBUG_EX(it == vec.end() || *it != ndx, ndx, vec.size());
     auto n = it - vec.begin();
     return ndx - n;
 }

src/realm/collection.hpp

@@ -601,6 +601,11 @@ class ObjCollectionBase : public Interface, public _impl::ObjListProxy {
         return _impl::real2virtual(m_unresolved, ndx);
     }
 
+    bool real_is_unresolved(size_t ndx) const noexcept
+    {
+        return std::find(m_unresolved.begin(), m_unresolved.end(), ndx) != m_unresolved.end();
+    }
+
     /// Rebuild the list of tombstones if there is a possibility that it has
     /// changed.
     ///

src/realm/set.hpp

@@ -932,6 +932,12 @@ inline void LnkSet::sort(std::vector<size_t>& indices, bool ascending) const
 
     m_set.sort(indices, ascending);
 
+    if (has_unresolved()) {
+        indices.erase(std::remove_if(indices.begin(), indices.end(), [&](size_t ndx) {
+            return real_is_unresolved(ndx);
+        }), indices.end());
+    }
+
     // Map the output indices to virtual indices.
     std::transform(indices.begin(), indices.end(), indices.begin(), [this](size_t ndx) {
         return real2virtual(ndx);
@@ -949,6 +955,12 @@ inline void LnkSet::distinct(std::vector<size_t>& indices, util::Optional<bool>
 
     m_set.distinct(indices, sort_order);
 
+    if (has_unresolved()) {
+        indices.erase(std::remove_if(indices.begin(), indices.end(), [&](size_t ndx) {
+            return real_is_unresolved(ndx);
+        }), indices.end());
+    }
+
     // Map the output indices to virtual indices.
     std::transform(indices.begin(), indices.end(), indices.begin(), [this](size_t ndx) {
         return real2virtual(ndx);

test/object-store/sync/client_reset.cpp

@@ -2532,6 +2532,14 @@ struct CollectionOperation {
     }
     void apply(collection_fixtures::LinkedCollectionBase* collection, Obj src_obj, TableRef dst_table)
     {
+        auto get_table = [&](std::string_view name) -> TableRef {
+            Group* group = dst_table->get_parent_group();
+            Group::TableNameBuffer buffer;
+            TableRef table =
+                group->get_table(Group::class_name_to_table_name(name, buffer));
+            REALM_ASSERT(table);
+            return table;
+        };
         mpark::visit(
             util::overload{
                 [&](Add add_link) {
@@ -2548,21 +2556,13 @@ struct CollectionOperation {
                     REALM_ASSERT(did_remove);
                 },
                 [&](RemoveObject remove_object) {
-                    Group* group = dst_table->get_parent_group();
-                    Group::TableNameBuffer buffer;
-                    TableRef table =
-                        group->get_table(Group::class_name_to_table_name(remove_object.class_name, buffer));
-                    REALM_ASSERT(table);
+                    TableRef table = get_table(remove_object.class_name);
                     ObjKey dst_key = table->find_primary_key(Mixed{remove_object.pk});
                     REALM_ASSERT(dst_key);
                     table->remove_object(dst_key);
                 },
                 [&](CreateObject create_object) {
-                    Group* group = dst_table->get_parent_group();
-                    Group::TableNameBuffer buffer;
-                    TableRef table =
-                        group->get_table(Group::class_name_to_table_name(create_object.class_name, buffer));
-                    REALM_ASSERT(table);
+                    TableRef table = get_table(create_object.class_name);
                     table->create_object_with_primary_key(Mixed{create_object.pk});
                 },
                 [&](Clear) {
@@ -2587,8 +2587,8 @@ struct CollectionOperation {
 } // namespace test_instructions
 
 TEMPLATE_TEST_CASE("client reset collections of links", "[sync][pbs][client reset][links][collections]",
-                   cf::ListOfObjects, cf::ListOfMixedLinks/*, cf::SetOfObjects, cf::SetOfMixedLinks,
-                   cf::DictionaryOfObjects, cf::DictionaryOfMixedLinks*/)
+                   cf::ListOfObjects, cf::ListOfMixedLinks, cf::SetOfObjects, cf::SetOfMixedLinks,
+                   cf::DictionaryOfObjects, cf::DictionaryOfMixedLinks)
 {
     if (!util::EventLoop::has_implementation())
         return;
@@ -2753,31 +2753,41 @@ TEMPLATE_TEST_CASE("client reset collections of links", "[sync][pbs][client rese
                     REQUIRE_NOTHROW(advance_and_notify(*realm));
                     CHECK(results.size() == 1);
                     CHECK(object.is_valid());
-                    auto linked_objects = test_type.get_links(results.get(0));
+                    Obj origin = results.get(0);
+                    auto linked_objects = test_type.get_links(origin);
                     std::vector<util::Optional<int64_t>>& expected_links = remote_pks;
+                    const size_t actual_size = test_type.size_of_collection(origin);
                     if (test_mode == ClientResyncMode::Recover) {
                         expected_links = expected_recovered_state;
                         size_t expected_size = expected_links.size();
                         if (!test_type.will_erase_removed_object_links()) {
                             // dictionary size will remain the same because the key is preserved with a null value
                             expected_size += num_expected_nulls;
                         }
-                        CHECK(test_type.size_of_collection(results.get(0)) == expected_size);
+                        CHECK(actual_size == expected_size);
+                        if (actual_size != expected_size) {
+                            std::vector<Obj> links = test_type.get_links(origin);
+                            std::cout << "actual {";
+                            for (auto link : links) {
+                                std::cout << link.get_primary_key() << ", ";
+                            }
+                            std::cout << "}\n";
+                        }
                     }
                     if (!test_type_is_array) {
                         // order should not matter except for lists
                         std::sort(local_pks.begin(), local_pks.end());
                         std::sort(expected_links.begin(), expected_links.end());
                     }
                     require_links_to_match_ids(linked_objects, expected_links, !test_type_is_array);
-                    if (local_pks == expected_links) {
-                        REQUIRE_INDICES(results_changes.modifications);
-                        REQUIRE_INDICES(object_changes.modifications);
-                    }
-                    else {
+                    if (local_pks != expected_links) {
                         REQUIRE_INDICES(results_changes.modifications, 0);
                         REQUIRE_INDICES(object_changes.modifications, 0);
                     }
+                    else {
+                        REQUIRE_INDICES(results_changes.modifications);
+                        REQUIRE_INDICES(object_changes.modifications);
+                    }
                     REQUIRE_INDICES(results_changes.insertions);
                     REQUIRE_INDICES(results_changes.deletions);
                     REQUIRE_INDICES(object_changes.insertions);
@@ -2802,8 +2812,7 @@ TEMPLATE_TEST_CASE("client reset collections of links", "[sync][pbs][client rese
             })
             ->run();
     };
-
-    test_reset->setup([&](SharedRealm realm) {
+    auto populate_initial_state = [&](SharedRealm realm) {
         test_type.reset_test_state();
         // add a container collection with three valid links
         ObjLink dest1 = create_one_dest_object(realm, dest_pk_1);
@@ -2812,6 +2821,10 @@ TEMPLATE_TEST_CASE("client reset collections of links", "[sync][pbs][client rese
         create_one_dest_object(realm, dest_pk_4);
         create_one_dest_object(realm, dest_pk_5);
         create_one_source_object(realm, source_pk, {dest1, dest2, dest3});
+    };
+
+    test_reset->setup([&](SharedRealm realm) {
+        populate_initial_state(realm);
     });
 
     SECTION("no changes") {
@@ -2933,6 +2946,53 @@ TEMPLATE_TEST_CASE("client reset collections of links", "[sync][pbs][client rese
         reset_collection({RemoveObject("dest", dest_pk_1), RemoveObject("dest", dest_pk_2), CreateObject("dest", 6), Add{6}}, {},
                             {dest_pk_3, 6}, 2);
     }
+    SECTION("local has unresolved links") {
+        test_reset->setup([&](SharedRealm realm) {
+            populate_initial_state(realm);
+
+            auto invalidate_object = [&](SharedRealm realm, std::string_view table_name, Mixed pk) {
+                TableRef table = get_table(*realm, table_name);
+                Obj obj = table->get_object_with_primary_key(pk);
+                REALM_ASSERT(obj.is_valid());
+                if (realm->config().path == config.path) {
+                    // the local realm does an invalidation
+                    table->invalidate_object(obj.get_key());
+                }
+                else {
+                    // the remote realm has deleted it
+                    table->remove_object(obj.get_key());
+                }
+            };
+
+            invalidate_object(realm, "dest", dest_pk_1);
+        });
+
+        SECTION("remote adds a link") {
+            reset_collection({}, {Add{dest_pk_4}},
+                                {dest_pk_2, dest_pk_3, dest_pk_4}, 1);
+        }
+        SECTION("remote removes a link") {
+            reset_collection({}, {Remove{dest_pk_2}},
+                                {dest_pk_3}, 1);
+        }
+        SECTION("remote deletes a dest object that local links to") {
+            reset_collection({Add{dest_pk_4}}, {RemoveObject{"dest", dest_pk_4}},
+                                {dest_pk_2, dest_pk_3}, 2);
+        }
+        SECTION("remote deletes a different dest object") {
+            reset_collection({Add{dest_pk_4}}, {RemoveObject{"dest", dest_pk_2}},
+                                {dest_pk_3, dest_pk_4}, 2);
+        }
+        SECTION("local adds two new links and remote deletes a different dest object") {
+            reset_collection({Add{dest_pk_4}, Add{dest_pk_5}}, {RemoveObject{"dest", dest_pk_2}},
+                                {dest_pk_3, dest_pk_4, dest_pk_5}, 2);
+        }
+        SECTION("remote deletes an object, then removes and adds to the list") {
+            reset_collection({}, {RemoveObject{"dest", dest_pk_2}, Remove{dest_pk_3}, Add{dest_pk_4}},
+                                {dest_pk_4}, 2);
+        }
+    }
+
     if (test_mode == ClientResyncMode::Recover) {
         SECTION("local adds a list item and removes source object, remote modifies list") {
             reset_collection_removing_source_object({Add{dest_pk_4}, RemoveObject{"source", source_pk}},

test/test_unresolved_links.cpp

@@ -262,6 +262,20 @@ TEST(Unresolved_LinkList)
 
 TEST(Unresolved_LinkSet)
 {
+    auto check_sorted = [&](LnkSet& set, std::vector<Obj> expected) {
+        std::vector<size_t> indices;
+        set.sort(indices);
+        CHECK_EQUAL(indices.size(), expected.size());
+        for (size_t i = 0; i < indices.size(); ++i) {
+            CHECK_EQUAL(set.get(indices[i]), expected[i].get_key());
+        }
+        indices.clear();
+        set.distinct(indices);
+        CHECK_EQUAL(indices.size(), expected.size());
+        for (size_t i = 0; i < indices.size(); ++i) {
+            CHECK_EQUAL(set.get(indices[i]), expected[i].get_key());
+        }
+    };
     Group g;
 
     auto cars = g.add_table_with_primary_key("Car", type_String, "model");
@@ -285,14 +299,17 @@ TEST(Unresolved_LinkSet)
 
     CHECK_EQUAL(stock1.size(), 4);
     CHECK_EQUAL(stock2.size(), 4);
+    check_sorted(stock1, {skoda, tesla, volvo, bmw});
     tesla.invalidate();
     CHECK_EQUAL(stock1.size(), 3);
     CHECK_EQUAL(stock2.size(), 3);
+    check_sorted(stock1, {skoda, volvo, bmw});
 
     stock1.insert(mercedes.get_key());
     // If REALM_MAX_BPNODE_SIZE is 4, we test that context flag is copied over when replacing root
     CHECK_EQUAL(stock1.size(), 4);
     CHECK_EQUAL(stock2.size(), 4);
+    check_sorted(stock1, {skoda, volvo, bmw, mercedes});
 
     LnkSet stock_copy{stock1};
     CHECK_EQUAL(stock_copy.get(3), mercedes.get_key());