Migrations validation: duplicate names and commit call (#902)

migrations/migrations.go

@@ -3,6 +3,7 @@ package migrations
 import (
 	"bytes"
 	"errors"
+	"fmt"
 
 	"github.com/ledgerwatch/turbo-geth/common"
 	"github.com/ledgerwatch/turbo-geth/common/dbutils"
@@ -63,6 +64,11 @@ type Migration struct {
 	Up   func(db ethdb.Database, dataDir string, OnLoadCommit etl.LoadCommitHandler) error
 }
 
+var (
+	ErrMigrationNonUniqueName   = fmt.Errorf("please provide unique migration name")
+	ErrMigrationCommitNotCalled = fmt.Errorf("migraion commit function was not called")
+)
+
 func NewMigrator() *Migrator {
 	return &Migrator{
 		Migrations: migrations,
@@ -96,16 +102,31 @@ func (m *Migrator) Apply(db ethdb.Database, datadir string) error {
 		return err
 	}
 
+	// migration names must be unique, protection against people's mistake
+	uniqueNameCheck := map[string]bool{}
+	for i := range m.Migrations {
+		_, ok := uniqueNameCheck[m.Migrations[i].Name]
+		if ok {
+			return fmt.Errorf("%w, duplicate: %s", ErrMigrationNonUniqueName, m.Migrations[i].Name)
+		}
+		uniqueNameCheck[m.Migrations[i].Name] = true
+	}
+
 	for i := range m.Migrations {
 		v := m.Migrations[i]
 		if _, ok := applied[v.Name]; ok {
 			continue
 		}
+
+		commitFuncCalled := false // commit function must be called if no error, protection against people's mistake
+
 		log.Info("Apply migration", "name", v.Name)
 		if err := v.Up(db, datadir, func(putter ethdb.Putter, key []byte, isDone bool) error {
 			if !isDone {
 				return nil // don't save partial progress
 			}
+			commitFuncCalled = true
+
 			stagesProgress, err := MarshalMigrationPayload(db)
 			if err != nil {
 				return err
@@ -119,6 +140,9 @@ func (m *Migrator) Apply(db ethdb.Database, datadir string) error {
 			return err
 		}
 
+		if !commitFuncCalled {
+			return ErrMigrationCommitNotCalled
+		}
 		log.Info("Applied migration", "name", v.Name)
 	}
 	return nil

migrations/migrations_test.go

 package migrations
 
 import (
+	"errors"
 	"github.com/ledgerwatch/turbo-geth/eth/stagedsync/stages"
 	"testing"
 
@@ -152,3 +153,49 @@ func TestMarshalStages(t *testing.T) {
 	require.True(ok)
 	require.NotNil(v)
 }
+
+func TestValidation(t *testing.T) {
+	require, db := require.New(t), ethdb.NewMemDatabase()
+	migrations = []Migration{
+		{
+			Name: "repeated_name",
+			Up: func(db ethdb.Database, datadir string, OnLoadCommit etl.LoadCommitHandler) error {
+				return OnLoadCommit(db, nil, true)
+			},
+		},
+		{
+			Name: "repeated_name",
+			Up: func(db ethdb.Database, datadir string, OnLoadCommit etl.LoadCommitHandler) error {
+				return OnLoadCommit(db, nil, true)
+			},
+		},
+	}
+	migrator := NewMigrator()
+	migrator.Migrations = migrations
+	err := migrator.Apply(db, "")
+	require.True(errors.Is(err, ErrMigrationNonUniqueName))
+
+	applied, err := AppliedMigrations(db, false)
+	require.NoError(err)
+	require.Equal(0, len(applied))
+}
+
+func TestCommitCallRequired(t *testing.T) {
+	require, db := require.New(t), ethdb.NewMemDatabase()
+	migrations = []Migration{
+		{
+			Name: "one",
+			Up: func(db ethdb.Database, datadir string, OnLoadCommit etl.LoadCommitHandler) error {
+				return nil // don't call OnLoadCommit
+			},
+		},
+	}
+	migrator := NewMigrator()
+	migrator.Migrations = migrations
+	err := migrator.Apply(db, "")
+	require.True(errors.Is(err, ErrMigrationCommitNotCalled))
+
+	applied, err := AppliedMigrations(db, false)
+	require.NoError(err)
+	require.Equal(0, len(applied))
+}