feat: [#280] Optimize migrate:rollback command#688
Conversation
6ecabee to
c17ffd3
Compare
WalkthroughThe pull request introduces extensive modifications across multiple files, primarily focusing on the restructuring of data models, migration functionalities, and ORM capabilities. Key changes include the enhancement of the 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #688 +/- ##
==========================================
+ Coverage 69.20% 69.53% +0.32%
==========================================
Files 193 193
Lines 14845 14959 +114
==========================================
+ Hits 10274 10401 +127
+ Misses 3988 3972 -16
- Partials 583 586 +3 ☔ View full report in Codecov by Sentry. |
| // Refresh resets the Orm instance. | ||
| Refresh() | ||
| // SetQuery sets the query builder instance. | ||
| SetQuery(query Query) |
There was a problem hiding this comment.
Currently, this method is used when running the migration up method, the up method will be wrapped in an orm transaction, set tx to the default query of orm, implementing all Sqls in the up method will be executed in a transaction.
| type QueryWithSetContext interface { | ||
| SetContext(ctx context.Context) | ||
| } | ||
|
|
||
| type QueryWithObserver interface { | ||
| Observe(model any, observer Observer) | ||
| } |
There was a problem hiding this comment.
Query doesn't need these two methods directly, because there is facades().Orm().WithContext(), facades().Orm().Query().WithContext() is unnecessary, but we need a way to set context to query, the same with Observe.
| type Model struct { | ||
| ID uint `gorm:"primaryKey" json:"id"` | ||
| Timestamps | ||
| } | ||
|
|
||
| type SoftDeletes struct { | ||
| DeletedAt gormio.DeletedAt `gorm:"column:deleted_at" json:"deleted_at"` | ||
| } | ||
|
|
||
| type Timestamps struct { | ||
| CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at"` | ||
| UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at"` | ||
| } |
There was a problem hiding this comment.
To avoid circular dependencies, orm.Model can be used here directly.
| type Model struct { | ||
| ID uint `gorm:"primaryKey" json:"id"` | ||
| Timestamps | ||
| } | ||
|
|
||
| type SoftDeletes struct { | ||
| DeletedAt gorm.DeletedAt `gorm:"column:deleted_at" json:"deleted_at"` | ||
| } | ||
|
|
||
| type Timestamps struct { | ||
| CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at"` | ||
| UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at"` | ||
| } |
| var Observers = make([]Observer, 0) | ||
|
|
||
| type Observer struct { | ||
| Model any | ||
| Observer contractsorm.Observer | ||
| } |
There was a problem hiding this comment.
Replace by orm.modelToObserver, to avoid circular dependencies.
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (27)
contracts/database/migration/migrator.go (1)
13-14: Enhance method documentation for better clarity.The addition of the
Rollbackmethod aligns well with the PR objective to optimize themigrate:rollbackcommand. However, the documentation could be more descriptive about the parameters.Consider expanding the documentation to clarify the parameters:
- // Rollback the last migration operation. + // Rollback the last migration operation. + // Parameters: + // - step: Number of migrations to roll back (0 means all migrations in the batch) + // - batch: Specific batch number to roll back (0 means the last batch)contracts/database/orm/observer.go (1)
28-31: Consider adding type safety mechanisms for model registrationThe
ModelToObserverstruct provides a clean mapping between models and their observers. However, the use ofanytype for the Model field could lead to runtime type assertion issues.Consider these improvements:
- Add validation methods to ensure model type compatibility at registration time
- Consider using generics to provide compile-time type safety:
type ModelToObserver[T any] struct { Model T Observer Observer }
- Document the expected model type requirements (e.g., must implement specific interfaces)
database/schema/test_utils.go (1)
21-23: Consider adding structured logging for better test diagnostics.The current implementation uses
Errorffor logging, but consider enhancing the logging to include:
- The specific driver being configured
- Any initialization errors
- Configuration details (sanitized)
This would make test failures more debuggable.
-mockLog.EXPECT().Errorf(mock.Anything).Maybe() +mockLog.EXPECT().WithFields(mock.Anything).Return(mockLog).Maybe() +mockLog.EXPECT().Errorf(mock.Anything, mock.Anything).Maybe()database/seeder/seeder.go (1)
Line range hint
21-33: Consider enhancing error handling and warning messages in Register method.While the current implementation works, consider these improvements:
- Return an error instead of just printing warnings for duplicate seeders
- Add more context to the warning message (e.g., file location)
- Consider logging the warning instead of direct printing
Here's a suggested improvement:
func (s *SeederFacade) Register(seeders []seeder.Seeder) { existingSignatures := make(map[string]bool) + existingSignatureDetails := make(map[string]string) for _, seeder := range seeders { signature := seeder.Signature() if existingSignatures[signature] { - color.Red().Printf("Duplicate seeder signature: %s in %T\n", signature, seeder) + color.Red().Printf("Duplicate seeder signature detected:\n"+ + " Signature: %s\n"+ + " New Implementation: %T\n"+ + " Existing Implementation: %s\n", + signature, seeder, existingSignatureDetails[signature]) } else { existingSignatures[signature] = true + existingSignatureDetails[signature] = fmt.Sprintf("%T", seeder) s.Seeders = append(s.Seeders, seeder) } } }database/seeder/seeder_test.go (4)
Line range hint
11-22: Consider adding TearDownTest method for proper cleanup.While the test suite setup is good, adding a TearDownTest method would ensure proper cleanup between tests.
+func (s *SeederTestSuite) TearDownTest() { + s.seederFacade = nil +}
Line range hint
82-82: Remove debug log statement.Debug log statements should not be committed to production code.
- log.Println(s.seederFacade.Called)
Line range hint
91-110: Enhance mock implementations for better test coverage.Consider making the mock seeders more configurable to test various scenarios:
type MockSeeder struct { + shouldError bool + runCount int } func (m *MockSeeder) Run() error { + m.runCount++ + if m.shouldError { + return fmt.Errorf("mock error") + } return nil }
Based on the findings, I can now provide the final response:
Add test coverage for error scenarios in seeder tests
The current test suite lacks coverage for important error scenarios. The mock implementations (
MockSeederandMockSeeder2) always returnnilfor theirRun()method, and there are no tests for:
- Error propagation when
seeder.Run()fails- Duplicate seeder registration scenarios
- Attempting to run non-registered seeders
Specific areas needing test coverage:
- Add a mock seeder that returns an error in its
Run()method to test error propagation inCall()andCallOnce()- Add test cases for registering seeders with duplicate signatures
- Add test cases for calling non-registered seeders
🔗 Analysis chain
Line range hint
24-89: Verify test coverage for error scenarios.While the happy path tests are well covered, consider adding test cases for error scenarios:
- Registration of duplicate seeders
- Running non-registered seeders
- Error handling when seeder.Run() returns an error
Let me check the actual error handling implementation and tests to make a more informed decision.
Let me check the mock implementations to understand how errors are simulated in tests.
Let me try a different approach to find the mock implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any error handling tests in the codebase rg -l "Error|err.*!= nil" --type go "database/seeder"Length of output: 110
Script:
#!/bin/bash # Check the error handling implementation in seeder.go rg -A 5 "Error|err.*!= nil" database/seeder/seeder.go # Check error handling tests rg -A 5 "Error|err.*!= nil" database/seeder/seeder_test.goLength of output: 715
Script:
#!/bin/bash # Look for mock seeder implementations ast-grep --pattern 'type Mock$_Seeder struct { $$$ }' # Also check for Run method implementations in mocks ast-grep --pattern 'func ($_ *Mock$_Seeder) Run() error { $$$ }'Length of output: 126
Script:
#!/bin/bash # Search for mock struct and interface implementations in the test file rg -A 10 "type Mock" database/seeder/seeder_test.go # Search for Run method implementations rg -A 5 "func .* Run\(\) error" database/seeder/seeder_test.goLength of output: 523
database/console/migration/migrate_rollback_command_test.go (1)
24-63: Consider adding more edge cases.The table-driven test structure is excellent, but consider adding test cases for:
- Zero/negative step values
- Very large step values
- Multiple concurrent rollbacks
Would you like me to help generate additional test cases?
database/migration/default_creator_test.go (1)
Line range hint
1-143: Consider enhancing test coverage for rollback scenariosWhile the existing tests cover basic migration creation, considering this PR's focus on optimizing the
migrate:rollbackcommand, it might be beneficial to add test cases that verify the generated migration files handle rollback operations correctly.Consider adding test cases that verify:
- Rollback operations in the Down() method
- Complex schema modifications that need careful rollback handling
- Edge cases in rollback scenarios
Example addition:
{ name: "Create stub with complex rollback", stub: Stubs{}.Create(), signature: "202410131203_add_user_settings", table: "user_settings", expected: `package migrations import "github.com/goravel/framework/contracts/database/schema" type M202410131203AddUserSettings struct { } func (r *M202410131203AddUserSettings) Signature() string { return "202410131203_add_user_settings" } func (r *M202410131203AddUserSettings) Up() error { return facades.Schema().Create("user_settings", func(table schema.Blueprint) { table.BigIncrements("id") table.BigInteger("user_id") table.Json("settings") table.Timestamps() table.Foreign("user_id").References("id").On("users").OnDelete("cascade") }) } func (r *M202410131203AddUserSettings) Down() error { return facades.Schema().DropIfExists("user_settings") } `, table: "user_settings", },database/migration/test_utils.go (1)
10-13: Consider adding index tags for timestamp fieldsWhile the model structure is good, consider adding index tags to match the MySQL schema which defines indexes on
created_atandupdated_atfields.type Agent struct { ID uint `gorm:"primaryKey" json:"id"` Name string - CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at"` - UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at"` + CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at;index:idx_agents_created_at" json:"created_at"` + UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at;index:idx_agents_updated_at" json:"updated_at"` }database/factory/factory.go (4)
Line range hint
11-21: Consider adding validation in the constructor.While the implementation is clean, the constructor could benefit from validating that the
queryparameter is not nil to prevent potential nil pointer dereferences later.func NewFactoryImpl(query ormcontract.Query) *FactoryImpl { + if query == nil { + panic("query cannot be nil") + } return &FactoryImpl{ query: query, } }
Line range hint
23-44: Add validation for count parameter.The Count method should validate that the count is positive to prevent potential issues with zero or negative counts.
func (f *FactoryImpl) Count(count int) ormcontract.Factory { + if count <= 0 { + panic("count must be positive") + } return f.newInstance(map[string]any{"count": count}) }
Line range hint
46-95: Consider breaking down the Make method for better maintainability.The Make method has high cyclomatic complexity. Consider extracting the slice/array and single instance handling into separate methods for better readability and maintainability.
func (f *FactoryImpl) Make(value any, attributes ...map[string]any) error { reflectValue := reflect.Indirect(reflect.ValueOf(value)) switch reflectValue.Kind() { case reflect.Array, reflect.Slice: - count := 1 - if f.count != nil { - count = *f.count - } - for i := 0; i < count; i++ { - elemValue := reflect.New(reflectValue.Type().Elem()).Interface() - attributes, err := f.getRawAttributes(elemValue, attributes...) - if err != nil { - return err - } - if attributes == nil { - return errors.OrmFactoryMissingAttributes.SetModule(errors.ModuleOrm) - } - decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - Squash: true, - Result: elemValue, - }) - if err != nil { - return err - } - if err := decoder.Decode(attributes); err != nil { - return err - } - reflectValue = reflect.Append(reflectValue, reflect.ValueOf(elemValue).Elem()) - } - - reflect.ValueOf(value).Elem().Set(reflectValue) - - return nil + return f.makeSlice(value, reflectValue, attributes...) default: - attributes, err := f.getRawAttributes(value, attributes...) - if err != nil { - return err - } - if attributes == nil { - return errors.OrmFactoryMissingAttributes.SetModule(errors.ModuleOrm) - } - decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - Squash: true, - Result: value, - }) - if err != nil { - return err - } - - return decoder.Decode(attributes) + return f.makeSingle(value, attributes...) } } +func (f *FactoryImpl) makeSlice(value any, reflectValue reflect.Value, attributes ...map[string]any) error { + count := 1 + if f.count != nil { + count = *f.count + } + for i := 0; i < count; i++ { + elemValue := reflect.New(reflectValue.Type().Elem()).Interface() + if err := f.makeSingle(elemValue, attributes...); err != nil { + return err + } + reflectValue = reflect.Append(reflectValue, reflect.ValueOf(elemValue).Elem()) + } + reflect.ValueOf(value).Elem().Set(reflectValue) + return nil +} + +func (f *FactoryImpl) makeSingle(value any, attributes ...map[string]any) error { + attrs, err := f.getRawAttributes(value, attributes...) + if err != nil { + return err + } + if attrs == nil { + return errors.OrmFactoryMissingAttributes.SetModule(errors.ModuleOrm) + } + decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + Squash: true, + Result: value, + }) + if err != nil { + return err + } + return decoder.Decode(attrs) +}
Line range hint
97-127: Add documentation for helper methods.The helper methods would benefit from more detailed documentation explaining their purpose, parameters, and return values.
+// getRawAttributes retrieves the raw attributes for a model by merging the model's factory +// definition with any provided override attributes. It returns an error if the model +// doesn't implement the factory.Model interface. func (f *FactoryImpl) getRawAttributes(value any, attributes ...map[string]any) (map[string]any, error) { +// newInstance creates a new factory instance with the same query and count settings. +// It accepts optional attributes to override the count setting. func (f *FactoryImpl) newInstance(attributes ...map[string]any) ormcontract.Factory {support/docker/docker.go (1)
Line range hint
11-18: Document the performance vs. coverage tradeoff.Consider adding a comment explaining the performance impact and coverage tradeoff of this change. This will help other contributors understand why
TestModelMinimumis now the default.// Define different test model, to improve the local testing speed. // The minimum model only initials one Sqlite and two Postgres, // and the normal model initials one Mysql, two Postgres, one Sqlite and one Sqlserver. +// TestModelMinimum is the default as it provides faster test execution while maintaining +// adequate coverage for core functionality. Use TestModelNormal when testing features +// that require specific database implementations. const ( TestModelMinimum = iota TestModelNormalsupport/database/database_test.go (1)
13-21: Consider adding struct documentation.The new
ModelandTimestampsstructs are well-structured and follow good practices:
- Proper GORM and JSON tags
- Clean separation of concerns with
Timestampsstruct- Consistent use of
carbon.DateTimefor timestamp fieldsConsider adding GoDoc comments to describe the purpose and usage of these structs:
+// Model represents the base model structure with primary key and timestamps type Model struct { ID uint `gorm:"primaryKey" json:"id"` Timestamps } +// Timestamps provides created_at and updated_at fields for models type Timestamps struct { CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at"` UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at"` }database/migration/stubs.go (1)
Line range hint
85-196: Consider future improvements to SQL stubs structure.While not part of the current changes, consider these future improvements:
- Extract common values (like column names and types) into constants
- Define a common interface for database stubs
- Add documentation for each database-specific implementation
This would improve maintainability and make it easier to add support for new databases.
Example refactor:
// Define common constants const ( DefaultIDColumn = "id" DefaultCreatedAtCol = "created_at" DefaultUpdatedAtCol = "updated_at" ) // Define common interface type DatabaseStubs interface { CreateUp() string CreateDown() string UpdateUp() string UpdateDown() string }database/migration/sql_migrator.go (1)
Based on the gathered information, I can now provide a final response:
Removal of SqlMigrator is confirmed, but DefaultMigrator already implements the required functionality
The verification reveals:
- The
SqlMigratoris indeed marked for removal in v1.16DefaultMigratoris the replacement implementation that already implements theMigratorinterface, which includes theRollbackmethod- There's no direct dependency between
DefaultMigratorandSqlMigrator, indicating they are separate implementationsSince:
- The
DefaultMigratoris already in place and implements the completeMigratorinterface- The
SqlMigratorremoval is explicitly planned- The new Rollback functionality is already part of the interface contract
You should implement the Rollback functionality in the replacement
DefaultMigratorinstead of adding it to the soon-to-be-removedSqlMigrator.🔗 Analysis chain
Line range hint
24-24: Address the TODO comment about SqlMigrator removal.The comment indicates this struct will be removed in v1.16. Since we're adding new functionality to it, we should:
- Verify if this removal is still planned
- Ensure the new Rollback functionality is implemented in the replacement structure
- Consider adding a deprecation notice if removal is confirmed
Let's check for the replacement implementation:
Let me gather more information about the migration-related changes and the planned removal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential replacement implementations rg -l "type.*Migrator" --type go # Check for any migration-related TODOs rg -i "todo.*migrat" --type goLength of output: 602
Script:
#!/bin/bash # Check the DefaultMigrator implementation and its relationship with SqlMigrator ast-grep --pattern 'type DefaultMigrator struct { $$$ }' # Look for deprecation notices or version-related comments rg -B2 -A2 "v1.16" --type go # Check if SqlMigrator is being used as a field in DefaultMigrator rg "SqlMigrator" database/migration/default_migrator.go # Check for any migration-related interface definitions ast-grep --pattern 'type Migrator interface { $$$ }'Length of output: 1569
database/schema/schema_test.go (1)
Line range hint
83-89: Implement pending test cases for type and view operations.The TODO comments indicate missing implementations for
DropAllTypesandDropAllViews. These are important for complete database cleanup during migrations.Would you like me to help implement these test cases or create GitHub issues to track them?
database/factory/factory_test.go (1)
28-31: Consider standardizing JSON tag format for timestamps.The
CreatedAtandUpdatedAtfields use snake_case in JSON tags while Go convention typically uses camelCase. Consider using consistent casing across all JSON tags.type Timestamps struct { - CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"created_at"` - UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updated_at"` + CreatedAt carbon.DateTime `gorm:"autoCreateTime;column:created_at" json:"createdAt"` + UpdatedAt carbon.DateTime `gorm:"autoUpdateTime;column:updated_at" json:"updatedAt"` }contracts/database/orm/orm.go (1)
176-182: LGTM! Well-designed interface segregation.The new interfaces
QueryWithSetContextandQueryWithObserverfollow good design principles:
- Interface segregation principle (ISP) is properly applied
- Optional functionality through composition
- Clear and focused responsibilities
Consider documenting these interfaces with examples in the package documentation to help implementers understand when and how to use them versus the facade methods.
foundation/container.go (1)
Line range hint
322-328: Remove extra blank line after Make call.While the binding reference update looks good, there's an inconsistent blank line after the Make call that should be removed to maintain consistency with other similar methods in this file.
Apply this diff to fix the formatting:
func (c *Container) MakeSeeder() contractsseerder.Facade { - instance, err := c.Make(seeder.BindingSeeder) - + instance, err := c.Make(seeder.BindingSeeder) if err != nil { color.Red().Println(err) return nil }auth/auth_test.go (1)
26-29: LGTM! Well-structured model definition with proper GORM tags.The User struct follows GORM conventions with:
- Properly defined primary key using
gorm:"primaryKey"tag- Automatic timestamp handling using
autoCreateTimeandautoUpdateTimetags- JSON tags for serialization
Consider extracting this User struct into a shared models package if it's used across multiple test files to maintain DRY principles and ensure consistent test data structures.
database/gorm/test_models.go (1)
22-24: Redundantgorm:"column:deleted_at"tag inSoftDeletesstructThe
gorm.DeletedAttype already defaults to thedeleted_atcolumn. Specifyinggorm:"column:deleted_at"is redundant.Consider removing the redundant tag:
type SoftDeletes struct { - DeletedAt gorm.DeletedAt `gorm:"column:deleted_at" json:"deleted_at"` + DeletedAt gorm.DeletedAt `json:"deleted_at"` }database/migration/default_migrator_test.go (2)
43-43: Reminder: Implement support for additional drivers inSetupTestThere's a TODO comment indicating that other database drivers need to be added in the
SetupTest()method. To enhance test coverage and ensure compatibility across different databases, consider adding support for additional drivers like MySQL, SQLite, etc.Would you like assistance in integrating other database drivers into the test setup?
68-68: Reminder: Add rollback test cases inTestRollbackThe TODO comment suggests adding rollback test cases after implementing the SQLite driver. Implementing these test cases will ensure that the rollback functionality is thoroughly tested across different database systems.
Do you need help in writing rollback test cases for multiple database drivers?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
mocks/database/migration/Migrator.gois excluded by!mocks/**mocks/database/orm/Orm.gois excluded by!mocks/**mocks/database/orm/Query.gois excluded by!mocks/**mocks/database/orm/QueryWithObserver.gois excluded by!mocks/**mocks/database/orm/QueryWithSetContext.gois excluded by!mocks/**mocks/http/Context.gois excluded by!mocks/**
📒 Files selected for processing (35)
- auth/auth_test.go (1 hunks)
- contracts/database/migration/migrator.go (1 hunks)
- contracts/database/orm/observer.go (1 hunks)
- contracts/database/orm/orm.go (4 hunks)
- database/console/migration/migrate_rollback_command.go (1 hunks)
- database/console/migration/migrate_rollback_command_test.go (1 hunks)
- database/factory/factory.go (1 hunks)
- database/factory/factory_test.go (4 hunks)
- database/gorm/event.go (1 hunks)
- database/gorm/event_test.go (4 hunks)
- database/gorm/query.go (22 hunks)
- database/gorm/query_test.go (24 hunks)
- database/gorm/test_models.go (7 hunks)
- database/gorm/test_utils.go (2 hunks)
- database/migration/default_creator_test.go (4 hunks)
- database/migration/default_migrator.go (4 hunks)
- database/migration/default_migrator_test.go (6 hunks)
- database/migration/repository_test.go (3 hunks)
- database/migration/sql_migrator.go (1 hunks)
- database/migration/sql_migrator_test.go (2 hunks)
- database/migration/stubs.go (3 hunks)
- database/migration/test_utils.go (1 hunks)
- database/orm/model.go (0 hunks)
- database/orm/orm.go (5 hunks)
- database/orm/orm_test.go (2 hunks)
- database/schema/schema.go (2 hunks)
- database/schema/schema_test.go (5 hunks)
- database/schema/test_utils.go (1 hunks)
- database/seeder/seeder.go (1 hunks)
- database/seeder/seeder_test.go (1 hunks)
- database/service_provider.go (4 hunks)
- foundation/application_test.go (2 hunks)
- foundation/container.go (4 hunks)
- support/database/database_test.go (3 hunks)
- support/docker/docker.go (1 hunks)
💤 Files with no reviewable changes (1)
- database/orm/model.go
🔇 Additional comments (84)
contracts/database/migration/migrator.go (1)
13-14: Verify implementation in all migrators.Let's ensure all migrator implementations (
DefaultMigratorandSqlMigrator) correctly implement the newRollbackmethod.✅ Verification successful
Let me try a different approach to find the implementations, as the previous pattern might have been too strict.
This should give us a better picture of the migrator implementations and their Rollback methods. The previous pattern might have failed due to formatting differences or intermediate lines between the struct and method definitions.
Based on the results, I can now generate the final response as we have found clear evidence of the implementations.
Rollback method is properly implemented across all migrators ✅
Both concrete implementations of the Migrator interface have the correct Rollback method signature:
DefaultMigrator.Rollback(step, batch int) errorindatabase/migration/default_migrator.goSqlMigrator.Rollback(step, batch int) errorindatabase/migration/sql_migrator.goThe mock implementation in
mocks/database/migration/Migrator.goalso has the correct signature, which further validates the interface consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all migrator implementations have the Rollback method # Expected: Find implementations in DefaultMigrator and SqlMigrator echo "Checking Rollback method implementations..." ast-grep --pattern 'type $_ struct { $$$ } func (m $_) Rollback(step, batch int) error { $$$ }'Length of output: 199
Script:
#!/bin/bash # Let's first find all files that might contain migrator implementations echo "Finding potential migrator files..." fd migrator .go$ # Then let's look for Rollback method implementations with more context echo -e "\nChecking for Rollback implementations..." rg "func.*Rollback.*\(.*step.*batch.*int\).*error" -A 5 # Let's also check for struct definitions that might be migrators echo -e "\nChecking for migrator structs..." rg "type.*Migrator struct" -A 3Length of output: 2890
contracts/database/orm/observer.go (1)
Line range hint
1-26: LGTM! Well-defined Observer interfaceThe Observer interface provides a comprehensive set of lifecycle hooks that cover all major database operations including CRUD and soft/hard deletes.
database/schema/test_utils.go (4)
4-4: LGTM: Import organization is clean and purposeful.The imports are well-organized with standard library imports separated from third-party imports. The new imports support the enhanced functionality for multi-driver testing.
Also applies to: 6-12
23-24: Consider adding validation for required dependencies.The function passes
nilfor the last two parameters of bothNewOrmandNewSchema. While this might be intentional for testing, it would be helpful to:
- Document why these dependencies are optional
- Add validation if they're actually required in some scenarios
#!/bin/bash # Description: Check how these nil dependencies are handled # Expected: Find validation or documentation about nil handling # Search for NewOrm implementation ast-grep --pattern 'func NewOrm($$$) $_ { $$$ }' # Search for NewSchema implementation ast-grep --pattern 'func NewSchema($$$) $_ { $$$ }'
15-26: Verify test coverage for multi-driver scenarios.The function now supports multiple database drivers, which is a significant enhancement. Ensure that test cases exist for:
- Multiple concurrent drivers
- Error scenarios for each driver
- Edge cases like empty driver map
#!/bin/bash # Description: Check for comprehensive test coverage # Expected: Find test cases covering multiple drivers and error scenarios # Search for test files using this utility rg -g '*_test.go' 'GetTestSchema.*map\[.*Driver\]' # Look for actual test scenarios rg -g '*_test.go' -B 5 -A 10 'func Test.*Schema'
15-19: Verify the driver string uniqueness across different database implementations.The code creates a map using
driver.String()as keys. While the implementation looks correct, we should ensure that different database drivers don't return the same string representation to avoid key collisions.✅ Verification successful
Let me gather more specific information about the Driver implementations.
Let me gather one final piece of information to specifically look at database driver implementations.
Based on the gathered information, I can now provide a final response:
The database driver string uniqueness is verified and safe from collisions.
The codebase shows that:
- The Driver type is defined as a string type in
contracts/database/config.gowith a simple String() method that returns the underlying string value- Driver constants are used consistently throughout the codebase with distinct values for each database type:
- DriverMysql
- DriverPostgres
- DriverSqlite
- DriverSqlserver
The implementation ensures uniqueness as:
- Drivers are defined as constants at the package level
- The String() method directly returns the constant value without any runtime manipulation
- The usage pattern across the codebase shows consistent mapping between driver types and their string representations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential driver string collisions # Expected: Each driver should have a unique string representation # Search for Driver interface implementations and their String() methods ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) String() string { $$$ }' # Also search for direct string constants that might be used as driver names rg -g '*.go' 'const.*Driver.*=' -A 1Length of output: 3260
Script:
#!/bin/bash # Let's find the Driver interface definition and its implementations # Find the Driver interface definition ast-grep --pattern 'type Driver interface { $$$ }' # Find all implementations of String() method that might be related to database drivers rg -g '*.go' -B 3 -A 1 'func \(.*\) String\(\) string' # Find where Driver types are defined or used rg -g '*.go' 'type.*Driver.*struct'Length of output: 16823
Script:
#!/bin/bash # Let's find database-specific Driver implementations and their String() methods # Search for database driver related constants and their string values rg -g '*.go' 'const.*=.*("mysql"|"postgres"|"sqlite")' # Look for database driver type definitions and implementations ast-grep --pattern 'type $_ struct { $$$ } func ($_ Driver) String() string { $$$ }' # Find where database drivers are registered or used rg -g '*.go' 'database.Driver'Length of output: 17615
database/seeder/seeder.go (3)
1-7: LGTM! Package restructuring looks good.The package name change to
seederaligns well with the PR's objective of better organization. The imports are clean and the interface compliance check is a good practice.
Line range hint
82-90: Consider movingcontainsto a shared utility package.The
containshelper function is a good candidate for reuse. If this functionality is needed elsewhere in the codebase, consider moving it to a shared utilities package.#!/bin/bash # Check if similar string slice contains functionality exists elsewhere echo "Checking for similar contains functionality:" rg "func.*contains.*\[\]string.*string.*bool" -A 3
8-8: Verify consistent usage of the new binding constant.The new
BindingSeederconstant looks good, but let's verify its usage across the codebase to ensure consistency.✅ Verification successful
Let me generate additional shell scripts to better verify the constant usage.
Based on the results, I can now generate the final response as we have sufficient information about the constant's usage.
The
BindingSeederconstant is properly defined and consistently used across the codebaseThe constant is correctly used in the dependency injection system:
- Defined in
database/seeder/seeder.go- Used in service registration in
database/service_provider.go- Referenced in container resolution in
foundation/container.goThere are no instances of hardcoded string literals or inconsistent binding keys found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new BindingSeeder constant # and check for any remaining old binding references # Check for usage of the new constant echo "Checking usage of new BindingSeeder constant:" rg "goravel.seeder" -A 2 # Check for potential old binding references that might need updating echo -e "\nChecking for potential old binding patterns:" rg "database\.seeder" -A 2Length of output: 457
Script:
#!/bin/bash # Let's do a more thorough check with different approaches # Check for any usage of the BindingSeeder constant echo "Checking for BindingSeeder constant usage:" ast-grep --pattern 'const BindingSeeder = $_' # Check for string literal usage echo -e "\nChecking for string literal usage:" rg '"goravel\.seeder"' # Check for any seeder-related bindings echo -e "\nChecking for seeder-related bindings:" rg "Bind.*[Ss]eeder" # Check for any seeder-related provider registrations echo -e "\nChecking for seeder provider registrations:" rg "Register.*[Ss]eeder"Length of output: 2184
database/seeder/seeder_test.go (1)
Line range hint
1-9: LGTM! Package structure aligns with PR objectives.The package name change from 'database' to 'seeder' correctly reflects the restructuring mentioned in the PR objectives, and the imports are clean and appropriate.
database/console/migration/migrate_rollback_command_test.go (4)
8-10: LGTM! Well-organized imports.The import statements are properly organized and use clear aliases for mock packages.
14-22: Great use of beforeEach pattern!The test setup is well-structured using the beforeEach pattern, which:
- Centralizes mock initialization
- Ensures clean state for each test
- Reduces code duplication
65-74: 🛠️ Refactor suggestionEnhance error handling in failed test cases.
While the test execution is correct, consider modifying the error assertion for failed cases:
- assert.NoError(t, err) + if test.name == "Rollback failed" { + assert.Error(t, err) + } else { + assert.NoError(t, err) + }Likely invalid or redundant comment.
1-74: Verify multiple database driver support.The PR objectives mention supporting multiple database drivers, but the current tests don't explicitly verify this functionality.
database/migration/default_creator_test.go (3)
65-65: LGTM: Import path update aligns with package restructuringThe change from
migrationtoschemapackage for Blueprint type aligns with the architectural changes mentioned in the PR objectives.
95-95: LGTM: Update stub test case modificationsThe update stub test case correctly reflects the schema package changes and maintains consistency with the create stub implementation.
Also applies to: 107-107
77-85: Verify schema facade method invocation patternThe changes to use
facades.Schema()method invocation syntax look correct. However, we should verify this pattern is consistently applied across the codebase.✅ Verification successful
Let me generate additional shell scripts to verify the schema facade method invocation pattern more thoroughly.
This will help us see:
- All usages of facades.Schema and the line after it to verify the parentheses
- Any other Schema-related patterns that might need attention
Based on the search results, I can now provide a final response:
Schema facade method invocation pattern is consistently applied
The verification shows that all usages of
facades.Schema()in the codebase follow the correct method invocation pattern with parentheses. The search results show consistent usage in:
- Migration files (e.g.,
database/migration/stubs.go,database/migration/default_creator_test.go)- No instances of incorrect usage (without parentheses) were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of Schema() method invocation # Test: Search for any direct Schema access without parentheses rg -g '*.go' 'facades\.Schema[^(]'Length of output: 36
Script:
#!/bin/bash # Search for all usages of facades.Schema to verify the invocation pattern rg -g '*.go' 'facades\.Schema' -A 1 # Also check for any Schema-related imports and usage patterns rg -g '*.go' 'Schema'Length of output: 44982
database/migration/repository_test.go (3)
39-39: LGTM! Clean repository initialization.The simplified repository initialization improves test readability while maintaining the necessary test coverage.
52-52: LGTM! Comprehensive test setup.The repository initialization is properly integrated with the comprehensive test cases that follow, covering all essential migration operations.
114-117: 🛠️ Refactor suggestionConsider adding error handling in initRepository.
While the implementation is clean, consider handling potential errors from
schema.GetTestSchemato ensure test failures are more descriptive.Here's a suggested improvement:
-func (s *RepositoryTestSuite) initRepository(testQuery *gorm.TestQuery) *Repository { - testSchema := schema.GetTestSchema(testQuery, s.driverToTestQuery) - - return NewRepository(testSchema, "migrations") +func (s *RepositoryTestSuite) initRepository(testQuery *gorm.TestQuery) *Repository { + testSchema, err := schema.GetTestSchema(testQuery, s.driverToTestQuery) + if err != nil { + s.T().Fatalf("Failed to initialize test schema: %v", err) + } + + return NewRepository(testSchema, "migrations") }Likely invalid or redundant comment.
database/migration/test_utils.go (2)
5-5: LGTM: Import changes align with the model restructuringThe replacement of the
ormimport withcarbonis consistent with the transition from embeddedorm.Modelto explicit timestamp fields.
Line range hint
48-106: Ensure schema consistency across database driversWhile the basic schema structure is consistent, there are some differences in index definitions:
- MySQL includes indexes on timestamp fields
- PostgreSQL, SQLite, and SQL Server schemas don't have corresponding indexes
Consider adding these indexes to maintain consistent query performance across all database drivers.
database/factory/factory.go (2)
Line range hint
1-9: LGTM! Clean package structure and imports.The package organization and import structure follow Go best practices with clear separation of dependencies.
Line range hint
1-127: Implementation aligns well with PR objectives.The factory implementation has been successfully relocated to its own package and maintains compatibility with multiple database drivers through the
ormcontract.Queryinterface. The code is well-structured and follows Go best practices.support/docker/docker.go (1)
18-18: Verify test coverage with reduced database set.Switching to
TestModelMinimumremoves MySQL and SQLServer from the test matrix. While this optimizes test execution, we should ensure that database-specific migration rollback logic is still adequately tested.✅ Verification successful
Let's gather more specific information about the test coverage and database-specific code to make a more informed decision.
This will help us understand:
- The nature of database-specific tests (whether they're critical for rollback functionality)
- If there's database-specific migration/rollback logic that needs testing
- The coverage of rollback tests across different databases
Based on the comprehensive results, I can now provide a final response:
Switching to TestModelMinimum is safe for migration rollback testing
The analysis of database-specific code and tests reveals:
While MySQL and SQLServer tests exist, the migration rollback functionality is database-agnostic:
- The rollback tests in
database/migration/sql_migrator_test.goanddatabase/migration/default_migrator_test.gouse a driver-independent approach- The core rollback logic in
database/console/migration/migrate.gohandles each database type through standardized interfaces- Migration rollback tests focus on the migration process itself rather than database-specific behaviors
The remaining test coverage with PostgreSQL and SQLite is sufficient because:
- The migration system uses database-agnostic interfaces through
golang-migrate- Critical rollback functionality is tested through the common migration interface
- Database-specific code is limited to connection handling, not rollback logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for database-specific test cases or migrations that might be affected # Test 1: Look for MySQL-specific test cases echo "Checking for MySQL-specific tests..." rg -l 'mysql|MySQL' --type go -g '*_test.go' # Test 2: Look for SQLServer-specific test cases echo "Checking for SQLServer-specific tests..." rg -l 'sqlserver|SQLServer' --type go -g '*_test.go' # Test 3: Check migration files for database-specific code echo "Checking migrations for database-specific code..." rg -l '(mysql|sqlserver)' --type go -g '*migrate*.go'Length of output: 999
Script:
#!/bin/bash # Check the actual content of database-specific tests and migrations # Look for database-specific assertions or test cases echo "=== MySQL-specific test assertions ===" rg 'mysql|MySQL' --type go -g '*_test.go' -C 2 echo "=== SQLServer-specific test assertions ===" rg 'sqlserver|SQLServer' --type go -g '*_test.go' -C 2 # Check migration-related code for database-specific handling echo "=== Database-specific migration code ===" rg '(mysql|sqlserver)' database/console/migration/migrate*.go -C 2 # Check if there are any database-specific rollback tests echo "=== Rollback-specific tests ===" rg 'rollback|Rollback' --type go -g '*_test.go' -C 2Length of output: 32860
database/migration/sql_migrator_test.go (2)
34-34: LGTM! Good test isolation practice.Moving the initialization to
SetupTestensures each test starts with a fresh state, preventing potential test interference.
84-105: LGTM! Well-structured rollback test.The test effectively verifies the rollback functionality across multiple database drivers with clear assertions.
database/service_provider.go (5)
12-14: LGTM: Import statements correctly updatedThe import statements have been properly updated to reflect the new package structure after relocating the files.
Line range hint
22-41: LGTM: ORM registration properly encapsulatedThe ORM registration has been improved by:
- Moving the binding constant to the orm package
- Using the encapsulated
orm.BuildOrmfunction- Maintaining proper error handling
Line range hint
42-59: LGTM: Schema registration properly structuredThe Schema registration follows the same clean pattern as the ORM changes, with proper dependency checks and error handling.
60-62: LGTM: Seeder registration follows consistent patternThe Seeder registration has been updated to follow the same encapsulation pattern as other services.
97-97: Verify the MigrateRollbackCommand implementationThe command registration has been correctly updated to use the migrator instance. This change aligns with the PR objective to optimize the migrate:rollback command.
Let's verify the MigrateRollbackCommand implementation:
✅ Verification successful
Let me gather more information about the implementation to verify how the migrator is being used in the Execute method.
Let me try one more search to find the Execute method implementation specifically:
MigrateRollbackCommand implementation is correctly using the migrator
The implementation has been verified and shows proper usage of the migrator:
- The command correctly accepts and stores the migrator instance
- The Execute method (Handle) properly uses the migrator.Rollback method with step and batch parameters
- The command structure includes appropriate flags for controlling rollback behavior
- Error handling and success messaging are properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify MigrateRollbackCommand implementation # Expected: Should find the command implementation using migrator instead of config # Check the command implementation ast-grep --pattern 'type MigrateRollbackCommand struct { $$$ migrator $_ $$$ }' # Verify the command's Execute method uses migrator ast-grep --pattern 'func ($_ *MigrateRollbackCommand) Execute($_) error { $$$ migrator.$_($$$) $$$ }'Length of output: 439
Script:
#!/bin/bash # Let's check the complete implementation of the Execute method rg -U "func \(.*MigrateRollbackCommand.*\) Execute.*\{.*?\}" -A 20 --multiline # Also check how the command is constructed rg "func NewMigrateRollbackCommand" -A 5Length of output: 600
Script:
#!/bin/bash # Search for the Execute method implementation with more context cat database/console/migration/migrate_rollback_command.goLength of output: 1648
support/database/database_test.go (2)
10-10: LGTM: Import changes align with new timestamp handling.The switch from
ormtocarbonpackage for timestamp management is consistent with the PR's restructuring objectives.
Line range hint
62-72: Consider adding edge cases to test suite.While the existing test cases properly verify basic Model functionality, consider adding tests for:
- Zero values in Model fields
- JSON serialization/deserialization of the Model struct
- Behavior with nil timestamps
Also applies to: 99-109
database/schema/schema.go (2)
15-16: LGTM! Well-defined constant following framework conventions.The constant follows proper naming conventions and uses appropriate namespacing.
149-152: LGTM! Smart optimization to prevent nested transactions.The change intelligently prevents nested transactions while maintaining proper transaction semantics. This is particularly important for migration operations.
Let's verify that all supported database drivers implement the
InTransaction()method correctly:✅ Verification successful
Let me gather more information about the database drivers and transaction handling to ensure comprehensive verification.
This will help us verify:
- Available database drivers
- Transaction-related interfaces and their requirements
- Transaction implementation patterns across the codebase
Let me check one more aspect to verify the transaction handling implementation in GORM.
This will help us understand:
- How transactions are implemented in GORM
- The Query interface contract
- Any error handling specific to transactions
Let me check one final aspect about the Query interface definition since it wasn't found in the previous search.
This will help us understand:
- The complete Query interface contract
- The ORM interface that might include transaction-related methods
- All transaction-related method definitions in the contracts
Based on the comprehensive verification results, I can now provide the final response:
Transaction handling implementation is properly verified across the codebase
The
InTransaction()method is correctly implemented and the transaction handling is robust because:
- The Query interface in
contracts/database/orm/orm.goproperly defines the transaction-related contract includingBegin(),Commit(),Rollback(), andInTransaction()- GORM implementation in
database/gorm/query.gocorrectly implementsInTransaction()by checking the underlying connection pool- Test coverage exists specifically for transaction scenarios (
TestInTransaction,TestTransactionSuccess,TestTransactionError)- The implementation properly uses GORM's
TxCommitterinterface to detect transaction state🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check InTransaction() implementation across database drivers # Expected: Find all database driver implementations to ensure they properly implement InTransaction() echo "Checking for Query interface implementations:" ast-grep --pattern 'type Query struct { $$$ }' echo -e "\nChecking for InTransaction method implementations:" ast-grep --pattern 'func ($_) InTransaction() bool { $$$ }'Length of output: 2083
Script:
#!/bin/bash # Check for database driver implementations and transaction-related interfaces echo "Checking for database driver implementations:" rg -g "!mocks/*" "type.*Driver" -A 2 echo -e "\nChecking for transaction-related interfaces:" rg -g "!mocks/*" "interface.*Transaction" -A 5 echo -e "\nChecking GORM transaction usage:" ast-grep --pattern 'func ($x) Transaction($$$) error { $$$ }'Length of output: 2321
Script:
#!/bin/bash # Check for transaction-related code in GORM implementation echo "Checking GORM transaction implementation:" rg -g "database/gorm/*" "func.*Transaction" -A 5 echo -e "\nChecking Query interface definition:" rg -g "contracts/*" "type.*Query.*interface" -A 10 echo -e "\nChecking for transaction-related errors or edge cases:" rg -g "database/gorm/*" "Transaction.*error" -B 2 -A 2Length of output: 2135
Script:
#!/bin/bash # Check for Query interface and transaction-related methods echo "Checking Query interface definition in contracts:" rg -g "contracts/**" "type\s+Query\s+" -A 15 echo -e "\nChecking ORM interface definition:" rg -g "contracts/**" "type\s+Orm\s+" -A 15 echo -e "\nChecking transaction-related methods in interfaces:" rg -g "contracts/**" "Begin|Commit|Rollback|Transaction" -B 1 -A 1Length of output: 4248
database/migration/stubs.go (2)
44-44: LGTM! Verify Schema() method call pattern.The update to use method call syntax (
facades.Schema()) is more idiomatic Go. This change suggests Schema is now a function returning an interface, which is a good practice for better dependency management.Let's verify this pattern is used consistently:
Also applies to: 52-52, 72-72
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence about the Schema() usage pattern across the codebase.
Schema() method call pattern is consistently used across the codebase
The verification shows that:
- There are no instances of direct Schema usage without parentheses
- The
facades.Schema()method call pattern is consistently used across all relevant files:
database/migration/stubs.godatabase/migration/default_creator_test.goAll occurrences follow the same pattern with Schema(), indicating proper implementation of the function-returning-interface pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of Schema() method call syntax # Search for any direct Schema usage without parentheses echo "Checking for potential inconsistent Schema usage:" rg "facades\.Schema\s+\." --type go # Search for the correct Schema() usage pattern echo "Checking correct Schema() usage:" rg "facades\.Schema\(\)\." --type goLength of output: 870
32-32: LGTM! Verify import path consistency.The migration of
Blueprintto theschemapackage is a good architectural decision that better reflects its purpose. The import path changes look correct.Let's verify this change is consistent across the codebase:
Also applies to: 60-60
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Import path change is consistent and correct
The migration from
migration.Blueprinttoschema.Blueprintis correctly implemented across the codebase. The search results confirm:
- All Blueprint implementations and interfaces are now under the
schemapackage- All usages of Blueprint are properly importing from
contracts/database/schema- The old import path
contracts/database/migrationis only used for other migration-related functionality, not for Blueprint🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old migration.Blueprint import # and ensure all files are using the new schema.Blueprint # Search for old import path echo "Checking for old import path usage:" rg "github.com/goravel/framework/contracts/database/migration" # Search for Blueprint usage to ensure consistent typing echo "Checking Blueprint usage:" rg "Blueprint" -A 1Length of output: 44873
database/migration/sql_migrator.go (1)
84-95: Verify Rollback functionality across all supported database drivers.The Rollback implementation needs to be tested against all supported database drivers (MySQL, PostgreSQL, SQLite, SQLServer) to ensure consistent behavior.
Let's check for existing tests:
✅ Verification successful
Let's examine the rollback tests in detail to verify the coverage across different database drivers.
Let's check for the test setup and driver configurations more specifically:
Let's check one more thing to verify the test coverage across different drivers:
Based on the gathered evidence, I can now provide a final response:
Rollback functionality is already verified across all supported database drivers
The codebase shows comprehensive testing of the Rollback functionality:
- The
SqlMigratorSuitetests run against all supported database drivers throughgorm.NewTestQueries().Queries()- Test utilities (
test_utils.go) include specific migration scripts for MySQL, PostgreSQL, SQLite, and SQLServer- The test setup creates and verifies migrations for each database type, including both up and down migrations
- The rollback tests verify the functionality by first running migrations and then rolling them back, checking the database state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for migration-related test files fd -e go test # Look for rollback tests in these files rg -l "TestRollback"Length of output: 4353
Script:
#!/bin/bash # Check the content of rollback tests rg -A 10 "TestRollback" database/migration/sql_migrator_test.go database/migration/default_migrator_test.go # Check for database driver setup in tests rg -B 5 "func Test.*Rollback" database/migration/sql_migrator_test.go database/migration/default_migrator_test.go # Look for database driver configurations rg "NewSqlMigrator|NewDefaultMigrator" database/migration/test_utils.goLength of output: 2587
Script:
#!/bin/bash # Check the test suite setup and driver configurations rg -B 10 "type SqlMigratorSuite struct" database/migration/sql_migrator_test.go # Look for driver initialization in test files rg "driverToTestQuery.*=" database/migration/sql_migrator_test.go # Check the test utils for driver setup cat database/migration/test_utils.goLength of output: 3880
Script:
#!/bin/bash # Check how test queries are initialized for different drivers rg -A 10 "NewTestQueries" database/gorm/test_utils.go # Check the SqlMigratorSuite setup rg -A 20 "func \(s \*SqlMigratorSuite\) SetupTest\(\)" database/migration/sql_migrator_test.goLength of output: 1027
database/schema/schema_test.go (4)
40-49: LGTM! Well-structured table operation tests.The test properly validates the full lifecycle of table operations (create, check existence, drop) for each database driver.
Line range hint
157-170: LGTM! SQL execution test is well implemented.The test properly validates direct SQL execution capabilities with actual database verification.
Line range hint
29-35: Consider expanding driver support in test setup.The TODO comment indicates plans to add other drivers. Given that the PR objective is to support multiple database drivers, consider adding tests for MySQL and SQLite to ensure comprehensive coverage.
Line range hint
93-153: Consider uncommenting and implementing column-level tests.The large commented block contains important tests for column operations including:
- Column existence checks
- Column property validations
- Column modifications
These tests would be valuable for ensuring proper schema modifications, especially given the PR's focus on migration improvements.✅ Verification successful
Let me try a different approach to verify the implementation status of column-related operations.
Let me check the actual implementation of Schema methods to better understand the current state.
Based on the search results, I can now provide a final response:
Column-related methods need to be implemented before uncommenting tests
The interface and implementation checks reveal that the Schema interface currently doesn't declare the required column-related methods (
HasColumn,GetColumns,HasColumns) that the commented tests are trying to verify. This confirms that:
- The review comment is correct in suggesting these tests should remain commented until the implementation is complete
- The tests serve as a good specification for the methods that need to be implemented:
HasColumn(table, column string)GetColumns(table string)HasColumns(table string, columns []string)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if column operations are implemented in the schema package ast-grep --pattern 'func ($schema *Schema) HasColumn($table string, $column string)'Length of output: 86
Script:
#!/bin/bash # Search for column-related method implementations in Schema rg -A 2 "func.*HasColumn|func.*GetColumns|func.*HasColumns" --type go # Also check for any column-related tests that might be already implemented rg -A 2 "TestColumn|test.*Column" --type goLength of output: 758
Script:
#!/bin/bash # Check Schema interface and implementation rg -A 10 "type Schema interface" --type go rg -A 10 "type Schema struct" --type go # Look for any column-related method implementations in progress rg -B 2 -A 5 "HasColumn|GetColumns|HasColumns" --type go --type-add go:*.{go,golang} --no-filenameLength of output: 2074
database/orm/orm_test.go (3)
Line range hint
1-12: LGTM! Package restructuring looks good.The package name change from
databasetoormaligns well with the PR's objective of reorganizing the database-related packages for better modularity.
Line range hint
27-211: Test implementation is comprehensive and well-structured.The test suite effectively covers critical ORM functionality:
- Multiple database driver support
- Transaction handling with rollback scenarios
- Context propagation across different ORM operations
- Observer pattern implementation
- Thread safety (concurrent query execution)
The test cases align well with the PR objective of supporting multiple database drivers in the migrate:rollback command.
21-22: Verify the embedded type accessibility.The removal of the
ormprefix fromModelandSoftDeletesis correct since we're now in theormpackage. Let's verify these types are properly accessible.✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information to verify the embedded type accessibility.
The embedded types
ModelandSoftDeletesare properly defined and accessibleBoth
ModelandSoftDeletestypes are correctly defined indatabase/orm/model.go, which is in the same package as the test file. The removal of theormprefix is appropriate since:
Modelis defined atdatabase/orm/model.go:12SoftDeletesis defined atdatabase/orm/model.go:17🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Model and SoftDeletes types are properly defined # Test: Search for Model and SoftDeletes type definitions ast-grep --pattern 'type Model struct { $$$ }' ast-grep --pattern 'type SoftDeletes struct { $$$ }'Length of output: 1635
database/factory/factory_test.go (2)
Line range hint
84-256: Test suite implementation looks solid!The test suite demonstrates good practices:
- Proper environment handling for Windows
- Docker-based test database setup
- Comprehensive test cases covering various factory scenarios
58-61: Consider using dynamic IDs in factory definitions.The
Authormodel now uses the baseModelstruct, which is good for consistency. However, ensure that theBookIDfield's value is not hardcoded in the factory definition to prevent potential test conflicts.Let's check if there are any test files that might be affected by this change:
✅ Verification successful
Let me gather more context about how BookID is used in the test file to provide a more accurate verification.
Let me check if there are any other test files that depend on or reference the Author model to ensure we have a complete picture of test dependencies.
Let me check how the Author model is used in the query_test.go file to understand if the hardcoded BookID affects other tests.
Based on the analysis of both test files, I can now provide the final response:
The hardcoded BookID in the Author factory is safe to use
The hardcoded BookID (value: 2) in the Author factory is only used within
factory_test.gofor testing the factory itself. Inquery_test.go, where Author is used in relationship testing, the IDs are dynamically generated through the ORM's creation process. The hardcoded ID doesn't cause any conflicts because:
- The factory test is isolated and only verifies the factory's behavior
- All other tests using Author model (like in
query_test.go) create their own records with auto-generated IDs- There are no test dependencies that rely on specific BookID values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that might be affected by the Author model's BookID rg -l "BookID.*2" --type goLength of output: 62
Script:
#!/bin/bash # Get the context around BookID usage in factory_test.go rg "BookID" database/factory/factory_test.go -B 2 -A 2Length of output: 427
Script:
#!/bin/bash # Search for other test files that might use the Author model rg -l "Author" --type go | grep "_test.go" # Also check for any Book model references since Author has BookID rg -l "Book" --type go | grep "_test.go"Length of output: 201
Script:
#!/bin/bash # Check how Author and Book are used in query_test.go rg "Author|Book" database/gorm/query_test.go -B 2 -A 2Length of output: 17465
database/gorm/event.go (1)
95-95: LGTM: Clean implementation of observer pattern support.The addition of
modelToObserverparameter maintains proper state propagation when creating new Query instances, which is essential for the observer pattern implementation. The use ofSession(&gorm.Session{NewDB: true})ensures thread-safety by creating a new session.Let's verify the observer pattern implementation:
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have comprehensive evidence of the observer pattern implementation:
Verified: Observer pattern is properly implemented and propagated
The
modelToObserverparameter addition is part of a well-structured observer pattern implementation:
- The observer interface is properly defined in
contracts/database/orm/observer.gowith comprehensive lifecycle hooks (Retrieved, Creating, Created, etc.)- The
modelToObserverfield is consistently propagated across the ORM stack:
- Properly initialized in
NewQueryconstructor- Correctly passed through in query chaining operations
- Maintained during session creation in the event handler
- The implementation maintains proper state isolation by using
Session(&gorm.Session{NewDB: true})🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the observer pattern implementation across the codebase # Test 1: Check for modelToObserver usage in Query struct and related files rg -A 5 "modelToObserver" # Test 2: Look for observer pattern implementation ast-grep --pattern 'type Observer interface { $$$ }'Length of output: 8028
contracts/database/orm/orm.go (2)
85-86: LGTM! Good addition for transaction state checking.The
InTransactionmethod is a valuable addition that enables checking the transaction state, which is particularly useful for migration rollback operations.
25-26: LGTM! Verify transaction handling.The
SetQuerymethod is a good addition for managing query instances, particularly useful during migration transactions.Let's verify the transaction handling implementation:
✅ Verification successful
Let's examine the actual implementation to verify the transaction handling in these files.
This will help us verify:
- How SetQuery is used in the migrator implementation
- Whether it's properly handling transactions
- If there are any test cases validating this behavior
Based on the implementation shown, I can now generate the final response as we have sufficient evidence of proper transaction handling.
✅ SetQuery implementation verified - correctly handles transactions
The implementation shows proper transaction management:
- SetQuery is used within transaction blocks to switch query contexts
- Proper cleanup is implemented by resetting to default query after operations
- Error handling includes restoring the default query state
- Test coverage validates the transaction behavior
Key verified patterns:
- Sets transaction query:
r.schema.Orm().SetQuery(tx)- Resets after operation:
r.schema.Orm().SetQuery(defaultQuery)- Handles errors with proper cleanup
- Test suite confirms the behavior with proper mocking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SetQuery usage in migration-related code rg -l "SetQuery.*Query" | rg -i "migrat"Length of output: 123
Script:
#!/bin/bash # Check the implementation in migrator files echo "=== default_migrator.go ===" rg -A 10 "SetQuery" database/migration/default_migrator.go echo -e "\n=== default_migrator_test.go ===" rg -A 10 "SetQuery" database/migration/default_migrator_test.goLength of output: 2914
foundation/container.go (4)
37-39: LGTM! Import paths properly reflect the new package structure.The reorganization of imports aligns well with the PR's objective of relocating database-related files into dedicated packages, improving code organization.
212-212: LGTM! Binding reference correctly updated for ORM.The change maintains consistency with the new package structure while preserving the existing error handling pattern.
262-262: LGTM! Binding reference correctly updated for Schema.The change maintains consistency with the new package structure while preserving the existing error handling pattern.
37-39: Verify all binding references have been updated.Let's ensure there are no remaining references to the old binding paths in the codebase.
Also applies to: 212-212, 262-262, 322-322
✅ Verification successful
Let me generate additional verification scripts to check for any references to these database-related bindings in a different way.
This script will:
- Look for any imports of the old database package
- Search for any references to the specific bindings using their type names
- Check container binding calls to see the current binding setup
This should give us a clearer picture of whether all references have been properly updated.
Let me check the specific content of the files that showed matches to see exactly how these bindings are being used.
Based on the verification results, I can now provide a final response:
All binding references are correctly updated and consistent
The verification shows that:
- All references to
orm.BindingOrm,schema.BindingSchema, andseeder.BindingSeederare using the new import paths from their respective packages- The imports in
foundation/container.go,foundation/application_test.go, anddatabase/service_provider.goare properly updated to use the new module structure- No references to the old
database.BindingOrm,database.BindingSchema, ordatabase.BindingSeederpaths were found🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to old binding paths echo "Checking for old database.BindingOrm references..." rg "database\.BindingOrm" echo "Checking for old database.BindingSchema references..." rg "database\.BindingSchema" echo "Checking for old database.BindingSeeder references..." rg "database\.BindingSeeder"Length of output: 434
Script:
#!/bin/bash # Search for any imports or usage of the old database package paths echo "Checking for old database package imports and usages..." rg -A 2 "\"github\.com/goravel/framework/database\"" # Search for any direct references to orm, schema, and seeder echo "Checking for orm, schema, and seeder references..." rg -l "orm\.BindingOrm|schema\.BindingSchema|seeder\.BindingSeeder" # Check container bindings specifically echo "Checking container bindings..." rg "container\.Bind\(" -A 2Length of output: 1127
Script:
#!/bin/bash # Check the content of files that showed matches echo "Checking foundation/container.go..." rg -A 5 "BindingOrm|BindingSchema|BindingSeeder" foundation/container.go echo "Checking foundation/application_test.go..." rg -A 5 "BindingOrm|BindingSchema|BindingSeeder" foundation/application_test.go echo "Checking database/service_provider.go..." rg -A 5 "BindingOrm|BindingSchema|BindingSeeder" database/service_provider.go # Also check the actual imports in these files echo "Checking imports in matched files..." rg -A 10 "^import \(" foundation/container.go foundation/application_test.go database/service_provider.goLength of output: 3844
database/gorm/event_test.go (3)
64-70: LGTM! Consistent model initialization.The test model initialization has been properly updated to use the new model structure while maintaining the same test coverage and data patterns.
Also applies to: 73-79
Line range hint
150-156: LGTM! Consistent test model structure.The GetAttribute test case maintains consistency with the updated model structure while preserving the test coverage.
15-15: Verify package organization changes.The model structure changes look good, removing the
ormpackage prefix aligns with the PR's restructuring objectives. However, let's verify that the necessary types are properly imported.Also applies to: 29-29
✅ Verification successful
Let me gather more information to understand the full context of these model type definitions.
Let me check the test_models.go file since it was found to contain the type definitions.
I can now generate the final response based on the gathered information.
Model and Timestamps types are properly defined and imported
The changes from
orm.ModeltoModelandorm.TimestampstoTimestampsare correct as these types are properly defined in the same package (database/gorm/test_models.go). The test file correctly imports only the requiredgorm.io/gormpackage and doesn't need the oldormpackage anymore.
Modelstruct is defined in test_models.go with proper fieldsTimestampsstruct is defined in test_models.go with proper datetime fields- No direct dependency on the old
ormpackage in event_test.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Model and Timestamps type definitions are available in the current package # and that there are no remaining references to the old orm package. # Check for any remaining references to the old orm package rg -l "orm\.(Model|Timestamps)" --type go # Verify the new Model and Timestamps type definitions ast-grep --pattern 'type (Model|Timestamps) struct'Length of output: 209
Script:
#!/bin/bash # Check imports in the test file rg -A 5 "^import \(" database/gorm/event_test.go # Look for Model and Timestamps definitions in nearby files fd -e go . database/gorm/ -x rg -l "type (Model|Timestamps)" # Check the actual content of query.go since it was found in previous search cat database/gorm/query.go | rg -A 10 "type (Model|Timestamps)"Length of output: 312
Script:
#!/bin/bash # Check the content of test_models.go for Model and Timestamps definitions cat database/gorm/test_models.go # Also check if there are any imports of orm package in event_test.go rg "\".*orm\"" database/gorm/event_test.goLength of output: 11363
foundation/application_test.go (1)
19-19: LGTM! Package restructuring changes look good.The changes correctly reflect the migration of ORM-related code to dedicated packages, maintaining functionality while improving code organization.
Let's verify that all ORM-related imports and bindings have been consistently updated across the codebase:
Also applies to: 163-163
✅ Verification successful
Let me analyze the results:
- No references to
database.BindingOrmwere found, indicating all instances have been updated.- Three instances of
orm.BindingOrmwere found in:
- foundation/container.go
- foundation/application_test.go
- database/service_provider.go
- The import
github.com/goravel/framework/databaseis still present infoundation/application_test.goalong with the newdatabase/ormimport.Let's verify if this remaining database import is still needed:
Based on the results, I can now provide the final response:
The database package import is still required
The
databasepackage import infoundation/application_test.gois still necessary as it's used for:
- Configuration keys in the format
database.*for database connection settings- Instantiating and registering the
database.ServiceProviderThe changes to use
orm.BindingOrminstead ofdatabase.BindingOrmare correct and consistent across the codebase, but thedatabasepackage itself serves additional purposes beyond just the ORM binding.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of new ORM package and binding # Test 1: Check for any remaining references to the old package echo "Checking for old package references..." rg "database\.BindingOrm" # Test 2: Verify consistent usage of new package echo "Verifying new package usage..." rg "orm\.BindingOrm" # Test 3: Check for potential missed imports echo "Checking import statements..." rg "\"github.com/goravel/framework/database\"" -A 5 -B 5Length of output: 1467
Script:
#!/bin/bash # Check how the database package is used in application_test.go rg -l "database\." foundation/application_test.go # If found, show the actual usage with context rg "database\." foundation/application_test.go -A 2 -B 2Length of output: 2060
database/gorm/test_utils.go (1)
169-169: Verify BuildQuery interface changes across the codebase.The addition of a new parameter to
BuildQuerycalls appears to be part of supporting multiple database drivers. While the implementation is consistent across all calls in this file, we should ensure this change is properly propagated throughout the codebase.Let's verify the BuildQuery interface implementation across the codebase:
Also applies to: 172-172, 211-211
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have comprehensive information about the
BuildQueryinterface and its usage across the codebase.BuildQuery interface is consistent across the codebase
The verification shows that all
BuildQuerycalls are properly aligned with the function signature:
- Function definition in
database/gorm/query.goaccepts 5 parameters:(ctx context.Context, config config.Config, connection string, log log.Log, modelToObserver []contractsorm.ModelToObserver)- All calls to
BuildQueryacross the codebase pass the correct number of parameters, including the test cases wherenilis passed forlogandmodelToObserverThe changes in the test file are consistent with the function's interface and its usage pattern throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any BuildQuery calls that might not have been updated # with the new parameter signature # Search for BuildQuery function definition echo "=== BuildQuery function definition ===" ast-grep --pattern 'func BuildQuery($$$) ($$$)' # Search for all BuildQuery calls echo -e "\n=== BuildQuery function calls ===" rg 'BuildQuery\(' -A 1Length of output: 2253
database/gorm/query_test.go (4)
2025-2037: LGTM! Well-structured test for observer functionality.The test thoroughly verifies the observer mapping behavior by checking both the presence and absence of observers for different models.
2201-2223: LGTM! Comprehensive test coverage for transaction states.The test effectively verifies the complete transaction lifecycle, including state transitions through begin, commit, and rollback operations.
Line range hint
1-3700: Well-structured test suite with comprehensive coverage.The test file demonstrates excellent testing practices:
- Organized test groups with clear descriptions
- Table-driven tests for thorough coverage
- Proper setup and teardown patterns
- Comprehensive assertions
72-72: Verify consistent usage of Associations constant.The
Associationsconstant is used consistently throughout the codebase for handling model relationships. However, it would be beneficial to verify that this constant is properly defined and documented.Also applies to: 467-477, 509-519
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get a complete picture.
The first script will verify the proper import of the GORM clause package. The second script will help us find all Select calls to verify consistent usage. The third script will look for any documentation about associations handling.
Let me generate one final verification script to complete our analysis.
This will help us verify the proper implementation context and import dependencies before making a final determination.
Based on the comprehensive verification results, I can now provide the final response:
Associationsconstant is properly defined and consistently usedThe verification confirms that:
- The
Associationsconstant is properly defined and imported fromgorm.io/gorm/clause- It's consistently used in create operations with relationships
- There's proper error handling for conflicts (
OrmQueryAssociationsConflict)- The usage pattern in tests demonstrates correct behavior with various relationship types (one-to-one, one-to-many)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and documentation of the Associations constant # Test: Search for the Associations constant definition rg -A 2 "^const.*Associations.*=" --type go # Test: Search for any documentation about Associations usage rg -B 2 "Associations.*=" --type goLength of output: 1241
Script:
#!/bin/bash # Check the import and usage of clause.Associations rg -A 2 'import.*"gorm.io/gorm/clause"' --type go # Check if there are any other usages of Associations that might be inconsistent ast-grep --pattern 'Select($expr)' # Look for any documentation or comments about Associations usage rg -B 2 '// .*[Aa]ssociation' --type goLength of output: 3527
Script:
#!/bin/bash # Check the actual implementation and usage context of Select with Associations rg -B 2 -A 2 "Select\(.*Associations.*\)" --type go # Check if clause package is properly imported where Associations is defined rg -B 5 "const.*Associations.*=" --type goLength of output: 6651
database/console/migration/migrate_rollback_command.go (1)
21-31: Consistent naming of method receiversThe receiver variable name has been changed from
receivertorin the methodsSignature,Description, andExtend. This improves code readability and follows common Go conventions for receiver naming. Good job on making the code more idiomatic.database/migration/default_migrator.go (1)
198-199: No need to check fornilin type assertionIn lines 198-199, you perform a type assertion:
if connectionMigration, ok := migration.(schema.Connection); ok { r.schema.SetConnection(connectionMigration.Connection()) }Since the type assertion uses the
, okform, there's no risk of a panic ifmigrationdoesn't implementschema.Connection.The current implementation is correct and safe.
database/gorm/test_models.go (14)
7-10: Imports updated appropriatelyThe imports have been updated to include
"gorm.io/gorm"andormcontract. This aligns with the new model definitions and replaces the dependency onorm.
17-29: Well-defined customModel,SoftDeletes, andTimestampsstructsThe new structs
Model,SoftDeletes, andTimestampsare correctly defined, effectively replacing the previous usage oform.Modelandorm.SoftDeletes. This change streamlines the model definitions and removes the dependency on theormpackage.
32-33:Userstruct now embedsModelandSoftDeletesThe
Userstruct correctly embeds the newModelandSoftDeletesstructs, ensuring it inherits the primary key and timestamp functionality.
313-313:Rolestruct now embedsModelThe
Rolestruct correctly embeds the newModelstruct, maintaining consistency with the updated model definitions.
319-319:Addressstruct now embedsModelThe
Addressstruct has been updated to embed theModelstruct, which is appropriate for the changes introduced.
327-327:Bookstruct now embedsModelThe
Bookstruct correctly embeds the newModelstruct, aligning with the updated model structure.
335-335:Authorstruct now embedsModelThe
Authorstruct has been updated to embed theModelstruct, which is consistent with other model definitions.
341-341:Housestruct now embedsModelThe
Housestruct correctly embeds theModelstruct, adhering to the new model conventions.
352-352:Phonestruct now embedsModelThe
Phonestruct has been updated to embed theModelstruct, ensuring consistency across models.
359-360:Productstruct now embedsModelandSoftDeletesThe
Productstruct correctly embeds bothModelandSoftDeletes, allowing for primary key, timestamp, and soft delete functionalities.
369-370:Reviewstruct now embedsModelandSoftDeletesThe
Reviewstruct has been updated appropriately with the new embeddings, supporting the necessary functionalities.
379-380:Peoplestruct now embedsModelandSoftDeletesThe
Peoplestruct correctly includes the newModelandSoftDeletesembeddings, maintaining consistency.
389-390:Personstruct now embedsModelandSoftDeletesThe
Personstruct has been updated to embedModelandSoftDeletes, which is appropriate for the changes.
399-400:Boxstruct now embedsModelandSoftDeletesThe
Boxstruct correctly embedsModelandSoftDeletes, aligning it with other models.database/gorm/query.go (4)
591-596:InTransactionmethod appears correctThe implementation of the
InTransactionmethod correctly checks if the current database instance is within a transaction.
Line range hint
1163-1168: Proper context passing inbuildWithfunctionThe
buildWithfunction creates a newQueryinstance with the appropriate context and observer mappings. This ensures that nested queries maintain the correct configurations.
1210-1210: Use ofAssociationsconstant increatemethodThe use of the
Associationsconstant to omit associations during creation is appropriate and enhances code readability.
Line range hint
1381-1386: Consistent observer passing inrefreshConnectionmethodWhen refreshing the connection, the
modelToObserverslice is passed to maintain observer mappings across different connections. This ensures that observers are consistently applied.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
database/migration/default_migrator_test.go (1)
29-71: Consider adding more database drivers to the test suite.The TODO comment on line 43 indicates that other drivers should be added. This is important for ensuring the migration functionality works correctly across different database types.
Would you like me to help implement test cases for additional database drivers like MySQL and SQLite?
database/gorm/query.go (1)
595-599: Consider adding error handling for edge cases.While the implementation is correct, consider handling the case where the Statement.ConnPool is nil to prevent potential panic.
func (r *Query) InTransaction() bool { + if r.Instance().Statement.ConnPool == nil { + return false + } committer, ok := r.Instance().Statement.ConnPool.(gormio.TxCommitter) return ok && committer != nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- database/console/migration/migrate_rollback_command.go (1 hunks)
- database/console/migration/migrate_rollback_command_test.go (1 hunks)
- database/gorm/query.go (23 hunks)
- database/migration/default_migrator.go (6 hunks)
- database/migration/default_migrator_test.go (6 hunks)
- database/migration/sql_migrator.go (1 hunks)
- database/orm/orm.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- database/console/migration/migrate_rollback_command.go
- database/console/migration/migrate_rollback_command_test.go
- database/migration/sql_migrator.go
🔇 Additional comments (17)
database/orm/orm.go (4)
1-14: LGTM! Package restructuring and imports look good.The package rename and import changes align well with the PR objectives of restructuring the codebase. The addition of the
syncpackage is a good step towards addressing concurrency concerns.
15-26: LGTM! Good improvements to thread safety and encapsulation.The addition of the mutex and instance-level
modelToObserverfield properly addresses the concurrency concerns raised in previous reviews. The newBindingOrmconstant provides a clear identifier for the ORM binding.
Line range hint
52-61: Reiterating past review: Observer initialization inconsistency.The past review comment about observers not being passed in
BuildOrmis still valid. Please refer to the previous review comment for the suggested fix.
120-122:⚠️ Potential issueAdd thread safety to SetQuery method.
The
SetQuerymethod modifies the sharedqueryfield without synchronization. This could lead to data races if called concurrently.Apply this diff to add thread safety:
func (r *Orm) SetQuery(query contractsorm.Query) { + r.mutex.Lock() + defer r.mutex.Unlock() r.query = query }Likely invalid or redundant comment.
database/migration/default_migrator.go (5)
7-9: LGTM: Well-structured changes to imports and struct fieldsThe addition of contract imports and the
migrationsmap field improves the organization and lookup efficiency of the migrator.Also applies to: 17-19
22-34: LGTM: Efficient initialization of migrations mapThe constructor now properly initializes the migrations map, implementing the previously suggested optimization for O(1) lookups by signature.
65-94: Add error context for better debuggingWhile the rollback implementation is solid, consider adding more context to errors for better debugging:
func (r *DefaultMigrator) Rollback(step, batch int) error { files, err := r.getFilesForRollback(step, batch) if err != nil { - return err + return fmt.Errorf("failed to get files for rollback: %w", err) } // ... for _, file := range files { migration := r.getMigrationViaFile(file) if migration == nil { color.Warnf("Migration not found: %s\n", file.Migration) continue } if err := r.runDown(migration); err != nil { - return err + return fmt.Errorf("failed to run down migration %s: %w", file.Migration, err) } // ... } return nil }
111-121: Clarify priority between step and batch parametersThe current implementation prioritizes
stepoverbatch. Please document this priority or consider handling the case where both parameters are non-zero more explicitly:func (r *DefaultMigrator) getFilesForRollback(step, batch int) ([]contractsmigration.File, error) { + // Step takes precedence over batch if both are specified if step > 0 { return r.repository.GetMigrations(step) }
174-200: LGTM: Robust connection handling with proper cleanupThe implementation of
runUpandrunDownnow properly handles connection state usingdefer, ensuring cleanup even in case of panics. The transaction handling and connection reset logic are well-documented and correctly implemented.Also applies to: 202-227
database/migration/default_migrator_test.go (4)
138-217: LGTM! Comprehensive test coverage for rollback file retrieval.The test cases thoroughly cover different scenarios for retrieving rollback files:
- Step-based rollback
- Batch-based rollback
- Last migration rollback
- Error handling for each case
219-285: LGTM! Well-structured test cases for rollback functionality.The test cases effectively cover:
- Empty rollback scenario
- Successful rollback with files
- Missing migration handling
- Error propagation
593-643: LGTM! Well-structured mock setup helpers.The
mockRunDownandmockRunUphelper functions improve test readability and reduce duplication by centralizing the mock setup logic.
654-709:⚠️ Potential issueMigration signatures must be unique across test migrations.
The test migrations (
TestMigration,TestConnectionMigration, andTestErrorMigration) all use the same timestamp "20240817214501" in their signatures. This could lead to conflicts when running migrations.Apply this diff to ensure unique timestamps:
func (r *TestMigration) Signature() string { - return "20240817214501_create_users_table" + return "20240817214501_create_users_table" } func (r *TestConnectionMigration) Signature() string { - return "20240817214501_create_agents_table" + return "20240817214502_create_agents_table" } func (r *TestErrorMigration) Signature() string { - return "20240817214501_create_companies_table" + return "20240817214503_create_companies_table" }Likely invalid or redundant comment.
database/gorm/query.go (4)
Line range hint
41-79: LGTM: Constructor changes properly handle new observer pattern.The constructor and BuildQuery changes correctly initialize and pass the modelToObserver field.
1300-1317: LGTM: Robust observer lookup implementation.The getObserver method correctly:
- Handles both pointer and non-pointer types
- Compares both package path and type name for accurate matching
- Returns nil when no observer is found
1214-1214: LGTM: Consistent association handling across operations.The code consistently uses the Associations constant and properly omits associations in create, save, and update operations.
Also applies to: 1406-1406, 1518-1518
532-539: Verify observer pattern usage across the codebase.Let's verify that the observer pattern is consistently used and that models implementing observers are properly registered.
✅ Verification successful
Let me gather more information about the observer interface and its implementation.
Based on the gathered information, I can now provide the final response:
Observer pattern implementation is correctly structured and consistently used
The verification shows:
- A well-defined Observer interface in
contracts/database/orm/observer.gowith comprehensive lifecycle methods- Proper observer registration mechanism in
database/gorm/query.go- Consistent implementation across test files and mocks
- Working examples in tests demonstrating correct usage (e.g.,
UserObserver)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for observer implementations and registrations # Test 1: Find all observer implementations echo "Finding observer implementations..." ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) Retrieved(event $_) error { $$$ }' # Test 2: Find observer registrations echo "Finding observer registrations..." rg "Observe\([^)]+\)" -A 2Length of output: 2567
Script:
#!/bin/bash # Find the Observer interface definition and implementations echo "Finding Observer interface..." ast-grep --pattern 'type Observer interface { $$$ }' echo "Finding observer implementations in test files..." rg "type.*Observer.*struct" -A 3 "*_test.go" # Find actual usage of observers in the codebase echo "Finding observer usage..." rg "Retrieved\(" -A 2Length of output: 3533
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
database/migration/default_migrator_test.go (1)
68-71: Add test cases for database-specific rollback scenariosThe TODO comment indicates missing test cases for different database drivers. Consider adding specific test cases that verify rollback behavior with different database schemas and constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- database/console/migration/migrate_rollback_command.go (1 hunks)
- database/migration/default_migrator_test.go (6 hunks)
🔇 Additional comments (7)
database/console/migration/migrate_rollback_command.go (5)
1-8: LGTM!The imports are well-organized and properly reflect the dependencies needed for the migrator interface.
10-19: LGTM! Good dependency injection pattern.The change from using
config.Configtomigration.Migratorimproves separation of concerns and makes the command's dependencies more explicit.
20-29: LGTM!The receiver name change to 'r' follows Go conventions, and the methods remain clear and focused.
40-44: Enforce driver compatibility for batch optionThe usage message indicates that the batch option is only for the default driver, but this restriction isn't enforced in the code.
Line range hint
59-67: LGTM!The error handling through the migrator interface is clean and provides good user feedback.
database/migration/default_migrator_test.go (2)
29-71: LGTM: Well-structured test suite initializationThe new
DefaultMigratorWithDBSuiteis well-organized with proper setup and Docker integration. The TODO comment about adding other drivers provides clear direction for future improvements.
138-217: LGTM: Comprehensive test coverage for getFilesForRollbackThe test cases cover all major scenarios including step-based, batch-based, and last migration rollbacks, along with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
database/console/migration/migrate_rollback_command.go (1)
Line range hint
73-81: Consider enhancing the success message with details.The success message could be more informative by including the number of migrations rolled back.
-ctx.Info("Migration rollback success") +ctx.Info(fmt.Sprintf("Successfully rolled back %d migration(s)", step))database/migration/default_migrator_test.go (3)
29-71: Consider adding cleanup in DefaultMigratorWithDBSuiteThe suite uses Docker for testing but doesn't clean up resources after tests. Consider adding a
TearDownTestmethod to ensure proper cleanup of Docker resources.+func (s *DefaultMigratorWithDBSuite) TearDownTest() { + for _, testQuery := range s.driverToTestQuery { + testQuery.Cleanup() + } +}
68-71: TODO comment needs implementationThe TODO comment indicates missing test cases for different database drivers.
Would you like me to help implement the rollback test cases for the SQLite driver? I can generate a comprehensive test implementation that covers various rollback scenarios.
664-667: Consider adding more schema fieldsThe test migrations only create tables with a single "name" field. Consider adding more fields to better test the schema creation functionality.
func (r *TestMigration) Up() error { return r.schema.Create("users", func(table contractsschema.Blueprint) { + table.ID() table.String("name") + table.String("email").Unique() + table.Timestamp("created_at").Nullable() + table.Timestamp("updated_at").Nullable() }) }Also applies to: 690-693
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- database/console/migration/migrate_rollback_command.go (1 hunks)
- database/console/migration/migrate_rollback_command_test.go (1 hunks)
- database/gorm/query_test.go (24 hunks)
- database/gorm/test_utils.go (2 hunks)
- database/migration/default_migrator_test.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- database/console/migration/migrate_rollback_command_test.go
- database/gorm/test_utils.go
🔇 Additional comments (7)
database/console/migration/migrate_rollback_command.go (3)
10-18: LGTM! Good dependency injection practice.The refactoring to use
migration.Migratorinstead ofconfig.Configimproves the command's design by:
- Following the Interface Segregation Principle
- Making dependencies explicit and focused
- Improving testability
54-71: LGTM! Well-structured input validation and defaults.The validation logic is comprehensive and includes:
- Positive value checks
- Mutual exclusivity of options
- Clear default value handling
- User-friendly error messages
40-44:⚠️ Potential issueImplement driver validation for batch option.
The usage message indicates that the batch option is only supported with the default driver, but this restriction isn't enforced in the code.
Add driver validation before processing the batch option:
if batch > 0 { + if driver := r.migrator.Driver(); driver != "default" { + ctx.Error("The batch option can only be used with the default driver") + return nil + } }Likely invalid or redundant comment.
database/migration/default_migrator_test.go (1)
138-217: LGTM! Well-structured table-driven testsThe
TestGetFilesForRollbackimplementation follows best practices:
- Comprehensive test cases covering success and error scenarios
- Clear test case names
- Proper error handling and assertions
database/gorm/query_test.go (3)
72-72: LGTM: Consistent usage ofAssociationsacross test cases.The changes consistently use
Associationsinstead of the qualifiedorm.Associationsacross all test cases, maintaining uniformity in the codebase.Also applies to: 96-96, 121-121, 146-146, 171-171, 196-196, 232-232, 257-257, 288-288, 467-467, 473-473, 509-509, 515-515, 2299-2299, 2320-2320, 2349-2349, 2372-2372, 2403-2403, 2447-2447, 2543-2543, 3504-3504, 3567-3567
2025-2037: LGTM: Well-structured test for observer retrieval.The test comprehensively covers both positive and negative cases for observer retrieval:
- Tests non-existent observer case with
Product{}- Tests existing observer case with
User{}- Uses clear assertions to verify the expected behavior
2201-2223: LGTM: Comprehensive transaction state testing.The test thoroughly verifies transaction state management:
- Checks initial non-transaction state
- Verifies state during active transaction
- Tests state after commit
- Tests state after rollback
- Includes proper error handling
67920e9 to
8761110
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (19)
database/console/migration/migrate_rollback_command.go (2)
55-62: Make error messages consistent.The error messages for negative values should follow the same pattern.
- ctx.Error("The step option should be a positive integer") + ctx.Error("The step option cannot be negative") - ctx.Error("The batch option should be a positive integer") + ctx.Error("The batch option cannot be negative")
Line range hint
78-81: Enhance success message with rollback details.The success message could be more informative by including the number of migrations rolled back.
- ctx.Info("Migration rollback success") + ctx.Info(fmt.Sprintf("Successfully rolled back %d migration(s)", step))Don't forget to add the import:
import "fmt"database/migration/repository_test.go (1)
114-117: Consider enhancing error handling and method signature.While the implementation is clean, consider these improvements:
- Add error handling for
GetTestSchema- Make the method signature more descriptive by renaming parameters
Consider this improvement:
-func (s *RepositoryTestSuite) initRepository(testQuery *gorm.TestQuery) *Repository { +func (s *RepositoryTestSuite) initRepository(dbQuery *gorm.TestQuery) (*Repository, error) { - testSchema := schema.GetTestSchema(testQuery, s.driverToTestQuery) + testSchema, err := schema.GetTestSchema(dbQuery, s.driverToTestQuery) + if err != nil { + return nil, err + } - return NewRepository(testSchema, "migrations") + return NewRepository(testSchema, "migrations"), nil }database/migration/test_utils.go (1)
Line range hint
31-47: Consider enhancing error handling and timestamp consistency.A few suggestions to improve the migration creation functions:
- The timestamp format in the test data insertions ('2023-03-11 16:07:41') might need to match the specific database's timestamp format requirements.
- Consider using a more robust error handling pattern instead of panic.
- The hardcoded file paths could be made configurable for better testing flexibility.
Here's an example of improved error handling:
func createMysqlMigrations() { + if err := createMigrationFiles(database.DriverMysql); err != nil { + log.Printf("Failed to create MySQL migration files: %v", err) + panic(err) + } } +func createMigrationFiles(driver database.Driver) error { + migrations := getMigrationConfig(driver) + for _, m := range migrations { + if err := file.Create(m.path, m.content); err != nil { + return fmt.Errorf("failed to create migration file %s: %w", m.path, err) + } + } + return nil +}Also applies to: 50-66, 69-86, 89-106
database/service_provider.go (2)
Line range hint
22-41: LGTM: Improved modularity in ORM registration.The changes properly encapsulate ORM-related functionality within the
ormpackage, improving code organization and maintainability. The error handling and dependency checks remain robust.This refactoring aligns well with the Single Responsibility Principle by keeping ORM-related code contained within its dedicated package.
60-62: Consider adding error handling in seeder registration.While the package organization is good, consider adding error handling similar to the ORM and Schema registrations to maintain consistency and robustness.
app.Singleton(seeder.BindingSeeder, func(app foundation.Application) (any, error) { - return seeder.NewSeederFacade(), nil + seederFacade := seeder.NewSeederFacade() + if seederFacade == nil { + return nil, errors.SeederInitialization.SetModule(errors.ModuleSeeder) + } + return seederFacade, nil })support/database/database_test.go (2)
13-21: Consider standardizing column naming conventionThe Model and Timestamps structs are well-structured with proper GORM and JSON tags. However, for consistency across the framework, consider explicitly defining the
idcolumn name in the Model struct, similar to howcreated_atandupdated_atare defined.type Model struct { - ID uint `gorm:"primaryKey" json:"id"` + ID uint `gorm:"primaryKey;column:id" json:"id"` Timestamps }
Line range hint
62-72: Enhance test coverage and descriptionsWhile the test cases properly verify the ID field functionality, consider these improvements:
- The test description "return value with Model" is duplicated. Consider more specific descriptions like "return value with Model (pointer receiver)" and "return value with Model (value receiver)".
- Add test cases to verify the proper handling of timestamp fields.
Example test case for timestamp handling:
{ description: "verify timestamp fields initialization", setup: func(description string) { type User struct { Model Name string } user := User{} assert.NotNil(t, user.CreatedAt) assert.NotNil(t, user.UpdatedAt) }, },Also applies to: 99-109
database/migration/sql_migrator.go (1)
Deprecation of SqlMigrator requires attention before adding new functionality
The search results confirm that
SqlMigratoris marked for removal in v1.16, and there's evidence of an active transition:
The codebase shows a parallel implementation with
DefaultMigratorwhich appears to be the replacement:
database/migration/default_migrator.gocontains the new implementationdatabase/migration/default_migrator_test.goshows comprehensive test coverageThe TODO comment at
database/migration/default_migrator.goexplicitly mentions moving functionality from SQL migrator to the new implementation:// TODO Remove this function and move the logic to the migrate:fresh command when the sql migrator is removed.Adding new functionality to
SqlMigratorshould be reconsidered:
- The code should be added to
DefaultMigratorinstead- Any new features should be documented in the new implementation
- The PR should be updated to reflect this architectural change
🔗 Analysis chain
Line range hint
26-26: Verify the deprecation strategy for SqlMigrator.The struct is marked for removal in v1.16, yet we're adding new functionality to it. This raises some concerns:
- What's the replacement for this functionality?
- Should we be adding new features to code that's being deprecated?
- How will users migrate to the new implementation?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any replacement implementations or deprecation notices echo "Searching for potential replacement implementations..." rg -l "TODO.*Remove in v1.16" rg -l "Deprecated.*v1.16" echo "Searching for related documentation..." fd -g "CHANGELOG.md" -g "UPGRADING.md" -g "README.md" -X rg "v1.16|SqlMigrator"Length of output: 532
Script:
#!/bin/bash # Let's try a more comprehensive search strategy # 1. Look for any migration-related files that might contain replacement implementations echo "Searching for migration-related files..." fd -e go -e md . | grep -i "migrat" # 2. Search for any deprecation or v1.16 mentions in the codebase echo -e "\nSearching for v1.16 mentions..." rg "v1\.16" -C 2 # 3. Look for potential replacement patterns or new migration implementations echo -e "\nSearching for potential new migration implementations..." ast-grep --pattern 'type $_ struct { $$$ }' | grep -A 5 -B 5 "[Mm]igrat" # 4. Check if there are any issues or PRs discussing this deprecation echo -e "\nSearching for TODOs and related comments..." rg "TODO|FIXME|@deprecated" -C 2Length of output: 31882
database/schema/schema_test.go (3)
Line range hint
29-35: Enhance TODO comment with specific drivers.Consider making the TODO comment more specific by listing the planned database drivers (e.g., MySQL, SQLite) to better track implementation progress.
-// TODO Add other drivers +// TODO Add support for other drivers: MySQL, SQLite, SQL Server
Line range hint
93-153: Consider moving commented code to a separate issue.The large block of commented test code could be moved to a separate GitHub issue for tracking future column-related implementations. This would improve code readability while preserving the implementation plans.
Would you like me to help create a GitHub issue to track these pending column-related test implementations?
Line range hint
157-171: Use parameter binding in SQL test.While this is a test, it's good practice to demonstrate proper SQL query parameter binding:
-schema.Sql("insert into goravel_sql (name) values ('goravel');") +schema.Sql("insert into goravel_sql (name) values (?);", "goravel")This promotes better security practices even in test code.
database/factory/factory_test.go (1)
Line range hint
74-82: Consider using dynamic IDs in AuthorFactory definition.The current implementation uses hardcoded values:
"ID": 1, "BookID": 2,This could make tests brittle and harder to maintain. Consider using dynamic values or sequence generators for better test isolation.
Example improvement:
func (a *AuthorFactory) Definition() map[string]any { faker := gofakeit.New(0) return map[string]any{ - "ID": 1, - "BookID": 2, + "ID": faker.Uint32(), + "BookID": faker.Uint32(), "Name": faker.Name(), // ... rest of the fields } }database/migration/default_migrator.go (1)
65-94: Add error context for better debugging.While the error handling is good, adding context to errors would help with debugging. Consider wrapping errors with additional information.
if err != nil { - return err + return fmt.Errorf("failed to get files for rollback: %w", err) }database/gorm/test_models.go (1)
17-29: Well-structured base types with proper GORM integration.The new base types provide a clean separation of concerns:
Modelcombines ID and timestampsSoftDeletesleverages GORM's native soft delete supportTimestampsuses framework's standardcarbon.DateTimeThis modular approach allows for flexible composition in model structs.
Consider documenting these base types in the framework's documentation to establish them as the standard pattern for model definitions.
auth/auth_test.go (1)
Line range hint
1-729: Consider grouping related test cases using subtests.While the test coverage is comprehensive, the test cases could be better organized using Go's subtests feature. This would improve readability and make it easier to run specific test scenarios.
Here's an example of how to refactor the login-related tests:
func (s *AuthTestSuite) TestLogin_Model() { + s.Run("when using default model", func() { s.mockConfig.On("GetString", "jwt.secret").Return("Goravel").Once() s.mockConfig.On("GetInt", "jwt.ttl").Return(2).Once() var user User user.ID = 1 user.Name = "Goravel" token, err := s.auth.Login(&user) s.NotEmpty(token) s.Nil(err) s.mockConfig.AssertExpectations(s.T()) + }) }database/migration/default_migrator_test.go (2)
68-71: TODO comment needs implementationThe TODO comment indicates missing test cases for the
TestRollbackmethod inDefaultMigratorWithDBSuite.Would you like me to help implement the test cases for different database drivers? This would include:
- Setting up test scenarios for PostgreSQL
- Verifying rollback behavior
- Testing error conditions
700-716: Consider adding error messages in TestErrorMigrationThe
TestErrorMigrationuses a generic error (assert.AnError). Consider using more specific error messages to better identify failure scenarios.-func (r *TestErrorMigration) Up() error { - return assert.AnError +func (r *TestErrorMigration) Up() error { + return errors.New("failed to create companies table") } -func (r *TestErrorMigration) Down() error { - return assert.AnError +func (r *TestErrorMigration) Down() error { + return errors.New("failed to drop companies table") }database/gorm/query_test.go (1)
Line range hint
2025-2223: Consider adding documentation for the new test methodsWhile the test methods are well-structured, consider adding documentation comments to explain:
- The purpose of each test method
- The specific scenarios being tested
- Any important assumptions or prerequisites
Example documentation format:
// TestGetObserver verifies that the correct observer is retrieved for a given model. // It tests both the case where an observer exists and where it doesn't.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
mocks/database/migration/Migrator.gois excluded by!mocks/**mocks/database/orm/Orm.gois excluded by!mocks/**mocks/database/orm/Query.gois excluded by!mocks/**mocks/database/orm/QueryWithObserver.gois excluded by!mocks/**mocks/database/orm/QueryWithSetContext.gois excluded by!mocks/**
📒 Files selected for processing (34)
- auth/auth_test.go (1 hunks)
- contracts/database/migration/migrator.go (1 hunks)
- contracts/database/orm/observer.go (1 hunks)
- contracts/database/orm/orm.go (4 hunks)
- database/console/migration/migrate_rollback_command.go (1 hunks)
- database/console/migration/migrate_rollback_command_test.go (1 hunks)
- database/factory/factory.go (1 hunks)
- database/factory/factory_test.go (4 hunks)
- database/gorm/event.go (1 hunks)
- database/gorm/event_test.go (4 hunks)
- database/gorm/query.go (23 hunks)
- database/gorm/query_test.go (24 hunks)
- database/gorm/test_models.go (7 hunks)
- database/gorm/test_utils.go (2 hunks)
- database/migration/default_creator_test.go (4 hunks)
- database/migration/default_migrator.go (6 hunks)
- database/migration/default_migrator_test.go (6 hunks)
- database/migration/repository_test.go (3 hunks)
- database/migration/sql_migrator.go (1 hunks)
- database/migration/sql_migrator_test.go (2 hunks)
- database/migration/stubs.go (3 hunks)
- database/migration/test_utils.go (1 hunks)
- database/orm/model.go (0 hunks)
- database/orm/orm.go (4 hunks)
- database/orm/orm_test.go (2 hunks)
- database/schema/schema.go (2 hunks)
- database/schema/schema_test.go (5 hunks)
- database/schema/test_utils.go (1 hunks)
- database/seeder/seeder.go (1 hunks)
- database/seeder/seeder_test.go (1 hunks)
- database/service_provider.go (4 hunks)
- foundation/application_test.go (2 hunks)
- foundation/container.go (4 hunks)
- support/database/database_test.go (3 hunks)
💤 Files with no reviewable changes (1)
- database/orm/model.go
🚧 Files skipped from review as they are similar to previous changes (17)
- contracts/database/migration/migrator.go
- contracts/database/orm/observer.go
- database/console/migration/migrate_rollback_command_test.go
- database/factory/factory.go
- database/gorm/event.go
- database/gorm/event_test.go
- database/gorm/test_utils.go
- database/migration/default_creator_test.go
- database/migration/sql_migrator_test.go
- database/migration/stubs.go
- database/orm/orm_test.go
- database/schema/schema.go
- database/schema/test_utils.go
- database/seeder/seeder.go
- database/seeder/seeder_test.go
- foundation/application_test.go
- foundation/container.go
🔇 Additional comments (42)
database/console/migration/migrate_rollback_command.go (1)
10-18: LGTM! Good dependency injection pattern.The change from
configtomigratorimproves the design by:
- Following interface segregation principle
- Making dependencies explicit
- Improving testability
database/migration/repository_test.go (3)
39-39: LGTM! Clean repository initialization.The change from mock to real database initialization is implemented correctly.
52-52: LGTM! Comprehensive test setup.The repository initialization and subsequent test cases provide good coverage of migration functionality.
Line range hint
1-117: Verify test coverage for multiple database drivers.The test suite currently only includes PostgreSQL. Consider adding tests for other supported database drivers to ensure the migration functionality works across all supported databases.
Let's check what other database drivers are supported:
Consider extending the test suite to cover all supported database drivers by:
- Adding Docker containers for each supported database
- Including them in the
driverToTestQuerymap- Running the same test cases against each driver
database/migration/test_utils.go (1)
9-13: Well-structured model definition!The explicit field declarations with proper GORM and JSON tags improve clarity and maintainability. The use of
carbon.DateTimefor timestamps and explicit ID field is a good practice.database/service_provider.go (3)
12-14: LGTM: Import changes align with package reorganization.The new imports correctly reflect the restructured package organization mentioned in the PR objectives.
Line range hint
42-59: LGTM: Schema registration follows consistent package organization.The schema registration follows the same pattern of improved modularity, maintaining consistent error handling and dependency validation.
97-97: Verify backward compatibility of MigrateRollbackCommand changes.The change to use
migratorinstead ofconfiginMigrateRollbackCommandlooks good and aligns with the PR objective. However, let's verify that this change maintains backward compatibility.✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the backward compatibility:
MigrateRollbackCommand changes maintain backward compatibility
The change to use
migratorinstead ofconfigis safe because:
- The command's public interface (Signature, Description, Extend) remains unchanged
- The command's functionality is properly tested in
migrate_rollback_command_test.go- No other parts of the codebase directly depend on the internal implementation of MigrateRollbackCommand
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing usages of MigrateRollbackCommand to ensure no breaking changes rg -A 5 "MigrateRollbackCommand" --type go # Look for any existing tests that verify rollback functionality rg -l "TestMigrateRollback" --type goLength of output: 4334
support/database/database_test.go (2)
10-10: LGTM: Import changes align with new timestamp handlingThe switch from orm to carbon package for timestamp handling is consistent with the PR's restructuring objectives.
13-21: Verify migration system compatibilityThe new Model and Timestamps structs look good, but we should verify their compatibility with the updated migrate:rollback command that now supports multiple database drivers.
Also applies to: 62-72, 99-109
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the migration system and rollback functionality.
Model and Timestamps structs are compatible with migration system
The new Model and Timestamps structs are fully compatible with the updated migrate:rollback command. The evidence shows:
- The migration system operates at the schema level through SQL queries, not through the model structs directly
- Both DefaultMigrator and SqlMigrator implementations handle rollbacks independently of the model structures
- The rollback functionality is database-driver agnostic, working through interfaces that don't depend on these model definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for migration files that might use these models rg -l "type.*struct.*{.*Model.*}" | grep -i "migration" # Check for database driver specific implementations rg -l "type.*struct.*{.*Timestamps.*}" | grep -i "driver"Length of output: 364
Script:
#!/bin/bash # Search for migration files that might use these models rg -l "type.*struct.*\{.*Model" | grep -i "migration" # Check for database driver specific implementations rg -l "type.*struct.*\{.*Timestamps" | grep -i "driver" # Look for usage of Model and Timestamps in migration-related files rg -l "Model|Timestamps" | grep -i "migr" # Check for database driver implementations that might interact with these structs rg -l "Model|Timestamps" | grep -i "driver" # Look for any migration rollback related code rg -C 3 "rollback" -iLength of output: 27095
database/orm/orm.go (8)
1-13: LGTM: Package organization and imports are well-structured.The package rename to
ormbetter reflects its location in thedatabase/ormdirectory, following Go's package naming conventions.
15-26: LGTM: Well-structured struct with proper synchronization.The addition of
mutexaddresses the concurrency concerns raised in previous reviews, andmodelToObserverproperly encapsulates the observer pattern implementation.
Line range hint
29-61: LGTM: Constructor properly initializes all fields.The
NewOrmconstructor correctly initializes all fields, including the newmodelToObserverand synchronization primitives.
63-81: LGTM: Robust connection management with proper error handling.The
Connectionmethod correctly:
- Propagates observers to new connections
- Handles connection failures gracefully
- Maintains connection state
83-89: LGTM: Clean and concise DB and Factory implementations.
95-113: LGTM: Thread-safe observer implementation with proper propagation.The observer implementation correctly:
- Uses mutex for thread-safe observer registration
- Propagates observers to all relevant queries
- Handles type assertions safely
115-122: LGTM: Query management methods are straightforward.
Line range hint
124-156: LGTM: Transaction and context handling with proper type assertions.The implementation correctly:
- Uses type assertions to check for context support
- Propagates context to all relevant queries
- Maintains transaction integrity
database/schema/schema_test.go (2)
39-50: LGTM! Well-structured test implementation.The test follows best practices with proper setup, teardown, and clear assertions.
57-75: Remove mock expectation for search_path.The test still contains a mock expectation that seems inconsistent with the PR's goal of removing mock dependencies:
testQuery.MockConfig().EXPECT().GetString("database.connections.postgres.search_path").Return("").Once()database/factory/factory_test.go (3)
19-31: Well-structured base types with proper separation of concerns!The base types are well-designed with:
- Clear separation between core fields (Model), soft deletion (SoftDeletes), and temporal tracking (Timestamps)
- Proper GORM and JSON tags
- Consistent use of carbon.DateTime for timestamp fields
33-38: Domain types follow consistent patterns and best practices!The domain types (User, Author, House) are well-structured with:
- Consistent use of base types (Model, SoftDeletes)
- Proper implementation of polymorphic relationships
- Clear field definitions
Also applies to: 58-61, 84-89
Line range hint
91-248: Comprehensive test coverage with proper setup and assertions!The test suite is well-implemented with:
- Complete coverage of factory operations (Make, Create, CreateQuietly)
- Proper database setup using Docker
- Platform-specific handling (Windows)
- Thorough assertions for all model fields
database/migration/default_migrator.go (2)
22-34: LGTM! Efficient initialization of migrations map.The constructor now properly initializes the migrations map, providing O(1) lookup performance for migrations by their signature. This is a good optimization that addresses previous performance concerns.
111-121: Clarify the priority between step and batch parameters.The method handles both
stepandbatchparameters, but the relationship between them isn't clearly defined. Consider:
- Adding validation to ensure only one parameter is provided
- Documenting the priority order in the method comments
contracts/database/orm/orm.go (3)
85-86: LGTM! Transaction awareness enhancement.The
InTransactionmethod is a valuable addition that improves transaction state awareness in the ORM, which is particularly useful for the migration rollback enhancement.
176-182: LGTM! Well-designed interface segregation.The introduction of
QueryWithSetContextandQueryWithObserveras separate interfaces follows the Interface Segregation Principle, allowing for flexible implementation of context and observer functionality. This is a cleaner approach than adding these methods directly to the Query interface.
25-26: LGTM! Verify transaction-related usage.The
SetQuerymethod is well-placed and follows Go conventions. Based on the PR objectives, this addition supports the multi-database driver enhancement for migrations.Let's verify its usage in migration-related code:
✅ Verification successful
SetQuery method is correctly implemented and used
The verification confirms that
SetQueryis properly used in the migration system:
- Used within transaction blocks to set the transaction query
- Properly reset to default query after transaction completion
- Implementation matches the intended purpose for multi-database driver support in migrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SetQuery usage in migration-related files rg -l "SetQuery" | rg -i "migrat"Length of output: 114
Script:
#!/bin/bash # Check the implementation of SetQuery in migration files rg "SetQuery" database/migration/default_migrator.go database/migration/default_migrator_test.go -B 2 -A 2Length of output: 4938
database/gorm/test_models.go (2)
7-10: LGTM: Import changes are appropriate.The new imports support the custom model types being introduced.
Line range hint
31-404: LGTM: Model updates are consistent and well-structured.All model structs have been properly updated to use the new base types while maintaining their existing functionality:
- Basic models correctly embed the
Modeltype- Soft-deletable models properly combine
ModelandSoftDeletes- All relationship definitions and custom methods remain intact
auth/auth_test.go (1)
26-29: LGTM! Well-structured model definition.The User struct follows GORM best practices with proper field definitions and tags. The explicit primary key and timestamp fields enhance the model's clarity and functionality.
database/migration/default_migrator_test.go (3)
138-217: LGTM! Comprehensive test coverage for getFilesForRollbackThe test cases thoroughly cover:
- Step-based rollback
- Batch-based rollback
- Last migration rollback
- Error handling for all scenarios
664-698: LGTM! Well-structured test migrationsThe test migrations (
TestMigrationandTestConnectionMigration) are well-structured with:
- Clear Up/Down implementations
- Proper schema modifications
- Connection specification where needed
660-662: Verify migration signature uniquenessMultiple migrations (
TestMigration,TestConnectionMigration, andTestErrorMigration) are using similar timestamps in their signatures. This could cause conflicts when running migrations.Let's verify the uniqueness of migration signatures in the codebase:
Also applies to: 682-684, 707-709
database/gorm/query.go (5)
27-39: LGTM! Well-structured changes to Query struct.The addition of mutex and modelToObserver fields improves thread safety and enables the observer pattern implementation.
532-539: LGTM! Thread-safe observer registration.The Observe method is properly protected by mutex, addressing the concurrency concerns raised in previous reviews.
576-578: LGTM! Clean implementation of transaction-related methods.The Instance and InTransaction methods provide clean interfaces for transaction management.
Also applies to: 595-599
1300-1317: LGTM! Robust observer lookup implementation.The getObserver method properly handles type comparison by checking both package path and type name, preventing incorrect matches across different packages.
1214-1214: LGTM! Consistent usage of Associations constant.The code consistently uses the locally defined Associations constant across all relevant methods, improving code organization and maintainability.
Also applies to: 1326-1344, 1365-1366, 1406-1406, 1420-1426, 1453-1455, 1487-1505, 1518-1518
database/gorm/query_test.go (3)
72-72: LGTM: Consistent replacement oform.AssociationswithAssociationsThe changes consistently replace
orm.AssociationswithAssociationsacross all test methods. This refactoring maintains functionality while simplifying the reference.Also applies to: 96-96, 121-121, 146-146, 171-171, 196-196, 232-232, 257-257, 288-288, 467-467, 473-473, 509-509, 515-515, 2299-2299, 2320-2320, 2349-2349, 2372-2372, 2403-2403, 2447-2447, 2543-2543, 3504-3504, 3567-3567
2025-2037: LGTM: Well-structured test for observer retrievalThe test method thoroughly verifies the observer retrieval functionality:
- Tests the case where no observer exists for a model (Product)
- Tests the case where an observer exists for a model (User)
- Uses appropriate assertions to verify the results
2201-2223: LGTM: Comprehensive transaction state testingThe test method effectively verifies transaction state management:
- Tests initial non-transaction state
- Verifies state during active transaction
- Tests state after commit
- Tests state after rollback
- Includes appropriate assertions for each state
| type DefaultMigratorWithDBSuite struct { | ||
| suite.Suite | ||
| driverToTestQuery map[contractsdatabase.Driver]*gorm.TestQuery | ||
| } | ||
|
|
||
| func TestDefaultMigratorWithDBSuite(t *testing.T) { | ||
| if env.IsWindows() { | ||
| t.Skip("Skipping tests that use Docker") | ||
| } | ||
|
|
||
| suite.Run(t, &DefaultMigratorWithDBSuite{}) | ||
| } | ||
|
|
||
| func (s *DefaultMigratorWithDBSuite) SetupTest() { | ||
| // TODO Add other drivers | ||
| postgresDocker := docker.Postgres() | ||
| postgresQuery := gorm.NewTestQuery(postgresDocker, true) | ||
| s.driverToTestQuery = map[contractsdatabase.Driver]*gorm.TestQuery{ | ||
| contractsdatabase.DriverPostgres: postgresQuery, | ||
| } | ||
| } | ||
|
|
||
| func (s *DefaultMigratorWithDBSuite) TestRun() { | ||
| for driver, testQuery := range s.driverToTestQuery { | ||
| s.Run(driver.String(), func() { | ||
| schema := databaseschema.GetTestSchema(testQuery, s.driverToTestQuery) | ||
| testMigration := NewTestMigration(schema) | ||
| schema.Register([]contractsschema.Migration{ | ||
| testMigration, | ||
| }) | ||
|
|
||
| migrator := NewDefaultMigrator(nil, schema, "migrations") | ||
|
|
||
| s.NoError(migrator.Run()) | ||
| s.True(schema.HasTable("users")) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TODO Add rollback test cases after implementing Sqlite driver, to test migrating different databases. | ||
| func (s *DefaultMigratorWithDBSuite) TestRollback() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Consider adding cleanup in DefaultMigratorWithDBSuite
The test suite uses Docker for PostgreSQL testing but doesn't clean up resources after tests. This could lead to resource leaks.
Add a TearDownTest method to clean up Docker resources:
+func (s *DefaultMigratorWithDBSuite) TearDownTest() {
+ for _, testQuery := range s.driverToTestQuery {
+ testQuery.Cleanup()
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type DefaultMigratorWithDBSuite struct { | |
| suite.Suite | |
| driverToTestQuery map[contractsdatabase.Driver]*gorm.TestQuery | |
| } | |
| func TestDefaultMigratorWithDBSuite(t *testing.T) { | |
| if env.IsWindows() { | |
| t.Skip("Skipping tests that use Docker") | |
| } | |
| suite.Run(t, &DefaultMigratorWithDBSuite{}) | |
| } | |
| func (s *DefaultMigratorWithDBSuite) SetupTest() { | |
| // TODO Add other drivers | |
| postgresDocker := docker.Postgres() | |
| postgresQuery := gorm.NewTestQuery(postgresDocker, true) | |
| s.driverToTestQuery = map[contractsdatabase.Driver]*gorm.TestQuery{ | |
| contractsdatabase.DriverPostgres: postgresQuery, | |
| } | |
| } | |
| func (s *DefaultMigratorWithDBSuite) TestRun() { | |
| for driver, testQuery := range s.driverToTestQuery { | |
| s.Run(driver.String(), func() { | |
| schema := databaseschema.GetTestSchema(testQuery, s.driverToTestQuery) | |
| testMigration := NewTestMigration(schema) | |
| schema.Register([]contractsschema.Migration{ | |
| testMigration, | |
| }) | |
| migrator := NewDefaultMigrator(nil, schema, "migrations") | |
| s.NoError(migrator.Run()) | |
| s.True(schema.HasTable("users")) | |
| }) | |
| } | |
| } | |
| // TODO Add rollback test cases after implementing Sqlite driver, to test migrating different databases. | |
| func (s *DefaultMigratorWithDBSuite) TestRollback() { | |
| } | |
| type DefaultMigratorWithDBSuite struct { | |
| suite.Suite | |
| driverToTestQuery map[contractsdatabase.Driver]*gorm.TestQuery | |
| } | |
| func TestDefaultMigratorWithDBSuite(t *testing.T) { | |
| if env.IsWindows() { | |
| t.Skip("Skipping tests that use Docker") | |
| } | |
| suite.Run(t, &DefaultMigratorWithDBSuite{}) | |
| } | |
| func (s *DefaultMigratorWithDBSuite) SetupTest() { | |
| // TODO Add other drivers | |
| postgresDocker := docker.Postgres() | |
| postgresQuery := gorm.NewTestQuery(postgresDocker, true) | |
| s.driverToTestQuery = map[contractsdatabase.Driver]*gorm.TestQuery{ | |
| contractsdatabase.DriverPostgres: postgresQuery, | |
| } | |
| } | |
| func (s *DefaultMigratorWithDBSuite) TearDownTest() { | |
| for _, testQuery := range s.driverToTestQuery { | |
| testQuery.Cleanup() | |
| } | |
| } | |
| func (s *DefaultMigratorWithDBSuite) TestRun() { | |
| for driver, testQuery := range s.driverToTestQuery { | |
| s.Run(driver.String(), func() { | |
| schema := databaseschema.GetTestSchema(testQuery, s.driverToTestQuery) | |
| testMigration := NewTestMigration(schema) | |
| schema.Register([]contractsschema.Migration{ | |
| testMigration, | |
| }) | |
| migrator := NewDefaultMigrator(nil, schema, "migrations") | |
| s.NoError(migrator.Run()) | |
| s.True(schema.HasTable("users")) | |
| }) | |
| } | |
| } | |
| // TODO Add rollback test cases after implementing Sqlite driver, to test migrating different databases. | |
| func (s *DefaultMigratorWithDBSuite) TestRollback() { | |
| } |
| modelToObserver []contractsorm.ModelToObserver | ||
| mutex sync.Mutex |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using RWMutex for better performance.
While the current mutex implementation is correct, using sync.RWMutex could provide better performance by allowing concurrent reads of modelToObserver while still protecting writes.
- mutex sync.Mutex
+ mutex sync.RWMutexThen update the getObserver method:
func (r *Query) getObserver(dest any) contractsorm.Observer {
+ r.mutex.RLock()
+ defer r.mutex.RUnlock()
destType := reflect.TypeOf(dest)
// ... rest of the method
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
database/migration/default_migrator.go (1)
65-94: Add error context for better debugging.While the Rollback implementation is solid, adding context to errors would improve debugging capabilities.
Consider wrapping errors with additional context:
func (r *DefaultMigrator) Rollback(step, batch int) error { files, err := r.getFilesForRollback(step, batch) if err != nil { - return err + return fmt.Errorf("failed to get files for rollback: %w", err) } // ... for _, file := range files { migration := r.getMigrationViaFile(file) if migration == nil { color.Warnf("Migration not found: %s\n", file.Migration) continue } if err := r.runDown(migration); err != nil { - return err + return fmt.Errorf("failed to run down migration %s: %w", migration.Signature(), err) } // ... } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- database/migration/default_migrator.go (6 hunks)
🔇 Additional comments (4)
database/migration/default_migrator.go (4)
7-11: LGTM! Well-structured imports and struct modifications.The addition of the
migrationsmap for O(1) lookups aligns with the previous review suggestion for performance optimization.Also applies to: 17-19
22-34: LGTM! Efficient constructor implementation.The constructor properly initializes the migrations map, enabling O(1) lookups when processing migrations. This is a performance improvement over the previous implementation.
174-216: LGTM! Well-structured migration execution with proper connection management.The implementation:
- Extracts common migration logic into a reusable helper
- Uses defer for reliable connection cleanup
- Properly handles transactions and connection resets
- Follows the suggestions from previous reviews
This is a significant improvement in code organization and reliability.
111-121: Verify the priority order of step and batch parameters.The method prioritizes
stepoverbatchwhen both are provided. Please verify if this is the intended behavior and consider documenting this priority in the method's comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
database/migration/default_migrator.go (2)
65-94: Add more context to error messages in Rollback method.While the error handling is present, adding more context would help with debugging. Consider wrapping errors with additional information.
Apply this diff to improve error messages:
func (r *DefaultMigrator) Rollback(step, batch int) error { files, err := r.getFilesForRollback(step, batch) if err != nil { - return err + return fmt.Errorf("failed to get files for rollback: %w", err) } // ... for _, file := range files { migration := r.getMigrationViaFile(file) // ... if err := r.runDown(migration); err != nil { - return err + return fmt.Errorf("failed to run down migration %s: %w", migration.Signature(), err) } // ... } return nil }
111-121: Document the priority between step and batch parameters.The method prioritizes
stepoverbatch, but this behavior isn't documented. Consider adding documentation to clarify this behavior.Add documentation like this:
+// getFilesForRollback retrieves migration files for rollback based on step or batch. +// If step > 0, it returns the last n migrations where n = step. +// If batch > 0, it returns all migrations in the specified batch. +// If neither is specified, it returns the last batch of migrations. func (r *DefaultMigrator) getFilesForRollback(step, batch int) ([]contractsmigration.File, error) {
| func (r *DefaultMigrator) runDown(migration contractsschema.Migration) error { | ||
| defaultConnection := r.schema.GetConnection() | ||
| defaultQuery := r.schema.Orm().Query() | ||
| if connectionMigration, ok := migration.(contractsschema.Connection); ok { | ||
| r.schema.SetConnection(connectionMigration.Connection()) | ||
| defer r.schema.SetConnection(previousConnection) | ||
| } | ||
|
|
||
| if err := file.Up(); err != nil { | ||
| return err | ||
| defer func() { | ||
| // reset the connection and query to default, to avoid err and panic | ||
| r.schema.Orm().SetQuery(defaultQuery) | ||
| r.schema.SetConnection(defaultConnection) | ||
| }() | ||
|
|
||
| return r.schema.Orm().Transaction(func(tx orm.Query) error { | ||
| r.schema.Orm().SetQuery(tx) | ||
|
|
||
| if err := migration.Down(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // repository.Log should be called in the default connection. | ||
| r.schema.Orm().SetQuery(defaultQuery) | ||
| r.schema.SetConnection(defaultConnection) | ||
|
|
||
| return r.repository.Delete(migration.Signature()) | ||
| }) | ||
| } | ||
|
|
||
| func (r *DefaultMigrator) runUp(migration contractsschema.Migration, batch int) error { | ||
| defaultConnection := r.schema.GetConnection() | ||
| defaultQuery := r.schema.Orm().Query() | ||
| if connectionMigration, ok := migration.(contractsschema.Connection); ok { | ||
| r.schema.SetConnection(connectionMigration.Connection()) | ||
| } | ||
|
|
||
| return r.repository.Log(file.Signature(), batch) | ||
| defer func() { | ||
| // reset the connection and query to default, to avoid err and panic | ||
| r.schema.Orm().SetQuery(defaultQuery) | ||
| r.schema.SetConnection(defaultConnection) | ||
| }() | ||
|
|
||
| return r.schema.Orm().Transaction(func(tx orm.Query) error { | ||
| r.schema.Orm().SetQuery(tx) | ||
|
|
||
| if err := migration.Up(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // repository.Log should be called in the default connection. | ||
| r.schema.Orm().SetQuery(defaultQuery) | ||
| r.schema.SetConnection(defaultConnection) | ||
|
|
||
| return r.repository.Log(migration.Signature(), batch) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common transaction logic to reduce duplication.
The runUp and runDown methods share similar connection management and transaction logic. Consider extracting this into a helper method.
Create a helper method like this:
func (r *DefaultMigrator) runMigrationWithTransaction(
migration contractsschema.Migration,
operation func(orm.Query) error,
) error {
defaultConnection := r.schema.GetConnection()
defaultQuery := r.schema.Orm().Query()
if connectionMigration, ok := migration.(contractsschema.Connection); ok {
r.schema.SetConnection(connectionMigration.Connection())
}
defer func() {
r.schema.Orm().SetQuery(defaultQuery)
r.schema.SetConnection(defaultConnection)
}()
return r.schema.Orm().Transaction(func(tx orm.Query) error {
r.schema.Orm().SetQuery(tx)
return operation(tx)
})
}Then simplify the existing methods:
func (r *DefaultMigrator) runDown(migration contractsschema.Migration) error {
return r.runMigrationWithTransaction(migration, func(tx orm.Query) error {
if err := migration.Down(); err != nil {
return err
}
// Use default connection for repository operations
r.schema.Orm().SetQuery(r.schema.Orm().Query())
r.schema.SetConnection(r.schema.GetConnection())
return r.repository.Delete(migration.Signature())
})
}
func (r *DefaultMigrator) runUp(migration contractsschema.Migration, batch int) error {
return r.runMigrationWithTransaction(migration, func(tx orm.Query) error {
if err := migration.Up(); err != nil {
return err
}
// Use default connection for repository operations
r.schema.Orm().SetQuery(r.schema.Orm().Query())
r.schema.SetConnection(r.schema.GetConnection())
return r.repository.Log(migration.Signature(), batch)
})
}
📑 Description
orm.gotoorm/orm.go;factory.gotofactory/factory.go;seeder.gotoseeder/seeder.go;migrate:rollbackcommand supports multiple drivers;Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
databasetoorm,schema, andseeder.Tests
✅ Checks