feat(be): boltdb to sql migration tool #3575
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a BoltDB to SQL database migration tool to help users migrate their data from BoltDB to SQL-based databases (MySQL, PostgreSQL, SQLite). The migration leverages existing backup/restore functionality from the project service to handle project data migration.
Changes:
- Added a new
db_migrationpackage with migration logic that iterates through users and projects - Extended the database interface with an
ImportUsermethod to preserve existing password hashes during migration - Enhanced the
migrateCLI command with a--from-boltdbflag to trigger BoltDB-to-SQL migrations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| db_migration/migration.go | Core migration logic that orchestrates user and project migration using backup/restore mechanism |
| db/Store.go | Adds ImportUser method to the UserManager interface for importing users with existing password hashes |
| db/sql/user.go | Implements ImportUser for SQL databases to insert users without re-hashing passwords |
| db/bolt/user.go | Adds stub ImportUser implementation that returns "unsupported operation" error |
| cli/cmd/migrate.go | Extends migrate command with --from-boltdb flag and migrateBoltDb function to handle the migration flow |
| func migrateBoltDb(boltDbPath string) { | ||
|
|
||
| boltCfg := util.DbConfig{ | ||
| Dialect: util.DbDriverBolt, | ||
| Hostname: boltDbPath, | ||
| } | ||
|
|
||
| if boltCfg.Dialect != util.DbDriverBolt { | ||
| fmt.Printf("Error: Source database must be BoltDB (dialect: %s)\n", boltCfg.Dialect) | ||
| return | ||
| } | ||
|
|
||
| _, err := os.Stat(boltDbPath) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| fmt.Println("File does not exist") | ||
| } else { | ||
| fmt.Printf("Error: %v\n", err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| boltStore := bolt.CreateBoltDB() | ||
| boltStore.Filename = boltDbPath | ||
| boltStore.Connect("migrate") | ||
|
|
||
| defer boltStore.Close("migrate") | ||
|
|
||
| util.ConfigInit(persistentFlags.configPath, persistentFlags.noConfig) | ||
|
|
||
| dialect, err := util.Config.GetDialect() | ||
| if err != nil { | ||
| fmt.Printf("Error reading SQL DB config: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| if dialect == util.DbDriverBolt { | ||
| fmt.Println("Error: Destination database must be a SQL database") | ||
| return | ||
| } | ||
|
|
||
| sqlStore := factory.CreateStore() | ||
| sqlStore.Connect("migrate") | ||
|
|
||
| // 3. Connect and migrate | ||
| fmt.Println("Starting migration...") | ||
| err = db_migration.Migrate(boltStore, sqlStore) | ||
| if err != nil { | ||
| fmt.Printf("Migration failed: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| defer sqlStore.Close("migrate") | ||
|
|
||
| fmt.Println("Migration finished successfully.") | ||
| } |
There was a problem hiding this comment.
After error conditions in the migrateBoltDb function, the function returns early without calling os.Exit(1). This is inconsistent with other CLI command error handling patterns in the codebase (see project_import.go lines 49, 58, 89, 106). Early returns mean the main migrate command will continue executing and print success messages even though migration failed.
| fmt.Println("Migrating users...") | ||
| if err := m.migrateUsers(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migrating projects...") | ||
| if err := m.migrateProjects(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migration completed successfully.") | ||
| return nil |
There was a problem hiding this comment.
Progress reporting uses simple fmt.Println statements which don't provide any indication of progress for large migrations. Consider adding progress indicators showing how many users or projects have been migrated out of the total count, especially since migrations could take significant time for large databases. For example: "Migrating users... (1/10)", "Migrating project X for user Y..."
| func (m *Migrator) Migrate(oldStore, newStore db.Store) error { | ||
| m.oldStore = oldStore | ||
| m.newStore = newStore | ||
|
|
||
| m.userIDs = make(map[int]db.User) | ||
|
|
||
| //m.integrationIDs = make(map[int]int) | ||
|
|
||
| fmt.Println("Migrating users...") | ||
| if err := m.migrateUsers(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migrating projects...") | ||
| if err := m.migrateProjects(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migration completed successfully.") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The migration does not implement any transaction management or rollback mechanism. If migration fails partway through (e.g., after migrating some users but failing on a project), the destination database will be left in an inconsistent state with partial data. Consider implementing atomic migration with transaction support or at minimum providing a clear mechanism to clean up failed migrations and retry.
| package db_migration | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/semaphoreui/semaphore/db" | ||
| projectService "github.com/semaphoreui/semaphore/services/project" | ||
| ) | ||
|
|
||
| type Migrator struct { | ||
| oldStore db.Store | ||
| newStore db.Store | ||
|
|
||
| userIDs map[int]db.User | ||
| } | ||
|
|
||
| func Migrate(oldStore, newStore db.Store) error { | ||
| migrator := &Migrator{} | ||
| return migrator.Migrate(oldStore, newStore) | ||
| } | ||
|
|
||
| func (m *Migrator) Migrate(oldStore, newStore db.Store) error { | ||
| m.oldStore = oldStore | ||
| m.newStore = newStore | ||
|
|
||
| m.userIDs = make(map[int]db.User) | ||
|
|
||
| //m.integrationIDs = make(map[int]int) | ||
|
|
||
| fmt.Println("Migrating users...") | ||
| if err := m.migrateUsers(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migrating projects...") | ||
| if err := m.migrateProjects(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migration completed successfully.") | ||
| return nil | ||
| } | ||
|
|
||
| func (m *Migrator) migrateUsers() error { | ||
| users, err := m.oldStore.GetUsers(db.RetrieveQueryParams{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, user := range users { | ||
| oldID := user.ID | ||
| user.ID = 0 | ||
| newUser, err := m.newStore.ImportUser(db.UserWithPwd{Pwd: user.Password, User: user}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| m.userIDs[oldID] = newUser | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (m *Migrator) migrateProjects() error { | ||
| users, err := m.oldStore.GetUsers(db.RetrieveQueryParams{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, user := range users { | ||
| newUser := m.userIDs[user.ID] | ||
| projects, err := m.oldStore.GetProjects(user.ID) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, project := range projects { | ||
|
|
||
| err = m.migrateProject(&newUser, &project) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (m *Migrator) migrateProject(user *db.User, project *db.Project) error { | ||
| backup, err := projectService.GetBackup(project.ID, m.oldStore) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| _, err = backup.Restore(*user, m.newStore) | ||
| return err | ||
| } |
There was a problem hiding this comment.
The new db_migration package and ImportUser function lack test coverage. Given that this package handles critical data migration from BoltDB to SQL databases, comprehensive tests are essential to verify data integrity, error handling, and edge cases. Consider adding tests similar to those found in services/project/backup_test.go which test backup and restore functionality.
| func migrateBoltDb(boltDbPath string) { | ||
|
|
||
| boltCfg := util.DbConfig{ | ||
| Dialect: util.DbDriverBolt, | ||
| Hostname: boltDbPath, | ||
| } | ||
|
|
||
| if boltCfg.Dialect != util.DbDriverBolt { | ||
| fmt.Printf("Error: Source database must be BoltDB (dialect: %s)\n", boltCfg.Dialect) | ||
| return | ||
| } | ||
|
|
||
| _, err := os.Stat(boltDbPath) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| fmt.Println("File does not exist") | ||
| } else { | ||
| fmt.Printf("Error: %v\n", err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| boltStore := bolt.CreateBoltDB() | ||
| boltStore.Filename = boltDbPath | ||
| boltStore.Connect("migrate") | ||
|
|
||
| defer boltStore.Close("migrate") | ||
|
|
||
| util.ConfigInit(persistentFlags.configPath, persistentFlags.noConfig) | ||
|
|
||
| dialect, err := util.Config.GetDialect() | ||
| if err != nil { | ||
| fmt.Printf("Error reading SQL DB config: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| if dialect == util.DbDriverBolt { | ||
| fmt.Println("Error: Destination database must be a SQL database") | ||
| return | ||
| } | ||
|
|
||
| sqlStore := factory.CreateStore() | ||
| sqlStore.Connect("migrate") | ||
|
|
||
| // 3. Connect and migrate | ||
| fmt.Println("Starting migration...") | ||
| err = db_migration.Migrate(boltStore, sqlStore) | ||
| if err != nil { | ||
| fmt.Printf("Migration failed: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| defer sqlStore.Close("migrate") | ||
|
|
||
| fmt.Println("Migration finished successfully.") | ||
| } |
There was a problem hiding this comment.
The migrateBoltDb function and the overall migration process lack documentation. Add comments explaining the function's purpose, parameters, expected behavior, and importantly, prerequisites (e.g., destination database should be empty, source database should be closed, etc.) and potential error scenarios. This is critical for a tool that performs irreversible data operations.
| func (m *Migrator) Migrate(oldStore, newStore db.Store) error { | ||
| m.oldStore = oldStore | ||
| m.newStore = newStore | ||
|
|
||
| m.userIDs = make(map[int]db.User) | ||
|
|
||
| //m.integrationIDs = make(map[int]int) | ||
|
|
||
| fmt.Println("Migrating users...") | ||
| if err := m.migrateUsers(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migrating projects...") | ||
| if err := m.migrateProjects(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Println("Migration completed successfully.") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The migration does not validate that the source database has data before starting or that the destination database is empty. Consider adding validation checks to confirm the source database has users/projects to migrate and warning the user if the destination database already contains data, to prevent accidental data corruption or duplication.
| boltCfg := util.DbConfig{ | ||
| Dialect: util.DbDriverBolt, | ||
| Hostname: boltDbPath, | ||
| } | ||
|
|
||
| if boltCfg.Dialect != util.DbDriverBolt { | ||
| fmt.Printf("Error: Source database must be BoltDB (dialect: %s)\n", boltCfg.Dialect) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
This validation check is redundant and will always be true. The boltCfg.Dialect is hardcoded to util.DbDriverBolt on line 61, so it will never not equal util.DbDriverBolt. This check should be removed or the validation logic should be reconsidered.
| boltCfg := util.DbConfig{ | |
| Dialect: util.DbDriverBolt, | |
| Hostname: boltDbPath, | |
| } | |
| if boltCfg.Dialect != util.DbDriverBolt { | |
| fmt.Printf("Error: Source database must be BoltDB (dialect: %s)\n", boltCfg.Dialect) | |
| return | |
| } |
| func (m *Migrator) migrateProjects() error { | ||
| users, err := m.oldStore.GetUsers(db.RetrieveQueryParams{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, user := range users { | ||
| newUser := m.userIDs[user.ID] | ||
| projects, err := m.oldStore.GetProjects(user.ID) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, project := range projects { | ||
|
|
||
| err = m.migrateProject(&newUser, &project) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The migrateProjects function iterates over all users from the old database, but this could include users that don't have any projects or users that failed to migrate. A more efficient approach would be to iterate over the migrated users from m.userIDs map. Additionally, if a user exists in the old database but doesn't have a corresponding entry in m.userIDs (e.g., due to migration failure), this will access an uninitialized map value, which defaults to an empty db.User struct with ID=0, potentially causing issues with project restoration.
| func (d *SqlDb) ImportUser(user db.UserWithPwd) (newUser db.User, err error) { | ||
| err = db.ValidateUser(user.User) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| user.Created = db.GetParsedTime(tz.Now()) | ||
|
|
||
| err = d.Sql().Insert(&user.User) | ||
|
|
||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| newUser = user.User | ||
| return | ||
| } |
There was a problem hiding this comment.
The ImportUser function does not use the Pwd field from UserWithPwd. It only inserts user.User which already contains the Password field. This is confusing and inconsistent with the CreateUser function (lines 32-56) which explicitly uses the Pwd field to hash the password. For migration purposes where you want to preserve the existing password hash, the function should document this behavior or explicitly use the Pwd field to set user.Password before insertion to make the intention clear.
| _, err := os.Stat(boltDbPath) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| fmt.Println("File does not exist") | ||
| } else { | ||
| fmt.Printf("Error: %v\n", err) | ||
| } | ||
| return |
There was a problem hiding this comment.
Error messages should be more descriptive and consistent with error handling patterns in the codebase. These simple fmt.Println and fmt.Printf statements don't provide structured logging or proper exit codes. Consider using log.Error or similar structured logging, and ensuring the function returns an error status or calls os.Exit(1) after logging errors.
BoltDB to SQL db migration tool based on export import functions