Conversation
WalkthroughThis pull request adds support for renaming columns in the database schema migrator. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as Blueprint
participant G as Grammar
U->>B: RenameColumn("oldColumn", "newColumn")
B->>B: Add CommandRenameColumn with from and to values
U->>B: ToSql(grammar)
B->>G: CompileRenameColumn(schema, blueprint, command)
G-->>B: Return SQL statement
B-->>U: SQL statement for renaming column
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=error msg="Running error: context loading failed: no go files to analyze: running ✨ Finishing Touches
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #851 +/- ##
=======================================
Coverage 67.54% 67.54%
=======================================
Files 151 151
Lines 10343 10343
=======================================
Hits 6986 6986
Misses 2990 2990
Partials 367 367 ☔ View full report in Codecov by Sentry. |
|
Thanks for the PRs, FYI, please add the |
We need to merge the PR in the driver repository first to ensure the unit tests run successfully. |
|
Yes, is this PR ready? |
It’s been ready for a while, except for one small question about MySQL that needs to be confirmed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/schema_test.go (1)
1860-1862: Consider adding test cases for edge cases.While the basic functionality is well tested, consider adding test cases for:
- Renaming a column with foreign key constraints
- Renaming a column that's part of an index
- Renaming a column with default values or other constraints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/database/schema/blueprint.go(1 hunks)contracts/database/schema/grammar.go(1 hunks)database/schema/blueprint.go(3 hunks)mocks/database/schema/Blueprint.go(1 hunks)mocks/database/schema/Grammar.go(1 hunks)tests/schema_test.go(1 hunks)
🔇 Additional comments (10)
contracts/database/schema/grammar.go (1)
52-53: LGTM! Well-structured interface addition.The new
CompileRenameColumnmethod follows the established pattern of other Compile* methods in the interface, with clear documentation and proper parameter types.contracts/database/schema/blueprint.go (1)
106-107: LGTM! Clean interface extension.The new
RenameColumnmethod has a clear, intuitive signature with well-named parameters that match the method's purpose.database/schema/blueprint.go (3)
30-30: LGTM! Consistent constant definition.The new command constant follows the established naming pattern and is properly placed in alphabetical order.
339-347: LGTM! Clean implementation of RenameColumn.The implementation is straightforward and follows the established pattern of command construction in the codebase.
517-518: LGTM! Proper ToSql extension.The new case for handling rename column commands follows the existing pattern and is correctly placed in alphabetical order.
mocks/database/schema/Grammar.go (1)
1178-1223: LGTM! Well-structured mock implementation.The mock implementation follows the established mockery pattern and includes all necessary helper methods for testing.
tests/schema_test.go (1)
1854-1872: LGTM! The test implementation is thorough and well-structured.The test case effectively verifies the column renaming functionality:
- Tests across all supported database drivers
- Validates both the existence and non-existence of columns
- Uses clear and descriptive variable names
mocks/database/schema/Blueprint.go (3)
1-9: LGTM! Standard generated code header and imports.
2309-2312: LGTM! Clean and correct mock implementation of RenameColumn.The implementation follows the standard mock pattern and correctly handles the method parameters.
2314-2341: LGTM! Complete implementation of mock helper types and methods.The helper types and methods are correctly implemented following the mockery pattern, providing all necessary functionality for testing:
- Blueprint_RenameColumn_Call struct
- Run method for executing custom logic
- Return method for specifying return values
- RunAndReturn method for combining both
hwbrzzl
left a comment
There was a problem hiding this comment.
LGTM, we can finish the mysql PR first before merging this.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: b808b5f | Previous: b635f38 | Ratio |
|---|---|---|---|
Benchmark_DecryptString |
5275 ns/op 1592 B/op 17 allocs/op |
1975 ns/op 1592 B/op 17 allocs/op |
2.67 |
Benchmark_DecryptString - ns/op |
5275 ns/op |
1975 ns/op |
2.67 |
This comment was automatically generated by workflow using github-action-benchmark.
accb5a8 to
63fe51e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/schema/blueprint_test.go (1)
664-665: Add test cases for error scenarios and RenameColumn functionality.The test suite should be expanded to include:
- Error scenarios for ToSql
- Test cases for the new RenameColumn functionality
Add these test cases to the tests array:
{ name: "Return error when CompileRenameColumn fails", setup: func() { s.blueprint.RenameColumn("old_name", "new_name") mockGrammar.EXPECT().GetAttributeCommands().Return([]string{}).Once() mockGrammar.EXPECT().CompileRenameColumn(s.blueprint.schema, s.blueprint, s.blueprint.commands[0]). Return("", assert.AnError).Once() }, expectedSQL: nil, expectedErr: assert.AnError, }, { name: "Successfully rename column", setup: func() { s.blueprint.RenameColumn("old_name", "new_name") mockGrammar.EXPECT().GetAttributeCommands().Return([]string{}).Once() mockGrammar.EXPECT().CompileRenameColumn(s.blueprint.schema, s.blueprint, s.blueprint.commands[0]). Return("ALTER TABLE users RENAME COLUMN old_name TO new_name", nil).Once() }, expectedSQL: []string{"ALTER TABLE users RENAME COLUMN old_name TO new_name"}, expectedErr: nil, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
contracts/database/schema/blueprint.go(2 hunks)contracts/database/schema/column.go(1 hunks)contracts/database/schema/grammar.go(1 hunks)database/schema/blueprint.go(5 hunks)database/schema/blueprint_test.go(1 hunks)database/schema/column.go(1 hunks)mocks/database/schema/Blueprint.go(4 hunks)mocks/database/schema/Grammar.go(1 hunks)tests/query.go(1 hunks)tests/schema_test.go(1 hunks)tests/table.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/database/schema/blueprint.go
- tests/schema_test.go
- mocks/database/schema/Blueprint.go
🔇 Additional comments (8)
contracts/database/schema/column.go (1)
67-67: LGTM!The addition of the
Extrafield to store additional column metadata is well-placed and follows Go naming conventions.database/schema/column.go (1)
27-32: LGTM!The
NewColumnDefinitionconstructor is well-implemented, following Go conventions and properly handling pointer conversions.tests/table.go (1)
35-51: LGTM!The changes consistently improve error handling across all methods, following Go's best practices for error handling.
Also applies to: 53-207
contracts/database/schema/grammar.go (1)
52-53: LGTM!The
CompileRenameColumnmethod is well-defined, following the interface's patterns and Go's error handling conventions.database/schema/blueprint.go (3)
30-30: LGTM!The new command constant follows the existing naming convention.
344-352: LGTM!The RenameColumn implementation follows the established pattern and correctly sets up the command structure.
522-528: LGTM!The RenameColumn case in ToSql properly handles errors and follows the existing pattern.
mocks/database/schema/Grammar.go (1)
1178-1234: LGTM!The mock implementation for CompileRenameColumn is correctly generated and follows the established patterns for error handling and type assertions.
📑 Description
Go migrator support to rename column
Closes goravel/goravel#572
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✅ Checks