Merge pull request #1063 from uptrace/fix/automigrator-create-migrations

🐛 AutoMigrator: Do not create migrations if nothing to migrate

internal/dbtest/inspect_test.go

@@ -537,7 +537,7 @@ func TestBunModelInspector_Inspect(t *testing.T) {
 					Key: "name",
 					Value: &sqlschema.BaseColumn{
 						SQLType:      sqltype.VarChar,
-						DefaultValue: "'John Doe'",
+						DefaultValue: "John Doe",
 					},
 				},
 			))

internal/dbtest/migrate_test.go

@@ -254,8 +254,8 @@ func TestAutoMigrator_CreateSQLMigrations(t *testing.T) {
 
 			require.Len(t, migrations, 2, "expected up/down migration pair")
 			require.DirExists(t, migrationsDir)
-			checkMigrationFileContains(t, ".up.sql", "CREATE TABLE")
-			checkMigrationFileContains(t, ".down.sql", "DROP TABLE")
+			checkMigrationFileContains(t, "_auto.up.sql", "CREATE TABLE")
+			checkMigrationFileContains(t, "_auto.down.sql", "DROP TABLE")
 		})
 
 		t.Run("transactional", func(t *testing.T) {
@@ -264,8 +264,8 @@ func TestAutoMigrator_CreateSQLMigrations(t *testing.T) {
 
 			require.Len(t, migrations, 2, "expected up/down migration pair")
 			require.DirExists(t, migrationsDir)
-			checkMigrationFileContains(t, "tx.up.sql", "CREATE TABLE", "SET statement_timeout = 0")
-			checkMigrationFileContains(t, "tx.down.sql", "DROP TABLE", "SET statement_timeout = 0")
+			checkMigrationFileContains(t, "_auto.tx.up.sql", "CREATE TABLE", "SET statement_timeout = 0")
+			checkMigrationFileContains(t, "_auto.tx.down.sql", "DROP TABLE", "SET statement_timeout = 0")
 		})
 
 	})
@@ -291,7 +291,7 @@ func checkMigrationFileContains(t *testing.T, fileSuffix string, snippets ...str
 	t.Errorf("no *%s file in migrations directory (%s)", fileSuffix, migrationsDir)
 }
 
-// checkMigrationFilesExist makes sure both up- and down- SQL migration files were created.
+// checkMigrationFilesExist checks both up- and down- SQL migration files were created.
 func checkMigrationFilesExist(t *testing.T) {
 	t.Helper()
 
@@ -315,6 +315,7 @@ func checkMigrationFilesExist(t *testing.T) {
 	}
 }
 
+// runMigrations is a test helper to run AutoMigrator.Migrate(), check that it completed without error and migration files were created.
 func runMigrations(t *testing.T, m *migrate.AutoMigrator) {
 	t.Helper()
 
@@ -338,6 +339,7 @@ func TestAutoMigrator_Migrate(t *testing.T) {
 		{testUnique},
 		{testUniqueRenamedTable},
 		{testUpdatePrimaryKeys},
+		{testNothingToMigrate},
 	}
 
 	testEachDB(t, func(t *testing.T, dbName string, db *bun.DB) {
@@ -1103,3 +1105,24 @@ func testUpdatePrimaryKeys(t *testing.T, db *bun.DB) {
 	state := inspect(ctx)
 	cmpTables(t, db.Dialect().(sqlschema.InspectorDialect), wantTables, state.Tables)
 }
+
+func testNothingToMigrate(t *testing.T, db *bun.DB) {
+	type BoringThing struct {
+		AlwaysBlue string `bun:"colour,default:'blue'"`
+	}
+
+	ctx := context.Background()
+	mustResetModel(t, ctx, db, (*BoringThing)(nil))
+	m := newAutoMigratorOrSkip(t, db,
+		migrate.WithModel((*BoringThing)(nil)),
+	)
+
+	// Act
+	_, err := m.Migrate(ctx) // do not use runMigrations because we do not expect any files to be created
+	require.NoError(t, err, "auto migration failed")
+
+	migrator := migrate.NewMigrator(db, migrate.NewMigrations(), migrate.WithTableName(migrationsTable))
+	applied, err := migrator.AppliedMigrations(ctx)
+	require.NoError(t, err, "fetch applied migrations")
+	require.Empty(t, applied, "nothing to migrate, AppliedMigrations not empty")
+}

migrate/auto.go

@@ -196,6 +196,9 @@ func (am *AutoMigrator) plan(ctx context.Context) (*changeset, error) {
 func (am *AutoMigrator) Migrate(ctx context.Context, opts ...MigrationOption) (*MigrationGroup, error) {
 	migrations, _, err := am.createSQLMigrations(ctx, false)
 	if err != nil {
+		if err == errNothingToMigrate {
+			return new(MigrationGroup), nil
+		}
 		return nil, fmt.Errorf("auto migrate: %w", err)
 	}
 
@@ -214,23 +217,37 @@ func (am *AutoMigrator) Migrate(ctx context.Context, opts ...MigrationOption) (*
 // CreateSQLMigration writes required changes to a new migration file.
 // Use migrate.Migrator to apply the generated migrations.
 func (am *AutoMigrator) CreateSQLMigrations(ctx context.Context) ([]*MigrationFile, error) {
-	_, files, err := am.createSQLMigrations(ctx, true)
+	_, files, err := am.createSQLMigrations(ctx, false)
+	if err == errNothingToMigrate {
+		return files, nil
+	}
 	return files, err
 }
 
 // CreateTxSQLMigration writes required changes to a new migration file making sure they will be executed
 // in a transaction when applied. Use migrate.Migrator to apply the generated migrations.
 func (am *AutoMigrator) CreateTxSQLMigrations(ctx context.Context) ([]*MigrationFile, error) {
-	_, files, err := am.createSQLMigrations(ctx, false)
+	_, files, err := am.createSQLMigrations(ctx, true)
+	if err == errNothingToMigrate {
+		return files, nil
+	}
 	return files, err
 }
 
+// errNothingToMigrate is a sentinel error which means the database is already in a desired state.
+// Should not be returned to the user -- return a nil-error instead.
+var errNothingToMigrate = errors.New("nothing to migrate")
+
 func (am *AutoMigrator) createSQLMigrations(ctx context.Context, transactional bool) (*Migrations, []*MigrationFile, error) {
 	changes, err := am.plan(ctx)
 	if err != nil {
 		return nil, nil, fmt.Errorf("create sql migrations: %w", err)
 	}
 
+	if changes.Len() == 0 {
+		return nil, nil, errNothingToMigrate
+	}
+
 	name, _ := genMigrationName(am.schemaName + "_auto")
 	migrations := NewMigrations(am.migrationsOpts...)
 	migrations.Add(Migration{
@@ -282,6 +299,10 @@ func (am *AutoMigrator) createSQL(_ context.Context, migrations *Migrations, fna
 	return mf, nil
 }
 
+func (c *changeset) Len() int {
+	return len(c.operations)
+}
+
 // Func creates a MigrationFunc that applies all operations all the changeset.
 func (c *changeset) Func(m sqlschema.Migrator) MigrationFunc {
 	return func(ctx context.Context, db *bun.DB) error {

migrate/sqlschema/inspector.go

@@ -120,7 +120,7 @@ func (bmi *BunModelInspector) Inspect(ctx context.Context) (Database, error) {
 				Name:            f.Name,
 				SQLType:         strings.ToLower(sqlType), // TODO(dyma): maybe this is not necessary after Column.Eq()
 				VarcharLen:      length,
-				DefaultValue:    exprToLower(f.SQLDefault),
+				DefaultValue:    exprOrLiteral(f.SQLDefault),
 				IsNullable:      !f.NotNull,
 				IsAutoIncrement: f.AutoIncrement,
 				IsIdentity:      f.Identity,
@@ -211,12 +211,13 @@ func parseLen(typ string) (string, int, error) {
 	return typ[:paren], length, nil
 }
 
-// exprToLower converts string to lowercase, if it does not contain a string literal 'lit'.
+// exprOrLiteral converts string to lowercase, if it does not contain a string literal 'lit'
+// and trims the surrounding '' otherwise.
 // Use it to ensure that user-defined default values in the models are always comparable
 // to those returned by the database inspector, regardless of the case convention in individual drivers.
-func exprToLower(s string) string {
+func exprOrLiteral(s string) string {
 	if strings.HasPrefix(s, "'") && strings.HasSuffix(s, "'") {
-		return s
+		return strings.Trim(s, "'")
 	}
 	return strings.ToLower(s)
 }