fix(migration): skip v20 if column `sha256` already exists (#7354)

5 files changed, 33 insertions, 9 deletions

diff --git a/internal/db/migrations/migrations.go b/internal/db/migrations/migrations.go
index 664bea5f..04c1e363 100644
--- a/internal/db/migrations/migrations.go
+++ b/internal/db/migrations/migrations.go
@@ -56,8 +56,17 @@ var migrations = []Migration{
 	NewMigration("migrate access tokens to store SHA56", migrateAccessTokenToSHA256),
 	// v20 -> v21:v0.13.0
 	NewMigration("add index to action.user_id", addIndexToActionUserID),
+	// v21 -> v22:v0.13.0
+	//
+	// NOTE: There was a bug in calculating the value of the `version.version`
+	// column after a migration is done, thus some instances are on v21 but some are
+	// on v22. Let's make a noop v22 to make sure every instance will not miss a
+	// real future migration.
+	NewMigration("noop", func(*gorm.DB) error { return nil }),
 }
 
+var errMigrationSkipped = errors.New("the migration has been skipped")
+
 // Migrate migrates the database schema and/or data to the current version.
 func Migrate(db *gorm.DB) error {
 	// NOTE: GORM has problem migrating tables that happen to have columns with the
@@ -121,13 +130,16 @@ In case you're stilling getting this notice, go through instructions again until
 		return db.Where("id = ?", current.ID).Updates(current).Error
 	}
 
-	for i, m := range migrations[current.Version-minDBVersion:] {
+	for _, m := range migrations[current.Version-minDBVersion:] {
 		log.Info("Migration: %s", m.Description())
 		if err = m.Migrate(db); err != nil {
-			return errors.Wrap(err, "do migrate")
+			if err != errMigrationSkipped {
+				return errors.Wrap(err, "do migrate")
+			}
+			log.Trace("The migration %q has been skipped", m.Description())
 		}
 
-		current.Version += int64(i) + 1
+		current.Version++
 		err = db.Where("id = ?", current.ID).Updates(current).Error
 		if err != nil {
 			return errors.Wrap(err, "update the version record")
diff --git a/internal/db/migrations/v20.go b/internal/db/migrations/v20.go
index 28f406bb..bf58db5e 100644
--- a/internal/db/migrations/v20.go
+++ b/internal/db/migrations/v20.go
@@ -12,14 +12,18 @@ import (
 )
 
 func migrateAccessTokenToSHA256(db *gorm.DB) error {
+	type accessToken struct {
+		ID     int64
+		Sha1   string
+		SHA256 string `gorm:"TYPE:VARCHAR(64)"`
+	}
+
+	if db.Migrator().HasColumn(&accessToken{}, "SHA256") {
+		return errMigrationSkipped
+	}
 	return db.Transaction(func(tx *gorm.DB) error {
 		// 1. Add column without constraints because all rows have NULL values for the
 		// "sha256" column.
-		type accessToken struct {
-			ID     int64
-			Sha1   string
-			SHA256 string `gorm:"TYPE:VARCHAR(64)"`
-		}
 		err := tx.Migrator().AddColumn(&accessToken{}, "SHA256")
 		if err != nil {
 			return errors.Wrap(err, "add column")
diff --git a/internal/db/migrations/v20_test.go b/internal/db/migrations/v20_test.go
index b95360de..9bbf283f 100644
--- a/internal/db/migrations/v20_test.go
+++ b/internal/db/migrations/v20_test.go
@@ -67,4 +67,8 @@ func TestMigrateAccessTokenToSHA256(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "73da7bb9d2a475bbc2ab79da7d4e94940cb9f9d5", got.Sha1)
 	assert.Equal(t, "ab144c7bd170691bb9bb995f1541c608e33a78b40174f30fc8a1616c0bc3a477", got.SHA256)
+
+	// Re-run should be skipped
+	err = migrateAccessTokenToSHA256(db)
+	require.Equal(t, errMigrationSkipped, err)
 }
diff --git a/internal/db/migrations/v21.go b/internal/db/migrations/v21.go
index 0a1c74b0..7eb97a7f 100644
--- a/internal/db/migrations/v21.go
+++ b/internal/db/migrations/v21.go
@@ -13,7 +13,7 @@ func addIndexToActionUserID(db *gorm.DB) error {
 		UserID string `gorm:"index"`
 	}
 	if db.Migrator().HasIndex(&action{}, "UserID") {
-		return nil
+		return errMigrationSkipped
 	}
 	return db.Migrator().CreateIndex(&action{}, "UserID")
 }
diff --git a/internal/db/migrations/v21_test.go b/internal/db/migrations/v21_test.go
index d11c47c5..866e9074 100644
--- a/internal/db/migrations/v21_test.go
+++ b/internal/db/migrations/v21_test.go
@@ -79,4 +79,8 @@ func TestAddIndexToActionUserID(t *testing.T) {
 	err = addIndexToActionUserID(db)
 	require.NoError(t, err)
 	assert.True(t, db.Migrator().HasIndex(&actionV21{}, "UserID"))
+
+	// Re-run should be skipped
+	err = addIndexToActionUserID(db)
+	require.Equal(t, errMigrationSkipped, err)
 }