feat(databasesql): add DSN parser registry with extensibility and tests.#509
feat(databasesql): add DSN parser registry with extensibility and tests.#509karthik120710 wants to merge 3 commits into
Conversation
- Added a custom DSN parser registration mechanism to support various database drivers. - Implemented a fallback parsing method for unregistered drivers, improving robustness. - Updated logging to warn when the server address cannot be determined from the DSN. - Enhanced test cases to cover new parsing logic and driver mappings. This change improves the flexibility and reliability of database connection handling. Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
|
|
|
The title of this pull request does not match the conventional commits format. Refer to the CONTRIBUTING.md file for more information. |
|
The title of this pull request does not match the conventional commits format. Refer to the CONTRIBUTING.md file for more information. |
|
The title of this pull request does not match the conventional commits format. Refer to the CONTRIBUTING.md file for more information. |
|
hi @txabman42 i have added pluggable DSN parser registry and expand semconv driver mapping can u review my pr. |
|
Hey @karthik120710, nice work a few things I noticed while going through the diff Build issue: The refactor removes ParseDbName from parse.go but client.go:47 still calls it (dbName := ParseDbName(dataSourceName)). This would fail to compile. Was the removal accidental during the registry refactor? Test coverage: The new tests cover the semconv mapping changes (clickhouse, unknown driver) but there are no tests for RegisterDSNParser itself, bestEffortParse, or the new aliases like pgx and lib/pq going through parsePostgres. Since the registry is the main change here, a basic test showing registration and lookup works would help. Minor: The PR title has a leading space which is causing the conventional commit check to fail. Trimming it should fix the CI. |
…verage - Refactored DSN parsing logic to utilize a dedicated internal package for improved organization and maintainability. - Renamed functions for consistency and clarity, specifically changing to . - Added comprehensive unit tests for the new parsing logic, ensuring robust handling of various DSN formats and edge cases. This update enhances the overall structure of the codebase and improves the reliability of database connection handling. Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
|
Hi @waqar2403 I have add units test and updated process title , but still unit test are not running . |
|
@karthik120710 good progress on the registry refactoring. Two things here: Tests not running Your go.mod bumps a few OTel indirect deps (otlploghttp, otel/log, sdk/log) but the go.sum likely needs regeneration. Try this from the module root: If there are still import resolution errors after that, check if testify is listed under Minor style note on RegisterDSNParse Right now var RegisterDSNParser = dsnparse.RegisterDSNParserThis means any consumer can accidentally overwrite it like func RegisterDSNParser(driverName string, parser dsnparse.DSNParser) {
dsnparse.RegisterDSNParser(driverName, parser)
}Not a blocker just something to consider for safety. |
- Introduced a wrapper function for to maintain consistency in the registration process. - This change enhances the clarity of the parser registration mechanism while preserving existing functionality. This update contributes to better code organization and maintainability in the DSN parsing logic. Signed-off-by: Karthik Rajan <karthikrajanmr@gmail.com>
Summary
without modifying core code.
init()).
fell through to other_sql.
Test plan
DSN parser lacks extensibility and silently produces empty addresses for unknown drivers #508