Added pre-migration infra and a pre-migration for fixing schema ID migration numbers (#36870) (#37008)

Automatic Merge

server/channels/store/sqlstore/migrate.go

@@ -172,6 +172,18 @@ func (m *Migrator) MigrateWithPlan(plan *models.Plan, dryRun bool) error {
 	return m.engine.ApplyPlan(plan)
 }
 
+// PreMigrate runs the pre-migration handlers that normally execute during
+// server startup. Callers on the CLI up-migration path (e.g. `mattermost db
+// migrate`, used by cloud upgrades) must invoke this before MigrateWithPlan so
+// the same fixes apply outside of server startup. Skipped under dryRun because
+// preMigration writes directly via GetMaster().Exec and does not participate
+// in Morph's dry-run.
+// This is intentionally only called for forward migrations and skipped for
+// downgrades.
+func (m *Migrator) PreMigrate() error {
+	return m.store.preMigration()
+}
+
 func (m *Migrator) DowngradeMigrations(dryRun bool, versions ...string) error {
 	migrations, err := m.engine.Diff(models.Down)
 	if err != nil {

server/channels/store/sqlstore/migrate_test.go

@@ -35,3 +35,57 @@ func TestUpAndDownMigrations(t *testing.T) {
 		})
 	}
 }
+
+// TestMigratorPreMigrate exercises the exported entry point invoked by the
+// `mattermost db migrate` CLI. The handler exists so cloud upgrades, which
+// bypass sqlstore.New(), still apply the pre-migration fixes that ship with
+// each release.
+func TestMigratorPreMigrate(t *testing.T) {
+	if enableFullyParallelTests {
+		t.Parallel()
+	}
+
+	logger := mlog.CreateConsoleTestLogger(t)
+	const markerName = "renumber_roles_schemeid_migrations"
+
+	t.Run("runs pre-migrations and marks them complete", func(t *testing.T) {
+		settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+		if err != nil {
+			t.Skip(err)
+		}
+
+		store, err := New(*settings, logger, nil)
+		require.NoError(t, err)
+		defer store.Close()
+		_, err = store.GetMaster().Exec("DELETE FROM Systems WHERE Name = $1", markerName)
+		require.NoError(t, err)
+
+		migrator, err := NewMigrator(*settings, logger, false)
+		require.NoError(t, err)
+		defer migrator.Close()
+
+		require.NoError(t, migrator.PreMigrate())
+
+		done, err := store.isPreMigrationComplete(markerName)
+		require.NoError(t, err)
+		assert.True(t, done, "PreMigrate should set the completion marker on a non-dry-run")
+	})
+
+	t.Run("idempotent across repeated calls", func(t *testing.T) {
+		settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+		if err != nil {
+			t.Skip(err)
+		}
+
+		store, err := New(*settings, logger, nil)
+		require.NoError(t, err)
+		defer store.Close()
+
+		migrator, err := NewMigrator(*settings, logger, false)
+		require.NoError(t, err)
+		defer migrator.Close()
+
+		require.NoError(t, migrator.PreMigrate())
+		require.NoError(t, migrator.PreMigrate(), "second invocation must be a safe no-op")
+	})
+}

server/channels/store/sqlstore/store.go

