Skip to content
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

[RLMRealm] added +migrationRequiredAtPath:encryptionKey:error: #1723

Closed
wants to merge 5 commits into from

Conversation

jpsim
Copy link
Contributor

@jpsim jpsim commented Apr 2, 2015

}
}
for (RLMObjectSchema *objectSchema in [[RLMSchema sharedSchema] objectSchema]) {
RLMObjectSchema *tableSchema = [RLMObjectSchema schemaFromTableForClassName:objectSchema.className realm:realm];
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure you test this works when tableSchema == nil

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't. Should just continue if tableSchema is nil (since creating missing tables doesn't require a migration).

Returns whether or not the realm at the given path can be accessed without performing a migration.

@param realmPath Path to a Realm file.
@param key 64-byte encryption key.
Copy link
Member

Choose a reason for hiding this comment

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

"64-byte encryption key or nil for unencrypted Realms."

It's not optional most of the places where there's a key parameter, so it needs to call out that this one is.

@alazier
Copy link
Contributor

alazier commented Apr 2, 2015

I don't think this is the correct solution for #1692

The user of this issue should still be setting a schema version, and we should provide a mechanism to clear the Realm in case the migration is required and run. With this pr you are encouraging the user manually delete files which is very error prone, and you are also teaching them that ignoring schema version is ok in some cases (which it really isn't).

Instead of this pr we should instead add a mechanism to clear the realm safely, and this user should update the schema version when they make schema changes.

@jpsim
Copy link
Contributor Author

jpsim commented Apr 2, 2015

I like the idea of a safe realm deletion method, which would take care of checking if there are any open db connections, perhaps even closing them, before deleting all the relevant files. I think we need that regardless of what we do.

However, I do think letting users to know if their realm files require a migration will give them much more flexibility to migrate, delete or otherwise make better informed decisions about what to do with their realms at runtime.

I understand that we want to minimize the odds of accidental deletion, which is why I'd rather not add deleteRealmBeforeOpening or deleteRealmIfMigrationNeeded options like the java team is considering. But I do think that this PR requires users to be very deliberate about how their realm file is deleted, without requiring to increase a schema version on every change, which can definitely hinder development.

@segiddins
Copy link
Contributor

I don't think incrementing a schema version is really that much of a burden, it's literally changing one constant somewhere in your app.

@jpsim jpsim mentioned this pull request Apr 2, 2015
@segiddins
Copy link
Contributor

Let's revisit & try and get this merged?

@jpsim jpsim self-assigned this May 5, 2015
@jpsim
Copy link
Contributor Author

jpsim commented May 5, 2015

This has been updated and is ready for review again /cc @segiddins @tgoyne @bdash

@jpsim
Copy link
Contributor Author

jpsim commented May 5, 2015

Closing in favor of #1584 (comment).

@jpsim jpsim closed this May 5, 2015
@jpsim jpsim removed the pr label May 5, 2015
@jpsim jpsim deleted the jp-migration-required branch May 5, 2015 18:04
@natanrolnik
Copy link
Contributor

@jpsim is this method public? I can't find it to check if my realm needs a migration or not.

@jpsim
Copy link
Contributor Author

jpsim commented Jul 1, 2015

@natanrolnik, no this PR was closed without merging. We opted to go with #1584 (comment) instead, although that has yet to be implemented.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
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.

Allow configuring a Realm file to be deleted when a migration is required
5 participants