Merge pull request #5897 from realm/tg/migration-tests

Improve the migration tests and some error reporting from invalid changes
realm · Oct 7, 2022 · ea51c9c · ea51c9c
2 parents ef3b138 + bf7acf6
commit ea51c9c
Show file tree

Hide file tree

Showing 30 changed files with 1,527 additions and 1,894 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,14 +2,14 @@
 
 ### Enhancements
 * <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
-* None.
+* Improve performance of client reset with automatic recovery and converting top-level tables into embedded tables.
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
 * None.
 
 ### Breaking changes
-* None.
+* Rename RealmConfig::automatic_handle_backlicks_in_migrations to RealmConfig::automatically_handle_backlinks_in_migrations.
 
 ### Compatibility
 * Fileformat: Generates files with format v22. Reads and automatically upgrade from fileformat v5.
@@ -18,6 +18,7 @@
 
 ### Internals
 * Remove the unused utility function `copy_dir_recursive()`.
+* StringData and Timestamp are now constexpr-constructible.
 
 ----------------------------------------------
 

diff --git a/src/realm/cluster.cpp b/src/realm/cluster.cpp
@@ -972,8 +972,8 @@ void Cluster::nullify_incoming_links(ObjKey key, CascadeState& state)
 {
     size_t ndx = get_ndx(key, 0);
     if (ndx == realm::npos)
-        throw KeyNotFound(util::format("When nullify incoming links for key '%1' in '%2'", key.value,
-                                       get_owning_table()->get_name()));
+        throw KeyNotFound(util::format("Key '%1' not found in '%2' when nullifying incoming links", key.value,
+                                       get_owning_table()->get_class_name()));
 
     // We must start with backlink columns in case the corresponding link
     // columns are in the same table so that we can nullify links before

diff --git a/src/realm/group.cpp b/src/realm/group.cpp
@@ -53,9 +53,6 @@ Initialization initialization;
 
 } // anonymous namespace
 
-constexpr char Group::g_class_name_prefix[];
-constexpr size_t Group::g_class_name_prefix_len;
-
 Group::Group()
     : m_local_alloc(new SlabAlloc)
     , m_alloc(*m_local_alloc) // Throws

