diff --git a/CHANGELOG.md b/CHANGELOG.md index 4753a4295a6..620b089370f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* Fixes prior_size history corruption when replacing an embedded object in a list ([#4845](https://github.com/realm/realm-core/issues/4845)) ### Breaking changes * None. diff --git a/src/realm/sync/instruction_replication.cpp b/src/realm/sync/instruction_replication.cpp index b95f9113aaf..63f129dc408 100644 --- a/src/realm/sync/instruction_replication.cpp +++ b/src/realm/sync/instruction_replication.cpp @@ -14,8 +14,8 @@ SyncReplication::SyncReplication(const std::string& realm_path) void SyncReplication::initialize(DB& sg) { - REALM_ASSERT(!m_sg); - m_sg = &sg; + REALM_ASSERT(!m_db); + m_db = &sg; } void SyncReplication::reset() @@ -424,12 +424,40 @@ void SyncReplication::list_set(const CollectionBase& list, size_t ndx, Mixed val } if (select_collection(list)) { - Instruction::Update instr; - populate_path_instr(instr, list, uint32_t(ndx)); - REALM_ASSERT(instr.is_array_update()); - instr.value = as_payload(list, value); - instr.prior_size = uint32_t(list.size()); - emit(instr); + // If this is an embedded object then we need to emit and erase/insert instruction so that the old + // object gets cleared, otherwise you'll only see the Update ObjectValue instruction, which is idempotent, + // and that will lead to corrupted prior size for array operations inside the embedded object during + // changeset application. + auto needs_insert_erase_sequence = [&] { + if (value.is_type(type_Link)) { + return list.get_target_table()->is_embedded(); + } + else if (value.is_type(type_TypedLink)) { + return m_transaction->get_table(value.get_link().get_table_key())->is_embedded(); + } + return false; + }; + if (needs_insert_erase_sequence()) { + REALM_ASSERT(!list.is_null(ndx)); + Instruction::ArrayErase erase_instr; + populate_path_instr(erase_instr, list, static_cast(ndx)); + erase_instr.prior_size = uint32_t(list.size()); + emit(erase_instr); + + Instruction::ArrayInsert insert_instr; + populate_path_instr(insert_instr, list, static_cast(ndx)); + insert_instr.prior_size = erase_instr.prior_size - 1; + insert_instr.value = as_payload(list, value); + emit(insert_instr); + } + else { + Instruction::Update instr; + populate_path_instr(instr, list, uint32_t(ndx)); + REALM_ASSERT(instr.is_array_update()); + instr.value = as_payload(list, value); + instr.prior_size = uint32_t(list.size()); + emit(instr); + } } } diff --git a/src/realm/sync/instruction_replication.hpp b/src/realm/sync/instruction_replication.hpp index cd1f66a39cb..33382bc98ed 100644 --- a/src/realm/sync/instruction_replication.hpp +++ b/src/realm/sync/instruction_replication.hpp @@ -124,7 +124,7 @@ class SyncReplication : public TrivialReplication { bool m_short_circuit = false; ChangesetEncoder m_encoder; - DB* m_sg = nullptr; + DB* m_db = nullptr; Transaction* m_transaction; // Consistency checks: diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 24ce2bc5ea7..a50ca9139fb 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -1681,10 +1681,40 @@ TEST_CASE("app: token refresh", "[sync][app][token]") { // MARK: - Sync Tests -TEST_CASE("app: set new embedded object", "[sync][app]") { +App::Config make_app_config(const std::string client_app_id) +{ auto factory = []() -> std::unique_ptr { return std::make_unique(); }; + return App::Config{client_app_id, + factory, + get_base_url(), + util::none, + Optional("A Local App Version"), + util::none, + "Object Store Platform Tests", + "Object Store Platform Version Blah", + "An sdk version"}; +} + +realm::Realm::Config make_realm_config(const std::shared_ptr user, const Schema& schema, + const std::string base_path) +{ + realm::Realm::Config realm_config; + realm_config.sync_config = std::make_shared(user, bson::Bson("foo")); + realm_config.sync_config->client_resync_mode = ClientResyncMode::Manual; + realm_config.sync_config->error_handler = [](std::shared_ptr, SyncError error) { + std::cerr << error.message << std::endl; + }; + realm_config.schema_version = 1; + realm_config.path = base_path + "/default.realm"; + realm_config.schema = schema; + return realm_config; +} + +// MARK: - Sync Tests +TEST_CASE("app: mixed lists with object links", "[sync][app]") { + std::string base_url = get_base_url(); const std::string valid_pk_name = "_id"; REQUIRE(!base_url.empty()); @@ -1693,50 +1723,137 @@ TEST_CASE("app: set new embedded object", "[sync][app]") { ObjectSchema("TopLevel", { {valid_pk_name, PropertyType::ObjectId, Property::IsPrimary{true}}, - {"embedded", PropertyType::Object | PropertyType::Nullable, "TopLevel_embedded"}, + {"mixed_array", PropertyType::Mixed | PropertyType::Array | PropertyType::Nullable}, }), - ObjectSchema("TopLevel_embedded", ObjectSchema::IsEmbedded{true}, + ObjectSchema("Target", { - {"array", PropertyType::Int | PropertyType::Array}, + {valid_pk_name, PropertyType::ObjectId, Property::IsPrimary{true}}, + {"value", PropertyType::Int}, }), }; auto server_app_config = minimal_app_config(base_url, "set_new_embedded_object", schema); auto app_session = create_app(server_app_config); - - auto app_config = App::Config{app_session.client_app_id, - factory, - base_url, - util::none, - Optional("A Local App Version"), - util::none, - "Object Store Platform Tests", - "Object Store Platform Version Blah", - "An sdk version"}; + auto app_config = make_app_config(app_session.client_app_id); auto base_path = util::make_temp_dir() + app_config.app_id; util::try_remove_dir_recursive(base_path); util::try_make_dir(base_path); - auto make_realm_config = [&](const std::shared_ptr user) { - realm::Realm::Config realm_config; - realm_config.sync_config = std::make_shared(user, bson::Bson("foo")); - realm_config.sync_config->client_resync_mode = ClientResyncMode::Manual; - realm_config.sync_config->error_handler = [](std::shared_ptr, SyncError error) { - std::cout << error.message << std::endl; - }; - realm_config.schema_version = 1; - realm_config.path = base_path + "/default.realm"; - realm_config.schema = server_app_config.schema; - return realm_config; + auto email = util::format("realm_tests_do_autoverify-test@example.com"); + auto password = std::string{"password"}; + auto obj_id = ObjectId::gen(); + auto target_id = ObjectId::gen(); + auto mixed_list_values = AnyVector{ + Mixed{int64_t(1234)}, + Mixed{}, + Mixed{target_id}, }; + { + TestSyncManager::Config tsm_config(app_config); + TestSyncManager sync_manager(tsm_config, {}); + auto app = sync_manager.app(); + app->provider_client().register_email( + email, password, [&](Optional error) { + CHECK(!error); + }); + app->log_in_with_credentials(realm::app::AppCredentials::username_password(email, password), + [&](std::shared_ptr user, Optional error) { + REQUIRE(user); + CHECK(!error); + }); + + auto realm = realm::Realm::get_shared_realm(make_realm_config(app->current_user(), schema, base_path)); + auto session = app->current_user()->session_for_on_disk_path(realm->config().path); + + CppContext c(realm); + realm->begin_transaction(); + auto target_obj = + Object::create(c, realm, "Target", + util::Any(AnyDict{{valid_pk_name, target_id}, {"value", static_cast(1234)}})); + mixed_list_values.push_back(Mixed(target_obj.obj().get_link())); + + auto array_of_objs = Object::create(c, realm, "TopLevel", + util::Any(AnyDict{ + {valid_pk_name, obj_id}, + {"mixed_array", mixed_list_values}, + }), + CreatePolicy::ForceCreate); + realm->commit_transaction(); + CHECK(!wait_for_upload(*realm)); + } + + { + util::try_remove_dir_recursive(base_path); + util::try_make_dir(base_path); + + TestSyncManager sync_manager(TestSyncManager::Config(app_config), {}); + auto app = sync_manager.app(); + app->log_in_with_credentials(realm::app::AppCredentials::username_password(email, password), + [&](std::shared_ptr user, Optional error) { + REQUIRE(user); + CHECK(!error); + }); + + auto realm = realm::Realm::get_shared_realm(make_realm_config(app->current_user(), schema, base_path)); + auto session = app->current_user()->session_for_on_disk_path(realm->config().path); - auto top_level_id = ObjectId::gen(); + CHECK(!wait_for_download(*realm)); + CppContext c(realm); + auto obj = Object::get_for_primary_key(c, realm, "TopLevel", util::Any{obj_id}); + auto list = any_cast(obj.get_property_value(c, "mixed_array")); + for (size_t idx = 0; idx < list.size(); ++idx) { + Mixed mixed = list.get_any(idx); + CHECK(mixed == any_cast(mixed_list_values[idx])); + } + } +} + +TEST_CASE("app: set new embedded object", "[sync][app]") { + std::string base_url = get_base_url(); + const std::string valid_pk_name = "_id"; + REQUIRE(!base_url.empty()); + + Schema schema{ + ObjectSchema("TopLevel", + { + {valid_pk_name, PropertyType::ObjectId, Property::IsPrimary{true}}, + {"array_of_objs", PropertyType::Object | PropertyType::Array, "TopLevel_array_of_objs"}, + {"embedded_obj", PropertyType::Object | PropertyType::Nullable, "TopLevel_embedded_obj"}, + {"embedded_dict", PropertyType::Object | PropertyType::Dictionary | PropertyType::Nullable, + "TopLevel_embedded_dict"}, + }), + ObjectSchema("TopLevel_array_of_objs", ObjectSchema::IsEmbedded{true}, + { + {"array", PropertyType::Int | PropertyType::Array}, + }), + ObjectSchema("TopLevel_embedded_obj", ObjectSchema::IsEmbedded{true}, + { + {"array", PropertyType::Int | PropertyType::Array}, + }), + ObjectSchema("TopLevel_embedded_dict", ObjectSchema::IsEmbedded{true}, + { + {"array", PropertyType::Int | PropertyType::Array}, + }), + }; + + auto server_app_config = minimal_app_config(base_url, "set_new_embedded_object", schema); + auto app_session = create_app(server_app_config); + auto app_config = make_app_config(app_session.client_app_id); + + auto base_path = util::make_temp_dir() + app_config.app_id; + util::try_remove_dir_recursive(base_path); + util::try_make_dir(base_path); + + auto array_of_objs_id = ObjectId::gen(); + auto embedded_obj_id = ObjectId::gen(); + auto dict_obj_id = ObjectId::gen(); auto email = util::format("realm_tests_do_autoverify-test@example.com"); auto password = std::string{"password"}; { - TestSyncManager sync_manager(TestSyncManager::Config(app_config), {}); + TestSyncManager::Config tsm_config(app_config); + TestSyncManager sync_manager(tsm_config, {}); auto app = sync_manager.app(); app->provider_client().register_email( email, password, [&](Optional error) { @@ -1748,38 +1865,63 @@ TEST_CASE("app: set new embedded object", "[sync][app]") { CHECK(!error); }); - auto realm = realm::Realm::get_shared_realm(make_realm_config(app->current_user())); + auto realm = realm::Realm::get_shared_realm(make_realm_config(app->current_user(), schema, base_path)); auto session = app->current_user()->session_for_on_disk_path(realm->config().path); CppContext c(realm); realm->begin_transaction(); - auto obj = Object::create(c, realm, "TopLevel", - util::Any(AnyDict{ - {valid_pk_name, top_level_id}, - {"embedded", AnyDict{{"array", AnyVector{INT64_C(1), INT64_C(2)}}}}, - }), - CreatePolicy::ForceCreate); - realm->commit_transaction(); + auto array_of_objs = + Object::create(c, realm, "TopLevel", + util::Any(AnyDict{ + {valid_pk_name, array_of_objs_id}, + {"array_of_objs", AnyVector{AnyDict{{"array", AnyVector{INT64_C(1), INT64_C(2)}}}}}, + }), + CreatePolicy::ForceCreate); + + auto embedded_obj = + Object::create(c, realm, "TopLevel", + util::Any(AnyDict{ + {valid_pk_name, embedded_obj_id}, + {"embedded_obj", AnyDict{{"array", AnyVector{INT64_C(1), INT64_C(2)}}}}, + }), + CreatePolicy::ForceCreate); + + auto dict_obj = Object::create( + c, realm, "TopLevel", + util::Any(AnyDict{ + {valid_pk_name, dict_obj_id}, + {"embedded_dict", AnyDict{{"foo", AnyDict{{"array", AnyVector{INT64_C(1), INT64_C(2)}}}}}}, + }), + CreatePolicy::ForceCreate); - realm->begin_transaction(); - obj.set_property_value(c, "embedded", - util::Any(AnyDict{{ - "array", - AnyVector{INT64_C(3), INT64_C(4)}, - }}), - realm::CreatePolicy::UpdateAll); realm->commit_transaction(); + { + realm->begin_transaction(); + embedded_obj.set_property_value(c, "embedded_obj", + util::Any(AnyDict{{ + "array", + AnyVector{INT64_C(3), INT64_C(4)}, + }}), + realm::CreatePolicy::UpdateAll); + realm->commit_transaction(); + } - std::promise promise; - auto future = promise.get_future(); - auto shared_promise = std::make_shared>(std::move(promise)); - session->wait_for_download_completion( - [shared_promise = std::move(shared_promise)](std::error_code ec) mutable { - REALM_ASSERT(!ec); - shared_promise->set_value(); - }); + { + realm->begin_transaction(); + List array(array_of_objs, array_of_objs.get_object_schema().property_for_name("array_of_objs")); + CppContext c2(realm, &array.get_object_schema()); + array.set(c2, 0, util::Any{AnyDict{{"array", AnyVector{INT64_C(5), INT64_C(6)}}}}); + realm->commit_transaction(); + } - future.wait(); + { + realm->begin_transaction(); + object_store::Dictionary dict(dict_obj, dict_obj.get_object_schema().property_for_name("embedded_dict")); + CppContext c2(realm, &dict.get_object_schema()); + dict.insert(c2, "foo", util::Any{AnyDict{{"array", AnyVector{INT64_C(7), INT64_C(8)}}}}); + realm->commit_transaction(); + } + CHECK(!wait_for_upload(*realm)); } { @@ -1794,26 +1936,43 @@ TEST_CASE("app: set new embedded object", "[sync][app]") { CHECK(!error); }); - auto realm = realm::Realm::get_shared_realm(make_realm_config(app->current_user())); + auto realm = realm::Realm::get_shared_realm(make_realm_config(app->current_user(), schema, base_path)); auto session = app->current_user()->session_for_on_disk_path(realm->config().path); - std::promise promise; - auto future = promise.get_future(); - auto shared_promise = std::make_shared>(std::move(promise)); - session->wait_for_download_completion( - [shared_promise = std::move(shared_promise)](std::error_code ec) mutable { - REALM_ASSERT(!ec); - shared_promise->set_value(); - }); + CHECK(!wait_for_download(*realm)); + { + CppContext c(realm); + auto obj = Object::get_for_primary_key(c, realm, "TopLevel", util::Any{embedded_obj_id}); + auto embedded_obj = any_cast(obj.get_property_value(c, "embedded_obj")); + auto array_list = any_cast(embedded_obj.get_property_value(c, "array")); + CHECK(array_list.size() == 2); + CHECK(array_list.get(0) == int64_t(3)); + CHECK(array_list.get(1) == int64_t(4)); + } - future.wait(); - CppContext c(realm); - auto obj = Object::get_for_primary_key(c, realm, "TopLevel", util::Any{top_level_id}); - auto embedded_obj = any_cast(obj.get_property_value(c, "embedded")); - auto array_list = any_cast(embedded_obj.get_property_value(c, "array")); - CHECK(array_list.size() == 2); - CHECK(array_list.get(0) == int64_t(3)); - CHECK(array_list.get(1) == int64_t(4)); + { + CppContext c(realm); + auto obj = Object::get_for_primary_key(c, realm, "TopLevel", util::Any{array_of_objs_id}); + auto embedded_list = any_cast(obj.get_property_value(c, "array_of_objs")); + CppContext c2(realm, &embedded_list.get_object_schema()); + auto embedded_array_obj = any_cast(embedded_list.get(c2, 0)); + auto array_list = any_cast(embedded_array_obj.get_property_value(c2, "array")); + CHECK(array_list.size() == 2); + CHECK(array_list.get(0) == int64_t(5)); + CHECK(array_list.get(1) == int64_t(6)); + } + + { + CppContext c(realm); + auto obj = Object::get_for_primary_key(c, realm, "TopLevel", util::Any{dict_obj_id}); + object_store::Dictionary dict(obj, obj.get_object_schema().property_for_name("embedded_dict")); + CppContext c2(realm, &dict.get_object_schema()); + auto embedded_obj = any_cast(dict.get(c2, "foo")); + auto array_list = any_cast(embedded_obj.get_property_value(c2, "array")); + CHECK(array_list.size() == 2); + CHECK(array_list.get(0) == int64_t(7)); + CHECK(array_list.get(1) == int64_t(8)); + } } } @@ -3904,7 +4063,6 @@ TEST_CASE("app: refresh access token unit tests", "[sync][app]") { completion_block({200, 0, {}, user_json(good_access_token).dump()}); } else if (request.url.find("/profile") != std::string::npos) { - CHECK(login_hit); auto access_token = request.headers.at("Authorization");