@@ -230,6 +230,11 @@ func New(settings model.SqlSettings, logger mlog.LoggerIFace, metrics einterface
 	}
 
 	if !store.skipMigrations {
+		err = store.preMigration()
+		if err != nil {
+			return nil, errors.Wrap(err, "error while running pre-migrations")
+		}
+
 		err = store.migrate(migrationsDirectionUp, false, !store.disableMorphLogging)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to apply database migrations")
@@ -1088,3 +1093,150 @@ func (ss *SqlStore) ScheduledPost() store.ScheduledPostStore {
 func (ss *SqlStore) ContentFlagging() store.ContentFlaggingStore {
 	return ss.stores.ContentFlagging
 }
+
+// preMigration	runs before running the actual Morph migrations.
+// The main reason for having a preMigration is to fix https://mattermost.atlassian.net/browse/MM-68848?focusedCommentId=217769
+// However, using that as an opportunity, a general purpose system is created that allows running
+// arbitrary code just before the Morph migrations run. This allows us to have a way to fix
+// any issues in the DB that might prevent the Morph migrations from running successfully.
+// The pre-migrations are tracked in the Systems table to make sure they run only once.
+func (ss *SqlStore) preMigration() error {
+	type sqlMigration struct {
+		name string
+		// minDBMigration is the schema migration version that must already have
+		// been applied (recorded in db_migrations) before this pre-migration is
+		// allowed to run. A pre-migration that touches data in a table created
+		// by schema migration N should set this to N.
+		minDBMigration int
+		handler        func() error
+	}
+
+	migrations := []sqlMigration{
+		{"renumber_roles_schemeid_migrations", 144, ss.doRenumberRolesSchemeIdMigrations},
+	}
+
+	// To check for empty DB and skip
+	exists, err := ss.tableExists("db_migrations")
+	if err != nil {
+		return errors.Wrap(err, "failed to check if db_migrations table exists")
+	}
+	if !exists {
+		return nil
+	}
+
+	// Checking for systems table because that's where the run status of individual pre migrations are stored
+	exists, err = ss.tableExists("systems")
+	if err != nil {
+		return errors.Wrap(err, "failed to check if Systems table exists")
+	}
+	if !exists {
+		return nil
+	}
+
+	for _, m := range migrations {
+		applied, err := ss.isDBMigrationApplied(m.minDBMigration)
+		if err != nil {
+			return errors.Wrapf(err, "failed to check DB migration %d status", m.minDBMigration)
+		}
+		if !applied {
+			continue
+		}
+
+		done, err := ss.isPreMigrationComplete(m.name)
+		if err != nil {
+			return errors.Wrapf(err, "failed to check pre-migration %q status", m.name)
+		}
+		if done {
+			continue
+		}
+
+		ss.logger.Debug("Running pre-migration", mlog.String("name", m.name), mlog.Int("min_db_migration", m.minDBMigration))
+		if err := m.handler(); err != nil {
+			return errors.Wrapf(err, "failed to run pre-migration %q", m.name)
+		}
+
+		if err := ss.markPreMigrationComplete(m.name); err != nil {
+			return errors.Wrapf(err, "failed to mark pre-migration %q complete", m.name)
+		}
+		ss.logger.Debug("Completed pre-migration", mlog.String("name", m.name))
+	}
+
+	return nil
+}
+
+// This is for resolving the issue described here - https://mattermost.atlassian.net/browse/MM-68848?focusedCommentId=217769
+// Briefly describing here for quick reference -
+// The DB migrations originally numbered 156, 157 and 158 were cherrypicked onto v10.11 release branch but their numbers
+// were changed. When upgrading from v10.11.17 to v11.7, the migrations which were actually supposed to be 142, 143 and 144
+// could never run due to migration version conflict in db_migrations table.
+// This pre-migration functon fixes that issue. When someone upgrades from v10.11 to new release, this function fixes the migration
+// numbers to what they originally were. For example, 142 gets renamed to 156. This lets the missing migrations run successfully and complete the upgrade.
+func (ss *SqlStore) doRenumberRolesSchemeIdMigrations() error {
+	query := `
+UPDATE db_migrations
+SET Version = CASE
+    WHEN Version = 142 AND Name = 'add_schemeid_to_roles'    THEN 156
+    WHEN Version = 143 AND Name = 'backfill_roles_schemeid'  THEN 157
+    WHEN Version = 144 AND Name = 'add_roles_schemeid_index' THEN 158
+END
+WHERE (Version, Name) IN (
+    (142, 'add_schemeid_to_roles'),
+    (143, 'backfill_roles_schemeid'),
+    (144, 'add_roles_schemeid_index')
+)`
+
+	if _, err := ss.GetMaster().Exec(query); err != nil {
+		return errors.Wrap(err, "failed to renumber schema ID related migrations")
+	}
+
+	return nil
+}
+
+func (ss *SqlStore) isDBMigrationApplied(version int) (bool, error) {
+	var exists bool
+	if err := ss.GetMaster().Get(&exists, `
+		SELECT EXISTS (
+			SELECT 1 FROM db_migrations WHERE Version >= $1
+		)
+	`, version); err != nil {
+		return false, errors.Wrap(err, "unable to query db_migrations")
+	}
+	return exists, nil
+}
+
+func (ss *SqlStore) tableExists(tableName string) (bool, error) {
+	var exists bool
+	if err := ss.GetMaster().Get(&exists, `
+		SELECT EXISTS (
+			SELECT 1 FROM information_schema.tables
+			WHERE LOWER(table_name) = $1
+		    AND table_schema = current_schema()
+		)
+	`, strings.ToLower(tableName)); err != nil {
+		return false, errors.Wrap(err, "unable to query information_schema.tables")
+	}
+	return exists, nil
+}
+
+func (ss *SqlStore) isPreMigrationComplete(name string) (bool, error) {
+	var value string
+	err := ss.GetMaster().Get(&value, "SELECT Value FROM Systems WHERE Name = $1", name)
+	if errors.Is(err, sql.ErrNoRows) {
+		return false, nil
+	}
+	if err != nil {
+		return false, errors.Wrap(err, "unable to select from Systems")
+	}
+	return value == "true", nil
+}
+
+func (ss *SqlStore) markPreMigrationComplete(name string) error {
+	if _, err := ss.GetMaster().Exec(
+		`INSERT INTO Systems (Name, Value) VALUES ($1, $2)
+		 ON CONFLICT (Name) DO UPDATE SET Value = $2`,
+		name, "true",
+	); err != nil {
+		return errors.Wrap(err, "failed to upsert system property")
+	}
+	return nil
+}

server/channels/store/sqlstore/store_test.go

@@ -1105,3 +1105,210 @@ func TestSkipMigrationsOption(t *testing.T) {
 		})
 	}
 }
