Handle unknown migrations #1

ericklaus-wf · 2016-10-24T21:20:06Z

Fixes bad behavior when some migrations in the db table are unknown to the current sql-migrate instance. That might happen when one branch applies some migrations and a branch lacking those migrations tries to add some of its own.

Fixed behaviors:

If most recent migration is unknown, migrate up reports "nothing to do"
- Migrations are now applied
If any migrations are unknown, sql-migrate status panics
- A log warning is now emitted indicating which migrations are unknown

Some weirdness remains:

If the latest migration is unknown sql-migrate redo reports "nothing to do"
I'd rather that sql-migrate status include the fact of unknown statuses in the table it prints, rather than as separate warnings. I want to get some UX feedback on how that might look. Currently, the printout looks like:

Migration in database "20160901151959_def-1.sql" unknown to sql-migrate; it will not be possible to migrate down this migration
Migration in database "20160921154454_def-2.sql" unknown to sql-migrate; it will not be possible to migrate down this migration
+-----------------------------------------------+-------------------------------+
|                   MIGRATION                   |            APPLIED            |
+-----------------------------------------------+-------------------------------+
| 00000000000000_initial.sql         | 1970-01-01 00:00:00 +0000 UTC |
| 20160406182900_abc.sql             | 2016-04-12 20:24:51 +0000 UTC |
| 20160414135200_def.sql               | 2016-10-12 20:54:49 +0000 UTC |
| 20161021194400_hig.sql                | no                            |
+-----------------------------------------------+-------------------------------+

ericklaus-wf · 2016-10-24T21:21:08Z

Perhaps we could add a column to the sql-migrate status table (probably before migration) that includes a status code. Maybe "!" for "unknown" and "a" for "not applied"?

ericklaus-wf · 2016-10-25T14:22:18Z

migrate.go

