Allow frozen Realms to be opened with additive schema changes

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
 * None.

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;

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;

src/realm/object-store/shared_realm.cpp

@@ -406,9 +406,12 @@ 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. Adding new tables is allowed as those can be backed
+    // by empty Results, breaking changes are not allowed.
     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;
     }

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

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

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]") {