+
+func TestIsDBMigrationApplied(t *testing.T) {
+	if enableFullyParallelTests {
+		t.Parallel()
+	}
+
+	logger := mlog.CreateConsoleTestLogger(t)
+	settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+	if err != nil {
+		t.Skip(err)
+	}
+	ss, err := New(*settings, logger, nil)
+	require.NoError(t, err)
+
+	maxVersion, err := ss.GetDBSchemaVersion()
+	require.NoError(t, err)
+	require.Greater(t, maxVersion, 0)
+
+	t.Run("returns true for version 1", func(t *testing.T) {
+		applied, err := ss.isDBMigrationApplied(1)
+		require.NoError(t, err)
+		assert.True(t, applied)
+	})
+
+	t.Run("returns true for the current max version", func(t *testing.T) {
+		applied, err := ss.isDBMigrationApplied(maxVersion)
+		require.NoError(t, err)
+		assert.True(t, applied)
+	})
+
+	t.Run("returns false for a version beyond max", func(t *testing.T) {
+		applied, err := ss.isDBMigrationApplied(maxVersion + 1000)
+		require.NoError(t, err)
+		assert.False(t, applied)
+	})
+}
+
+func TestPreMigrationCompletionMarker(t *testing.T) {
+	if enableFullyParallelTests {
+		t.Parallel()
+	}
+
+	logger := mlog.CreateConsoleTestLogger(t)
+	settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+	if err != nil {
+		t.Skip(err)
+	}
+	ss, err := New(*settings, logger, nil)
+	require.NoError(t, err)
+
+	const name = "test_pre_migration_marker"
+
+	done, err := ss.isPreMigrationComplete(name)
+	require.NoError(t, err)
+	assert.False(t, done, "marker should be absent before being set")
+
+	require.NoError(t, ss.markPreMigrationComplete(name))
+
+	done, err = ss.isPreMigrationComplete(name)
+	require.NoError(t, err)
+	assert.True(t, done, "marker should be present after being set")
+
+	// Idempotent: re-marking is a no-op due to ON CONFLICT.
+	require.NoError(t, ss.markPreMigrationComplete(name))
+	done, err = ss.isPreMigrationComplete(name)
+	require.NoError(t, err)
+	assert.True(t, done)
+}
+
+func TestDoRenumberRolesSchemeIdMigrations(t *testing.T) {
+	if enableFullyParallelTests {
+		t.Parallel()
+	}
+
+	logger := mlog.CreateConsoleTestLogger(t)
+	settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+	if err != nil {
+		t.Skip(err)
+	}
+	ss, err := New(*settings, logger, nil)
+	require.NoError(t, err)
+
+	// Simulate an old install: replace the rows that Morph wrote at 156/157/158
+	// with rows at the historical numbering 142/143/144.
+	_, err = ss.GetMaster().Exec("DELETE FROM db_migrations WHERE Version IN (156, 157, 158, 142, 143, 144)")
+	require.NoError(t, err)
+	_, err = ss.GetMaster().Exec(`
+		INSERT INTO db_migrations (Version, Name) VALUES
+			(142, 'add_schemeid_to_roles'),
+			(143, 'backfill_roles_schemeid'),
+			(144, 'add_roles_schemeid_index')
+	`)
+	require.NoError(t, err)
+
+	require.NoError(t, ss.doRenumberRolesSchemeIdMigrations())
+
+	type row struct {
+		Version int
+		Name    string
+	}
+	rows := []row{}
+	require.NoError(t, ss.GetMaster().Select(&rows,
+		"SELECT Version, Name FROM db_migrations WHERE Version IN (142, 143, 144, 156, 157, 158) ORDER BY Version"))
+
+	assert.Equal(t, []row{
+		{156, "add_schemeid_to_roles"},
+		{157, "backfill_roles_schemeid"},
+		{158, "add_roles_schemeid_index"},
+	}, rows)
+
+	// Running again is a safe no-op: WHERE clauses match nothing now.
+	require.NoError(t, ss.doRenumberRolesSchemeIdMigrations())
+
+	rows = []row{}
+	require.NoError(t, ss.GetMaster().Select(&rows,
+		"SELECT Version, Name FROM db_migrations WHERE Version IN (142, 143, 144, 156, 157, 158) ORDER BY Version"))
+	assert.Equal(t, []row{
+		{156, "add_schemeid_to_roles"},
+		{157, "backfill_roles_schemeid"},
+		{158, "add_roles_schemeid_index"},
+	}, rows)
+}
+
+func TestTableExists(t *testing.T) {
+	if enableFullyParallelTests {
+		t.Parallel()
+	}
+
+	logger := mlog.CreateConsoleTestLogger(t)
+	settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+	if err != nil {
+		t.Skip(err)
+	}
+	ss, err := New(*settings, logger, nil)
+	require.NoError(t, err)
+
+	t.Run("returns true for an existing table", func(t *testing.T) {
+		exists, err := ss.tableExists("systems")
+		require.NoError(t, err)
+		assert.True(t, exists)
+	})
+
+	t.Run("returns true for another existing table", func(t *testing.T) {
+		exists, err := ss.tableExists("db_migrations")
+		require.NoError(t, err)
+		assert.True(t, exists)
+	})
+
+	t.Run("returns false for a non-existent table", func(t *testing.T) {
+		exists, err := ss.tableExists("this_table_does_not_exist")
+		require.NoError(t, err)
+		assert.False(t, exists)
+	})
+
+	t.Run("returns false for empty table name", func(t *testing.T) {
+		exists, err := ss.tableExists("")
+		require.NoError(t, err)
+		assert.False(t, exists)
+	})
+
+	t.Run("match is case-insensitive on stored table name", func(t *testing.T) {
+		exists, err := ss.tableExists("systems")
+		require.NoError(t, err)
+		assert.True(t, exists)
+
+		exists, err = ss.tableExists("SYSTEMS")
+		require.NoError(t, err)
+		assert.True(t, exists)
+	})
+}
+
+func TestPreMigration(t *testing.T) {
+	if enableFullyParallelTests {
+		t.Parallel()
+	}
+
+	logger := mlog.CreateConsoleTestLogger(t)
+
+	t.Run("no-op when Systems table does not exist", func(t *testing.T) {
+		settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+		if err != nil {
+			t.Skip(err)
+		}
+		ss, err := New(*settings, logger, nil, SkipMigrations())
+		require.NoError(t, err)
+
+		require.NoError(t, ss.preMigration())
+	})
+
+	t.Run("idempotent across repeated runs", func(t *testing.T) {
+		settings, err := makeSqlSettings(model.DatabaseDriverPostgres)
+		if err != nil {
+			t.Skip(err)
+		}
+		ss, err := New(*settings, logger, nil)
+		require.NoError(t, err)
+
+		// First run on a clean DB: gate passes, handler is a no-op, marker is set.
+		require.NoError(t, ss.preMigration())
+		done, err := ss.isPreMigrationComplete("renumber_roles_schemeid_migrations")
+		require.NoError(t, err)
+		assert.True(t, done)
+
+		// second run should produce no errors
+		require.NoError(t, ss.preMigration())
+	})
+}

server/cmd/mattermost/commands/db.go

@@ -216,6 +216,12 @@ func migrateCmdF(command *cobra.Command, args []string) error {
 				strings.Repeat("*", 80), fileName+".json", strings.Repeat("*", 80)))
 	}
 
+	if !dryRun {
+		if preMigrationError := migrator.PreMigrate(); preMigrationError != nil {
+			return errors.Wrap(preMigrationError, "failed to run pre-migrations")
+		}
+	}
+
 	err = migrator.MigrateWithPlan(plan, dryRun)
 	if err != nil {
 		return errors.Wrap(err, "failed to migrate with the plan")