chore: Optimize lanuch error message when database is disabled #785
Conversation
WalkthroughThis pull request introduces a series of modifications across multiple files, primarily focusing on error handling and configuration management. The changes span various components including authentication, database services, migration handling, and error messaging. The modifications aim to simplify error handling, provide more generic error messages, and enhance the robustness of instance retrieval and initialization processes. 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 (
|
| } | ||
|
|
||
| connection := config.GetString("database.default") | ||
| if connection == "" { |
There was a problem hiding this comment.
Use doesn't want to use the Orm module, return nil directly.
| info = pterm.PrefixPrinter{ | ||
| MessageStyle: &pterm.ThemeDefault.DefaultText, | ||
| Prefix: pterm.Prefix{ | ||
| Style: &pterm.Style{pterm.FgBlack, pterm.BgLightWhite}, | ||
| Text: " INFO ", | ||
| }, | ||
| Writer: os.Stdout, | ||
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #785 +/- ##
==========================================
- Coverage 69.78% 69.72% -0.07%
==========================================
Files 215 215
Lines 18440 18456 +16
==========================================
Hits 12869 12869
- Misses 4869 4882 +13
- Partials 702 705 +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 (3)
database/gorm/dialector.go (1)
22-22: Use a context-specific error message or logging for better debuggability.Reverting to a generic error (
errors.OrmFailedToGenerateDNS) removes the clarity about which database configuration caused the issue. Although simplifying error messages can prevent information overload, consider some minimal contextual hint (e.g., log the driver or config details at debug level) to speed up troubleshooting in production.database/service_provider.go (1)
36-38: Graceful handling of missing database connection.Returning
nil, nilwhen the default connection string is empty prevents a panic but may obscure whether this is an intended or unexpected state. If this is truly optional, ensure that downstream code properly handles anilORM object. Otherwise, consider logging a more explicit warning about the missing configuration.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-38: database/service_provider.go#L37-L38
Added lines #L37 - L38 were not covered by testserrors/list.go (1)
90-90: Retaining clarity in error messages.The updated error message improves clarity by directing users to "check the database configuration." However, dropping the connection-specific placeholder (“%s”) might remove useful debugging context when dealing with multiple connections. Consider reintroducing minimal context if users often juggle multiple databases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
auth/service_provider.go(1 hunks)database/gorm/dialector.go(1 hunks)database/gorm/dialector_test.go(1 hunks)database/migration/sql_migrator.go(1 hunks)database/service_provider.go(3 hunks)errors/list.go(1 hunks)foundation/container.go(3 hunks)support/color/color.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
auth/service_provider.go
[warning] 35-36: auth/service_provider.go#L35-L36
Added lines #L35 - L36 were not covered by tests
database/service_provider.go
[warning] 37-38: database/service_provider.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-44: database/service_provider.go#L42-L44
Added lines #L42 - L44 were not covered by tests
[warning] 62-63: database/service_provider.go#L62-L63
Added lines #L62 - L63 were not covered by tests
database/migration/sql_migrator.go
[warning] 151-151: database/migration/sql_migrator.go#L151
Added line #L151 was not covered by tests
foundation/container.go
[warning] 106-107: foundation/container.go#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 221-222: foundation/container.go#L221-L222
Added lines #L221 - L222 were not covered by tests
[warning] 273-275: foundation/container.go#L273-L275
Added lines #L273 - L275 were not covered by tests
🔇 Additional comments (11)
database/service_provider.go (3)
62-63: Schema initialization with a nil ORM.
This code implies that an empty schema is sufficient when the ORM is unavailable. Confirm whether skipping migrations or schema validations is acceptable. Otherwise, consider returning a descriptive error or ensuring the application logs a warning for clarity.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-63: database/service_provider.go#L62-L63
Added lines #L62 - L63 were not covered by tests
16-16: Unused import check.
Ensure that the color package is indeed necessary at this line in case future refactoring removes direct usage; otherwise, it may cause confusion.
✅ Verification successful
Import is used correctly
The color package is actively used in the codebase to display warning messages when there are ORM initialization errors. The import is necessary and properly utilized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of the color package in service_provider.go
rg --context 5 'color(\.| )' database/service_provider.go
Length of output: 317
42-44: Test coverage gap for warning message logic.
The newly introduced warning (color.Warningln(...)) and nil return block are not covered by tests according to static analysis. Consider adding a test case that simulates an invalid connection and captures the warning output to ensure it meets expectations.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-44: database/service_provider.go#L42-L44
Added lines #L42 - L44 were not covered by tests
support/color/color.go (2)
6-6: OS import usage verification.
Confirm that writing output directly to os.Stdout in all contexts (e.g., testing, production) is intended. In some automated or test environments, redirecting or mocking output might be preferable.
42-49: User-friendly prefix addition with styled information messages.
The new pterm.PrefixPrinter featuring a custom " INFO " label provides a clear and attractive output. This generally improves user experience and logging consistency. Ensure it aligns with the style of warning and error outputs for uniformity across the entire logging experience.
database/migration/sql_migrator.go (1)
151-151: Consistent error usage across DSN lookups.
Aligning with the generic errors.OrmFailedToGenerateDNS message maintains consistency but reduces debugging context. If you need more granular logs, consider logging the DSN or driver details before returning the generic error.
Also note that this line is not covered by tests according to static analysis. You may wish to add a test scenario where the DSN is empty or invalid to confirm that the migration initialization short-circuits and fails gracefully.
✅ Verification successful
DSN error handling is properly tested across the codebase
The error OrmFailedToGenerateDNS is well covered in tests:
- In
database/gorm/dialector_test.go, there's a specific test case "Sad path - dsn is empty" that verifies this error - The error is used consistently in both
sql_migrator.goanddialector.gowhen DSN generation fails - The
getMigratorfunction is tested indirectly through multiple test cases insql_migrator_test.go(TestFresh, TestRollback, TestRun, TestStatus)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of line 151 in sql_migrator.go
rg -A 10 -B 10 'getMigrator.*OrmFailedToGenerateDNS' tests
Length of output: 106
Script:
#!/bin/bash
# First, let's find the test files related to sql_migrator
fd -e go test database
# Then, let's search for test cases related to DSN or error handling in sql_migrator
rg -A 10 -B 10 'OrmFailedToGenerateDNS|TestSqlMigrator|getMigrator' --type go
Length of output: 19853
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 151-151: database/migration/sql_migrator.go#L151
Added line #L151 was not covered by tests
foundation/container.go (3)
105-107: Add test coverage for nil-check scenario.
Lines 105-107 handle a crucial case by returning nil when MakeAuth fails to retrieve an instance. Ensure you have tests that verify this flow, since the static analysis indicates these lines are currently uncovered.
Would you like me to generate a test snippet for this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-107: foundation/container.go#L106-L107
Added lines #L106 - L107 were not covered by tests
220-222: Test the nil-check for ORM retrieval.
Similar to MakeAuth, these lines add a safeguard by returning nil if MakeOrm fails to retrieve an instance. Please include a unit test to cover this path and confirm the application doesn't panic or misbehave.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 221-222: foundation/container.go#L221-L222
Added lines #L221 - L222 were not covered by tests
273-275: Include coverage for nil-check in schema handling.
These lines prevent a potential panic by returning nil when schema retrieval fails. Again, ensure that your tests verify the correct behavior (e.g., fallback strategies or user-facing warning messages).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 273-275: foundation/container.go#L273-L275
Added lines #L273 - L275 were not covered by tests
auth/service_provider.go (1)
35-36: Add tests for when ORM is not available.
The code now returns nil rather than an error if the ORM facade is missing. Please add targeted test coverage to confirm that the application proceeds without crashing, and that the omitted ORM behavior is handled gracefully.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 35-36: auth/service_provider.go#L35-L36
Added lines #L35 - L36 were not covered by tests
database/gorm/dialector_test.go (1)
35-35: Consistent error expectations for empty DSN.
The test now expects a general OrmFailedToGenerateDNS error for an empty DSN. This aligns with the newly simplified error messaging elsewhere. Ensure all references throughout the codebase have been updated accordingly.
📑 Description
Previously, the application would panic if database is disabled:
Currently, console will print a warning message and run the application normally:
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
OrmFailedToGenerateDNSto provide clearer guidance on database configuration issues.✅ Checks