From 55b9f83b02a1f34d09b3d1199da687ac2b10387f Mon Sep 17 00:00:00 2001 From: Nicola Cabiddu Date: Tue, 18 Jul 2023 15:53:59 +0200 Subject: [PATCH] code review + some fixes in the client reset logic for handling mixed --- src/realm/object_converter.cpp | 1 - .../sync/noinst/client_reset_recovery.cpp | 57 +++- test/object-store/sync/client_reset.cpp | 315 +++++++++++++++--- 3 files changed, 309 insertions(+), 64 deletions(-) diff --git a/src/realm/object_converter.cpp b/src/realm/object_converter.cpp index ccc309f356c..3114f5c5ee0 100644 --- a/src/realm/object_converter.cpp +++ b/src/realm/object_converter.cpp @@ -15,7 +15,6 @@ * limitations under the License. * **************************************************************************/ -#include #include diff --git a/src/realm/sync/noinst/client_reset_recovery.cpp b/src/realm/sync/noinst/client_reset_recovery.cpp index 7896b88466c..f68f1fa3596 100644 --- a/src/realm/sync/noinst/client_reset_recovery.cpp +++ b/src/realm/sync/noinst/client_reset_recovery.cpp @@ -547,14 +547,55 @@ bool RecoverLocalChangesetsHandler::resolve_path(ListPath& path, Obj remote_obj, return false; } } - else { // single link to embedded object - // Neither embedded object sets nor Mixed(TypedLink) to embedded objects are supported. - REALM_ASSERT_EX(!col.is_collection(), col); - REALM_ASSERT_EX(col.get_type() == col_type_Link, col); - StringData col_name = remote_obj.get_table()->get_column_name(col); - remote_obj = remote_obj.get_linked_object(col); - local_obj = local_obj.get_linked_object(col_name); - ++it; + else { + if (col.get_type() == col_type_Mixed) { + StringData col_name = remote_obj.get_table()->get_column_name(col); + auto local_any = local_obj.get_any(col_name); + auto remote_any = remote_obj.get_any(col); + + if (local_any.is_type(type_List) && remote_any.is_type(type_List)) { + ++it; + if (it == path.end()) { + auto local_col = local_obj.get_table()->get_column_key(col_name); + Lst local_list{local_obj, local_col}; + Lst remote_list{remote_obj, col}; + callback(remote_list, local_list); + return true; + } + else { + // same as above. + REALM_UNREACHABLE(); + } + } + else if (local_any.is_type(type_Dictionary) && remote_any.is_type(type_Dictionary)) { + ++it; + REALM_ASSERT(it != path.end()); + REALM_ASSERT(it->type == ListPath::Element::Type::InternKey); + StringData col_name = remote_obj.get_table()->get_column_name(col); + auto local_col = local_obj.get_table()->get_column_key(col_name); + Dictionary remote_dict{remote_obj, col}; + Dictionary local_dict{local_obj, local_col}; + StringData dict_key = m_intern_keys.get_key(it->intern_key); + if (remote_dict.contains(dict_key) && local_dict.contains(dict_key)) { + remote_obj = remote_dict.get_object(dict_key); + local_obj = local_dict.get_object(dict_key); + ++it; + } + else { + return false; + } + } + } + else { + // single link to embedded object + // Neither embedded object sets nor Mixed(TypedLink) to embedded objects are supported. + REALM_ASSERT_EX(!col.is_collection(), col); + REALM_ASSERT_EX(col.get_type() == col_type_Link, col); + StringData col_name = remote_obj.get_table()->get_column_name(col); + remote_obj = remote_obj.get_linked_object(col); + local_obj = local_obj.get_linked_object(col_name); + ++it; + } } } return false; diff --git a/test/object-store/sync/client_reset.cpp b/test/object-store/sync/client_reset.cpp index cfedf9489b1..5c8b8ea4687 100644 --- a/test/object-store/sync/client_reset.cpp +++ b/test/object-store/sync/client_reset.cpp @@ -4189,6 +4189,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c config2.schema = Schema{shared_class}; auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); TableRef table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4207,6 +4208,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c else { test_reset ->on_post_reset([&](SharedRealm local) { + advance_and_notify(*local); TableRef table = get_table(*local, "TopLevel"); REQUIRE(table->size() == 1); auto obj = table->get_object(0); @@ -4354,6 +4356,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset ->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); auto table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4363,6 +4366,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c REQUIRE(list.size() == 1); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); auto table = get_table(*remote_realm, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4372,9 +4376,9 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c REQUIRE(set.size() == 1); }) ->on_post_reset([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); if (test_mode == ClientResyncMode::DiscardLocal) return; - advance_and_notify(*local_realm); TableRef table = get_table(*local_realm, "TopLevel"); REQUIRE(table->size() == 1); auto obj = table->get_object(0); @@ -4392,6 +4396,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset ->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); auto table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4401,6 +4406,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c REQUIRE(list.size() == 1); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); auto table = get_table(*remote_realm, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4430,6 +4436,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset ->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); auto table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4441,6 +4448,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c REQUIRE(list.size() == 1); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); auto table = get_table(*remote_realm, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4490,6 +4498,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset ->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); auto table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4504,6 +4513,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c REQUIRE(list.size() == 2); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); auto table = get_table(*remote_realm, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4557,6 +4567,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset ->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); auto table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4572,6 +4583,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c REQUIRE(list.size() == 3); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); auto table = get_table(*remote_realm, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4631,6 +4643,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); test_reset ->make_local_changes([&](SharedRealm local) { + advance_and_notify(*local); auto table = get_table(*local, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4641,6 +4654,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c n_list.insert(0, Mixed{30}); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); auto table = get_table(*remote_realm, "TopLevel"); auto obj = table->create_object_with_primary_key(pk_val); auto col = table->get_column_key("any_mixed"); @@ -4690,6 +4704,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c n_list.insert(0, Mixed{30}); }) ->make_local_changes([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); TableRef table = get_table(*local_realm, "TopLevel"); REQUIRE(table->size() == 1); auto obj = table->get_object(0); @@ -4700,6 +4715,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c n_list.insert(0, Mixed{50}); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); TableRef table = get_table(*remote_realm, "TopLevel"); REQUIRE(table->size() == 1); auto obj = table->get_object(0); @@ -4732,58 +4748,58 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c SyncTestFile config2(init_sync_manager.app(), "default"); config2.schema = config.schema; auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); - auto test = test_reset - ->setup([&](SharedRealm realm) { - auto table = get_table(*realm, "TopLevel"); - auto obj = table->create_object_with_primary_key(pk_val); - auto col = table->get_column_key("any_mixed"); - obj.set_collection(col, CollectionType::List); - List list{realm, obj, col}; - list.insert_collection(0, CollectionType::List); - auto n_list = list.get_list(0); - n_list.insert(0, Mixed{30}); - }) - ->make_local_changes([&](SharedRealm local_realm) { - TableRef table = get_table(*local_realm, "TopLevel"); - REQUIRE(table->size() == 1); - auto obj = table->get_object(0); - auto col = table->get_column_key("any_mixed"); - List list{local_realm, obj, col}; - list.insert_collection(0, CollectionType::List); - auto n_list = list.get_list(0); - n_list.insert(0, Mixed{50}); - }) - ->make_remote_changes([&](SharedRealm remote_realm) { - TableRef table = get_table(*remote_realm, "TopLevel"); - REQUIRE(table->size() == 1); - auto obj = table->get_object(0); - auto col = table->get_column_key("any_mixed"); - List list{remote_realm, obj, col}; - REQUIRE(list.size() == 1); - list.remove(0); - }); - { - test->on_post_reset([&](SharedRealm local_realm) { - advance_and_notify(*local_realm); - if (test_mode == ClientResyncMode::DiscardLocal) { - TableRef table = get_table(*local_realm, "TopLevel"); - REQUIRE(table->size() == 1); - auto obj = table->get_object(0); - auto col = table->get_column_key("any_mixed"); - List list{local_realm, obj, col}; - REQUIRE(list.size() == 0); - } - else { - TableRef table = get_table(*local_realm, "TopLevel"); - REQUIRE(table->size() == 1); - auto obj = table->get_object(0); - auto col = table->get_column_key("any_mixed"); - List list{local_realm, obj, col}; - REQUIRE(list.size() == 1); - } - }) - ->run(); - } + test_reset + ->setup([&](SharedRealm realm) { + auto table = get_table(*realm, "TopLevel"); + auto obj = table->create_object_with_primary_key(pk_val); + auto col = table->get_column_key("any_mixed"); + obj.set_collection(col, CollectionType::List); + List list{realm, obj, col}; + list.insert_collection(0, CollectionType::List); + auto n_list = list.get_list(0); + n_list.insert(0, Mixed{30}); + }) + ->make_local_changes([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{local_realm, obj, col}; + list.insert_collection(0, CollectionType::List); + auto n_list = list.get_list(0); + n_list.insert(0, Mixed{50}); + }) + ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); + TableRef table = get_table(*remote_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{remote_realm, obj, col}; + REQUIRE(list.size() == 1); + list.remove(0); + }) + ->on_post_reset([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + if (test_mode == ClientResyncMode::DiscardLocal) { + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{local_realm, obj, col}; + REQUIRE(list.size() == 0); + } + else { + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{local_realm, obj, col}; + REQUIRE(list.size() == 1); + } + }) + ->run(); } SECTION("shift collection remotely and locally") { ObjectId pk_val = ObjectId::gen(); @@ -4802,6 +4818,7 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c n_list.insert(0, Mixed{30}); }) ->make_local_changes([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); TableRef table = get_table(*local_realm, "TopLevel"); REQUIRE(table->size() == 1); auto obj = table->get_object(0); @@ -4809,12 +4826,12 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c List list{local_realm, obj, col}; auto n_list = list.get_list(0); n_list.insert(0, Mixed{50}); - list.insert_collection(0, CollectionType::List); // shift auto n_list1 = list.get_list(0); n_list1.insert(0, Mixed{150}); }) ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); TableRef table = get_table(*remote_realm, "TopLevel"); REQUIRE(table->size() == 1); auto obj = table->get_object(0); @@ -4822,7 +4839,6 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c List list{remote_realm, obj, col}; auto n_list = list.get_list(0); n_list.insert(1, Mixed{100}); - list.insert_collection(0, CollectionType::List); // shift auto n_list1 = list.get_list(0); n_list1.insert(0, Mixed{42}); @@ -4862,4 +4878,193 @@ TEST_CASE("client reset with nested collection", "[client reset][local][nested c }) ->run(); } + SECTION("delete collection locally (list). Local should win") { + ObjectId pk_val = ObjectId::gen(); + SyncTestFile config2(init_sync_manager.app(), "default"); + config2.schema = config.schema; + auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); + test_reset + ->setup([&](SharedRealm realm) { + auto table = get_table(*realm, "TopLevel"); + auto obj = table->create_object_with_primary_key(pk_val); + auto col = table->get_column_key("any_mixed"); + obj.set_collection(col, CollectionType::List); + List list{realm, obj, col}; + list.insert_collection(0, CollectionType::List); + auto n_list = list.get_list(0); + n_list.insert(0, Mixed{30}); + }) + ->make_local_changes([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{local_realm, obj, col}; + REQUIRE(list.size() == 1); + list.remove(0); + }) + ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); + TableRef table = get_table(*remote_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{remote_realm, obj, col}; + list.add(Mixed{10}); + REQUIRE(list.size() == 2); + }) + ->on_post_reset([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + if (test_mode == ClientResyncMode::DiscardLocal) { + // local state wins + List list{local_realm, obj, col}; + REQUIRE(list.size() == 2); + auto n_list1 = list.get_list(0); + auto n_list2 = list.get_list(1); + REQUIRE(n_list1.size() == 1); + REQUIRE(n_list2.size() == 0); + REQUIRE(n_list1.get_any(0).get_int() == 30); + } + else { + List list{local_realm, obj, col}; + REQUIRE(list.size() == 0); + } + }) + ->run(); + } + SECTION("move collection locally (list). Local should win") { + ObjectId pk_val = ObjectId::gen(); + SyncTestFile config2(init_sync_manager.app(), "default"); + config2.schema = config.schema; + auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); + test_reset + ->setup([&](SharedRealm realm) { + auto table = get_table(*realm, "TopLevel"); + auto obj = table->create_object_with_primary_key(pk_val); + auto col = table->get_column_key("any_mixed"); + obj.set_collection(col, CollectionType::List); + List list{realm, obj, col}; + list.insert_collection(0, CollectionType::List); + auto n_list = list.get_list(0); + n_list.insert(0, Mixed{30}); + n_list.insert(1, Mixed{10}); + }) + ->make_local_changes([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{local_realm, obj, col}; + auto nlist = list.get_list(0); + nlist.move(0, 1); // move value 30 in pos 1. + REQUIRE(nlist.size() == 2); + REQUIRE(nlist.get_any(0).get_int() == 10); + REQUIRE(nlist.get_any(1).get_int() == 30); + }) + ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); + TableRef table = get_table(*remote_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + List list{remote_realm, obj, col}; + REQUIRE(list.size() == 1); + auto nlist = list.get_list(0); + REQUIRE(nlist.size() == 2); + nlist.add(Mixed{2}); + REQUIRE(nlist.size() == 3); + }) + ->on_post_reset([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + if (test_mode == ClientResyncMode::DiscardLocal) { + // local state is preserved + List list{local_realm, obj, col}; + REQUIRE(list.size() == 1); + auto nlist = list.get_list(0); + REQUIRE(nlist.size() == 3); + REQUIRE(nlist.get_any(0).get_int() == 30); + REQUIRE(nlist.get_any(1).get_int() == 10); + REQUIRE(nlist.get_any(2).get_int() == 2); + } + else { + // local change wins + List list{local_realm, obj, col}; + REQUIRE(list.size() == 1); + auto nlist = list.get_list(0); + REQUIRE(nlist.size() == 2); + REQUIRE(nlist.get_any(0).get_int() == 10); + REQUIRE(nlist.get_any(1).get_int() == 30); + } + }) + ->run(); + } + SECTION("delete collection locally (dictionary). Local should win") { + ObjectId pk_val = ObjectId::gen(); + SyncTestFile config2(init_sync_manager.app(), "default"); + config2.schema = config.schema; + auto test_reset = reset_utils::make_fake_local_client_reset(config, config2); + test_reset + ->setup([&](SharedRealm realm) { + auto table = get_table(*realm, "TopLevel"); + auto obj = table->create_object_with_primary_key(pk_val); + auto col = table->get_column_key("any_mixed"); + obj.set_collection(col, CollectionType::Dictionary); + object_store::Dictionary dictionary{realm, obj, col}; + dictionary.insert_collection("Test", CollectionType::Dictionary); + auto n_dictionary = dictionary.get_dictionary("Test"); + n_dictionary.insert("Val", 30); + }) + ->make_local_changes([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + object_store::Dictionary dictionary{local_realm, obj, col}; + REQUIRE(dictionary.size() == 1); + dictionary.erase("Test"); + }) + ->make_remote_changes([&](SharedRealm remote_realm) { + advance_and_notify(*remote_realm); + TableRef table = get_table(*remote_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + object_store::Dictionary dictionary{remote_realm, obj, col}; + REQUIRE(dictionary.size() == 1); + auto n_dictionary = dictionary.get_dictionary("Test"); + n_dictionary.insert("Val1", 31); + }) + ->on_post_reset([&](SharedRealm local_realm) { + advance_and_notify(*local_realm); + TableRef table = get_table(*local_realm, "TopLevel"); + REQUIRE(table->size() == 1); + auto obj = table->get_object(0); + auto col = table->get_column_key("any_mixed"); + if (test_mode == ClientResyncMode::DiscardLocal) { + // local state wins + object_store::Dictionary dictionary{local_realm, obj, col}; + REQUIRE(dictionary.size() == 1); + auto n_dictionary = dictionary.get_dictionary("Test"); + REQUIRE(n_dictionary.get_any("Val").get_int() == 30); + REQUIRE(n_dictionary.get_any("Val1").get_int() == 31); + } + else { + // local change wins + object_store::Dictionary dictionary{local_realm, obj, col}; + REQUIRE(dictionary.size() == 0); + } + }) + ->run(); + } }