diff --git a/CHANGELOG.md b/CHANGELOG.md index 859e9194b6d..46ac8802eb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* Fixed a bug that prevented an ObjectSchema with incoming links from being marked as embedded during migrations. ([#4414](https://github.com/realm/realm-core/pull/4414)) ### Breaking changes * None. diff --git a/src/realm/object-store/object_store.cpp b/src/realm/object-store/object_store.cpp index cea72a6a69a..38bb79c1c5f 100644 --- a/src/realm/object-store/object_store.cpp +++ b/src/realm/object-store/object_store.cpp @@ -563,16 +563,6 @@ static void apply_non_migration_changes(Group& group, std::vector verify_no_errors(applier, changes); } -static void set_embedded(Table& table, ObjectSchema::IsEmbedded is_embedded) -{ - if (!table.set_embedded(is_embedded) && is_embedded) { - auto msg = - util::format("Cannot convert object type '%1' to embedded because objects have multiple incoming links.", - ObjectStore::object_type_for_table_name(table.get_name())); - throw std::logic_error(std::move(msg)); - } -} - static void set_primary_key(Table& table, const Property* property) { ColKey col; @@ -612,7 +602,7 @@ static void create_initial_tables(Group& group, std::vector const& // downside. void operator()(ChangeTableType op) { - set_embedded(table(op.object), op.object->is_embedded); + table(op.object).set_embedded(op.object->is_embedded); } void operator()(AddProperty op) { @@ -736,9 +726,8 @@ static void apply_pre_migration_changes(Group& group, std::vector create_table(group, *op.object); } void operator()(RemoveTable) {} - void operator()(ChangeTableType op) - { - set_embedded(table(op.object), op.object->is_embedded); + void operator()(ChangeTableType) + { /* delayed until after the migration */ } void operator()(AddInitialProperties op) { @@ -840,7 +829,10 @@ static void apply_post_migration_changes(Group& group, std::vector table(op.object).remove_search_index(op.property->column_key); } - void operator()(ChangeTableType) {} + void operator()(ChangeTableType op) + { + table(op.object).set_embedded(op.object->is_embedded); + } void operator()(RemoveTable) {} void operator()(ChangePropertyType) {} void operator()(MakePropertyNullable) {} diff --git a/src/realm/table.cpp b/src/realm/table.cpp index 2501a3293ba..6ca72b6f06c 100644 --- a/src/realm/table.cpp +++ b/src/realm/table.cpp @@ -1029,42 +1029,51 @@ void Table::do_erase_root_column(ColKey col_key) bump_storage_version(); } -bool Table::set_embedded(bool embedded) +void Table::set_embedded(bool embedded) { - if (embedded == m_is_embedded) - return true; + if (embedded == m_is_embedded) { + return; + } if (Replication* repl = get_repl()) { if (repl->get_history_type() == Replication::HistoryType::hist_SyncClient) { - throw std::logic_error("Cannot change embedded property in sync client"); + throw std::logic_error("Cannot change table to embedded when using Sync."); } } + if (embedded == false) { + do_set_embedded(false); + return; + } + + // Embedded objects cannot have a primary key. if (get_primary_key_column()) { - return false; + throw std::logic_error("Cannot change table to embedded when using a primary key."); } - if (size() > 0) { - // Check if the table has any backlink columns. If not, it is not required - // to check all objects for backlinks. - bool has_backlink_columns = false; - for_each_backlink_column([&has_backlink_columns](ColKey) { - has_backlink_columns = true; - return true; // Done - }); - if (has_backlink_columns) { - for (auto o : *this) { - // each object should be owned by one and only one parent - if (o.get_backlink_count() != 1) { - return false; - } + // `has_backlink_columns` indicates if the table is embedded in any other table. + bool has_backlink_columns = false; + for_each_backlink_column([&has_backlink_columns](ColKey) { + has_backlink_columns = true; + return true; + }); + if (!has_backlink_columns) { + throw std::logic_error("Cannot change table to embedded without backlink columns. Table must be embedded in " + "at least one other table."); + } + else if (size() > 0) { + for (auto object : *this) { + size_t backlink_count = object.get_backlink_count(); + if (backlink_count == 0) { + throw std::logic_error("At least one object does not have a backlink (data would get lost)."); + } + else if (backlink_count > 1) { + throw std::logic_error("At least one object does have multiple backlinks."); } } } do_set_embedded(embedded); - - return true; } void Table::do_set_embedded(bool embedded) diff --git a/src/realm/table.hpp b/src/realm/table.hpp index d6f5c0f883f..3c91a628fae 100644 --- a/src/realm/table.hpp +++ b/src/realm/table.hpp @@ -161,9 +161,8 @@ class Table { bool valid_column(ColKey col_key) const noexcept; void check_column(ColKey col_key) const; // Change the embedded property of a table. If switching to being embedded, the table must - // not have a primary key and all objects must have exactly 1 backlink. Return value - // indicates if the conversion was done - bool set_embedded(bool embedded); + // not have a primary key and all objects must have exactly 1 backlink. + void set_embedded(bool embedded); //@} /// True for `col_type_Link` and `col_type_LinkList`. diff --git a/test/object-store/migrations.cpp b/test/object-store/migrations.cpp index b0dfcdfbdce..85893264875 100644 --- a/test/object-store/migrations.cpp +++ b/test/object-store/migrations.cpp @@ -434,7 +434,7 @@ TEST_CASE("migration: Automatic") { REQUIRE_UPDATE_SUCCEEDS(*realm, schema2, 1); } - SECTION("change table from embedded to top-level") { + SECTION("change table from embedded to top-level without version bump") { auto realm = Realm::get_shared_realm(config); Schema schema = { @@ -448,7 +448,7 @@ TEST_CASE("migration: Automatic") { REQUIRE_MIGRATION_NEEDED(*realm, schema, set_embedded(schema, "object", false)); } - SECTION("change table from top-level to embedded") { + SECTION("change table from top-level to embedded without version bump") { auto realm = Realm::get_shared_realm(config); Schema schema = { @@ -577,29 +577,286 @@ TEST_CASE("migration: Automatic") { REQUIRE(realm->schema() == schema1); } - SECTION("make object with multiple pre-existing incoming links embedded") { + SECTION("change empty table from top-level to embedded") { Schema schema = { - {"target", + {"child_table", { {"value", PropertyType::Int}, }}, - {"link", + {"parent_table", { - {"link", PropertyType::Object | PropertyType::Nullable, "target"}, + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_NOTHROW(realm->update_schema(set_embedded(schema, "child_table", true), 2, nullptr)); + + REQUIRE(realm->schema_version() == 2); + REQUIRE(child_table->is_embedded()); + } + + SECTION("change empty table from embedded to top-level") { + Schema schema = { + {"child_table", + ObjectSchema::IsEmbedded{true}, + { + {"value", PropertyType::Int}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, }}, }; auto realm = Realm::get_shared_realm(config); realm->update_schema(schema, 1); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + REQUIRE(child_table->is_embedded()); + REQUIRE_NOTHROW(realm->update_schema(set_embedded(schema, "child_table", false), 2, nullptr)); + + REQUIRE(realm->schema_version() == 2); + REQUIRE_FALSE(child_table->is_embedded()); + } + + SECTION("re-apply embedded flag to table") { + Schema schema = { + {"child_table", + ObjectSchema::IsEmbedded{true}, + { + {"value", PropertyType::Int}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + REQUIRE(child_table->is_embedded()); + + REQUIRE_NOTHROW(realm->update_schema(set_embedded(schema, "child_table", true), 2, nullptr)); + + REQUIRE(realm->schema_version() == 2); + REQUIRE(child_table->is_embedded()); + } + + SECTION("change table to embedded - table has primary key") { + Schema schema = { + {"child_table", + { + {"value", PropertyType::Int, Property::IsPrimary{true}}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_THROWS(realm->update_schema(set_embedded(schema, "child_table", true), 2, nullptr)); + } + + SECTION("change table to embedded - no migration block") { + Schema schema = { + {"object", + { + {"value", PropertyType::Int}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "object"); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_THROWS(realm->update_schema(set_embedded(schema, "object", true), 2, nullptr)); + } + + SECTION("change table to embedded - table has no backlinks") { + Schema schema = { + {"object", + { + {"value", PropertyType::Int}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "object"); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_THROWS(realm->update_schema(set_embedded(schema, "object", true), 2, [](auto, auto, auto&) {})); + } + + SECTION("change table to embedded - one incoming link per object") { + Schema schema = { + {"child_table", + { + {"value", PropertyType::Int}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + realm->begin_transaction(); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + Obj child_object1 = child_table->create_object(); + child_object1.set("value", 42); + Obj child_object2 = child_table->create_object(); + child_object2.set("value", 43); + auto parent_table = ObjectStore::table_for_object_type(realm->read_group(), "parent_table"); + auto child_object_key1 = child_object1.get_key(); + auto child_object_key2 = child_object2.get_key(); + parent_table->create_object().set_all(child_object_key1); + parent_table->create_object().set_all(child_object_key2); + realm->commit_transaction(); + REQUIRE(parent_table->size() == 2); + REQUIRE(child_table->size() == 2); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_NOTHROW(realm->update_schema(set_embedded(schema, "child_table", true), 2, nullptr)); + + REQUIRE(realm->schema_version() == 2); + REQUIRE(parent_table->size() == 2); + REQUIRE(child_table->size() == 2); + REQUIRE(child_table->is_embedded()); + for (int i = 0; i < 2; i++) { + Object parent_object(realm, "parent_table", i); + CppContext context(realm); + Object child_object = + any_cast(parent_object.get_property_value(context, "child_property")); + Int value = any_cast(child_object.get_property_value(context, "value")); + REQUIRE(value == 42 + i); + } + } + + SECTION("change table to embedded - multiple incoming link per object") { + Schema schema = { + {"child_table", + { + {"value", PropertyType::Int}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); realm->begin_transaction(); - auto target_table = ObjectStore::table_for_object_type(realm->read_group(), "target"); - auto obj = target_table->create_object().get_key(); - auto link_table = ObjectStore::table_for_object_type(realm->read_group(), "link"); - link_table->create_object().set_all(obj); - link_table->create_object().set_all(obj); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + Obj child_object = child_table->create_object(); + child_object.set("value", 42); + auto parent_table = ObjectStore::table_for_object_type(realm->read_group(), "parent_table"); + auto child_object_key = child_object.get_key(); + parent_table->create_object().set_all(child_object_key); + parent_table->create_object().set_all(child_object_key); realm->commit_transaction(); + REQUIRE(parent_table->size() == 2); + REQUIRE(child_table->size() == 1); + REQUIRE_FALSE(child_table->is_embedded()); - REQUIRE_THROWS(realm->update_schema(set_embedded(schema, "target", true), 2, nullptr)); + REQUIRE_THROWS(realm->update_schema(set_embedded(schema, "child_table", true), 2, nullptr)); + } + + SECTION("change table to embedded - multiple incoming links - resolved in migration block") { + Schema schema = { + {"child_table", + { + {"value", PropertyType::Int}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + realm->begin_transaction(); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + Obj child_object = child_table->create_object(); + child_object.set("value", 42); + auto parent_table = ObjectStore::table_for_object_type(realm->read_group(), "parent_table"); + auto child_object_key = child_object.get_key(); + parent_table->create_object().set_all(child_object_key); + parent_table->create_object().set_all(child_object_key); + realm->commit_transaction(); + REQUIRE(parent_table->size() == 2); + REQUIRE(child_table->size() == 1); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_NOTHROW( + realm->update_schema(set_embedded(schema, "child_table", true), 2, [](auto, auto new_realm, auto&) { + Object parent_object1(new_realm, "parent_table", 0); + CppContext context(new_realm); + Object child_object1 = + any_cast(parent_object1.get_property_value(context, "child_property")); + Int value = any_cast(child_object1.get_property_value(context, "value")); + + auto child_table = ObjectStore::table_for_object_type(new_realm->read_group(), "child_table"); + Obj child_object2 = child_table->create_object(); + child_object2.set("value", value); + + Object parent_object2(new_realm, "parent_table", 1); + parent_object2.set_property_value(context, "child_property", util::Any(child_object2)); + })); + + REQUIRE(realm->schema_version() == 2); + REQUIRE(parent_table->size() == 2); + REQUIRE(child_table->size() == 2); + REQUIRE(child_table->is_embedded()); + for (int i = 0; i < 2; i++) { + Object parent_object(realm, "parent_table", i); + CppContext context(realm); + Object child_object = + any_cast(parent_object.get_property_value(context, "child_property")); + Int value = any_cast(child_object.get_property_value(context, "value")); + REQUIRE(value == 42); + } + } + + SECTION("change table to embedded - adding more links in migration block") { + Schema schema = { + {"child_table", + { + {"value", PropertyType::Int}, + }}, + {"parent_table", + { + {"child_property", PropertyType::Object | PropertyType::Nullable, "child_table"}, + }}, + }; + auto realm = Realm::get_shared_realm(config); + realm->update_schema(schema, 1); + realm->begin_transaction(); + auto child_table = ObjectStore::table_for_object_type(realm->read_group(), "child_table"); + Obj child_object = child_table->create_object(); + auto parent_table = ObjectStore::table_for_object_type(realm->read_group(), "parent_table"); + auto child_object_key = child_object.get_key(); + parent_table->create_object().set_all(child_object_key); + realm->commit_transaction(); + REQUIRE(parent_table->size() == 1); + REQUIRE(child_table->size() == 1); + REQUIRE_FALSE(child_table->is_embedded()); + + REQUIRE_THROWS( + realm->update_schema(set_embedded(schema, "child_table", true), 2, [](auto, auto new_realm, auto&) { + Object child_object(new_realm, "child_table", 0); + auto parent_table = ObjectStore::table_for_object_type(new_realm->read_group(), "parent_table"); + Obj parent_obj = parent_table->create_object(); + Object parent_object(new_realm, parent_obj); + CppContext context(new_realm); + parent_object.set_property_value(context, "child_property", util::Any(child_object)); + })); } } diff --git a/test/test_group.cpp b/test/test_group.cpp index 25358141ce5..e022c4f8e4a 100644 --- a/test/test_group.cpp +++ b/test/test_group.cpp @@ -1618,21 +1618,24 @@ TEST(Group_ChangeEmbeddedness) p2.set(col, obj2.get_key()); // obj2 has no owner, so we can't make the table embedded - CHECK_NOT(t->set_embedded(true)); + std::string message; + CHECK_THROW_ANY_GET_MESSAGE(t->set_embedded(true), message); + CHECK_EQUAL(message, "At least one object does not have a backlink (data would get lost)."); CHECK_NOT(t->is_embedded()); // Now it has owner p3.set(col, obj3.get_key()); - CHECK(t->set_embedded(true)); + CHECK_NOTHROW(t->set_embedded(true)); CHECK(t->is_embedded()); - CHECK(t->set_embedded(false)); + CHECK_NOTHROW(t->set_embedded(false)); p3.set(col, obj2.get_key()); obj3.remove(); // Now obj2 has 2 parents CHECK_EQUAL(obj2.get_backlink_count(), 2); - CHECK_NOT(t->set_embedded(true)); + CHECK_THROW_ANY_GET_MESSAGE(t->set_embedded(true), message); + CHECK_EQUAL(message, "At least one object does have multiple backlinks."); CHECK_NOT(t->is_embedded()); }