New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RealmConfiguration + new constructors #929

Merged
merged 37 commits into from Jun 4, 2015

Conversation

Projects
None yet
6 participants
@cmelchior
Contributor

cmelchior commented Mar 6, 2015

This fixes #511
This PR is part of #880

This PR changes how Realms are created. Our current approach with telescoping constructor arguments are not scalable especially as Migrations introduce new arguments and we have in-memory/read-only realms coming as well. It also allows us to move the Android Context away from Realm.java.

API Overview

This PR replaces the current constructors with a RealmConfiguration that are constructed using the Builder pattern:

// Creating a configuration, using a Builder. This makes it very scalable without having to add new constructors in Realm.java

// Full set of options are in this PR are:
RealmConfiguration config = new RealmConfiguration.Builder(getContext())
                        .name("default.realm")
                        .encryptionKey(getKey())
                        .schemaVersion(42)
                        .migration(new MyMigration())
                        .addModule(new MyModule())  // Add 1 module to already set modules
                        .setModules(new MyModule(),  new MyLibraryModule()) // Replace current modules
                        .deleteRealmIfMigrationNeeded()
                        .schema(Foo.class, Bar.class) // package protected method, only used for internal testing.
                        .build()

// Constructors are:
RealmConfiguration.Builder builder = new RealmConfiguration.Builder(Context ctx);
RealmConfiguration.Builder builder = new RealmConfiguration.Builder(File realmDir);

I don't think a directory is needed for in-memory Realms, but requiring a folder in the constructor makes it a lot less error prone.

// Introduce the concept of a default Realm (This is also the same on Cocoa where you can configure a default Realm)
Realm.setDefaultConfiguration(config); // A default configuration must be set first
Realm.getDefaultInstance(); // Making it even easier to get a default instance
Realm.getInstance(config); // Getting the instance for any given configuration

// These two are similar
Realm.getInstance(getContext())
Realm.getInstance(new RealmConfiguration.Builder(getContext()).build())

// Deprecated methods
Realm.getInstance(Context context, String fileName)
Realm.getInstance(Context context, byte[] key)
Realm.getInstance(Context context, String fileName, byte[] key)
Realm.getInstance(File writeableFolder)
Realm.getInstance(File writeableFolder, String fileName)
Realm.getInstance(File writeableFolder, byte[] key)
Realm.getInstance(File writeableFolder, String fileName, byte[] key)
Realm.deleteRealmFile(Context context, String fileName)
Realm.deleteRealmFile(Context context)
Realm.migrateRealmAtPath(String realmPath, RealmMigration migration)
Realm.migrateRealmAtPath(String realmPath, byte[] key, RealmMigration migration)
Realm.migrateRealmAtPath(String realmPath, RealmMigration migration, boolean autoRefresh)
Realm.migrateRealmAtPath(String realmPath, byte[] key, RealmMigration migration, boolean autoUpdate)
Realm.compactRealmFile(Context context, String fileName)
Realm.compactRealmFile(Context context)

// Note that the following constructor is kept to make it easier for Realm beginners? Is this just API bloat?
Realm.getInstance(Context context)

// New methods replacing them
Realm.getDefaultInstance();
Realm.getInstance(RealmConfiguration realmConfig)
Realm.deleteRealm(RealmConfiguration realmConfig);
Realm.migrateRealm(RealmConfiguration realmConfig);
Realm.migrateRealm(RealmConfiguration realmConfig, RealmMigration migration);
Realm.compactRealm(RealmConfiguration realmConfig);

SchemaVersion and Migration

The current interaction between using .schemaVersion() and .migration() is the following:

  1. If only schemaVersion is set we make a no-op migration behind the scenes that try to upgrade the version number. Schema is validated afterwards and throw an exception if there is mismatch.

  2. If newSchemaVersion > currentSchemaVersion and a migration block is set. This migration will be called when first opening the Realm. We also have a method on the Realm for manually triggering this migration. Useful if you want to upgrade on a background thread: Realm.migrateRealm(RealmConfiguration)

  3. Setting a migration block without a schemaVersion is a no-op, ie. migration code is never called. We might want to log a warning or throw an exception for this.

  4. Setting a newSchemaVersion < currentSchemaVersion will throw an exception when opening the Realm.

  5. Setting newSchemaVersion == currentSchemaVersion and a schema mismatch is detected will throw an exception when opening the Realm.

  6. The current RealmMigration interface will be kept until we introduce the new Migration API as it will introduce breaking changes.

Discussion

Some implementation details that could be discussed:

  • A RealmConfiguration is immutable once created.
  • How to handle multiple configurations pointing to the same Realm. Currently it is allowed (eg. needed for ReadOnly realms).

This PR will also potentially deprecate a lot of methods (all constructors + all migrateRealmAtPath() + deleteRealmFile) , so I would rather introduce it all at once, and then having the option of extending RealmConfiguration afterwards.

