Allow frozen Realms to be opened with additive schema changes (#6693)

* allow frozen Realms to be opened with additive schema changes * lint * strengthen tests and comments * Update src/realm/object-store/shared_realm.cpp Co-authored-by: Thomas Goyne <[email protected]> --------- Co-authored-by: Thomas Goyne <[email protected]>
realm · Jun 9, 2023 · 5d4a092 · 5d4a092
1 parent f972ba9
commit 5d4a092
Show file tree

Hide file tree

Showing 7 changed files with 141 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,11 +2,11 @@
 
 ### Enhancements
 * <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
-* None.
+* It is now allowed to open old frozen versions with a schema that contains additional classes, but not additional properties. ([PR 6693](https://github.com/realm/realm-core/issues/6693))
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* None.
+* Properties in the frozen _before_ Realm instance in the client reset callbacks may have had properties reordered which could lead to exceptions if accessed. ([PR 6693](https://github.com/realm/realm-core/issues/6693), since v13.11.0)
 
 ### Breaking changes
 * A new provider called `IdentityProviderAPIKey` replaces both `IdentityProviderUserAPIKey` and `IdentityProviderServerAPIKey` since those two did the same thing. If SDKs wish to keep the old behaviour without requiring users to make code changes, they can adapt both their existing server and user API key providers to use the new core type. ([#5914](https://github.com/realm/realm-core/issues/5914))

diff --git a/src/realm/object-store/schema.cpp b/src/realm/object-store/schema.cpp
@@ -34,6 +34,18 @@ bool operator==(Schema const& a, Schema const& b) noexcept
 {
     return static_cast<Schema::base const&>(a) == static_cast<Schema::base const&>(b);
 }
+
+std::ostream& operator<<(std::ostream& os, const Schema& schema)
+{
+    for (auto& o : schema) {
+        os << o.name << ":\n";
+        for (auto& p : o.persisted_properties) {
+            os << util::format("\t%1<%2>\n", p.name, string_for_property_type(p.type));
+        }
+    }
+    return os;
+}
+
 } // namespace realm
 
 Schema::Schema() noexcept = default;

diff --git a/src/realm/object-store/schema.hpp b/src/realm/object-store/schema.hpp
@@ -19,6 +19,7 @@
 #ifndef REALM_SCHEMA_HPP
 #define REALM_SCHEMA_HPP
 
+#include <ostream>
 #include <string>
 #include <vector>
 
@@ -200,6 +201,7 @@ class Schema : private std::vector<ObjectSchema> {
     {
         return !(a == b);
     }
+    friend std::ostream& operator<<(std::ostream&, const Schema&);
 
     using base::begin;
     using base::const_iterator;

diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp
@@ -406,9 +406,13 @@ void Realm::update_schema(Schema schema, uint64_t version, MigrationFunction mig
     Schema actual_schema = get_full_schema();
 
     // Frozen Realms never modify the schema on disk and we just need to verify
-    // that the requested schema is a subset of what actually exists
+    // that the requested schema is compatible with what actually exists on disk
+    // at that frozen version. Tables are allowed to be missing as those can be
+    // represented by empty Results, but tables which exist must have all of the
+    // requested properties with the correct type.
     if (m_frozen_version) {
-        ObjectStore::verify_valid_external_changes(schema.compare(actual_schema, m_config.schema_mode, true));
+        ObjectStore::verify_compatible_for_immutable_and_readonly(
+            actual_schema.compare(schema, m_config.schema_mode, true));
         set_schema(actual_schema, std::move(schema));
         return;
     }

diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp
@@ -876,30 +876,21 @@ static sync::Session::Config::ClientReset make_client_reset_config(RealmConfig s
 
     session_config.sync_config = std::make_shared<SyncConfig>(*session_config.sync_config); // deep copy
     session_config.scheduler = nullptr;
-    auto make_frozen_config = [](RealmConfig config) {
-        // Opening without using any explicit schema implies that we will use whatever is on disk without making any
-        // schema changes. Otherwise we may hit a schema error if we are in a client reset when opening a Realm that
-        // has additional tables in the schema config than the one on disk. This may happen in an async open.
-        config.schema.reset();
-        return config;
-    };
     if (session_config.sync_config->notify_after_client_reset) {
-        config.notify_after_client_reset = [config = session_config, &make_frozen_config](VersionID previous_version,
-                                                                                          bool did_recover) {
+        config.notify_after_client_reset = [config = session_config](VersionID previous_version, bool did_recover) {
             auto local_coordinator = RealmCoordinator::get_coordinator(config);
             REALM_ASSERT(local_coordinator);
             ThreadSafeReference active_after = local_coordinator->get_unbound_realm();
-            SharedRealm frozen_before = local_coordinator->get_realm(make_frozen_config(config), previous_version);
+            SharedRealm frozen_before = local_coordinator->get_realm(config, previous_version);
             REALM_ASSERT(frozen_before);
             REALM_ASSERT(frozen_before->is_frozen());
             config.sync_config->notify_after_client_reset(std::move(frozen_before), std::move(active_after),
                                                           did_recover);
         };
     }
     if (session_config.sync_config->notify_before_client_reset) {
-        config.notify_before_client_reset = [config = session_config, &make_frozen_config](VersionID version) {
-            config.sync_config->notify_before_client_reset(
-                Realm::get_frozen_realm(make_frozen_config(config), version));
+        config.notify_before_client_reset = [config = session_config](VersionID version) {
+            config.sync_config->notify_before_client_reset(Realm::get_frozen_realm(config, version));
         };
     }
 

diff --git a/test/object-store/realm.cpp b/test/object-store/realm.cpp
@@ -844,11 +844,19 @@ TEST_CASE("SharedRealm: schema_subset_mode") {
     SECTION("obtaining a frozen realm with an incompatible schema throws") {
         config.schema = Schema{{"object", {{"value", PropertyType::Int}}}};
         auto old_realm = Realm::get_shared_realm(config);
+        {
+            auto tr = db->start_write();
+            auto table = tr->get_table("class_object");
+            table->create_object();
+            tr->commit();
+        }
         old_realm->read_group();
 
         {
             auto tr = db->start_write();
-            tr->add_table("class_object 2")->add_column(type_Int, "value");
+            auto table = tr->add_table("class_object 2");
+            ColKey val_col = table->add_column(type_Int, "value");
+            table->create_object().set(val_col, 1);
             tr->commit();
         }
 
@@ -862,10 +870,46 @@ TEST_CASE("SharedRealm: schema_subset_mode") {
         REQUIRE(old_realm->freeze()->schema().size() == 1);
         REQUIRE(new_realm->freeze()->schema().size() == 2);
         REQUIRE(Realm::get_frozen_realm(config, new_realm->read_transaction_version())->schema().size() == 2);
-        // Fails because the requested version doesn't have the "object 2" table
-        // required by the config
+        // An additive change is allowed, the unknown table is empty
+        REQUIRE(Realm::get_frozen_realm(config, old_realm->read_transaction_version())->schema().size() == 2);
+
+        config.schema = Schema{{"object", {{"value", PropertyType::String}}}}; // int -> string
+        // Fails because the schema has an invalid breaking change
+        REQUIRE_THROWS_AS(Realm::get_frozen_realm(config, new_realm->read_transaction_version()),
+                          InvalidReadOnlySchemaChangeException);
+        REQUIRE_THROWS_AS(Realm::get_frozen_realm(config, old_realm->read_transaction_version()),
+                          InvalidReadOnlySchemaChangeException);
+        config.schema = Schema{
+            {"object", {{"value", PropertyType::Int}}},
+            {"object 2", {{"value", PropertyType::String}}}, // int -> string
+        };
+        // fails due to invalid change on object 2 type
+        REQUIRE_THROWS_AS(Realm::get_frozen_realm(config, new_realm->read_transaction_version()),
+                          InvalidReadOnlySchemaChangeException);
+        // opening the old state does not fail because the schema is an additive change
+        auto frozen_old = Realm::get_frozen_realm(config, old_realm->read_transaction_version());
+        REQUIRE(frozen_old->schema().size() == 2);
+        {
+            TableRef table = frozen_old->read_group().get_table("class_object");
+            Results results(frozen_old, table);
+            REQUIRE(results.is_frozen());
+            REQUIRE(results.size() == 1);
+        }
+        {
+            TableRef table = frozen_old->read_group().get_table("class_object 2");
+            REQUIRE(!table);
+            Results results(frozen_old, table);
+            REQUIRE(results.is_frozen());
+            REQUIRE(results.size() == 0);
+        }
+        config.schema = Schema{
+            {"object", {{"value", PropertyType::Int}, {"value 2", PropertyType::String}}}, // add property
+        };
+        // fails due to additional property on object
         REQUIRE_THROWS_AS(Realm::get_frozen_realm(config, old_realm->read_transaction_version()),
-                          InvalidExternalSchemaChangeException);
+                          InvalidReadOnlySchemaChangeException);
+        REQUIRE_THROWS_AS(Realm::get_frozen_realm(config, new_realm->read_transaction_version()),
+                          InvalidReadOnlySchemaChangeException);
     }
 }
 

diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp
@@ -273,7 +273,7 @@ static auto make_client_reset_handler()
 }
 
 TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") {
-    Schema schema{
+    std::vector<ObjectSchema> schema{
         {"TopLevel",
          {
              {"_id", PropertyType::ObjectId, Property::IsPrimary{true}},
@@ -899,6 +899,72 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") {
                   sync::make_error_code(sync::ClientError::auto_client_reset_failure));
         }
     }
+
+    SECTION("Recover: schema indexes match in before and after states") {
+        {
+            config_local.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError) {
+                // ignore spurious failures on this instance
+            };
+            SharedRealm realm = Realm::get_shared_realm(config_local);
+            subscribe_to_and_add_objects(realm, 1);
+            auto subs = realm->get_latest_subscription_set();
+            auto result = subs.get_state_change_notification(sync::SubscriptionSet::State::Complete).get();
+            CHECK(result == sync::SubscriptionSet::State::Complete);
+            reset_utils::trigger_client_reset(harness.session().app_session(), realm);
+            realm->close();
+        }
+        {
+            Realm::Config config_copy = config_local;
+            config_copy.sync_config->error_handler = [](std::shared_ptr<SyncSession>, SyncError err) {
+                FAIL(err);
+            };
+            // reorder a property such that it does not match the on disk property order
+            std::vector<ObjectSchema> local_schema = schema;
+            std::swap(local_schema[0].persisted_properties[1], local_schema[0].persisted_properties[2]);
+            config_copy.schema = local_schema;
+            config_copy.sync_config->client_resync_mode = ClientResyncMode::Recover;
+            auto [promise, future] = util::make_promise_future<void>();
+            auto shared_promise = std::make_shared<decltype(promise)>(std::move(promise));
+            config_copy.sync_config->notify_before_client_reset = [&](SharedRealm frozen_before) {
+                ++before_reset_count;
+                REQUIRE(frozen_before->schema().size() > 0);
+                REQUIRE(frozen_before->schema_version() != ObjectStore::NotVersioned);
+                REQUIRE(frozen_before->schema() == Schema(local_schema));
+            };
+            config_copy.sync_config->notify_after_client_reset =
+                [&, promise = std::move(shared_promise)](SharedRealm frozen_before, ThreadSafeReference after_ref,
+                                                         bool did_recover) {
+                    ++after_reset_count;
+                    REQUIRE(frozen_before->schema().size() > 0);
+                    REQUIRE(frozen_before->schema_version() != ObjectStore::NotVersioned);
+                    REQUIRE(frozen_before->schema() == Schema(local_schema));
+                    SharedRealm after =
+                        Realm::get_shared_realm(std::move(after_ref), util::Scheduler::make_default());
+                    REQUIRE(after);
+                    REQUIRE(after->schema() == Schema(local_schema));
+                    // the above check is sufficent unless operator==() is changed to not care about ordering
+                    // so future proof that by explicitly checking the order of properties here as well
+                    REQUIRE(after->schema().size() == frozen_before->schema().size());
+                    auto after_it = after->schema().find("TopLevel");
+                    auto before_it = frozen_before->schema().find("TopLevel");
+                    REQUIRE(after_it != after->schema().end());
+                    REQUIRE(before_it != frozen_before->schema().end());
+                    REQUIRE(after_it->name == before_it->name);
+                    REQUIRE(after_it->persisted_properties.size() == before_it->persisted_properties.size());
+                    REQUIRE(after_it->persisted_properties[1].name == "queryable_int_field");
+                    REQUIRE(after_it->persisted_properties[2].name == "queryable_str_field");
+                    REQUIRE(before_it->persisted_properties[1].name == "queryable_int_field");
+                    REQUIRE(before_it->persisted_properties[2].name == "queryable_str_field");
+                    REQUIRE(did_recover);
+                    promise->emplace_value();
+                };
+
+            SharedRealm realm = Realm::get_shared_realm(config_copy);
+            future.get();
+            CHECK(before_reset_count == 1);
+            CHECK(after_reset_count == 1);
+        }
+    }
 }
 
 TEST_CASE("flx: creating an object on a class with no subscription throws", "[sync][flx][app]") {