migration: better error message on optout attempt (#3404)

better error message on optout attempt

Author: EgeCaner

cmd/juno/juno.go

@@ -101,7 +101,7 @@ const (
 	dbCompactionConcurrencyF            = "db-compaction-concurrency"
 	dbMemtableSizeF                     = "db-memtable-size"
 	dbCompressionF                      = "db-compression"
-	transactionCombinedLayoutF          = "transaction-combined-layout"
+	transactionCombinedLayoutF          = node.FlagTransactionCombinedLayout
 
 	defaultConfig                             = ""
 	defaultHost                               = "localhost"

migration/registry.go

@@ -6,17 +6,19 @@ const maxMigrations = 64
 
 // Registry holds all migrations and builds the target version.
 type Registry struct {
-	entries       [maxMigrations]Migration
-	targetVersion SchemaVersion
-	nextIndex     uint8 // Next available index for registering a migration
+	entries                [maxMigrations]Migration
+	targetVersion          SchemaVersion
+	nextIndex              uint8 // Next available index for registering a migration
+	optionalMigrationFlags [maxMigrations]string
 }
 
 // NewRegistry creates a new migration registry.
 func NewRegistry() *Registry {
 	return &Registry{
-		entries:       [maxMigrations]Migration{},
-		targetVersion: 0,
-		nextIndex:     0,
+		entries:                [maxMigrations]Migration{},
+		targetVersion:          0,
+		nextIndex:              0,
+		optionalMigrationFlags: [maxMigrations]string{},
 	}
 }
 
@@ -35,12 +37,14 @@ func (r *Registry) With(m Migration) *Registry {
 
 // WithOptional adds an optional migration to the registry.
 // If enabled is true, the migration is included in the target version.
+// name is the flag/name used to enable this migration.
 // Returns the registry for method chaining.
-func (r *Registry) WithOptional(m Migration, enabled bool) *Registry {
+func (r *Registry) WithOptional(m Migration, enabled bool, name string) *Registry {
 	if r.nextIndex >= maxMigrations {
 		panic("exceeded maximum number of 64 migrations")
 	}
 	r.entries[r.nextIndex] = m
+	r.optionalMigrationFlags[r.nextIndex] = name
 
 	if enabled {
 		r.targetVersion.Set(r.nextIndex)
@@ -64,3 +68,10 @@ func (r *Registry) Entries() []Migration {
 func (r *Registry) Count() int {
 	return int(r.nextIndex)
 }
+
+// OptionalMigrationFlags returns the array of optional migration flags.
+// The array is indexed by migration index, and the value is the flag/name
+// used to enable this migration.
+func (r *Registry) OptionalMigrationFlags() []string {
+	return r.optionalMigrationFlags[:r.nextIndex]
+}

migration/registry_test.go

@@ -90,15 +90,15 @@ func TestRegistry_WithOptional(t *testing.T) {
 
 	t.Run("Enabled false", func(t *testing.T) {
 		r := migration.NewRegistry()
-		result := r.WithOptional(m, false)
+		result := r.WithOptional(m, false, "test-migration")
 		require.Same(t, r, result)
 		require.Equal(t, 1, r.Count())
 		require.Equal(t, migration.SchemaVersion(0), r.TargetVersion())
 		require.Equal(t, m, r.Entries()[0])
 	})
 	t.Run("Enabled true", func(t *testing.T) {
 		r := migration.NewRegistry()
-		result := r.WithOptional(m, true)
+		result := r.WithOptional(m, true, "test-migration")
 		require.Same(t, r, result)
 		require.Equal(t, 1, r.Count())
 		require.Equal(t, migration.SchemaVersion(1), r.TargetVersion())
@@ -118,7 +118,7 @@ func TestRegistry_WithOptional_PanicOnMaxMigrations(t *testing.T) {
 	require.Panics(
 		t,
 		func() {
-			r.WithOptional(m, true)
+			r.WithOptional(m, true, "test-migration")
 		},
 		"expected panic when exceeding 64 migrations",
 	)
@@ -131,8 +131,8 @@ func TestRegistry_Entries(t *testing.T) {
 	m2 := &mockMigration{}
 
 	r.With(m0)
-	r.WithOptional(m1, false)
-	r.WithOptional(m2, false)
+	r.WithOptional(m1, false, "migration-1")
+	r.WithOptional(m2, false, "migration-2")
 
 	entries := r.Entries()
 	require.Equal(t, 3, len(entries))
@@ -148,11 +148,28 @@ func TestRegistry_TargetVersion(t *testing.T) {
 	m2 := &mockMigration{}
 
 	r.With(m0)
-	r.WithOptional(m1, true)
-	r.WithOptional(m2, false)
+	r.WithOptional(m1, true, "migration-1")
+	r.WithOptional(m2, false, "migration-2")
 
 	target := r.TargetVersion()
 	require.True(t, target.Has(0))
 	require.True(t, target.Has(1))
 	require.False(t, target.Has(2))
 }
+
+func TestRegistry_OptionalMigrationFlags(t *testing.T) {
+	r := migration.NewRegistry()
+	m0 := &mockMigration{}
+	m1 := &mockMigration{}
+	m2 := &mockMigration{}
+
+	r.With(m0)
+	r.WithOptional(m1, true, "migration-1")
+	r.WithOptional(m2, false, "migration-2")
+
+	flags := r.OptionalMigrationFlags()
+	require.Equal(t, 3, len(flags))
+	require.Equal(t, "", flags[0])
+	require.Equal(t, "migration-1", flags[1])
+	require.Equal(t, "migration-2", flags[2])
+}

migration/runner.go

@@ -61,28 +61,16 @@ type MigrationRunner struct {
 // NewRunner creates a migration runner that executes pending migrations.
 //
 // Parameters:
-//   - entries: All registered migrations
-//   - targetVersion: End state to reach after running all migrations
+//   - registry: Migration registry containing all migrations
 //   - database: Database store
 //   - network: Network configuration
 //   - log: Logger for migration progress
 func NewRunner(
-	entries []Migration,
-	targetVersion SchemaVersion,
+	registry *Registry,
 	database db.KeyValueStore,
 	network *utils.Network,
 	log utils.StructuredLogger,
 ) (*MigrationRunner, error) {
-	// Validate that we have entries for all migrations up to the highest index in targetVersion.
-	// Having more entries is acceptable (optional migrations may be registered but not enabled).
-	// If targetVersion is empty (no migrations), HighestBit() returns -1
-	expectedLen := targetVersion.HighestBit() + 1
-	if len(entries) < expectedLen {
-		return nil, fmt.Errorf("insufficient entries: have %d, need at least %d",
-			len(entries),
-			expectedLen,
-		)
-	}
 	metadata, err := GetSchemaMetadata(database)
 	if err != nil && !errors.Is(err, db.ErrKeyNotFound) {
 		return nil, err
@@ -95,18 +83,25 @@ func NewRunner(
 		}
 	}
 
+	targetVersion := registry.TargetVersion()
 	// Validate: check if database was migrated with a newer version of Juno (version downgrade)
-	if err := validateNoVersionDowngrade(metadata.CurrentVersion, targetVersion); err != nil {
+	err = validateNoVersionDowngrade(metadata.CurrentVersion, targetVersion)
+	if err != nil {
 		return nil, err
 	}
 
 	// Validate: check if any previously opted-in migrations are now missing (opt-out attempt)
-	if err := validateNoOptOut(targetVersion, metadata.LastTargetVersion); err != nil {
+	err = validateNoOptOut(
+		targetVersion,
+		metadata.LastTargetVersion,
+		registry.OptionalMigrationFlags(),
+	)
+	if err != nil {
 		return nil, err
 	}
 
 	return &MigrationRunner{
-		entries:       entries,
+		entries:       registry.Entries(),
 		targetVersion: targetVersion,
 		metadata:      metadata,
 		database:      database,
@@ -212,10 +207,30 @@ func validateNoVersionDowngrade(current, target SchemaVersion) error {
 // validateNoOptOut checks if any previously opted-in migrations (in lastTargetVersion)
 // are missing from the target version (opt-out attempt).
 // Returns an error if opt-out attempts are detected, nil otherwise.
-func validateNoOptOut(target, lastTargetVersion SchemaVersion) error {
+func validateNoOptOut(
+	target,
+	lastTargetVersion SchemaVersion,
+	optionalMigrationFlags []string,
+) error {
 	optOutAttempts := lastTargetVersion.Difference(target)
-	if optOutAttempts != 0 {
-		return fmt.Errorf("cannot opt out of previously enabled migrations: %s", optOutAttempts.String())
+	if optOutAttempts == 0 {
+		return nil
+	}
+	// Build a list of migration flags that cannot be opted out of
+	flagList := make([]string, optOutAttempts.Len())
+	i := 0
+	for idx := range optOutAttempts.Iter() {
+		if flag := optionalMigrationFlags[idx]; flag != "" {
+			flagList[i] = fmt.Sprintf("--%s", flag)
+		} else {
+			// Fallback to index if flag is not available
+			flagList[i] = fmt.Sprintf("--migration-%d", idx)
+		}
+		i++
 	}
-	return nil
+
+	return fmt.Errorf(
+		"cannot opt out of previously enabled migrations: %v (set these flags to enable)",
+		flagList,
+	)
 }