Missing:

  • Determine policy for when migrations are needed and interaction with schemaVersion. Check Cocoa: realm/realm-cocoa#1584
  • Determine policy for multiple configurations pointing to the same Realm file. See #1156
  • Update examples (separate PR)
  • Add example for using library projects with Realm (separate PR)
  • Update website/documentation (separate PR)

@emanuelez

This comment has been minimized.

Show comment
Hide comment
@emanuelez

emanuelez Mar 6, 2015

Contributor

I like the direction, but we're breaking the API drastically. I think we should maintain the old constructors while deprecating them until the following big release.

Contributor

emanuelez commented Mar 6, 2015

I like the direction, but we're breaking the API drastically. I think we should maintain the old constructors while deprecating them until the following big release.

@cmelchior

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior Mar 6, 2015

Contributor

Yeah that was my plan as well, this commit might have cleaned up a bit to drastically

Contributor

cmelchior commented Mar 6, 2015

Yeah that was my plan as well, this commit might have cleaned up a bit to drastically

@timanglade

This comment has been minimized.

Show comment
Hide comment
@timanglade

timanglade Mar 8, 2015

Contributor

@alazier Would love to get your thoughts on this. See also #java on slack for additional conversation about this PR.

Contributor

timanglade commented Mar 8, 2015

@alazier Would love to get your thoughts on this. See also #java on slack for additional conversation about this PR.

@emanuelez

This comment has been minimized.

Show comment
Hide comment
@emanuelez

emanuelez Mar 9, 2015

Contributor

Also, we might want to keep static constructor for the most common case:

  • Context or File
  • Context of File + Migration + Version
Contributor

emanuelez commented Mar 9, 2015

Also, we might want to keep static constructor for the most common case:

  • Context or File
  • Context of File + Migration + Version
@alazier

This comment has been minimized.

Show comment
Hide comment
@alazier

alazier Mar 9, 2015

Contributor

@timanglade this is something I have proposed for the cocoa apis multiple times (and think I have mentioned it to @cmelchior). As the number of global configuration parameters grows this will likely be a necessity.

My concerns with this proposal would be:

  • version should be schemaVersion to remove any ambiguity
  • In cocoa we will also need parameters for readOnly, inMemoryIdentifier - not sure if you support these in java
  • We might also want to consider support notifications so that all realm configuration can be done in one place. We would still want register/unregister methods though for users who want to change notifications after realm initialization
  • Is name the same thing as path? Do we just use different terminologies on java vs cocoa?
Contributor

alazier commented Mar 9, 2015

@timanglade this is something I have proposed for the cocoa apis multiple times (and think I have mentioned it to @cmelchior). As the number of global configuration parameters grows this will likely be a necessity.

My concerns with this proposal would be:

  • version should be schemaVersion to remove any ambiguity
  • In cocoa we will also need parameters for readOnly, inMemoryIdentifier - not sure if you support these in java
  • We might also want to consider support notifications so that all realm configuration can be done in one place. We would still want register/unregister methods though for users who want to change notifications after realm initialization
  • Is name the same thing as path? Do we just use different terminologies on java vs cocoa?
@cmelchior

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior Mar 9, 2015

Contributor

@alazier
We have actually been talking about trying to move away from SQL terminology on the Android side, ie.
version instead of schemaVersion, Modules/ClassCollection instead of schema etc. to avoid people associating Realm with SQL too much. However it has not been decided yet and should probably be agreeded on for both platforms. What is your stance on that?

We don't support ReadOnly and InMemory yet, but it is coming.

I don't think the configuration is a good place for adding notification setup as they are added/removed a lot more while the Realm is used. I would prefer to keep the setup methods to initialisation properties.

Name is filename, on android path is split into 2 methods, one for folder and one for filename as getting a default document folder is a bit more complicated.

Contributor

cmelchior commented Mar 9, 2015

@alazier
We have actually been talking about trying to move away from SQL terminology on the Android side, ie.
version instead of schemaVersion, Modules/ClassCollection instead of schema etc. to avoid people associating Realm with SQL too much. However it has not been decided yet and should probably be agreeded on for both platforms. What is your stance on that?

We don't support ReadOnly and InMemory yet, but it is coming.

I don't think the configuration is a good place for adding notification setup as they are added/removed a lot more while the Realm is used. I would prefer to keep the setup methods to initialisation properties.

Name is filename, on android path is split into 2 methods, one for folder and one for filename as getting a default document folder is a bit more complicated.

@cmelchior cmelchior added the PR label Mar 20, 2015

@cmelchior

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior May 6, 2015

Contributor

I have updated the top note with current API design. Feedback welcome. We still miss support for custom schemas but that will be included ons #938 has been merged.

@segiddins @bmunkholm @emanuelez @kneth

Contributor

cmelchior commented May 6, 2015

I have updated the top note with current API design. Feedback welcome. We still miss support for custom schemas but that will be included ons #938 has been merged.

