feat: [#473] ORM has its own logger instead of using facades.Log() #781
Conversation
WalkthroughThis pull request introduces a comprehensive overhaul of the logging mechanism in the GORM package. The changes focus on creating a more flexible and customizable logging approach by integrating a new logging interface ( 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 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 (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: 74eb04d | Previous: 8e5426f | Ratio |
|---|---|---|---|
Benchmark_DecryptString |
5191 ns/op 1592 B/op 17 allocs/op |
2037 ns/op 1592 B/op 17 allocs/op |
2.55 |
Benchmark_DecryptString - ns/op |
5191 ns/op |
2037 ns/op |
2.55 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #781 +/- ##
==========================================
- Coverage 69.91% 69.76% -0.15%
==========================================
Files 213 214 +1
Lines 18246 18337 +91
==========================================
+ Hits 12756 12793 +37
- Misses 4785 4842 +57
+ Partials 705 702 -3 ☔ View full report in Codecov by Sentry. |
![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 (1)
database/gorm/logger.go (1)
19-23: Consider renaming the 'log' parameter to avoid overshadowing the imported package name.Currently, the function signature uses 'log log.Log', which may be confusing when referencing the standard library or other log-related objects. Renaming 'log' to something like 'appLog' could improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
database/gorm/gorm.go(3 hunks)database/gorm/logger.go(2 hunks)database/gorm/logger_test.go(1 hunks)database/gorm/query.go(1 hunks)database/gorm/test_utils.go(14 hunks)foundation/application_test.go(1 hunks)testing/mock/mock.go(2 hunks)testing/utils/log.go(1 hunks)
🔇 Additional comments (31)
database/gorm/logger.go (8)
25-27: Validate behavior for extremely high or very low threshold values.
Currently, negative or zero values revert to 200. This could be documented or guarded more clearly if the user is expected to supply out-of-range values.
31-33: Instantiation looks good.
Properly assigns the logger, logging level, and slow-threshold. No further issues detected.
38-40: Thread-safety considerations.
Since Logger is a struct with mutable fields (level, slowThreshold), changing these fields in a concurrent environment might require synchronization. Ensure the framework's usage pattern prevents simultaneous writes to this struct.
44-47: LogMode method returns the same instance.
Returning the pointer to the same logger instance is the standard approach in GORM logger implementations. This looks correct, but double-check that there’s no concurrency hazard if the log level changes at runtime.
51-53: Info method is straightforward.
Logs at the Info level if the current logger level is Info or higher. Implementation is correct.
58-60: Warn method is aligned with best practices.
No issues found with the logic or syntax here.
Line range hint 65-88: Error method's connection refusal check.
Skipping the log for "connection refused" is sensible. Also, the "Access denied" duplication avoidance is well-handled. Implementation looks good.
93-125: Trace method’s slow query logic.
- Adheres to the configured slowThreshold.
- Properly logs at Info, Warn, or Error levels.
Implementation is clear, with robust coverage for error handling and slow queries.
database/gorm/logger_test.go (8)
19-68: Comprehensive tests for logger initialization.
The table-driven tests ensure different scenarios of debug mode and threshold. This validates the NewLogger function thoroughly.
70-88: LoggerTestSuite struct provides good test grouping.
Using testify’s suite structure is a clean approach for organizing and reusing mock setup across tests.
80-87: SetupTest ensures each test starts with a fresh mock.
This method is well-organized; the logger fields are consistently reset, ensuring tests remain isolated.
89-93: TestLogMode coverage is sufficient.
Asserts that LogMode updates the logger’s level and returns the same instance. Straightforward yet crucial test.
95-99: TestInfo method.
Confirms that Info logs the expected message. Simple and effective test coverage.
101-105: TestWarn method.
Similarly confirms the Warn functionality with mock expectations. No issues.
107-139: TestError method’s coverage of known error patterns.
- Connection refused is skipped.
- “Access denied” is deduplicated.
This ensures correct branching logic is verified. Thorough approach.
141-227: TestTrace method covers every scenario.
- Error cases (rows -1, typical row counts).
- Slow queries.
- Normal queries.
- Silent mode.
Very robust coverage ensuring well-tested logic under multiple conditions.
database/gorm/gorm.go (4)
12-12: Meaningful introduction of 'log' field in the Builder struct.
Associating a dedicated logger with the builder is an excellent move, creating a more self-contained approach to logging.
21-21: Declaration of a new struct field.
The 'log' field is logically placed alongside config fields, enhancing the Builder’s ability to manage logging centrally.
24-28: NewGorm function signature.
Accepting the log parameter explicitly further clarifies dependencies, aligning well with dependency injection best practices.
102-106: Instantiation of logger in init.
The call to NewLogger is neat and ensures consistent config-based initialization. Also references the newly injected log. Implementation looks correct.
testing/utils/log.go (4)
Line range hint 1-19: Refactor from 'mock' to 'utils' package.
Clearer naming aligns with the utility-based nature of these log test helpers. Good practice to keep testing utilities in a well-labeled package.
Line range hint 21-39: TestLog and TestLogWriter design is flexible.
By returning new writers from WithContext, Channel, and Stack, you create ephemeral contexts. Implementation is straightforward for test usage.
Line range hint 41-68: Debug, Info, Warning, Error, Fatal, Panic methods.
Simple print-based approach that’s suitable for logs in test environments. The prefix method is a nice touch for consistent formatting.
Line range hint 70-108: Contextual data features are well-structured.
Storing user, owner, hints, and other fields in a map is convenient for verbose logging. printData conditionally logs them, preventing noise when no extra data is available.
testing/mock/mock.go (1)
26-26: LGTM! The changes align with the PR objectives.
The modification to use utils.NewTestLog() instead of a direct implementation is a good approach for test mocking, providing a consistent logging interface across tests.
Also applies to: 141-141
foundation/application_test.go (1)
337-338: LGTM! Enhanced test coverage for database configuration.
The addition of slow query threshold configuration expectations improves test coverage and validates the ORM's logging configuration.
database/gorm/query.go (1)
74-77: LGTM! Proper implementation of logging integration.
The modification to pass the log parameter to NewGorm is consistent with the PR objective of implementing a dedicated logger for ORM operations. The error handling pattern is maintained appropriately.
database/gorm/test_utils.go (4)
13-13: LGTM: Import added for test logging utilities
The addition of the testing utils import aligns with the PR's objective of implementing dedicated logging for ORM.
180-180: LGTM: Consistent implementation of test logging
The BuildQuery calls have been consistently updated to use utils.NewTestLog() across all query building scenarios, ensuring proper test coverage for the new logging functionality.
Also applies to: 200-200, 219-219, 238-238, 275-275
319-334: LGTM: Consistent mock expectations across all database drivers
The mock configurations have been updated to use the EXPECT() method consistently across all database drivers (MySQL, PostgreSQL, SQLite, SQLServer). Each driver properly mocks its specific configuration requirements while maintaining a uniform approach to expectation setting.
Also applies to: 339-340, 350-357, 362-368, 395-397, 403-412, 417-419, 425-427, 433-441, 446-452, 473-474, 480-487, 492-493, 503-507, 512-514, 541-542, 548-556, 561-562, 572-578, 583-589
1244-1247: LGTM: Reasonable connection pool configurations
The mockPool function implements reasonable default values for connection pool settings:
- 10 max idle connections
- 100 max open connections
- 1-hour connection lifetime limits
These settings provide a good balance between resource utilization and connection management.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
database/gorm/query.go (1)
Based on the gathered context, I can now generate the final response:
Nil check for logger is essential based on codebase patterns
The codebase consistently validates logger parameters across multiple service providers and components. This pattern is evident in various modules (cache, queue, schedule, translation) where
log == nilchecks are implemented with proper error handling usingerrors.LogFacadeNotSet. The Query struct actively uses the logger field, making validation important for consistency and reliability.
- Add nil check in
BuildQueryfunction similar to other components:func BuildQuery(ctx context.Context, config config.Config, connection string, log log.Log, modelToObserver []contractsorm.ModelToObserver) (*Query, error) { + if log == nil { + return nil, errors.LogFacadeNotSet.SetModule(errors.ModuleOrm) + } configBuilder := db.NewConfigBuilder(config, connection)🔗 Analysis chain
Line range hint
74-79: Consider adding nil check for logger parameterWhile the constructor changes are well-structured, consider adding validation for the
logparameter to prevent potential nil pointer dereferences in logging operations.Consider adding this validation:
func BuildQuery(ctx context.Context, config config.Config, connection string, log log.Log, modelToObserver []contractsorm.ModelToObserver) (*Query, error) { + if log == nil { + return nil, errors.New("logger cannot be nil") + } configBuilder := db.NewConfigBuilder(config, connection)
Let me gather more context about the logger usage in the specific file and related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any nil checks for the logger in the codebase rg -A 2 "if.*log.*nil"Length of output: 1316
Script:
#!/bin/bash # Check the Query struct and its logger usage ast-grep --pattern 'type Query struct { $$$ }' database/gorm/query.go # Check if there are any other nil checks in the same file rg "if.*nil" database/gorm/query.go # Check how other constructors in the same package handle logger rg "func New.*log" database/gorm/Length of output: 4764
🧹 Nitpick comments (1)
database/gorm/query.go (1)
Line range hint
37-47: Consider adding documentation for logger functionalityThe logger implementation is well-structured, but would benefit from documentation explaining:
- Expected logger interface implementation requirements
- Logging patterns and levels used by the ORM
- Examples of custom logger implementation
Consider adding a documentation block above the Query struct:
+// Query implements the ORM query builder with logging support. +// The log field expects an implementation of contracts/log.Log interface +// and is used to log query operations, errors, and debug information. type Query struct {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
database/gorm/query.go(1 hunks)
🔇 Additional comments (2)
database/gorm/query.go (2)
Line range hint 37-47: LGTM: Query struct changes align with PR objectives
The addition of the log log.Log field to the Query struct is well-structured and properly uses the contracts/log interface.
Line range hint 1018-1024: LGTM: Proper logger propagation pattern
The logger is correctly propagated through the Query chain via the new() method, maintaining consistency across child queries. This ensures logging context is preserved throughout query operations.
📑 Description
Closes goravel/goravel#473
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✅ Checks