Schema migrations: Remove columns before removing the tables (#7607)

* Schema migrations: Remove backlink columns before removing the tables

* Add links to schema in schema migration tests

* Fix tests

* Remove all columns before removing any tables

---------

Co-authored-by: Thomas Goyne <[email protected]>

Author: danieltabacaru

src/realm/group.cpp

@@ -798,27 +798,27 @@ void Group::recycle_table_accessor(Table* to_be_recycled)
     g_table_recycler_1.push_back(to_be_recycled);
 }
 
-void Group::remove_table(StringData name, bool ignore_backlinks)
+void Group::remove_table(StringData name)
 {
     check_attached();
     size_t table_ndx = m_table_names.find_first(name);
     if (table_ndx == not_found)
         throw NoSuchTable();
     auto key = ndx2key(table_ndx);
-    remove_table(table_ndx, key, ignore_backlinks); // Throws
+    remove_table(table_ndx, key); // Throws
 }
 
 
-void Group::remove_table(TableKey key, bool ignore_backlinks)
+void Group::remove_table(TableKey key)
 {
     check_attached();
 
     size_t table_ndx = key2ndx_checked(key);
-    remove_table(table_ndx, key, ignore_backlinks);
+    remove_table(table_ndx, key);
 }
 
 
-void Group::remove_table(size_t table_ndx, TableKey key, bool ignore_backlinks)
+void Group::remove_table(size_t table_ndx, TableKey key)
 {
     if (!m_is_writable)
         throw LogicError(ErrorCodes::ReadOnlyDB, "Database not writable");
@@ -832,17 +832,14 @@ void Group::remove_table(size_t table_ndx, TableKey key, bool ignore_backlinks)
     // tables. Such a behaviour is deemed too obscure, and we shall therefore
     // require that a removed table does not contain foreign origin backlink
     // columns.
-    if (!ignore_backlinks && table->is_cross_table_link_target())
+    if (table->is_cross_table_link_target())
         throw CrossTableLinkTarget(table->get_name());
 
     {
         // We don't want to replicate the individual column removals along the
         // way as they're covered by the table removal
         Table::DisableReplication dr(*table);
-        for (size_t i = table->get_column_count(); i > 0; --i) {
-            ColKey col_key = table->spec_ndx2colkey(i - 1);
-            table->remove_column(col_key);
-        }
+        table->remove_columns();
     }
 
     size_t prior_num_tables = m_tables.size();

src/realm/group.hpp

@@ -321,10 +321,8 @@ class Group : public ArrayParent {
     TableRef get_or_add_table_with_primary_key(StringData name, DataType pk_type, StringData pk_name,
                                                bool nullable = false, Table::Type table_type = Table::Type::TopLevel);
 
-    // Use 'ignore_backlinks' with caution. ignore_backlinks=true will leave things in an invalid state
-    // if the target table (or column) is not removed as well.
-    void remove_table(TableKey key, bool ignore_backlinks = false);
-    void remove_table(StringData name, bool ignore_backlinks = false);
+    void remove_table(TableKey key);
+    void remove_table(StringData name);
 
     void rename_table(TableKey key, StringData new_name, bool require_unique_name = true);
     void rename_table(StringData name, StringData new_name, bool require_unique_name = true);
@@ -633,7 +631,7 @@ class Group : public ArrayParent {
     void attach_shared(ref_type new_top_ref, size_t new_file_size, bool writable, VersionID version);
 
     void create_empty_group();
-    void remove_table(size_t table_ndx, TableKey key, bool ignore_backlinks);
+    void remove_table(size_t table_ndx, TableKey key);
 
     void reset_free_space_tracking();
 

src/realm/sync/noinst/sync_schema_migration.cpp

@@ -116,10 +116,12 @@ void perform_schema_migration(DB& db)
     sync::TempShortCircuitReplication sync_history_guard(repl);
     repl.set_write_validator_factory(nullptr);
 
-    // Delete all tables (and their columns).
-    const bool ignore_backlinks = true;
-    for (const auto& tk : tr->get_table_keys()) {
-        tr->remove_table(tk, ignore_backlinks);
+    // Delete all columns before deleting tables to avoid complications with links
+    for (auto tk : tr->get_table_keys()) {
+        tr->get_table(tk)->remove_columns();
+    }
+    for (auto tk : tr->get_table_keys()) {
+        tr->remove_table(tk);
     }
 
     // Clear sync history, reset the file ident and the server version in the download and upload progress.
@@ -134,4 +136,4 @@ void perform_schema_migration(DB& db)
     tr->commit();
 }
 
-} // namespace realm::_impl::sync_schema_migration
+} // namespace realm::_impl::sync_schema_migration

src/realm/table.cpp

@@ -511,6 +511,14 @@ CollectionType Table::get_collection_type(ColKey col_key) const
     return CollectionType::Dictionary;
 }
 