@segiddins @bmunkholm @emanuelez @kneth

@kneth

This comment has been minimized.

Show comment
Hide comment
@kneth

kneth May 6, 2015

Contributor

Will you add an option to compact the Realm file before opening?

Contributor

kneth commented May 6, 2015

Will you add an option to compact the Realm file before opening?

@cmelchior

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior May 6, 2015

Contributor

I didn't consider that. It would be fairly easy adding an option like:

config.compactRealmWhenClosing() or config.compactRealmWhenOpening()

However I am a bit worried it would introduce a unpredictable slowdown as compaction could potentially take a lot of time + plus it require additional space on the disc ?

Contributor

cmelchior commented May 6, 2015

I didn't consider that. It would be fairly easy adding an option like:

config.compactRealmWhenClosing() or config.compactRealmWhenOpening()

However I am a bit worried it would introduce a unpredictable slowdown as compaction could potentially take a lot of time + plus it require additional space on the disc ?

@kneth

This comment has been minimized.

Show comment
Hide comment
@kneth

kneth May 6, 2015

Contributor

Realm Core might be able to provide information about how much space is saved before actually calling compact.

Contributor

kneth commented May 6, 2015

Realm Core might be able to provide information about how much space is saved before actually calling compact.

private final File realmFolder;
private final String realmFileName;
private final String canonicalPath;
private final byte[] key;

This comment has been minimized.

@kneth

kneth May 29, 2015

Contributor

Are there any security concerns when we keep an encryption key in memory?

@kneth

kneth May 29, 2015

Contributor

Are there any security concerns when we keep an encryption key in memory?

This comment has been minimized.

@cmelchior

cmelchior May 29, 2015

Contributor

Not really. Core does the same thing. It is an attack vector, but the current policy is that we only want to guard against attacks on the filesystem.

@cmelchior

cmelchior May 29, 2015

Contributor

Not really. Core does the same thing. It is an attack vector, but the current policy is that we only want to guard against attacks on the filesystem.

@kneth

This comment has been minimized.

Show comment
Hide comment
@kneth

kneth May 29, 2015

Contributor

I believe it is ready to be merged (when my few last comments are addressed).

Contributor

kneth commented May 29, 2015

I believe it is ready to be merged (when my few last comments are addressed).

@emanuelez

This comment has been minimized.

Show comment
Hide comment
@emanuelez

emanuelez Jun 2, 2015

Contributor

Please make this merge-able and address @kneth 's comments. I think this is ready for prime time.

Contributor

emanuelez commented Jun 2, 2015

Please make this merge-able and address @kneth 's comments. I think this is ready for prime time.

cmelchior added some commits Jun 2, 2015

Merge branch 'master' into cm-configuration-builder
Conflicts:
	changelog.txt
	realm/src/main/java/io/realm/Realm.java
	realm/src/main/java/io/realm/internal/Util.java
@cmelchior

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior Jun 2, 2015

Contributor

Any final comments @kneth @emanuelez @nhachicha @bmunkholm before I merge this?

Contributor

cmelchior commented Jun 2, 2015

Any final comments @kneth @emanuelez @nhachicha @bmunkholm before I merge this?

@cmelchior

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior Jun 3, 2015

Contributor

RealmConfiguration.addModule() removed and Realm.getDefaultModule() introduced instead.

@emanuelez @nhachicha @kneth

Contributor

cmelchior commented Jun 3, 2015

RealmConfiguration.addModule() removed and Realm.getDefaultModule() introduced instead.

@emanuelez @nhachicha @kneth

@emanuelez

This comment has been minimized.

Show comment
Hide comment
@emanuelez

emanuelez Jun 3, 2015

Contributor

Long lines. Let's stick to 120

Long lines. Let's stick to 120

This comment has been minimized.

Show comment
Hide comment
@cmelchior

cmelchior Jun 3, 2015

Contributor

Its a generated class?

Contributor

cmelchior replied Jun 3, 2015

Its a generated class?

This comment has been minimized.

Show comment
Hide comment
@emanuelez

emanuelez Jun 3, 2015

Contributor

Yeah ok... that's alright :)

Contributor

emanuelez replied Jun 3, 2015

Yeah ok... that's alright :)

@emanuelez

This comment has been minimized.

Show comment
Hide comment
@emanuelez

emanuelez Jun 4, 2015

Contributor

:shipit:

Contributor

emanuelez commented Jun 4, 2015

:shipit:

cmelchior added a commit that referenced this pull request Jun 4, 2015

Merge pull request #929 from realm/cm-configuration-builder
RealmConfiguration + new constructors

@cmelchior cmelchior merged commit 4160757 into master Jun 4, 2015

1 check passed

default Build finished.
Details

@cmelchior cmelchior removed the PR label Jun 4, 2015

@cmelchior cmelchior deleted the cm-configuration-builder branch Jun 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment