Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCORE-788 Fix prior_size issues when replacing an embedded object in a list #4844

Merged
merged 6 commits into from
Aug 12, 2021
Merged

RCORE-788 Fix prior_size issues when replacing an embedded object in a list #4844

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ee1a01
RCORE-788 Fix prior_size issues when replacing an embedded object in …
jbreams Aug 11, 2021
ff3787f
changelog
jbreams Aug 11, 2021
ad17c80
do it all in sync
jbreams Aug 11, 2021
c4249b6
format
jbreams Aug 11, 2021
5881f41
add test for james
jbreams Aug 12, 2021
5e128e9
appease codefactor
jbreams Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

### Fixed
* <How to hit and notice issue? what was the impact?> ([#????](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.
Expand Down
31 changes: 25 additions & 6 deletions src/realm/sync/instruction_replication.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,31 @@ 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.
if (value.is_type(type_Link, type_TypedLink) && list.get_target_table()->is_embedded()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type is a list of mixed, then list.get_target_table() will return null. In that case, the mixed value will be a typed link and we should get the target table from value.get_link().get_table_key(). Could you add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I now handle type_Link and type_TypedLink separately.

REALM_ASSERT(!list.is_null(ndx));
Instruction::ArrayErase erase_instr;
populate_path_instr(erase_instr, list, static_cast<uint32_t>(ndx));
erase_instr.prior_size = uint32_t(list.size());
emit(erase_instr);

Instruction::ArrayInsert insert_instr;
populate_path_instr(insert_instr, list, static_cast<uint32_t>(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);
}
}
}

Expand Down
138 changes: 106 additions & 32 deletions test/object-store/sync/app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1693,9 +1693,20 @@ 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"},
{"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_embedded", ObjectSchema::IsEmbedded{true},
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},
}),
Expand Down Expand Up @@ -1723,20 +1734,23 @@ TEST_CASE("app: set new embedded object", "[sync][app]") {
realm_config.sync_config = std::make_shared<realm::SyncConfig>(user, bson::Bson("foo"));
realm_config.sync_config->client_resync_mode = ClientResyncMode::Manual;
realm_config.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError error) {
std::cout << error.message << std::endl;
std::cerr << 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 top_level_id = ObjectId::gen();
auto array_of_objs_id = ObjectId::gen();
auto embedded_obj_id = ObjectId::gen();
auto dict_obj_id = ObjectId::gen();
auto email = util::format("[email protected]");
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<App::UsernamePasswordProviderClient>().register_email(
email, password, [&](Optional<app::AppError> error) {
Expand All @@ -1753,31 +1767,65 @@ TEST_CASE("app: set new embedded object", "[sync][app]") {

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

{
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();
}

{
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();
}

std::promise<void> promise;
auto future = promise.get_future();
auto shared_promise = std::make_shared<std::promise<void>>(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();
});
session->wait_for_upload_completion([shared_promise = std::move(shared_promise)](std::error_code ec) mutable {
REALM_ASSERT(!ec);
shared_promise->set_value();
});

future.wait();
}
Expand All @@ -1802,18 +1850,44 @@ TEST_CASE("app: set new embedded object", "[sync][app]") {
auto shared_promise = std::make_shared<std::promise<void>>(std::move(promise));
session->wait_for_download_completion(
[shared_promise = std::move(shared_promise)](std::error_code ec) mutable {
REALM_ASSERT(!ec);
CHECK(!ec);
shared_promise->set_value();
});

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<Object&&>(obj.get_property_value<util::Any>(c, "embedded"));
auto array_list = any_cast<List&&>(embedded_obj.get_property_value<util::Any>(c, "array"));
CHECK(array_list.size() == 2);
CHECK(array_list.get<int64_t>(0) == int64_t(3));
CHECK(array_list.get<int64_t>(1) == int64_t(4));
{
CppContext c(realm);
auto obj = Object::get_for_primary_key(c, realm, "TopLevel", util::Any{embedded_obj_id});
auto embedded_obj = any_cast<Object&&>(obj.get_property_value<util::Any>(c, "embedded_obj"));
auto array_list = any_cast<List&&>(embedded_obj.get_property_value<util::Any>(c, "array"));
CHECK(array_list.size() == 2);
CHECK(array_list.get<int64_t>(0) == int64_t(3));
CHECK(array_list.get<int64_t>(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<List&&>(obj.get_property_value<util::Any>(c, "array_of_objs"));
CppContext c2(realm, &embedded_list.get_object_schema());
auto embedded_array_obj = any_cast<Object&&>(embedded_list.get(c2, 0));
auto array_list = any_cast<List&&>(embedded_array_obj.get_property_value<util::Any>(c2, "array"));
CHECK(array_list.size() == 2);
CHECK(array_list.get<int64_t>(0) == int64_t(5));
CHECK(array_list.get<int64_t>(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<Object&&>(dict.get(c2, "foo"));
auto array_list = any_cast<List&&>(embedded_obj.get_property_value<util::Any>(c2, "array"));
CHECK(array_list.size() == 2);
CHECK(array_list.get<int64_t>(0) == int64_t(7));
CHECK(array_list.get<int64_t>(1) == int64_t(8));
}
}
}

Expand Down