realm / realm-core

Core database component for the Realm Mobile Database SDKs
https://realm.io
Apache License 2.0
1.02k stars 163 forks source link

Frozen realm uses the wrong schema #4939

Closed papafe closed 3 years ago

papafe commented 3 years ago

When creating a frozen realm (with sync) we are passing an empty schema in config, so the realm gets his schema from disk, and this can be different from the one that was passed by when the realm was created. This causes a crash in .NET when property access is attempted on a frozen object, as we rely on property indexes for that.

This seems to be due to the fact that the schema in the config gets nulled when the schema is opened the first time: https://github.com/realm/realm-core/blob/680cbecf1351e04422eab5073b4dfc812e27d90c/src/realm/object-store/impl/realm_coordinator.cpp#L285-L295

And then, when creating the frozen schema, that is still null: https://github.com/realm/realm-core/blob/680cbecf1351e04422eab5073b4dfc812e27d90c/src/realm/object-store/shared_realm.cpp#L890-L896

It seems this could be fixed by setting the schema properly before creating the frozen realm like this:

SharedRealm Realm::freeze()
{
    auto config = m_config;
    auto version = read_transaction_version();
    config.scheduler = util::Scheduler::make_frozen(version);
    config.schema = m_schema;
    return Realm::get_frozen_realm(std::move(config), version);
}

This fixes the issue reported in .NET, but we are not sure if this is the correct way of resolving it.

jedelbo commented 3 years ago

@papafe I don't understand what you are trying to solve. The schema used must match the schema found on disk. The frozen realm will get the schema cached in RealmCoordinator.

nirinchev commented 3 years ago

The schema as exposed by OS doesn't necessarily match the schema on disk. It can be a subset of the on-disk schema as Sync may add columns and properties that the SDK didn't know about. I have filed a ticket to change this behavior here: https://github.com/realm/realm-core/issues/4584 but that hasn't been picked up.

Right now, we have a bug in that the live Realm's schema as reported by OS is different from the frozen Realm's schema as reported by OS. This is a bug and we should make sure they're identical. If/when we do get to fix https://github.com/realm/realm-core/issues/4584, we should make sure that this applies to both frozen and live Realms, but that is in the future, whereas this bug is affecting customers today.

jedelbo commented 3 years ago

@tgoyne I am going on holiday tomorrow, so can you see if the proposed solution has any downsides?

tgoyne commented 3 years ago

In Cocoa we do auto frozenRealm = _realm->freeze(); frozenRealm->set_schema_subset(_realm->schema());. I don't know offhand if this does something functionally different than setting the schema in the config would. It does rely on the fact that we have our own Realm caching and disable the OS one, as set_schema_subset() isn't thread-safe.

In theory the most optimal thing to do would be to call set_schema() directly to skip the schema validation (the frozen Realm is at the same version as the source Realm, so the schema can never be different).

nirinchev commented 3 years ago

Should this be done by OS or the SDK? I.e. do you see a downside of modifying Realm::freeze to handle this automatically like:

SharedRealm Realm::freeze()
{
    auto config = m_config;
    auto version = read_transaction_version();
    config.scheduler = util::Scheduler::make_frozen(version);
    auto frozen_realm = Realm::get_frozen_realm(std::move(config), version);
    frozen_realm->set_schema(frozen_realm->m_schema, m_schema);
    return frozen_realm;
}

Or am I misunderstanding your suggestion? Also, @jedelbo assigned you the issue as he was going on vacation, but I'm not sure if you want to actually work on it. If you don't have time, we can either try and find someone on the core team to take over or go ahead and submit a PR as the change seems reasonably minor.