fix: [#706] Setting timezone in MySQL DSN (e.g., Asia/Shanghai) causes “invalid DSN” error#51
Merged
fix: [#706] Setting timezone in MySQL DSN (e.g., Asia/Shanghai) causes “invalid DSN” error#51
Conversation
…s “invalid DSN” error
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refines the timezone handling in the database configuration by removing a hardcoded default and introducing a fallback to the application's timezone when a connection-specific timezone is not provided. Key changes include:
- Addition of a new test case in config_test.go ("success with app.timezone") to verify the fallback mechanism.
- Modification of fillDefault in config.go to use a fallback from "app.timezone" when the connection-specific timezone is empty.
- Update of README.md to remove a hardcoded timezone default.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| config_test.go | Added test case for verifying timezone fallback from "app.timezone". |
| config.go | Added fallback logic for setting timezone dynamically in the fillDefault method. |
| README.md | Removed the default "timezone" configuration value. |
Comment on lines
+95
to
+99
| timezone := r.config.GetString(fmt.Sprintf("database.connections.%s.timezone", r.connection)) | ||
| if timezone == "" { | ||
| timezone = r.config.GetString("app.timezone", "UTC") | ||
| } | ||
| fullConfig.Timezone = timezone |
There was a problem hiding this comment.
[nitpick] Consider refactoring the fallback logic by directly using a GetString call that allows specifying a default value (e.g., r.config.GetString(key, default)) to improve readability and reduce redundant calls.
Suggested change
| timezone := r.config.GetString(fmt.Sprintf("database.connections.%s.timezone", r.connection)) | |
| if timezone == "" { | |
| timezone = r.config.GetString("app.timezone", "UTC") | |
| } | |
| fullConfig.Timezone = timezone | |
| fullConfig.Timezone = r.config.GetString(fmt.Sprintf("database.connections.%s.timezone", r.connection), r.config.GetString("app.timezone", "UTC")) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📑 Description
Closes goravel/goravel#706
This pull request refines the handling of timezones in the configuration logic by removing hardcoded defaults and introducing a fallback mechanism to use the application's timezone when a database connection-specific timezone is not provided. Additionally, it updates the corresponding tests to ensure this behavior is properly validated.
Configuration Updates:
"timezone": "UTC"from the example configuration inREADME.md.fillDefaultinconfig.goto set the timezone dynamically. If no database connection-specific timezone is provided, it falls back to the application's timezone (app.timezone) with a default of"UTC".Testing Enhancements:
config_test.goto verify the fallback mechanism for the timezone whenapp.timezoneis used.✅ Checks