feat: [#280] Optimize migrate:reset command #702
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
contracts/database/migration/migrator.go (1)
13-14: Enhance the method documentation for clarity.The comment could be more descriptive about what "reset" means in this context and what users should expect when calling this method.
Consider updating the comment to be more descriptive:
- // Reset the migrations. + // Reset rolls back all migrations and then runs them again, effectively resetting the database to a fresh state while preserving the schema.database/service_provider.go (1)
98-98: LGTM! Architectural improvement in dependency injection.The change from passing
configto passingmigratortoNewMigrateResetCommandis a good architectural improvement. It:
- Follows the Dependency Inversion Principle by depending on the
Migratorinterface rather than concrete configuration- Provides better encapsulation as the command now only has access to migration-specific functionality
- Maintains consistency with how other migration commands are initialized
database/migration/sql_migrator.go (1)
85-93: Add method documentation and enhance error handling.The Reset method implementation could benefit from the following improvements:
- Add method documentation to describe its purpose and behavior
- Consider handling additional common migration errors
- Align error handling with other migration methods
Apply this diff to improve the implementation:
+// Reset reverts all migrations by calling Down on the migrator. +// It returns an error if the reset operation fails. func (r *SqlMigrator) Reset() error { - if err := r.migrator.Down(); err != nil && !errors.Is(err, migrate.ErrNoChange) { + if err := r.migrator.Down(); err != nil { + if errors.Is(err, migrate.ErrNoChange) { + color.Warningln("No migrations to reset") + return nil + } + if errors.Is(err, migrate.ErrNilVersion) { + color.Warningln("No migrations found") + return nil + } return errors.MigrationResetFailed.Args(err) } color.Successln("Migration reset success") return nil }database/migration/default_migrator.go (1)
66-73: LGTM! The Reset implementation is clean and efficient.The implementation correctly:
- Retrieves all ran migrations
- Handles errors appropriately
- Uses the existing Rollback functionality with batch 0 to ensure all migrations are rolled back
Consider adding a progress indicator or logging for large-scale migrations, as resetting all migrations could be a time-consuming operation. This would improve user experience by providing visibility into the reset progress.
database/migration/default_migrator_test.go (2)
327-371: Enhance test coverage for Reset functionality.The current test cases only cover basic scenarios. Consider adding the following test cases to ensure comprehensive coverage:
- Successful reset with multiple migrations
- Error during rollback operation
- Error in repository operations (e.g., Delete)
Here's a suggested implementation:
func (s *DefaultMigratorSuite) TestReset() { tests := []struct { name string setup func() expectErr string }{ + { + name: "Reset with multiple migrations", + setup: func() { + previousConnection := "postgres" + testMigration1 := NewTestMigration(s.mockSchema) + testMigration2 := NewTestConnectionMigration(s.mockSchema) + + s.mockRepository.EXPECT().GetRan().Return([]string{ + testMigration1.Signature(), + testMigration2.Signature(), + }, nil).Once() + + s.mockSchema.EXPECT().Migrations().Return([]contractsschema.Migration{ + testMigration1, + testMigration2, + }) + + s.mockRepository.EXPECT().GetMigrationsByStep(1).Return([]migration.File{ + {Migration: testMigration1.Signature()}, + {Migration: testMigration2.Signature()}, + }, nil).Once() + + mockOrm := mocksorm.NewOrm(s.T()) + s.mockRunDown(mockOrm, previousConnection, testMigration1.Signature(), "users", nil) + s.mockRunDown(mockOrm, previousConnection, testMigration2.Signature(), "agents", nil) + }, + }, + { + name: "Error during rollback", + setup: func() { + previousConnection := "postgres" + testMigration := NewTestMigration(s.mockSchema) + + s.mockRepository.EXPECT().GetRan().Return([]string{testMigration.Signature()}, nil).Once() + s.mockSchema.EXPECT().Migrations().Return([]contractsschema.Migration{testMigration}) + s.mockRepository.EXPECT().GetMigrationsByStep(1).Return([]migration.File{{Migration: testMigration.Signature()}}, nil).Once() + + mockOrm := mocksorm.NewOrm(s.T()) + s.mockRunDown(mockOrm, previousConnection, testMigration.Signature(), "users", assert.AnError) + }, + expectErr: assert.AnError.Error(), + },
Line range hint
1-1024: Consider adding documentation for test helper methods.The test file is well-structured, but adding documentation for helper methods like
mockRunDownandmockRunUpwould improve maintainability and make it easier for other developers to understand the test setup.Add documentation for helper methods:
+// mockRunDown sets up mock expectations for rolling back a migration +// Parameters: +// - mockOrm: mock orm instance +// - previousConnection: connection name to restore after migration +// - migrationSignature: signature of the migration being rolled back +// - table: name of the table being dropped +// - err: error to return from repository.Delete, or nil for success func (s *DefaultMigratorSuite) mockRunDown( mockOrm *mocksorm.Orm, previousConnection, migrationSignature, table string, err error, ) {+// mockRunUp sets up mock expectations for running a migration +// Parameters: +// - mockOrm: mock orm instance +// - previousConnection: connection name to restore after migration +// - migrationSignature: signature of the migration being run +// - table: name of the table being created +// - batch: batch number for the migration +// - err: error to return from repository.Log, or nil for success func (s *DefaultMigratorSuite) mockRunUp( mockOrm *mocksorm.Orm, previousConnection, migrationSignature, table string, batch int, err error, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mocks/database/migration/Migrator.gois excluded by!mocks/**
📒 Files selected for processing (7)
- contracts/database/migration/migrator.go (1 hunks)
- database/console/migration/migrate_reset_command.go (1 hunks)
- database/console/migration/migrate_reset_command_test.go (0 hunks)
- database/migration/default_migrator.go (1 hunks)
- database/migration/default_migrator_test.go (1 hunks)
- database/migration/sql_migrator.go (1 hunks)
- database/service_provider.go (1 hunks)
💤 Files with no reviewable changes (1)
- database/console/migration/migrate_reset_command_test.go
🔇 Additional comments (7)
contracts/database/migration/migrator.go (1)
13-14: Verify implementation in all migrators.The interface addition looks good, but let's verify that all migrator implementations have properly implemented this new method.
✅ Verification successful
Let me try a different search pattern to find the Reset implementations.
Reset() method is properly implemented in all migrators
The verification shows that the Reset() method is correctly implemented in:
- SqlMigrator (database/migration/sql_migrator.go)
- DefaultMigrator (database/migration/default_migrator.go)
- A mock implementation for testing (mocks/database/migration/Migrator.go)
These are all the migrator types found in the codebase, and each has a proper Reset() implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Reset() implementation in all migrators # Expected: Find Reset() implementation in DefaultMigrator and SqlMigrator echo "Checking Reset() implementation in migrators:" ast-grep --pattern 'type $_Migrator struct { $$$ } $$$ func (m *$_Migrator) Reset() error { $$$ }'Length of output: 207
Script:
#!/bin/bash # Search for Reset method implementations more broadly echo "Searching for Reset() implementations:" ast-grep --pattern 'func ($_ *$_) Reset() error { $$$ }' # Also search for struct definitions to identify migrator types echo -e "\nSearching for migrator types:" rg "type.*Migrator struct" -A 2Length of output: 2768
database/console/migration/migrate_reset_command.go (3)
6-6: LGTM: Import added for migration contract.The new import is correctly added to support the migrator interface dependency.
9-17: LGTM: Improved dependency injection with migrator interface.The change from using
config.Configtomigration.Migratoris a good architectural improvement that:
- Reduces coupling by depending on an interface rather than concrete config
- Better follows interface segregation principle
- Makes the command's dependencies more explicit
20-34: LGTM: Improved receiver naming.The change from
receivertorfollows Go's idiomatic receiver naming conventions.database/service_provider.go (1)
Line range hint
98-104: Verify command registration order.The migration commands seem to follow a logical order (make → migrate → rollback → reset → refresh → fresh → status). However, let's verify this is consistent with the framework's documentation and other similar frameworks.
✅ Verification successful
Command registration order is logically structured
The current command registration order follows a logical and intuitive sequence:
Migration-related commands are grouped together in a natural flow:
MigrateMake(create)Migrate(run)MigrateRollback(undo)MigrateReset(full reset)MigrateRefresh(reset + migrate)MigrateFresh(drop + migrate)MigrateStatus(check state)Followed by related make commands and database utilities:
- Model and Observer makers
- Seed-related commands
- Factory maker
- Database wipe
This organization is clear, consistent, and follows a natural progression from basic operations to more specialized ones.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the command registration order matches the documentation # and similar frameworks' patterns # Test 1: Look for command registration documentation rg -A 10 "Available Commands|Command List" README.md docs/ # Test 2: Look for similar patterns in framework code ast-grep --pattern 'artisan.Register([]contractsconsole.Command{$$$})'Length of output: 179
Script:
#!/bin/bash # Let's try a different approach to find command registration patterns # Look for command registration in service provider rg -A 15 "Register.*Command" database/service_provider.go # Look for any documentation or comments about command ordering rg -B 2 "migration.*command" -g "*.go" # Find all migration command definitions to see their natural grouping fd -e go -x rg -l "Migration.*Command"Length of output: 818
database/migration/sql_migrator.go (1)
85-93: 🛠️ Refactor suggestionAlign error handling with other migration methods.
The error handling pattern differs from other methods like
Rollback. Consider standardizing the approach across all migration methods.Let's analyze error handling patterns in other methods:
#!/bin/bash # Search for error handling patterns in migration methods rg -A 3 'if err := r\.migrator\.' database/migration/database/migration/default_migrator.go (1)
66-73: Verify rollback order for Reset operation.While the implementation is correct, please ensure that migrations are rolled back in the reverse order of their execution to maintain referential integrity.
Let's verify the rollback order implementation:
✅ Verification successful
Let me analyze the implementation details further to verify the rollback order.
Rollback order is correctly implemented
The implementation ensures migrations are rolled back in the correct reverse order:
GetMigrationsByStepusesOrderByDesc("batch").OrderByDesc("migration")to get the most recent migrations firstGetMigrationsByBatchusesOrderByDesc("migration")within a specific batchGetLastretrieves migrations from the last batch in descending orderThe original review comment's concern about rollback order is addressed by the existing implementation, which maintains referential integrity by rolling back migrations in reverse chronological order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the rollback order is properly handled in the codebase # Look for order-related code in repository implementation rg -A 5 "GetRan|GetLast|GetMigrationsByStep|GetMigrationsByBatch" --type go # Check for any explicit sorting or ordering logic ast-grep --pattern 'sort.$$$' --lang goLength of output: 27115
Script:
#!/bin/bash # Check the implementation of rollback and related methods rg -A 10 "func.*Rollback" --type go # Look for the actual SQL query implementation rg -A 5 "OrderBy.*migration|OrderByDesc.*migration" --type goLength of output: 16256
📑 Description
Summary by CodeRabbit
New Features
Resetmethod for migration management, allowing users to reset migrations effectively.Bug Fixes
Tests
Resetfunctionality to ensure reliable operation and coverage of edge cases.Documentation
✅ Checks