feat: [#477] Configure camel case columns instead of snake case GORM #773
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces enhancements to the database configuration system, specifically focusing on providing more flexibility in database naming strategies. The changes add two new configuration options: Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
contracts/database/config.go (1)
46-49: Consider adding documentation for the Replacer interface.The interface design is clean and follows Go conventions, but could benefit from documentation explaining its purpose and usage in the context of column naming.
Add documentation like:
+// Replacer provides custom column name transformation logic. +// It can be used to implement custom naming conventions (e.g., camelCase, PascalCase). type Replacer interface { + // Replace transforms the given database column name according to custom rules Replace(name string) string }database/db/config_builder.go (2)
58-61: Enhance error handling for invalid NameReplacer configuration.While the implementation works, it silently ignores invalid NameReplacer configurations. Consider adding validation and logging.
if nameReplacer := c.config.Get(fmt.Sprintf("database.connections.%s.name_replacer", c.connection)); nameReplacer != nil { if replacer, ok := nameReplacer.(database.Replacer); ok { + if replacer == nil { + log.Printf("Warning: nil NameReplacer implementation provided for connection %s", c.connection) + continue + } fullConfig.NameReplacer = replacer + } else { + log.Printf("Warning: invalid NameReplacer type for connection %s", c.connection) } }
Line range hint
46-49: Would you like an example CamelCaseReplacer implementation?The interface is well-designed, but users might benefit from a concrete example implementing camel case conversion.
I can help create an example implementation of the Replacer interface that converts snake_case to camelCase, along with its unit tests. Would you like me to generate this code or create a GitHub issue to track this enhancement?
database/db/config_builder_test.go (2)
76-77: Consider extracting duplicated mock expectationsThe mock expectations for
no_lower_caseandname_replacerare duplicated in the Writes method. Consider extracting these common expectations into a helper method to improve maintainability.Example refactor:
+func (s *ConfigTestSuite) setCommonConfigExpectations() { + s.mockConfig.EXPECT().GetBool(fmt.Sprintf("database.connections.%s.no_lower_case", s.connection)).Return(false) + s.mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.name_replacer", s.connection)).Return(nil) +} func (s *ConfigTestSuite) TestWrites() { // ... existing code ... - s.mockConfig.EXPECT().GetBool(fmt.Sprintf("database.connections.%s.no_lower_case", s.connection)).Return(false) - s.mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.name_replacer", s.connection)).Return(nil) + s.setCommonConfigExpectations() // ... later in the test ... - s.mockConfig.EXPECT().GetBool(fmt.Sprintf("database.connections.%s.no_lower_case", s.connection)).Return(false) - s.mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.name_replacer", s.connection)).Return(nil) + s.setCommonConfigExpectations()Also applies to: 99-100
125-125: LGTM: Test cases for new configuration optionsThe test cases properly verify the new configuration options. The
nameReplaceris well-defined and the expectations are correctly set up.Consider adding test cases for:
- Different name replacement patterns
- Edge cases where name replacer is invalid
Also applies to: 144-145, 164-165
database/gorm/test_utils.go (1)
298-299: LGTM: Consistent implementation across database driversThe new configuration expectations are consistently implemented across all database drivers (MySQL, PostgreSQL, SQLite, and SQL Server).
Consider extracting these common expectations into a shared helper function to reduce code duplication and make future maintenance easier.
Example refactor:
+func addCommonConfigExpectations(mockConfig *mocksconfig.Config, connection string) { + mockConfig.EXPECT().GetBool(fmt.Sprintf("database.connections.%s.no_lower_case", connection)).Return(false) + mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.name_replacer", connection)).Return(nil) +} func (r *MockMysql) basic() { // ... existing code ... - r.mockConfig.EXPECT().GetBool(fmt.Sprintf("database.connections.%s.no_lower_case", r.connection)).Return(false) - r.mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.name_replacer", r.connection)).Return(nil) + addCommonConfigExpectations(r.mockConfig, r.connection) mockPool(r.mockConfig) }Also applies to: 373-374, 436-437, 504-505
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mocks/database/Replacer.gois excluded by!mocks/**
📒 Files selected for processing (5)
contracts/database/config.go(1 hunks)database/db/config_builder.go(1 hunks)database/db/config_builder_test.go(9 hunks)database/gorm/gorm.go(1 hunks)database/gorm/test_utils.go(4 hunks)
🔇 Additional comments (5)
contracts/database/config.go (1)
37-38: LGTM! Well-designed configuration options for column naming.
The new fields provide flexible control over column naming:
NoLowerCase: Toggle to prevent automatic case conversionNameReplacer: Interface-based approach for custom naming strategies
database/gorm/gorm.go (1)
125-126: LGTM! Correct integration with GORM's naming strategy.
The implementation properly configures GORM's naming strategy with the new options:
NoLowerCaseis directly mapped to GORM's fieldNameReplaceris correctly integrated for custom naming
database/db/config_builder.go (1)
51-57: LGTM! Clean configuration loading implementation.
The configuration loading follows consistent patterns and properly initializes the new fields.
database/db/config_builder_test.go (1)
49-50: LGTM: Mock expectations for new configuration options
The new mock expectations for no_lower_case and name_replacer are properly added to the Reads test method.
database/gorm/test_utils.go (1)
Line range hint 1-1: Verify implementation files
The test changes look good, but the implementation files (FullConfig struct definition and the actual GORM configuration code) are not included in this review. Please ensure these files are also reviewed to verify:
- The implementation of
NoLowerCaseandNameReplacerin theFullConfigstruct - The GORM configuration code that uses these options to control column naming
✅ Verification successful
Implementation verified and properly integrated
The implementation is complete and correctly integrated across the codebase:
- The
FullConfigstruct incontracts/database/config.goproperly defines bothNoLowerCaseandNameReplacerfields - The GORM configuration in
database/gorm/gorm.gocorrectly applies these options to GORM'sNamingStrategy - The config builder in
database/db/config_builder.goproperly loads these settings from the configuration:NoLowerCaseis loaded from theno_lower_caseconfig keyNameReplaceris loaded with type safety checks from thename_replacerconfig key
- The implementation is well-tested in
config_builder_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the implementation files for the new configuration options
# Look for FullConfig struct definition
echo "Searching for FullConfig struct definition..."
ast-grep --pattern 'type FullConfig struct {
$$$
NoLowerCase bool
NameReplacer $$$
$$$
}'
# Look for GORM configuration code
echo "Searching for GORM configuration usage..."
rg -l "NoLowerCase|NameReplacer" --type go
Length of output: 1166
Script:
#!/bin/bash
# Let's examine the implementation of these configuration options
# Check the GORM implementation
echo "=== GORM Implementation ==="
rg "NoLowerCase|NameReplacer" database/gorm/gorm.go -C 5
# Check the config builder implementation
echo -e "\n=== Config Builder Implementation ==="
rg "NoLowerCase|NameReplacer" database/db/config_builder.go -C 5
# Look for any tests that verify this functionality
echo -e "\n=== Config Builder Tests ==="
rg "NoLowerCase|NameReplacer" database/db/config_builder_test.go -C 5
Length of output: 2474
devhaozi
dismissed stale reviews from kkumar-gcc and coderabbitai[bot]
via
e214f01
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #773 +/- ##
==========================================
+ Coverage 69.94% 69.96% +0.02%
==========================================
Files 213 213
Lines 18164 18177 +13
==========================================
+ Hits 12705 12718 +13
Misses 4762 4762
Partials 697 697 ☔ View full report in Codecov by Sentry. |
…773) * feat: [#477] Configure camel case columns instead of snake case GORM * chore: update mocks * fix test --------- Co-authored-by: hwbrzzl <[email protected]>
📑 Description
Closes goravel/goravel#477
Summary by CodeRabbit
New Features
NoLowerCaseandNameReplacerfields to enhance string handling.Bug Fixes
ConfigBuilderandApplicationclasses.Tests
✅ Checks