+void Table::remove_columns()
+{
+    for (size_t i = get_column_count(); i > 0; --i) {
+        ColKey col_key = spec_ndx2colkey(i - 1);
+        remove_column(col_key);
+    }
+}
+
 void Table::remove_column(ColKey col_key)
 {
     check_column(col_key);

src/realm/table.hpp

@@ -204,6 +204,7 @@ class Table {
 
     CollectionType get_collection_type(ColKey col_key) const;
 
+    void remove_columns();
     void remove_column(ColKey col_key);
     void rename_column(ColKey col_key, StringData new_name);
     bool valid_column(ColKey col_key) const noexcept;

test/object-store/sync/flx_schema_migration.cpp

@@ -89,6 +89,7 @@ std::pair<SharedRealm, std::exception_ptr> async_open_realm(const Realm::Config&
 std::vector<ObjectSchema> get_schema_v0()
 {
     return {
+        {"Embedded", ObjectSchema::ObjectType::Embedded, {{"str_field", PropertyType::String}}},
         {"TopLevel",
          {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
           {"queryable_str_field", PropertyType::String | PropertyType::Nullable},
@@ -101,7 +102,10 @@ std::vector<ObjectSchema> get_schema_v0()
           {"queryable_int_field", PropertyType::Int | PropertyType::Nullable},
           {"non_queryable_field", PropertyType::String | PropertyType::Nullable}}},
         {"TopLevel3",
-         {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}}, {"queryable_int_field", PropertyType::Int}}},
+         {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
+          {"queryable_int_field", PropertyType::Int},
+          {"link", PropertyType::Object | PropertyType::Nullable, "TopLevel"},
+          {"embedded_link", PropertyType::Object | PropertyType::Nullable, "Embedded"}}},
     };
 }
 
@@ -135,12 +139,16 @@ auto get_subscription_initializer_callback_for_schema_v0()
 std::vector<ObjectSchema> get_schema_v1()
 {
     return {
+        {"Embedded", ObjectSchema::ObjectType::Embedded, {{"str_field", PropertyType::String}}},
         {"TopLevel",
          {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
           {"queryable_int_field", PropertyType::Int | PropertyType::Nullable},
           {"non_queryable_field", PropertyType::String},
           {"non_queryable_field2", PropertyType::String | PropertyType::Nullable}}},
-        {"TopLevel3", {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}}}},
+        {"TopLevel3",
+         {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
+          {"link", PropertyType::Object | PropertyType::Nullable, "TopLevel"},
+          {"embedded_link", PropertyType::Object | PropertyType::Nullable, "Embedded"}}},
     };
 }
 
@@ -165,12 +173,16 @@ auto get_subscription_initializer_callback_for_schema_v1()
 std::vector<ObjectSchema> get_schema_v2()
 {
     return {
+        {"Embedded", ObjectSchema::ObjectType::Embedded, {{"str_field", PropertyType::String}}},
         {"TopLevel",
          {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
           {"queryable_int_field", PropertyType::Int},
           {"non_queryable_field", PropertyType::String},
           {"non_queryable_field2", PropertyType::String | PropertyType::Nullable}}},
-        {"TopLevel3", {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}}}},
+        {"TopLevel3",
+         {{"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
+          {"link", PropertyType::Object | PropertyType::Nullable, "TopLevel"},
+          {"embedded_link", PropertyType::Object | PropertyType::Nullable, "Embedded"}}},
     };
 }
 
@@ -262,7 +274,7 @@ TEST_CASE("Sync schema migrations don't work with sync open", "[sync][flx][flx s
 
     SECTION("Breaking change detected by client") {
         // Make field 'non_queryable_field2' of table 'TopLevel' optional.
-        schema_v1[0].persisted_properties.back() = {"non_queryable_field2",
+        schema_v1[1].persisted_properties.back() = {"non_queryable_field2",
                                                     PropertyType::String | PropertyType::Nullable};
         config.schema = schema_v1;
         create_schema(app_session, *config.schema, config.schema_version);
@@ -272,8 +284,8 @@ TEST_CASE("Sync schema migrations don't work with sync open", "[sync][flx][flx s
     }
 
     SECTION("Breaking change detected by server") {
-        // Remove table 'TopLevel3'.
-        schema_v1.pop_back();
+        // Remove table 'TopLevel2'.
+        schema_v1.erase(schema_v1.begin() + 2);
         config.schema = schema_v1;
         create_schema(app_session, *config.schema, config.schema_version);
 
@@ -294,8 +306,8 @@ TEST_CASE("Sync schema migrations don't work with sync open", "[sync][flx][flx s
         wait_for_download(*realm);
         wait_for_upload(*realm);
 
-        auto table = realm->read_group().get_table("class_TopLevel3");
-        // Migration did not succeed because table 'TopLevel3' still exists (but there is no error).
+        auto table = realm->read_group().get_table("class_TopLevel2");
+        // Migration did not succeed because table 'TopLevel2' still exists (but there is no error).
         CHECK(table);
         check_realm_schema(config.path, schema_v0, 1);
     }