Skip to content
This repository has been archived by the owner on Nov 8, 2021. It is now read-only.

Add ReadOnlyAlternative SchemaMode #511

Merged
merged 6 commits into from
Aug 21, 2017
Merged

Conversation

beeender
Copy link
Contributor

@beeender beeender commented Aug 9, 2017

realm-java has a different semantics of read-only mode. In realm-java,
there is no limitation that the read-only Realm won't be changed by
sync or by the other process. realm-java only has a restriction that
user cannot apply write transactions on the read-only Realm instance.

realm-java has a different semantics of read-only mode. In realm-java,
there is no limitation that the read-only Realm won't be changed by
sync or by the other process. realm-java only has a restriction that
user cannot apply write transactions on the read-only Realm instance.
@beeender beeender requested review from tgoyne, bdash and cmelchior and removed request for tgoyne August 9, 2017 09:41
// mode, sync Realm can be opened with ReadOnlyAlternative mode. Changes
// can be made to the Realm file through another writable Realm instance.
// Thus, notifications are also allowed in this mode.
ReadOnlyAlternative,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name feels slightly awkward, would it make sense to talk about ReadOnlyStrict instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent an hour to come with this name ... actually i feel the original ReadOnly is more strict than this one since it doesn't even allow advance_read. I am open for other better names :P

@tgoyne
Copy link
Member

tgoyne commented Aug 9, 2017

We've previously called this things like "soft read-only" since it opens the file in read-write mode and just doesn't allow making changes, but that name is awkward too. We probably should just rename the existing read-only mode to something like Immutable, since the current name seems to consistently mislead people.

And ReadOnlyAlternative has been renamed to ReadOnly.
@bdash
Copy link
Contributor

bdash commented Aug 10, 2017

The problem with renaming ReadOnly to Immutable and adding a new ReadOnly option with different semantics is that existing users of the read-only mode will see their behavior change without any indication. I'd suggest leaving the rename of ReadOnly to Immutable in place, but giving the new mode a different name for now so that we can leverage the resulting compiler errors to update all existing uses of the existing ReadOnly mode to the new name. We can then rename the new mode to ReadOnly after that's been dealt with.

@beeender
Copy link
Contributor Author

@bdash Is ReadOnlyAlternative as the new mode name OK for you?

@bdash
Copy link
Contributor

bdash commented Aug 10, 2017

NewReadOnly or ReadOnlyAlternative, or anything like that. The intent is for it to be temporary, so it doesn't matter too much what it's called so long as it's clear to people that if they want to preserve the semantics they had with ReadOnly they should switch to Immutable and not the new mode.

to avoid bindings mis-choose a wrong schema mode without any compile
errors.
@@ -188,7 +192,7 @@ class Realm : public std::enable_shared_from_this<Realm> {
ShouldCompactOnLaunchFunction should_compact_on_launch_function;

bool immutable() const { return schema_mode == SchemaMode::Immutable; }
bool read_only() const { return schema_mode == SchemaMode::ReadOnly; }
bool read_only() const { return schema_mode == SchemaMode::ReadOnlyAlternative; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a similar issue with this being named read_only(), since any existing calls to this really should be changed to immutable() to preserve the existing semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, right! forgot this one!

struct Verifier : SchemaDifferenceExplainer {
using SchemaDifferenceExplainer::operator();

void operator()(RemoveProperty) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that AddTable / AddInitialProperties are not handled here like they are for immutable mode? If I understand correctly, that will result in an error if the local schema contains any classes that aren't present in the file being opened. That would appear to preclude opening a synced Realm that doesn't already exist locally using this new read-only mode, since we'll think the schema isn't compatible until after the Realm has been downloaded.

With the immutable schema mode, missing tables are simply treated as if they were empty. That seems like it should also work for this mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is... That is the current realm-java's behavior. I have a discussion with @cmelchior before, we think it is better to keep this since it makes less sense to return an empty results when user query on a non-existing table than just throw earlier.

That would appear to preclude opening a synced Realm that doesn't already exist locally using this new read-only mode, since we'll think the schema isn't compatible until after the Realm has been downloaded.

This is handled in another way in java, open a dynamic realm, download changes, then open the typed realm. See https://github.com/realm/realm-java/blob/master/realm/realm-library/src/main/java/io/realm/RealmCache.java#L310

How does cocoa handle this? If we can unify this implementation in object store, it would be great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is... That is the current realm-java's behavior. I have a discussion with @cmelchior before, we think it is better to keep this since it makes less sense to return an empty results when user query on a non-existing table than just throw earlier.

What's the argument for this behaving differently than the immutable mode? It's not clear to me why this mode should throw if the app's schema contains classes not present in the Realm when immutable mode doesn't.

This is handled in another way in java, open a dynamic realm, download changes, then open the typed realm.

Sure, that's roughly how Cocoa implements its equivalent of "wait for initial remote data" as well. But what about using this schema mode without having to wait for all remote changes to be downloaded? Requiring that the set of classes in the schema match prevents using it for this. See realm/realm-swift#5186 for a scenario where this would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the argument for this behaving differently than the immutable mode? It's not clear to me why this mode should throw if the app's schema contains classes not present in the Realm when immutable mode doesn't.

No argument for that actually. It is simply because of java wants to keep current behaviour to throw earlier.

Sure, that's roughly how Cocoa implements its equivalent of "wait for initial remote data" as well. But what about using this schema mode without having to wait for all remote changes to be downloaded? Requiring that the set of classes in the schema match prevents using it for this. See realm/realm-swift#5186 for a scenario where this would be useful.

Yeah, that is the valid case that the schema validation should not throw. @cmelchior Any comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what is the conclusion to solve this? Shall we have two modes ReadonlyAlternative and ReadonlyStrict where the ReadonlyStrict will throw if the schema doesn't match?

@cmelchior
Copy link
Contributor

What's the argument for this behaving differently than the immutable mode? It's not clear to me why this mode should throw if the app's schema contains classes not present in the Realm when immutable mode doesn't.

The reason is two-fold:

  1. In Java you can, and should, define the explicit set of Classes in a Realm if you want to be resistant to new classes being added after the Realm was created. We call it modules: https://realm.io/docs/java/latest/#sharing-schemas

  2. readOnly() only works in combination with waitForInitialRemoteData() on Java. I'm not convinced that Cannot open Read Only Synced Realms without internet connection realm-swift#5186 is a reasonable behavior, to be honest. Any UI that depends on asyncOpen need to operate from the assumption that they don't know when it returns, so the distinction between never returning and returning an empty Realm should not matter in terms of setting up the UI.

@cmelchior
Copy link
Contributor

Note that I'm taking from a Java context, so if we lifted the restriction on readOnly() working in combination with waitForInitialRemoteData(), then returning the empty Realm immediately might be fine.

Except that you would then run into problems if the Realm downloaded from the server didn't match the schema. Then the Realm would already be open and you would have nowhere sensible to throw (except perhaps the Session ErrorHandler, which would also be a bit weird).

@nirinchev
Copy link
Member

nirinchev commented Aug 11, 2017

readOnly() only works in combination with waitForInitialRemoteData() on Java. I'm not convinced that realm/realm-swift#5186 is a reasonable behavior, to be honest. Any UI that depends on asyncOpen need to operate from the assumption that they don't know when it returns, so the distinction between never returning and returning an empty Realm should not matter in terms of setting up the UI.

I don’t agree. I don’t think it’s realistic to have an app where UI may never appear. I agree that currently showing an indeterminate spinner is less than ideal, but eventually we’ll provide a callback with downloaded/downloadable bytes to allow people to show proper loading bars.

Note that asyncOpen is not necessarily called when downloading the realm for the first time - it’s perfectly reasonable for a user to have downloaded the majority (even all) of the content of the Realm on a previous run, so being unable to open it and show the existing content is a major architectural flaw.

@beeender
Copy link
Contributor Author

@bdash @tgoyne updated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants