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

Scheme version/migration configuration clarifications #6821

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rorbech
Copy link
Contributor

@rorbech rorbech commented Apr 30, 2020

Scheme version/migration configuration clarifications:

Added additional version/migration configuration test to clarify existing semantics:

  • RealmMigration will never be called unless specifying a schemeVersion too
  • Increasing the schemeVersion will trigger a RealmMigration if present or will otherwise silently bump the version.

Proposing deprecation of old schemeVersion(long) and migration(RealmMigration) in favour of schemeVersion(long, RealmMigration) to highlight semantically connection.

@rorbech rorbech requested a review from cmelchior April 30, 2020 15:38
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally this would be correct, but since is this being merged into a branch that will be released as a major (breaking) version. It is fine to just remove this outright. But we do need to add a line about in the CHANGELOG.md Breaking Changes section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

/**
* Update the schema version of the Realm and apply the provided migration.
* <p>
* The schema version must be equal to or higher than the schema version of the existing
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the version is less?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conventional IllegalArgumentException with message like Provided schema version 1 is less than last set version 3. I will update the docs.

Don't know if it would make sense to differentiate errors a bit. Also if changing scheme without bumping version number the migration is not triggered and we just emit the usual RealmMigrationNeededException.

* @see #schemaVersion(long)
* @see #migration(RealmMigration)
*/
public Builder schemaVersion(long schemaVersion, RealmMigration migration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider alternatives to this name? Would just schema(long schemaVersion, RealmMigration migration) be more accurate? I'm not sure. Maybe it makes sense making "schemaVersion" the primary concept and the migration just a result of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also. Since we are in the process of making breaking changes. Would it make sense to prepare the API for #2530 ?

Should we already now rename RealmMigration to perhaps RealmManualMigration so in the future we can add ReamAutomaticMigration (just throwing ideas at the wall, we could also postpone and decide later. E.g. we can introduce both of these interfaces in a new update and just let RealmMigration be an alias for RealmManualMigration until the next major release).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the migration would never be fully automated for all cases, I guess a RealmAutomaticMigration would also need some customization points, so maybe better off separating the automatism into a helper and allowing people to take advantage of this in other scenarios. Thus, keeping the RealmMigration interface for customization but supplying a DefaultRealmMigration implementation for simple cases where the automatic migration helper is assumed to handle everything (simple additions/deletions, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I need to see some code to understand that 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you consider alternatives to this name? Would just schema(long schemaVersion, RealmMigration migration) be more accurate? I'm not sure. Maybe it makes sense making "schemaVersion" the primary concept and the migration just a result of that.

Shorter names are good, but maybe some keeping it to align with other platforms. If not bound by that I would go for version but maybe that is too overloaded?

Copy link
Contributor

Choose a reason for hiding this comment

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

JS has separate: schema, schemaVersion and migration blocks
Cocao has: schemaVersion and migrationBlock
.NET has SchemaVersion and MigrationCallback

That probably hints to us to keep schemaVersion(version, migration). Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Lets just keep it, to avoid too may different names.

Base automatically changed from v10 to master October 13, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants