-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix #436: ManualStepMigration not working #469
Conversation
@@ -186,6 +192,8 @@ public void run() { | |||
} | |||
} | |||
}); | |||
|
|||
saveStep(db, version, MIGRATION_COMPLETED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix.
Other changes are just refactoring and testing.
} | ||
|
||
@Test | ||
public void invokedWithSteps() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a failing test.
@k-kagurazaka Can you review it, please? |
Oops, some tests fail because of this change. Will fix them soon. |
@@ -170,11 +177,11 @@ public void run() { | |||
} | |||
} | |||
runTasksInTransaction(db, tasks); | |||
saveStep(db, newVersion, MIGRATION_COMPLETED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix.
It means to always update DB version (i.e. db.setVersion
) via saveStep
.
@@ -151,7 +151,7 @@ public void testIdempotenceWithNop() throws Exception { | |||
public void upgradeFull() throws Exception { | |||
migration.upgrade(db, 1, 100); | |||
|
|||
assertThat(migration.fetchDbVersion(db), is(16)); | |||
assertThat(migration.fetchDbVersion(db), is(100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because migration.upgrade(db, 1, 100)
is called the line before, the db version must be 100
. The assertion was my mistake!
Everything is green. |
OMG. You are right! Orma has to use OrmaMIgration by default even if no migration step is defined. |
@k-kagurazaka The actual failing test is 74e6411, which is fixed at c36a3c7. What do you think of it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! LGTM 👍
@k-kagurazaka Thank you for the review. It's a nice day! |
Fix #436
There is a bug that the DB version (user_version) is not updated when the manual step migrations do not call
execSQL
(or wrappers of it).To fix it, always update DB version just like as when migration steps are empty.
As @k-kagurazaka suggested in #469 (comment), there as another problem in #436 ... the default migration engine is
SchemaDiffMigration
.In this PR (esp. at c36a3c7), Orma always uses
OrmaMigraion
to correctly startManualStepMigration
.