From fe119438213fb8f4dfcadcaac933b192e9eef2e1 Mon Sep 17 00:00:00 2001 From: James Stone Date: Fri, 2 Jun 2023 12:05:56 -0700 Subject: [PATCH 1/4] allow frozen Realms to be opened with additive schema changes --- CHANGELOG.md | 4 +- src/realm/object-store/schema.cpp | 12 +++++ src/realm/object-store/schema.hpp | 2 + src/realm/object-store/shared_realm.cpp | 2 +- src/realm/object-store/sync/sync_session.cpp | 17 ++---- test/object-store/realm.cpp | 52 ++++++++++++++++-- test/object-store/sync/flx_sync.cpp | 55 +++++++++++++++++++- 7 files changed, 123 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75e03e2c13a..c555a6857a0 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 * None. 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..1e0361f970e 100644 --- a/src/realm/object-store/shared_realm.cpp +++ b/src/realm/object-store/shared_realm.cpp @@ -408,7 +408,7 @@ void Realm::update_schema(Schema schema, uint64_t version, MigrationFunction mig // 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 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 ef2afea00ec..95b1337b64c 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,59 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { sync::make_error_code(sync::ClientError::auto_client_reset_failure)); } } + + SECTION("DiscardLocal: 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)); + 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]") { From cb26716f573771ca8046a6821e22766e1eb78b4d Mon Sep 17 00:00:00 2001 From: James Stone Date: Fri, 2 Jun 2023 15:02:49 -0700 Subject: [PATCH 2/4] lint --- src/realm/object-store/shared_realm.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp index 1e0361f970e..4c4f798b042 100644 --- a/src/realm/object-store/shared_realm.cpp +++ b/src/realm/object-store/shared_realm.cpp @@ -408,7 +408,8 @@ void Realm::update_schema(Schema schema, uint64_t version, MigrationFunction mig // 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 if (m_frozen_version) { - ObjectStore::verify_compatible_for_immutable_and_readonly(actual_schema.compare(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; } From 515c1a17057db7f8eda6449d8029fc47cafc31f9 Mon Sep 17 00:00:00 2001 From: James Stone Date: Mon, 5 Jun 2023 16:07:27 -0700 Subject: [PATCH 3/4] strengthen tests and comments --- src/realm/object-store/shared_realm.cpp | 4 +++- test/object-store/sync/flx_sync.cpp | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp index 4c4f798b042..ad748062295 100644 --- a/src/realm/object-store/shared_realm.cpp +++ b/src/realm/object-store/shared_realm.cpp @@ -406,7 +406,9 @@ 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_compatible_for_immutable_and_readonly( actual_schema.compare(schema, m_config.schema_mode, true)); diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp index 95b1337b64c..61be352a77b 100644 --- a/test/object-store/sync/flx_sync.cpp +++ b/test/object-store/sync/flx_sync.cpp @@ -900,7 +900,7 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { } } - SECTION("DiscardLocal: schema indexes match in before and after states") { + 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 @@ -942,6 +942,19 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { 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(); }; From d5a8995a97635f1e5cbf1ec2f0b8cbea369fe921 Mon Sep 17 00:00:00 2001 From: James Stone Date: Thu, 8 Jun 2023 15:28:14 -0700 Subject: [PATCH 4/4] Update src/realm/object-store/shared_realm.cpp Co-authored-by: Thomas Goyne --- src/realm/object-store/shared_realm.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp index ad748062295..fa180f41b50 100644 --- a/src/realm/object-store/shared_realm.cpp +++ b/src/realm/object-store/shared_realm.cpp @@ -407,8 +407,9 @@ void Realm::update_schema(Schema schema, uint64_t version, MigrationFunction mig // Frozen Realms never modify the schema on disk and we just need to verify // 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. + // 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_compatible_for_immutable_and_readonly( actual_schema.compare(schema, m_config.schema_mode, true));