From 5d4a09250644c4a39111377f39a82b901b95829a Mon Sep 17 00:00:00 2001 From: James Stone Date: Thu, 8 Jun 2023 17:47:54 -0700 Subject: [PATCH] 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 --------- Co-authored-by: Thomas Goyne --- CHANGELOG.md | 4 +- src/realm/object-store/schema.cpp | 12 ++++ src/realm/object-store/schema.hpp | 2 + src/realm/object-store/shared_realm.cpp | 8 ++- src/realm/object-store/sync/sync_session.cpp | 17 ++--- test/object-store/realm.cpp | 52 +++++++++++++-- test/object-store/sync/flx_sync.cpp | 68 +++++++++++++++++++- 7 files changed, 141 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index db76b9e2186..447e928f9b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,11 @@ ### Enhancements * (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 * ([#????](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 index 00e54d9f163..bfded90ec00 100644 --- 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(a) == static_cast(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 index db28751474a..9998347090a 100644 --- 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 #include #include @@ -200,6 +201,7 @@ class Schema : private std::vector { { 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 index 1d6974cbb6c..fa180f41b50 100644 --- 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 index 6efeb2449f1..9e88c28341b 100644 --- a/src/realm/object-store/sync/sync_session.cpp +++ b/src/realm/object-store/sync/sync_session.cpp @@ -876,20 +876,12 @@ static sync::Session::Config::ClientReset make_client_reset_config(RealmConfig s session_config.sync_config = std::make_shared(*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), @@ -897,9 +889,8 @@ static sync::Session::Config::ClientReset make_client_reset_config(RealmConfig s }; } 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 index aa064738403..55f984c9ba2 100644 --- 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 index c0f85b1f9f5..0ed89184bcd 100644 --- 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 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, 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, SyncError err) { + FAIL(err); + }; + // reorder a property such that it does not match the on disk property order + std::vector 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(); + auto shared_promise = std::make_shared(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]") {