-			if migrations[index].Id == current {
+// Searches the arrays for the latest elements with common ids.
+// Returns the indexes of the common id.
+func lastCommon(x []*Migration, y []*Migration) (int, int) {


Rename x and y to left and right to avoid unreadable array accesses like x[i] and y[j].

ericklaus-wf · 2016-10-25T14:23:59Z

migrate.go

+		return []*Migration{}
+	}
+	migrationsMap := map[string]*Migration{}
+	for _, m := range existingMigrations {


Add note about the order of setting migrationsMap. It's important that we fill migrationsMap from existingMigrations first, then from migrations. migrations is the only array with Up and Down set, and we want those values if possible.

ericklaus-wf · 2016-10-25T14:25:46Z

migrate.go

+}

+// Filter a slice of migrations into ones that should be applied.
+func ToApply(migrations, existingMigrations []*Migration, direction MigrationDirection) []*Migration {


This is a breaking change to a public method. It doesn't feel like the kind of method that library clients would actually use. However, when PRing upstream, I'll offer to rewrite this as ToApplySafe or something similar and avoid breaking the signature of ToApply().

tomdeering-wf · 2016-10-25T20:55:15Z

migrate.go

 	// Figure out which migrations to apply
-	toApply := ToApply(migrations, record.Id, dir)
+	toApply := ToApply(migrations, existingMigrations, dir)
+	if err != nil {


I'm confused by this err check.

Oops. At one point, I made ToApply() return an error, but changed my mind. I never took out the error check.

tomdeering-wf · 2016-10-25T21:01:36Z

migrate.go

 		}
 	}
+	if xIndexMatch == -1 {
+		return -1, -1 // We never found a match; the arrays have nothing in common


Could use const none = -1 if you wanted

tomdeering-wf · 2016-10-25T21:06:16Z

migrate.go

+}
+
+func toApplyDown(migrations, existingMigrations []*Migration) []*Migration {
+	if len(existingMigrations) == -1 {


Should this be == 0?

tomdeering-wf · 2016-10-25T21:09:53Z

migrate.go

+	}
+	// When a Migration appears in both existingMigrations and migrations,
+	// we want the Migration from migrations, since only that list has
+	// the Up and Down fields set.


Would this comment make more sense if it were on the second loop, which does the overwriting you describe?

tomdeering-wf · 2016-10-25T21:13:38Z

Otherwise looks good. Thanks for taking care of this!

johnlockwood-wf · 2016-10-25T21:21:24Z

migrate.go

+	if len(existingMigrations) == 0 {
+		return migrations
+	}
+	_, index := lastCommon(existingMigrations, migrations)


If I didn't know what bug this was fixing, I'd wonder why it is being done. Could you add some comments about it?

johnlockwood-wf · 2016-10-25T21:29:49Z

Can there be an unknown, unapplied migration? such as when you sql-migrate status, does that add an entry into the migrations table?

johnlockwood-wf · 2016-10-25T21:32:43Z

Going ahead and applying a new migration on top of a database that has an unknown, unapplied migration seems safe, but if the unknown migration was applied, that seems like it could leave the db in a state incompatible with the new migration.

ericklaus-wf · 2016-10-26T13:49:29Z

@johnlockwood-wf Are you saying, if the database has had an unknown migration applied, when we apply our known migration, we may leave the db in an incompatible state?

johnlockwood-wf · 2016-10-26T15:46:24Z

@ericklaus-wf an applied, unknown migration may change the db into a state incompatible with the new, known migration.

johnlockwood-wf · 2016-10-26T15:50:57Z

the unknown migration applied may make the db incompatible with a new known migration

On Oct 26, 2016, at 6:49 AM, ericklaus-wf notifications@github.com wrote:

@johnlockwood-wf Are you saying, if the database has had an unknown migration applied, when we apply our known migration, we may leave the db in an incompatible state?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

ericklaus-wf · 2016-10-27T15:16:13Z

That's true, but the problem already exists. Consider this: a database has migrations 1 and 2 applied and these migrations are the master branch. Branch A adds migration 3. Branch B adds migration 4. Branch B is merged to master and deployed. Migration 4 is applied. The database is now in state "1, 2, 4". Branch A is merged to master and deployed. Migration 3 is applied. The database is now in state "1, 2, 3, 4".

This change does expand the area where this can happen though. It will now be possible for branch A to be deployed without merging to master and have migration 3 applied against the database.

johnlockwood-wf · 2016-10-27T17:14:22Z

If I was going to PR this back to the main fork, I might add a flag for the new behavior and without the flag, an unknown migration would cause the program to exit with a non-0 code. I'd consider an interactive mode, where the user was prompted for a choice of whether to go ahead with applying a migration after an unknown had been applied.

ericklaus-wf · 2016-10-27T19:28:35Z

To me, this is behavior that I expect, so I'd be annoyed by having this behind a flag. But I think I see your point.

It would be a little complicated to put this behind a flag. sql-migrate is both a library and a command line tool. Propagating the flag through would be a bit messy but possible.

ericklaus-wf commented Oct 25, 2016

View reviewed changes

ericklaus-wf changed the title ~~Handle unknown~~ Handle unknown migrations Oct 25, 2016

ericklaus-wf added 2 commits October 25, 2016 15:52

Handle unknown migrations in ToApply()

5136bb3

Handle unknown migrations in command_status

e03d678

ericklaus-wf force-pushed the handle-unknown branch from 6fff946 to e03d678 Compare October 25, 2016 20:52

tomdeering-wf reviewed Oct 25, 2016

View reviewed changes

johnlockwood-wf reviewed Oct 25, 2016

View reviewed changes

ericklaus-wf added 2 commits November 2, 2016 09:30

Clean up migrate

765a3c3

Use const none

8f48137

ericklaus-wf force-pushed the handle-unknown branch from 3181ab1 to 8f48137 Compare November 2, 2016 14:30

Handle unknown migrations #1

Are you sure you want to change the base?

Handle unknown migrations #1

Uh oh!

Conversation

ericklaus-wf commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericklaus-wf commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdeering-wf commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnlockwood-wf commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnlockwood-wf commented Oct 25, 2016

Uh oh!

ericklaus-wf commented Oct 26, 2016

Uh oh!

johnlockwood-wf commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnlockwood-wf commented Oct 26, 2016

Uh oh!

ericklaus-wf commented Oct 27, 2016

Uh oh!

johnlockwood-wf commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericklaus-wf commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericklaus-wf commented Oct 24, 2016 •

edited

Loading

tomdeering-wf commented Oct 25, 2016 •

edited

Loading

johnlockwood-wf commented Oct 25, 2016 •

edited

Loading

johnlockwood-wf commented Oct 26, 2016 •

edited

Loading

johnlockwood-wf commented Oct 27, 2016 •

edited

Loading

ericklaus-wf commented Oct 27, 2016 •

edited

Loading