diff --git a/src/realm/group.hpp b/src/realm/group.hpp
@@ -283,17 +283,19 @@ class Group : public ArrayParent {
     bool table_is_public(TableKey key) const;
     static StringData table_name_to_class_name(StringData table_name)
     {
-        REALM_ASSERT(table_name.begins_with(StringData(g_class_name_prefix, g_class_name_prefix_len)));
-        return table_name.substr(g_class_name_prefix_len);
+        if (table_name.begins_with(g_class_name_prefix)) {
+            return table_name.substr(g_class_name_prefix.size());
+        }
+        return table_name;
     }
 
     using TableNameBuffer = std::array<char, max_table_name_length>;
     static StringData class_name_to_table_name(StringData class_name, TableNameBuffer& buffer)
     {
-        char* p = std::copy_n(g_class_name_prefix, g_class_name_prefix_len, buffer.data());
-        size_t len = std::min(class_name.size(), buffer.size() - g_class_name_prefix_len);
+        char* p = std::copy_n(g_class_name_prefix.data(), g_class_name_prefix.size(), buffer.data());
+        size_t len = std::min(class_name.size(), buffer.size() - g_class_name_prefix.size());
         std::copy_n(class_name.data(), len, p);
-        return StringData(buffer.data(), g_class_name_prefix_len + len);
+        return StringData(buffer.data(), g_class_name_prefix.size() + len);
     }
 
     TableRef get_table(TableKey key);
@@ -532,8 +534,7 @@ class Group : public ArrayParent {
     }
 
 private:
-    static constexpr char g_class_name_prefix[] = "class_";
-    static constexpr size_t g_class_name_prefix_len = 6;
+    static constexpr StringData g_class_name_prefix = "class_";
 
     struct ToDeleteRef {
         ToDeleteRef(TableKey tk, ObjKey k)
@@ -926,7 +927,7 @@ inline StringData Group::get_table_name(TableKey key) const
 
 inline bool Group::table_is_public(TableKey key) const
 {
-    return get_table_name(key).begins_with(StringData(g_class_name_prefix, g_class_name_prefix_len));
+    return get_table_name(key).begins_with(g_class_name_prefix);
 }
 
 inline bool Group::has_table(StringData name) const noexcept

diff --git a/src/realm/keys.hpp b/src/realm/keys.hpp
@@ -95,10 +95,7 @@ struct ColKey {
         unsigned val;
     };
 
-    constexpr ColKey() noexcept
-        : value(null_value)
-    {
-    }
+    constexpr ColKey() noexcept = default;
     constexpr explicit ColKey(int64_t val) noexcept
         : value(val)
     {
@@ -164,7 +161,7 @@ struct ColKey {
     {
         return (value >> 30) & 0xFFFFFFFFUL;
     }
-    int64_t value;
+    int64_t value = null_value;
 };
 
 static_assert(ColKey::null_value == 0x7fffffffffffffff, "Fix this");
@@ -254,8 +251,8 @@ class ObjKeys : public std::vector<ObjKey> {
 
 struct ObjLink {
 public:
-    ObjLink() {}
-    ObjLink(TableKey table_key, ObjKey obj_key)
+    constexpr ObjLink() = default;
+    constexpr ObjLink(TableKey table_key, ObjKey obj_key)
         : m_obj_key(obj_key)
         , m_table_key(table_key)
     {

diff --git a/src/realm/obj.cpp b/src/realm/obj.cpp
@@ -2070,21 +2070,20 @@ struct EmbeddedObjectLinkMigrator : public LinkTranslator {
 void Obj::handle_multiple_backlinks_during_schema_migration()
 {
     REALM_ASSERT(!m_table->get_primary_key_column());
-    auto copy_links = [this](ColKey col) {
-        auto embedded_obj_tracker = std::make_shared<converters::EmbeddedObjectConverter>();
+    converters::EmbeddedObjectConverter embedded_obj_tracker;
+    auto copy_links = [&](ColKey col) {
         auto opposite_table = m_table->get_opposite_table(col);
         auto opposite_column = m_table->get_opposite_column(col);
         auto backlinks = get_all_backlinks(col);
         for (auto backlink : backlinks) {
             // create a new obj
             auto obj = m_table->create_object();
-            embedded_obj_tracker->track(*this, obj);
+            embedded_obj_tracker.track(*this, obj);
             auto linking_obj = opposite_table->get_object(backlink);
             // change incoming links to point to the newly created object
-            EmbeddedObjectLinkMigrator migrator{linking_obj, opposite_column, *this, obj};
-            migrator.run();
+            EmbeddedObjectLinkMigrator{linking_obj, opposite_column, *this, obj}.run();
         }
-        embedded_obj_tracker->process_pending();
+        embedded_obj_tracker.process_pending();
         return IteratorControl::AdvanceToNext;
     };
     m_table->for_each_backlink_column(copy_links);

diff --git a/src/realm/obj.hpp b/src/realm/obj.hpp
@@ -232,6 +232,10 @@ class Obj {
     // new object and link it. (To Be Implemented)
     Obj clear_linked_object(ColKey col_key);
     Obj& set_any(ColKey col_key, Mixed value, bool is_default = false);
+    Obj& set_any(StringData col_name, Mixed value, bool is_default = false)
+    {
+        return set_any(get_column_key(col_name), value, is_default);
+    }
 
     template <typename U>
     Obj& set(StringData col_name, U value, bool is_default = false)
@@ -310,6 +314,7 @@ class Obj {
     template <typename U>
     SetPtr<U> get_set_ptr(ColKey col_key) const;
     LnkSet get_linkset(ColKey col_key) const;
+    LnkSet get_linkset(StringData col_name) const;
     LnkSetPtr get_linkset_ptr(ColKey col_key) const;
     SetBasePtr get_setbase_ptr(ColKey col_key) const;
     Dictionary get_dictionary(ColKey col_key) const;

diff --git a/src/realm/object-store/c_api/config.cpp b/src/realm/object-store/c_api/config.cpp
@@ -226,5 +226,5 @@ RLM_API bool realm_config_get_cached(realm_config_t* realm_config) noexcept
 RLM_API void realm_config_set_automatic_backlink_handling(realm_config_t* realm_config,
                                                           bool enable_automatic_handling) noexcept
 {
-    realm_config->automatic_handle_backlicks_in_migrations = enable_automatic_handling;
+    realm_config->automatically_handle_backlinks_in_migrations = enable_automatic_handling;
 }
diff --git a/src/realm/object-store/object_schema.cpp b/src/realm/object-store/object_schema.cpp
@@ -448,4 +448,17 @@ bool operator==(ObjectSchema const& a, ObjectSchema const& b) noexcept
     return std::tie(a.name, a.table_type, a.primary_key, a.persisted_properties, a.computed_properties) ==
            std::tie(b.name, b.table_type, b.primary_key, b.persisted_properties, b.computed_properties);
 }
+
+std::ostream& operator<<(std::ostream& o, ObjectSchema::ObjectType table_type)
+{
+    switch (table_type) {
+        case ObjectSchema::ObjectType::TopLevel:
+            return o << "TopLevel";
+        case ObjectSchema::ObjectType::Embedded:
+            return o << "Embedded";
+        case ObjectSchema::ObjectType::TopLevelAsymmetric:
+            return o << "TopLevelAsymmetric";
+    }
+    return o << "Invalid table type: " << uint8_t(table_type);
+}
 } // namespace realm
diff --git a/src/realm/object-store/object_schema.hpp b/src/realm/object-store/object_schema.hpp
@@ -93,18 +93,7 @@ class ObjectSchema {
     void set_primary_key_property() noexcept;
 };
 
-inline std::ostream& operator<<(std::ostream& o, ObjectSchema::ObjectType table_type)
-{
-    switch (table_type) {
-        case ObjectSchema::ObjectType::TopLevel:
-            return o << "TopLevel";
-        case ObjectSchema::ObjectType::Embedded:
-            return o << "Embedded";
-        case ObjectSchema::ObjectType::TopLevelAsymmetric:
-            return o << "TopLevelAsymmetric";
-    }
-    return o << "Invalid table type: " << uint8_t(table_type);
-}
+std::ostream& operator<<(std::ostream& o, ObjectSchema::ObjectType table_type);
 
 } // namespace realm
 

diff --git a/src/realm/object-store/object_store.cpp b/src/realm/object-store/object_store.cpp
@@ -605,7 +605,7 @@ static void create_initial_tables(Group& group, std::vector<SchemaChange> const&
         // downside.
         void operator()(ChangeTableType op)
         {
-            table(op.object).set_table_type(static_cast<Table::Type>(*op.new_table_type));
+            table(op.object).set_table_type(static_cast<Table::Type>(*op.new_table_type), false);
         }
         void operator()(AddProperty op)
         {
@@ -826,51 +826,14 @@ static void apply_post_migration_changes(Group& group, std::vector<SchemaChange>
 
         void operator()(ChangeTableType op)
         {
-            if (handle_backlinks_automatically)
-                post_migration_embedded_objects_backlinks_handling(op.object);
-            table(op.object).set_table_type(static_cast<Table::Type>(*op.new_table_type));
+            table(op.object).set_table_type(static_cast<Table::Type>(*op.new_table_type),
+                                            handle_backlinks_automatically);
         }
         void operator()(RemoveTable) {}
         void operator()(ChangePropertyType) {}
         void operator()(MakePropertyNullable) {}
         void operator()(MakePropertyRequired) {}
         void operator()(AddProperty) {}
-
-        void post_migration_embedded_objects_backlinks_handling(const ObjectSchema* object_schema)
-        {
-            // check if we are doing a migration from TopLevel Object to Embedded.
-            // check back link count.
-            //  1. if object is an orphan (no backlicks, then delete it if instructed to do so)
-            //  2. if object has multiple backlicks, then just clone N times the object (for each backlick) and assign
-            //  to each a different parent
-            // object_schema->handle_automatically_backlinks_for_embedded_object = true;
-
-            if (object_schema->table_type == ObjectSchema::ObjectType::Embedded) {
-                auto original_object_schema = initial_schema.find(object_schema->name);
-                if (original_object_schema != initial_schema.end() &&
-                    (original_object_schema->table_type == ObjectSchema::ObjectType::TopLevel ||
-                     original_object_schema->table_type == ObjectSchema::ObjectType::TopLevelAsymmetric)) {
-                    auto table = table_for_object_schema(group, *original_object_schema);
-                    std::vector<Obj> objects_to_erase;
-                    std::vector<Obj> objects_to_fix_backlinks;
-                    for (auto& object : *table) {
-                        size_t backlink_count = object.get_backlink_count();
-                        if (backlink_count == 0)
-                            objects_to_erase.push_back(object);
-                        else if (backlink_count > 1)
-                            objects_to_fix_backlinks.push_back(object);
-                    }
-
-                    for (auto& object : objects_to_erase)
-                        object.remove();
-
-                    for (auto& object : objects_to_fix_backlinks) {
-                        object.handle_multiple_backlinks_during_schema_migration();
-                        object.remove();
-                    }
-                }
-            }
-        }
     } applier{group, initial_schema, did_reread_schema, handle_backlinks_automatically};
 
     for (auto& change : changes) {
@@ -1065,9 +1028,11 @@ void ObjectStore::rename_property(Group& group, Schema& target_schema, StringDat
     }
 }
 
-InvalidSchemaVersionException::InvalidSchemaVersionException(uint64_t old_version, uint64_t new_version)
-    : logic_error(
-          util::format("Provided schema version %1 is less than last set version %2.", new_version, old_version))
+InvalidSchemaVersionException::InvalidSchemaVersionException(uint64_t old_version, uint64_t new_version,
+                                                             bool must_exactly_equal)
+    : logic_error(util::format(must_exactly_equal ? "Provided schema version %1 does not equal last set version %2."
+                                                  : "Provided schema version %1 is less than last set version %2.",
+                               new_version, old_version))
     , m_old_version(old_version)
     , m_new_version(new_version)
 {

diff --git a/src/realm/object-store/object_store.hpp b/src/realm/object-store/object_store.hpp
@@ -117,7 +117,7 @@ class ObjectStore {
 
 class InvalidSchemaVersionException : public std::logic_error {
 public:
-    InvalidSchemaVersionException(uint64_t old_version, uint64_t new_version);
+    InvalidSchemaVersionException(uint64_t old_version, uint64_t new_version, bool must_exactly_equal);
     uint64_t old_version() const
     {
         return m_old_version;

diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp
@@ -285,12 +285,12 @@ bool Realm::schema_change_needs_write_transaction(Schema& schema, std::vector<Sc
     switch (m_config.schema_mode) {
         case SchemaMode::Automatic:
             if (version < m_schema_version && m_schema_version != ObjectStore::NotVersioned)
-                throw InvalidSchemaVersionException(m_schema_version, version);
+                throw InvalidSchemaVersionException(m_schema_version, version, false);
             return true;
 
         case SchemaMode::Immutable:
             if (version != m_schema_version)
-                throw InvalidSchemaVersionException(m_schema_version, version);
+                throw InvalidSchemaVersionException(m_schema_version, version, true);
             REALM_FALLTHROUGH;
         case SchemaMode::ReadOnly:
             ObjectStore::verify_compatible_for_immutable_and_readonly(changes);
@@ -316,7 +316,7 @@ bool Realm::schema_change_needs_write_transaction(Schema& schema, std::vector<Sc
 
         case SchemaMode::Manual:
             if (version < m_schema_version && m_schema_version != ObjectStore::NotVersioned)
-                throw InvalidSchemaVersionException(m_schema_version, version);
+                throw InvalidSchemaVersionException(m_schema_version, version, false);
             if (version == m_schema_version) {
                 ObjectStore::verify_no_changes_required(changes);
                 REALM_UNREACHABLE(); // changes is non-empty so above line always throws
@@ -464,12 +464,12 @@ void Realm::update_schema(Schema schema, uint64_t version, MigrationFunction mig
         });
 
         ObjectStore::apply_schema_changes(transaction(), version, m_schema, m_schema_version, m_config.schema_mode,
-                                          required_changes, m_config.automatic_handle_backlicks_in_migrations,
+                                          required_changes, m_config.automatically_handle_backlinks_in_migrations,
                                           wrapper);
     }
     else {
         ObjectStore::apply_schema_changes(transaction(), m_schema_version, schema, version, m_config.schema_mode,
-                                          required_changes, m_config.automatic_handle_backlicks_in_migrations);
+                                          required_changes, m_config.automatically_handle_backlinks_in_migrations);
         REALM_ASSERT_DEBUG(additive ||
                            (required_changes = ObjectStore::schema_from_group(read_group()).compare(schema)).empty());
     }

diff --git a/src/realm/object-store/shared_realm.hpp b/src/realm/object-store/shared_realm.hpp
@@ -168,10 +168,12 @@ struct RealmConfig {
     // Disable automatic backup at file format upgrade by setting to false
     bool backup_at_file_format_change = true;
 
-    // delete embedded orphan objects
-    bool automatic_handle_backlicks_in_migrations = false;
+    // By default converting a top-level table to embedded will fail if there
+    // are any objects without exactly one incoming link. Enabling this makes
+    // it instead delete orphans and duplicate objects with multiple incoming links.
+    bool automatically_handle_backlinks_in_migrations = false;
 
-    // Only for internal testing. Not to be used by SDKs.
+    // Only for internal testing. Not to be exposed by SDKs.
     //
     // Disable the background worker thread for producing change
     // notifications. Useful for tests for those notifications so that