From 5136bb347677feb109cab95d63f342360ec898a9 Mon Sep 17 00:00:00 2001 From: Eric Klaus Date: Mon, 24 Oct 2016 16:14:01 -0500 Subject: [PATCH 1/4] Handle unknown migrations in ToApply() --- migrate.go | 88 +++++++++++++++++++++++++++++++++++------------ migrate_test.go | 91 +++++++++++++++++++++++++++++++++++++++++++++++++ toapply_test.go | 29 ++++++---------- 3 files changed, 168 insertions(+), 40 deletions(-) diff --git a/migrate.go b/migrate.go index 4de82f32..614f1720 100644 --- a/migrate.go +++ b/migrate.go @@ -364,9 +364,11 @@ func PlanMigration(db *sql.DB, dialect string, m MigrationSource, dir MigrationD if len(existingMigrations) > 0 { result = append(result, ToCatchup(migrations, existingMigrations, record)...) } - // Figure out which migrations to apply - toApply := ToApply(migrations, record.Id, dir) + toApply := ToApply(migrations, existingMigrations, dir) + if err != nil { + return nil, nil, err + } toApplyCount := len(toApply) if max > 0 && max < toApplyCount { toApplyCount = max @@ -379,6 +381,10 @@ func PlanMigration(db *sql.DB, dialect string, m MigrationSource, dir MigrationD Queries: v.Up, }) } else if dir == Down { + if v.Down == nil { + // This happens if we are trying to migrate down a migration unknown to us + return nil, nil, fmt.Errorf("Unable to undo unknown migration %q", v.Id) + } result = append(result, &PlannedMigration{ Migration: v, Queries: v.Down, @@ -389,34 +395,74 @@ func PlanMigration(db *sql.DB, dialect string, m MigrationSource, dir MigrationD return result, dbMap, nil } -// Filter a slice of migrations into ones that should be applied. -func ToApply(migrations []*Migration, current string, direction MigrationDirection) []*Migration { - var index = -1 - if current != "" { - for index < len(migrations)-1 { - index++ - if migrations[index].Id == current { +// Searches the arrays for the latest elements with common ids. +// Returns the indexes of the common id. +func lastCommon(left []*Migration, right []*Migration) (int, int) { + xIndexMatch := -1 + yIndexMatch := 0 + for i := 0; i < len(left); i++ { + existingId := left[i].Id + for j := yIndexMatch; j < len(right); j++ { + if right[j].Id == existingId { + xIndexMatch = i + yIndexMatch = j break } } } + if xIndexMatch == -1 { + return -1, -1 // We never found a match; the arrays have nothing in common + } + return xIndexMatch, yIndexMatch +} +// Filter a slice of migrations into ones that should be applied. +func ToApply(migrations, existingMigrations []*Migration, direction MigrationDirection) []*Migration { if direction == Up { - return migrations[index+1:] - } else if direction == Down { - if index == -1 { - return []*Migration{} - } + return toApplyUp(migrations, existingMigrations) + } + if direction == Down { + return toApplyDown(migrations, existingMigrations) + } + panic("Unknown direction") +} - // Add in reverse order - toApply := make([]*Migration, index+1) - for i := 0; i < index+1; i++ { - toApply[index-i] = migrations[i] - } - return toApply +func toApplyUp(migrations, existingMigrations []*Migration) []*Migration { + if len(existingMigrations) == 0 { + return migrations + } + _, index := lastCommon(existingMigrations, migrations) + return migrations[index+1:] +} + +func toApplyDown(migrations, existingMigrations []*Migration) []*Migration { + if len(existingMigrations) == -1 { + return []*Migration{} + } + // 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. + migrationsMap := map[string]*Migration{} + for _, m := range existingMigrations { + // existingMigrations will not have Up or Down set + migrationsMap[m.Id] = m + } + for _, m := range migrations { + migrationsMap[m.Id] = m } + allMigrations := make([]*Migration, 0, len(migrationsMap)) + for _, m := range migrationsMap { + allMigrations = append(allMigrations, m) + } + sort.Sort(byId(allMigrations)) - panic("Not possible") + _, index := lastCommon(existingMigrations, allMigrations) + // Add in reverse order + toApply := make([]*Migration, index+1) + for i := 0; i < index+1; i++ { + toApply[index-i] = allMigrations[i] + } + return toApply } func ToCatchup(migrations, existingMigrations []*Migration, lastRun *Migration) []*PlannedMigration { diff --git a/migrate_test.go b/migrate_test.go index 4d573747..01888e8c 100644 --- a/migrate_test.go +++ b/migrate_test.go @@ -360,6 +360,97 @@ func (s *SqliteMigrateSuite) TestPlanMigrationWithHoles(c *C) { c.Assert(plannedMigrations[2].Queries[0], Equals, down) } +func (s *SqliteMigrateSuite) TestMigrationWithMostRecentUnknown(c *C) { + up := "SELECT 0" + down := "SELECT 1" + migrations := &MemoryMigrationSource{ + Migrations: []*Migration{ + &Migration{ + Id: "1", + Up: []string{up}, + Down: []string{down}, + }, + &Migration{ + Id: "3", + Up: []string{up}, + Down: []string{down}, + }, + &Migration{ + Id: "4", + Up: []string{up}, + Down: []string{down}, + }, + }, + } + n, err := Exec(s.Db, "sqlite3", migrations, Up) + c.Assert(err, IsNil) + c.Assert(n, Equals, 3) + + // remove "4" from our list of known migrations + migrations.Migrations = migrations.Migrations[0:2] + + migrations.Migrations = append(migrations.Migrations, &Migration{ + Id: "2", + Up: []string{up}, + Down: []string{down}, + }, &Migration{ + Id: "5", + Up: []string{up}, + Down: []string{down}, + }) + + // apply the latest migration + plannedMigrations, _, err := PlanMigration(s.Db, "sqlite3", migrations, Up, 0) + c.Assert(err, IsNil) + c.Assert(plannedMigrations, HasLen, 2) + c.Assert(plannedMigrations[0].Migration.Id, Equals, "2") + c.Assert(plannedMigrations[1].Migration.Id, Equals, "5") + + // The most recent migration is unknown; we can't undo it + plannedMigrations, _, err = PlanMigration(s.Db, "sqlite3", migrations, Down, 1) + c.Assert(err, Not(IsNil)) +} + +func (s *SqliteMigrateSuite) TestMigrationDownWithMiddleUnknown(c *C) { + up := "SELECT 0" + down := "SELECT 1" + migrations := &MemoryMigrationSource{ + Migrations: []*Migration{ + &Migration{ + Id: "1", + Up: []string{up}, + Down: []string{down}, + }, + &Migration{ + Id: "2", + Up: []string{up}, + Down: []string{down}, + }, + &Migration{ + Id: "3", + Up: []string{up}, + Down: []string{down}, + }, + }, + } + n, err := Exec(s.Db, "sqlite3", migrations, Up) + c.Assert(err, IsNil) + c.Assert(n, Equals, 3) + + // remove "2" from our list of known migrations + migrations.Migrations = []*Migration{migrations.Migrations[0], migrations.Migrations[2]} + + // undo "3" + plannedMigrations, _, err := PlanMigration(s.Db, "sqlite3", migrations, Down, 1) + c.Assert(err, IsNil) + c.Assert(plannedMigrations, HasLen, 1) + c.Assert(plannedMigrations[0].Id, Equals, "3") + + // will undo "3", then try "2", but "2" is unknown + plannedMigrations, _, err = PlanMigration(s.Db, "sqlite3", migrations, Down, 2) + c.Assert(err, Not(IsNil)) +} + func (s *SqliteMigrateSuite) TestLess(c *C) { c.Assert((Migration{Id: "1"}).Less(&Migration{Id: "2"}), Equals, true) // 1 less than 2 c.Assert((Migration{Id: "2"}).Less(&Migration{Id: "1"}), Equals, false) // 2 not less than 1 diff --git a/toapply_test.go b/toapply_test.go index 6206f59b..d3f0dc92 100644 --- a/toapply_test.go +++ b/toapply_test.go @@ -17,7 +17,7 @@ type ToApplyMigrateSuite struct { var _ = Suite(&ToApplyMigrateSuite{}) func (s *ToApplyMigrateSuite) TestGetAll(c *C) { - toApply := ToApply(toapplyMigrations, "", Up) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:0], Up) c.Assert(toApply, HasLen, 3) c.Assert(toApply[0], Equals, toapplyMigrations[0]) c.Assert(toApply[1], Equals, toapplyMigrations[1]) @@ -25,52 +25,43 @@ func (s *ToApplyMigrateSuite) TestGetAll(c *C) { } func (s *ToApplyMigrateSuite) TestGetAbc(c *C) { - toApply := ToApply(toapplyMigrations, "abc", Up) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:1], Up) c.Assert(toApply, HasLen, 2) c.Assert(toApply[0], Equals, toapplyMigrations[1]) c.Assert(toApply[1], Equals, toapplyMigrations[2]) } func (s *ToApplyMigrateSuite) TestGetCde(c *C) { - toApply := ToApply(toapplyMigrations, "cde", Up) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:2], Up) c.Assert(toApply, HasLen, 1) c.Assert(toApply[0], Equals, toapplyMigrations[2]) } func (s *ToApplyMigrateSuite) TestGetDone(c *C) { - toApply := ToApply(toapplyMigrations, "efg", Up) - c.Assert(toApply, HasLen, 0) - - toApply = ToApply(toapplyMigrations, "zzz", Up) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:3], Up) c.Assert(toApply, HasLen, 0) } func (s *ToApplyMigrateSuite) TestDownDone(c *C) { - toApply := ToApply(toapplyMigrations, "", Down) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:0], Down) c.Assert(toApply, HasLen, 0) } func (s *ToApplyMigrateSuite) TestDownCde(c *C) { - toApply := ToApply(toapplyMigrations, "cde", Down) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:2], Down) c.Assert(toApply, HasLen, 2) c.Assert(toApply[0], Equals, toapplyMigrations[1]) c.Assert(toApply[1], Equals, toapplyMigrations[0]) } func (s *ToApplyMigrateSuite) TestDownAbc(c *C) { - toApply := ToApply(toapplyMigrations, "abc", Down) + toApply := ToApply(toapplyMigrations, toapplyMigrations[0:1], Down) c.Assert(toApply, HasLen, 1) c.Assert(toApply[0], Equals, toapplyMigrations[0]) } func (s *ToApplyMigrateSuite) TestDownAll(c *C) { - toApply := ToApply(toapplyMigrations, "efg", Down) - c.Assert(toApply, HasLen, 3) - c.Assert(toApply[0], Equals, toapplyMigrations[2]) - c.Assert(toApply[1], Equals, toapplyMigrations[1]) - c.Assert(toApply[2], Equals, toapplyMigrations[0]) - - toApply = ToApply(toapplyMigrations, "zzz", Down) + toApply := ToApply(toapplyMigrations, toapplyMigrations, Down) c.Assert(toApply, HasLen, 3) c.Assert(toApply[0], Equals, toapplyMigrations[2]) c.Assert(toApply[1], Equals, toapplyMigrations[1]) @@ -88,13 +79,13 @@ func (s *ToApplyMigrateSuite) TestAlphaNumericMigrations(c *C) { sort.Sort(migrations) - toApplyUp := ToApply(migrations, "2_cde", Up) + toApplyUp := ToApply(migrations, migrations[0:2], Up) c.Assert(toApplyUp, HasLen, 3) c.Assert(toApplyUp[0].Id, Equals, "10_abc") c.Assert(toApplyUp[1].Id, Equals, "35_cde") c.Assert(toApplyUp[2].Id, Equals, "efg") - toApplyDown := ToApply(migrations, "2_cde", Down) + toApplyDown := ToApply(migrations, migrations[0:2], Down) c.Assert(toApplyDown, HasLen, 2) c.Assert(toApplyDown[0].Id, Equals, "2_cde") c.Assert(toApplyDown[1].Id, Equals, "1_abc") From e03d6789676d54b9866dbedc02440cb4f3524808 Mon Sep 17 00:00:00 2001 From: Eric Klaus Date: Mon, 24 Oct 2016 16:14:14 -0500 Subject: [PATCH 2/4] Handle unknown migrations in command_status --- sql-migrate/command_status.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sql-migrate/command_status.go b/sql-migrate/command_status.go index bc2ca70d..71fad08c 100644 --- a/sql-migrate/command_status.go +++ b/sql-migrate/command_status.go @@ -83,8 +83,13 @@ func (c *StatusCommand) Run(args []string) int { } for _, r := range records { - rows[r.Id].Migrated = true - rows[r.Id].AppliedAt = r.AppliedAt + status, ok := rows[r.Id] + if !ok { + ui.Warn(fmt.Sprintf("Migration in database %q unknown to sql-migrate; it will not be possible to migrate down this migration", r.Id)) + continue + } + status.Migrated = true + status.AppliedAt = r.AppliedAt } for _, m := range migrations { From 765a3c38b095aa64f7f49d6025ee41b65c8e2720 Mon Sep 17 00:00:00 2001 From: Eric Klaus Date: Thu, 27 Oct 2016 10:57:01 -0500 Subject: [PATCH 3/4] Clean up migrate --- migrate.go | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/migrate.go b/migrate.go index 614f1720..e6f7a9a3 100644 --- a/migrate.go +++ b/migrate.go @@ -366,9 +366,6 @@ func PlanMigration(db *sql.DB, dialect string, m MigrationSource, dir MigrationD } // Figure out which migrations to apply toApply := ToApply(migrations, existingMigrations, dir) - if err != nil { - return nil, nil, err - } toApplyCount := len(toApply) if max > 0 && max < toApplyCount { toApplyCount = max @@ -397,7 +394,7 @@ func PlanMigration(db *sql.DB, dialect string, m MigrationSource, dir MigrationD // Searches the arrays for the latest elements with common ids. // Returns the indexes of the common id. -func lastCommon(left []*Migration, right []*Migration) (int, int) { +func lastCommonMigration(left []*Migration, right []*Migration) (int, int) { xIndexMatch := -1 yIndexMatch := 0 for i := 0; i < len(left); i++ { @@ -431,38 +428,48 @@ func toApplyUp(migrations, existingMigrations []*Migration) []*Migration { if len(existingMigrations) == 0 { return migrations } - _, index := lastCommon(existingMigrations, migrations) + // All migrations after the last common migration need to be applied to the db. + // It's possible that there are some existingMigrations unknown to us-- + // we'll just ignore them and hope they don't matter. + _, index := lastCommonMigration(existingMigrations, migrations) return migrations[index+1:] } func toApplyDown(migrations, existingMigrations []*Migration) []*Migration { - if len(existingMigrations) == -1 { + if len(existingMigrations) == 0 { return []*Migration{} } - // 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. + allMigrations := mergeMigrations(existingMigrations, migrations) + _, index := lastCommonMigration(existingMigrations, allMigrations) + // Add in reverse order + toApply := make([]*Migration, index+1) + for i := 0; i < index+1; i++ { + toApply[index-i] = allMigrations[i] + } + return toApply +} + +// Returns a list of all migrations in either list, sorted by Id. +// Migrations present only in existingMigrations will not have Up or Down set. +func mergeMigrations(existingMigrations, migrations []*Migration) []*Migration { migrationsMap := map[string]*Migration{} for _, m := range existingMigrations { - // existingMigrations will not have Up or Down set migrationsMap[m.Id] = m } + // 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 for _, m := range migrations { migrationsMap[m.Id] = m } + + // Flatten our map back into a list that we can sort allMigrations := make([]*Migration, 0, len(migrationsMap)) for _, m := range migrationsMap { allMigrations = append(allMigrations, m) } sort.Sort(byId(allMigrations)) - - _, index := lastCommon(existingMigrations, allMigrations) - // Add in reverse order - toApply := make([]*Migration, index+1) - for i := 0; i < index+1; i++ { - toApply[index-i] = allMigrations[i] - } - return toApply + return allMigrations } func ToCatchup(migrations, existingMigrations []*Migration, lastRun *Migration) []*PlannedMigration { From 8f48137c8d7dcbc1f68c546089061b05aefe4cf9 Mon Sep 17 00:00:00 2001 From: Eric Klaus Date: Thu, 27 Oct 2016 10:59:37 -0500 Subject: [PATCH 4/4] Use `const none` --- migrate.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/migrate.go b/migrate.go index e6f7a9a3..71ca872b 100644 --- a/migrate.go +++ b/migrate.go @@ -395,7 +395,8 @@ func PlanMigration(db *sql.DB, dialect string, m MigrationSource, dir MigrationD // Searches the arrays for the latest elements with common ids. // Returns the indexes of the common id. func lastCommonMigration(left []*Migration, right []*Migration) (int, int) { - xIndexMatch := -1 + const none = -1 + xIndexMatch := none yIndexMatch := 0 for i := 0; i < len(left); i++ { existingId := left[i].Id @@ -407,8 +408,9 @@ func lastCommonMigration(left []*Migration, right []*Migration) (int, int) { } } } - if xIndexMatch == -1 { - return -1, -1 // We never found a match; the arrays have nothing in common + if xIndexMatch == none { + // We never found a match; the arrays have nothing in common + return none, none } return xIndexMatch, yIndexMatch }