allow unknown collation name #1604
Conversation
WalkthroughThe recent changes enhance the handling of charset and collation settings in the Go MySQL driver. The updates enable efficient collation handling by converting string-based collations to ID-based ones. The connection logic is simplified by consolidating system variable settings into a single command. Additionally, the changes improve error management and provide better support for custom collation specifications, ensuring accurate collation assignments during database interactions. Changes
Sequence Diagram(s)N/A Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add 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: 1
Outside diff range and nitpick comments (1)
dsn.go (1)
47-48: Document the charsets field.Ensure that the new
charsetsfield is documented in theConfigstruct's comments for clarity.// charsets: Connection charset. When set, this will be set in SET NAMES <charset> query
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- collations.go (1 hunks)
- connection.go (1 hunks)
- connector.go (1 hunks)
- dsn.go (3 hunks)
- dsn_test.go (1 hunks)
- packets.go (1 hunks)
Files skipped from review due to trivial changes (1)
- collations.go
Additional comments not posted (8)
connector.go (1)
183-200: Ensure proper error handling for charset and collation settings.The logic for setting character sets and collation during connection establishment is correct. However, ensure that any errors encountered during
SET NAMESare logged for debugging purposes.if err != nil { c.cfg.Logger.Print("failed to set charset '"+cs+"' with collation '"+mc.cfg.Collation+"': ", err.Error()) mc.Close() return nil, err }dsn_test.go (3)
34-34: Update charset handling in test configurations.The change from
Paramstocharsetsin the test configurations is correct. Ensure that all test cases are updated accordingly.
37-37: Update charset handling in test configurations.The change from
Paramstocharsetsin the test configurations is correct. Ensure that all test cases are updated accordingly.
40-40: Update charset handling in test configurations.The change from
Paramstocharsetsin the test configurations is correct. Ensure that all test cases are updated accordingly.dsn.go (2)
286-288: Handle multiple charsets in DSN formatting.The logic for handling multiple charsets in the DSN is correct. Ensure that the
FormatDSNmethod properly joins and formats the charset values.
509-512: Parse charset parameter as a slice.The logic for parsing the
charsetparameter as a slice of strings is correct. Ensure that theparseDSNParamsfunction handles this correctly.packets.go (2)
325-325: Use the correct variable for collation ID.Ensure that the
defaultCollationIDvariable is correctly defined and used.
326-333: Ensure proper handling of unknown collation names.The logic for handling unknown collation names looks correct. However, verify that the
collationsmap contains all necessary collation names to avoid unexpected errors.
| } | ||
|
|
||
| if cmdSet.Len() > 0 { | ||
| err = mc.exec(cmdSet.String()) | ||
| if err != nil { |
There was a problem hiding this comment.
I'm not sure if there's an expected reviewer set who will look this PR over, but to me this is the only thing in the PR I have reservations on. It might be worth mentioning the behavioral impact of not returning an error during connection param setting? I'm guessing this is to avoid errors where we're "falling through" different charsets or collations and multiple have been specified, but I'm not sure if it's worth still throwing an error if we're dealing with a problem like someone adding a param for a variable that should be a global var.
Ex:
mysql> SET max_connections = 1000;
ERROR 1229 (HY000): Variable 'max_connections' is a
GLOBAL variable and should be set with SET GLOBAL
type situation
There was a problem hiding this comment.
This function uses named return value. So this line returns the errorr.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- dsn.go (3 hunks)
Additional comments not posted (3)
dsn.go (3)
47-48: Addition ofcharsetsfield inConfigstruct.The new
charsetsfield allows specifying multiple character sets, enhancing flexibility in connection configuration. Ensure that the usage of this field is well-documented and that any interactions with theCollationfield are clearly defined.
286-288: Incorporation ofcharsetsin DSN formatting.The
FormatDSNfunction now correctly includes thecharsetsin the DSN string. Verify that this change does not adversely affect DSN string formation and that it integrates well with existing configurations.Verification successful
Incorporation of
charsetsin DSN formatting is verified.The
FormatDSNfunction correctly integrates thecharsetsinto the DSN string. This functionality is covered by existing tests indsn_test.goanddsn_fuzz_test.go, ensuring that it does not adversely affect DSN string formation. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `charsets` are correctly incorporated in DSN strings. # Test: Search for `FormatDSN` usage. Expect: Correct DSN string formation with `charsets`. ast-grep --lang go --pattern $'func ($_ *Config) FormatDSN() string { $$$ if charsets := $_.charsets; len(charsets) > 0 { $$$ } $$$ }'Length of output: 147
Script:
#!/bin/bash # Search for the definition of the `FormatDSN` function in the codebase. ast-grep --lang go --pattern 'func ($_ *Config) FormatDSN() string { $$$ }' # Search for usages of the `FormatDSN` function in the codebase. rg 'FormatDSN' -A 5 # Search for test files that might be testing `FormatDSN`. rg 'FormatDSN' --type go --glob '*_test.go' -A 5Length of output: 8094
509-511: Parsing ofcharsetparameter in DSN.The
parseDSNParamsfunction now correctly parses thecharsetparameter into a slice for thecharsetsfield. Ensure that this parsing logic can handle multiple charsets and that it aligns with expected DSN configurations.
Description
Fix #1603
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests