Improve DSN env vairable expansion - #92
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a three-phase DSN expansion pipeline: replace Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Connector
participant DBPkg as "database.Connect"
participant expandDSN
participant extractPH as "extractPlaceholders"
participant urlParse as "url.Parse"
participant CompExpand as "expandUser/Host/Path/Query/Frag"
participant buildURL as "buildConnectionURL"
Caller->>Connector: build ConnectOptions and call Connect(opts)
Connector->>DBPkg: Connect(ctx, opts)
DBPkg->>expandDSN: expandDSN(opts.DSN)
expandDSN->>extractPH: extractPlaceholders(dsn)
extractPH-->>expandDSN: safeDSN + mapping
expandDSN->>urlParse: parse safeDSN
urlParse-->>expandDSN: parsedURL
rect rgb(235,245,255)
Note over expandDSN,CompExpand: Component-wise expansion (encoding-aware)
expandDSN->>CompExpand: expandUserInfo/Host/Path/Query/Fragment(parsedURL, mapping)
CompExpand-->>expandDSN: expanded components
end
expandDSN->>buildURL: buildConnectionURL(parsedURL, opts, mapping)
buildURL-->>DBPkg: final connection URL
DBPkg-->>Caller: established DB connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a more robust method for expanding environment variables in database DSN (Data Source Name) strings, specifically handling special characters that could break URL parsing. The improvement replaces the previous updateFromEnv function with a new three-phase expandDSN approach that uses sentinel values to safely parse URLs before expanding environment variables.
Key Changes:
- Implements a sentinel-based URL expansion system that prevents special characters in environment variables from breaking DSN parsing
- Adds component-specific expansion functions for different URL parts (user info, host, path, query, fragment) with appropriate encoding
- Includes comprehensive test coverage with 67 test cases covering edge cases like special characters, Unicode, and various database types
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/database/database.go | Adds new expandDSN function and helper functions for sentinel-based URL expansion; updates Connect function to use new expansion method |
| pkg/database/database_test.go | Adds comprehensive test suites for expandDSN, extractPlaceholders, and expandWithMapping functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // extractPlaceholders replaces ${...} placeholders with unique sentinels that are URL-safe. | ||
| // This allows us to parse the URL structure before expanding environment variables. | ||
| // Returns: the string with sentinels, a mapping of sentinel->varName, and any error. | ||
| func extractPlaceholders(s string) (string, map[string]string, error) { |
There was a problem hiding this comment.
The function always returns nil for the error but has error in the return signature. Either remove the error return value or add error handling logic.
| if strings.Contains(result, sentinel) { | ||
| value, exists := os.LookupEnv(varName) | ||
| if !exists { | ||
| err = errors.Join(err, fmt.Errorf("environment variable %s is not set", varName)) | ||
| continue | ||
| } | ||
| result = strings.ReplaceAll(result, sentinel, value) | ||
| } |
There was a problem hiding this comment.
The strings.Contains check is redundant since strings.ReplaceAll will simply return the original string if the sentinel is not found. Removing this check simplifies the code without changing behavior.
| if strings.Contains(result, sentinel) { | |
| value, exists := os.LookupEnv(varName) | |
| if !exists { | |
| err = errors.Join(err, fmt.Errorf("environment variable %s is not set", varName)) | |
| continue | |
| } | |
| result = strings.ReplaceAll(result, sentinel, value) | |
| } | |
| value, exists := os.LookupEnv(varName) | |
| if !exists { | |
| err = errors.Join(err, fmt.Errorf("environment variable %s is not set", varName)) | |
| continue | |
| } | |
| result = strings.ReplaceAll(result, sentinel, value) |
![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)
pkg/database/database.go (2)
171-184: Note on query expansion and interpretation of special characters.
expandQuerywrites the expanded string directly intoparsedUrl.RawQuery, so any&,=or similar characters coming from env values are treated as full query syntax (splitting into multiple params), not encoded as part of a single parameter value. That matches yourQuery parameters with special chars/Ampersand in query parameter valuetests, but it’s worth being explicit in docs/comments so callers don’t assume value‑only semantics.
246-274: Consider adding more contextual error wrapping in Connect.
Connectnow relies onexpandDSN, which can fail for missing env vars or malformed DSNs, but errors are returned as‑is. Per your guidelines about contextual errors, wrapping here would make debugging easier, e.g.:- populatedDSN, err := expandDSN(dsn) - if err != nil { - return nil, Unknown, err - } + populatedDSN, err := expandDSN(dsn) + if err != nil { + return nil, Unknown, fmt.Errorf("expanding DSN: %w", err) + } @@ - parsedDsn, err := url.Parse(populatedDSN) - if err != nil { - return nil, Unknown, err - } + parsedDsn, err := url.Parse(populatedDSN) + if err != nil { + return nil, Unknown, fmt.Errorf("parsing DSN: %w", err) + }pkg/database/database_test.go (1)
86-570: Optional: consider testify/require for clearer test assertions.The tests are already table-driven and readable, but your guidelines call out
testify/require. Migrating the assertions (e.g.,require.NoError,require.Equal) would improve failure messages and keep the subtest bodies a bit leaner, if you decide to align fully with that style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/database/database.go(2 hunks)pkg/database/database_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/database/database_test.gopkg/database/database.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should be table-driven using testify/require; name tests
TestStructName_methodName
Files:
pkg/database/database_test.go
🧬 Code graph analysis (1)
pkg/database/database.go (1)
pkg/database/mysql/mysql.go (1)
Connect(47-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (8)
pkg/database/database.go (5)
54-70: Sentinel-based placeholder extraction looks correct and deterministic.
extractPlaceholderscleanly replaces${VAR}with URL-safe sentinels and tracks them in a mapping; tests cover multiple and repeated vars, and the use ofReplaceAllStringFuncwith a monotonic counter preserves left‑to‑right ordering as expected.
72-94: expandWithMapping error semantics are reasonable and match tests.The function correctly:
- Replaces only present sentinels in the input.
- Aggregates missing‑env errors via
errors.Join.- Returns an empty string on any error, avoiding partial DSN expansion (as your tests assert).
96-139: Userinfo expansion correctly handles both split and combined credentials.The logic to:
- Expand username and password separately via
expandWithMapping, and- Treat a single
${CREDENTIALS}asuser:password(with support for extra colons in the password, encoded byurl.UserPassword),matches the test expectations for “entire userinfo as single variable” and “multiple colons” cases.
141-199: Host/path/fragment expansions are straightforward and component-scoped.Each helper expands placeholders only when the component is non‑empty and writes back to the parsed
url.URL, lettingurl.URL.String()handle encoding (host/backslash, path with slashes, fragments, etc.), which aligns with the edge‑case tests (IPv6 host, SQL Server named instance, path with/, fragment variable).
201-244: Three-phase expandDSN pipeline is robust and well-structured.The sentinel → parse → component‑wise expansion flow:
- Avoids URL-parse breakage from raw secrets (
#,@,:,%, unicode, etc.).- Provides a clear error for invalid DSN structure (
fmt.Errorf("invalid DSN structure: %w", err)).- Properly bails out early when there are no placeholders, preserving the original DSN.
Overall this is a solid improvement over direct
${VAR}inlining.pkg/database/database_test.go (3)
86-447: Comprehensive DSN expansion tests; semantics align with the implementation.The
Test_expandDSNtable covers:
- No placeholders, simple substitutions, repeated vars.
- Userinfo with many special chars (including unicode and emoji), combined
user:pass, and multiple colons.- Host, path, port, fragment, and various schemes (mysql/postgres/sqlserver/oracle/hdb).
- Edge cases like sentinel collisions,
%literals, IPv6, and characters that should/shouldn’t be URL-escaped.This gives strong confidence in the new pipeline and nicely documents intended behaviors (especially around how query strings and userinfo are interpreted).
449-497: extractPlaceholders tests validate ordering and mapping cardinality well.The cases for no placeholders, single, multiple, and repeated variables assert both the transformed string and the mapping size, which matches how
extractPlaceholdersassigns incrementing__PH_n__sentinels.
499-570: expandWithMapping tests cover both success and missing-env error paths.These cases exercise:
- No sentinels (no-op).
- Single and multiple sentinel expansions with env-backed values.
- The missing-variable scenario, where
wantErris true.That matches the implementation’s contract and ensures errors from
os.LookupEnvare surfaced.
ggreer
left a comment
ggreer
left a comment
There was a problem hiding this comment.
This breaks port numbers in variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/database/database_test.go (1)
86-455: Consider using testify/require for assertions.The coding guidelines specify that tests should use testify/require, but these tests use standard library testing assertions. Consider refactoring to use
require.NoError,require.Equal, etc., for more expressive assertions and better error messages.As per coding guidelines
Example refactor for one test case:
+import ( + "github.com/stretchr/testify/require" +) + func Test_expandDSN(t *testing.T) { // ... test cases ... for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { for k, v := range tt.env { t.Setenv(k, v) } got, err := expandDSN(tt.dsn) - if (err != nil) != tt.wantErr { - t.Errorf("expandDSN() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("expandDSN() got = %v, want %v", got, tt.want) - } + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tt.want, got) + } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/database/database_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/database/database_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should be table-driven using testify/require; name tests
TestStructName_methodName
Files:
pkg/database/database_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (3)
pkg/database/database_test.go (3)
441-443: Good use of t.Setenv for test isolation.The tests correctly use
t.Setenvfor environment variable setup, which automatically cleans up after each test. This is cleaner and more reliable than manual cleanup withos.Unsetenv.
259-435: Excellent edge case coverage.The test suite includes comprehensive edge cases covering security concerns (sentinel collision, injection attempts), internationalization (Unicode, emoji), control characters (newline, tab), and various database-specific formats (IPv6, SQL Server backslash notation). This demonstrates thorough testing of the DSN expansion implementation.
232-238: Remove duplicate test case.The test cases "Empty password" (lines 232-238) and "Empty password (explicitly set)" (lines 389-395) are identical in their environment variables, DSN input, and expected output.
Apply this diff to remove the duplicate:
- { - name: "Empty password (explicitly set)", - env: map[string]string{ - "DB_PASSWORD": "", - }, - dsn: "mysql://user:${DB_PASSWORD}@localhost/db", - want: "mysql://user:@localhost/db", - },Also applies to: 389-395
⛔ Skipped due to learnings
Learnt from: CR Repo: ConductorOne/baton-sql PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-09-27T00:53:43.641Z Learning: Applies to **/*_test.go : Tests should be table-driven using testify/require; name tests `TestStructName_methodName`
a73c63d to
43b54f0
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/database/database.go (1)
214-219: Minor: Ensure comment ends with a period per coding guidelines.The switch to
expandDSNcorrectly implements the new robust expansion approach. However, the comment at line 215 should end with a period to comply with the coding guideline requiring complete sentences.Apply this diff:
- // Use the new expandDSN function which handles special characters correctly + // Use the new expandDSN function which handles special characters correctly. populatedDSN, err := expandDSN(dsn)As per coding guidelines.
pkg/database/database_test.go (3)
86-479: Consider using testify/require for consistency with coding guidelines.The coding guidelines specify that "Tests should be table-driven using testify/require". While the test is table-driven and functionally correct, adopting testify/require would improve consistency and provide better assertion failure messages.
Example migration:
import ( "github.com/stretchr/testify/require" ) // In the test: got, err := expandDSN(tt.dsn) if tt.wantErr { require.Error(t, err) return } require.NoError(t, err) require.Equal(t, tt.want, got)As per coding guidelines.
481-568: Consider using testify/require for consistency with coding guidelines.Same recommendation as for
Test_expandDSN—adopting testify/require would align with the project's coding guidelines and provide clearer assertion failures.As per coding guidelines.
570-619: Consider using testify/require for consistency with coding guidelines.Same recommendation as for the other test functions—adopting testify/require would align with the project's coding guidelines.
As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/database/database.go(2 hunks)pkg/database/database_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/database/database_test.gopkg/database/database.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should be table-driven using testify/require; name tests
TestStructName_methodName
Files:
pkg/database/database_test.go
🧬 Code graph analysis (1)
pkg/database/database.go (3)
pkg/database/hdb/hdb.go (1)
Connect(10-17)pkg/database/mysql/mysql.go (1)
Connect(47-59)pkg/database/oracle/oracle.go (1)
Connect(10-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (11)
pkg/database/database.go (8)
11-11: LGTM!The
stringsimport is correctly placed and alphabetically ordered within the standard library imports group.
88-96: LGTM!The function correctly performs sentinel-to-value replacement using
strings.ReplaceAll. The implementation is clean and addresses the past review feedback about removing redundant checks.
98-133: LGTM! Smart handling of combined credentials.The function correctly handles both separate username/password variables and the special case where a single variable contains "user:password" (lines 120-125). The use of
url.UserPassword()ensures proper URL encoding of special characters like@,#,:, etc.The logic correctly only triggers the colon-splitting behavior when no password component was present in the original DSN structure (
!hasPass), avoiding false positives for legitimate usernames containing colons.
135-147: LGTM!Both functions correctly perform simple sentinel replacement. The lack of additional encoding in
expandPathis intentional and correct—path components like database names that contain slashes should be preserved as structural elements of the URL path rather than being percent-encoded.
149-170: LGTM! Proper query parameter encoding.The function correctly handles query parameter expansion and encoding:
- Parses the query string to extract key-value pairs
- Expands sentinels in both keys and values
- Re-encodes using
url.Values.Encode(), which properly percent-encodes special characters (e.g.,&becomes%26, spaces become+)The fallback at lines 154-157 is a reasonable defensive measure to preserve functionality if parsing unexpectedly fails.
172-177: LGTM!Simple and correct implementation for fragment expansion.
179-212: LGTM! Well-structured three-phase expansion.The function correctly orchestrates the DSN expansion pipeline:
- Phase 1: Extracts placeholders, looks up environment variables, and generates sentinels
- Phase 2: Parses the DSN with sentinels to establish URL structure safely
- Phase 3: Expands each component with appropriate encoding
The early return optimization (lines 193-196) avoids unnecessary parsing when no expansion is needed. Error handling follows the coding guidelines with contextual messages.
54-86: Verification complete: the 1000 placeholder limit is sufficient for production use.The codebase test suite demonstrates a maximum of 5 placeholders in any single DSN string (e.g.,
${DB_USER}:${DB_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME}). This is far below the 1000-placeholder capacity provided by the999%03dsentinel format. DSN strings are inherently limited in length and complexity, making the limit a non-issue in practice.The function's design—returning sentinels even when environment variables are missing—is intentional and well-reasoned, allowing URL parsing to succeed so error messages can identify which variables failed. The implementation is production-ready as-is.
pkg/database/database_test.go (3)
86-479: Excellent test coverage with comprehensive edge cases!The test suite thoroughly validates the DSN expansion logic across numerous scenarios including:
- Special character encoding in passwords and usernames (lines 101-140)
- Port injection concerns (lines 109-115)
- Combined credentials handling (lines 169-175)
- Query parameter encoding (lines 185-199, 349-355)
- Unicode and emoji support (lines 365-379)
- Security scenarios like sentinel collision (lines 276-282)
The test expectations correctly encode ampersands as
%26in query parameter values (lines 190, 354), addressing the concern from previous reviews. The tests also correctly show that ampersands in passwords remain unencoded per RFC 3986 (line 346).
481-568: LGTM! Good coverage of sentinel generation logic.The tests validate key behaviors:
- Sentinel format (999000, 999001, etc.)
- Mapping size correctness
- Handling of repeated variables (lines 518-525)
- Port placeholder validation (lines 527-534)
- Error handling for missing environment variables (lines 536-542)
The test at lines 518-525 importantly confirms that the same variable used twice generates two distinct sentinels, which is necessary for the expansion logic.
570-619: LGTM! Adequate coverage for the helper function.The tests appropriately validate the sentinel replacement logic including the edge case where a sentinel exists in the mapping but not in the input string (lines 602-608).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/database/database.go (2)
71-103: Document the sentinel count limitation.The sentinel format
"999%03d"at line 83 limits the system to 1000 placeholders (000-999). While this is more than sufficient for typical DSN configurations, documenting this limitation in the function comment would be helpful for maintainability.Consider adding to the comment:
// extractPlaceholders replaces ${...} placeholders with unique numeric sentinels // (supports up to 1000 placeholders: 999000-999999) and looks up the environment // variable values immediately.
282-407: Well-structured URL construction with proper override behavior.The function correctly implements the precedence logic where structured fields override DSN components. The IPv6 handling (lines 342-344) properly strips and re-adds brackets via
net.JoinHostPort, and the database path normalization ensures consistent leading slash handling.While the function is 125 lines long, it's well-organized into logical sections (scheme, host/port, database, credentials, params), and the complexity is inherent to supporting both DSN and structured configurations with environment variable expansion.
If future maintainability becomes a concern, consider extracting host/port resolution (lines 309-351) into a helper function like
resolveHostPort(parsedUrl *url.URL, host, port string).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/example.ymlis excluded by none and included by none
📒 Files selected for processing (4)
pkg/bsql/config.go(1 hunks)pkg/connector/connector.go(1 hunks)pkg/database/database.go(4 hunks)pkg/database/database_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/config.gopkg/database/database.gopkg/connector/connector.gopkg/database/database_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Tests should be table-driven using testify/require; name tests
TestStructName_methodName
Files:
pkg/database/database_test.go
🧬 Code graph analysis (3)
pkg/database/database.go (3)
pkg/database/hdb/hdb.go (1)
Connect(10-17)pkg/database/mysql/mysql.go (1)
Connect(47-59)pkg/database/oracle/oracle.go (1)
Connect(10-16)
pkg/connector/connector.go (2)
pkg/database/database.go (2)
ConnectOptions(37-49)Connect(231-280)pkg/database/mysql/mysql.go (1)
Connect(47-59)
pkg/database/database_test.go (1)
pkg/database/database.go (1)
ConnectOptions(37-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (12)
pkg/connector/connector.go (1)
97-108: LGTM! Clean refactoring to use the new ConnectOptions API.The construction of
ConnectOptionsproperly maps all configuration fields fromc.Connectto the new structured options format. This aligns well with the updateddatabase.Connectsignature.pkg/bsql/config.go (1)
39-68: LGTM! Well-documented public API extension.The additions to
DatabaseConfigare comprehensive and well-commented. The documentation clearly explains the relationship between the DSN string and structured fields, including the override behavior and environment variable expansion syntax. All fields follow Go naming conventions and include appropriate yaml/json tags.pkg/database/database_test.go (3)
86-479: Excellent test coverage for DSN expansion!The test suite is comprehensive and well-structured, covering a wide range of scenarios including:
- Multiple database schemes (MySQL, PostgreSQL, SQL Server, Oracle, HDB)
- Special characters in credentials (# @ : / ? % etc.)
- IPv6 addresses
- Unicode and emoji characters
- Query parameter encoding (properly expects %26 for ampersands)
- Edge cases like sentinel collision and missing environment variables
The table-driven approach follows Go testing best practices.
481-568: LGTM! Good coverage of placeholder extraction logic.The tests properly verify sentinel generation, mapping creation, and error handling for missing environment variables. The coverage includes edge cases like the same variable appearing multiple times.
621-724: LGTM! Thorough testing of URL construction logic.The tests properly verify the URL building process including:
- DSN-only configuration
- Override behavior where structured fields take precedence
- Environment variable expansion in structured fields
- IPv6 address handling with proper bracket handling
- Error conditions like port without host
pkg/database/database.go (7)
35-49: LGTM! Well-designed configuration struct.The
ConnectOptionsstruct provides a clean, flexible interface supporting both DSN strings and structured configuration. The documentation clearly explains that environment variable placeholders are supported in any field.
105-113: LGTM! Clean implementation of sentinel expansion.The function efficiently replaces sentinels with their corresponding values using straightforward string replacement.
115-150: LGTM! Proper handling of credentials with special characters.The function correctly handles URL encoding of special characters in userinfo components. The special case for combined "user:password" credentials (lines 137-142) properly splits on the first colon, and
url.UserPassword()ensures all special characters are percent-encoded.
152-164: LGTM! Clean component-wise expansion helpers.The
expandHostandexpandPathfunctions follow a consistent pattern, delegating toexpandWithMappingfor their respective URL components.
166-187: LGTM! Correct query parameter expansion with proper encoding.The function properly expands sentinels in query parameters and uses
url.Values.Encode()to ensure special characters (including ampersands and spaces) are correctly percent-encoded. The fallback handling for unparseable query strings preserves robustness.
196-229: LGTM! Elegant three-phase expansion solves URL parsing challenges.The three-phase approach elegantly handles the core challenge: special characters in environment variables (like
#,@,:) would break URL parsing if expanded directly. By using numeric sentinels as intermediaries, the URL structure is safely parsed before component-wise expansion with proper encoding.
409-417: LGTM! Clean helper for conditional environment variable expansion.The function efficiently handles the three cases: empty strings, values with placeholders, and plain values without unnecessary processing.
|
|
||
| // ConnectOptions represents the structured configuration used to build a DSN. | ||
| // Any field may include ${ENV_VAR} placeholders that will be expanded before use. | ||
| type ConnectOptions struct { |
There was a problem hiding this comment.
Isn't ConnectOptions identical to DatabaseConfig? Why make two structs that have the exact same things in them? It's just losing the json & yaml tags.
| var err error | ||
|
|
||
| result := DSNREnvRegex.ReplaceAllStringFunc(s, func(match string) string { | ||
| sentinel := fmt.Sprintf("999%03d", counter) |
There was a problem hiding this comment.
Won't this replace any value that contains three 9s followed by three more numbers? eg: If DB_PASSWORD is "1234" and the connection string is mysql://admin:${DB_PASSWORD}@localhost:3306/db-999000, then the final DSN will be mysql://admin:1234@localhost:3306/db-1234
There was a problem hiding this comment.
You can also run into this if you put certain values in environment variables. eg:
env: map[string]string{
"DB_USER": "user-999001",
"DB_PASSWORD": "1234",
},
dsn: "mysql://${DB_USER}:${DB_PASSWORD}@localhost:3306/dbname",...will generate mysql://user-1234:1234@localhost:3306/dbname instead of mysql://user-999001:1234@localhost:3306/dbname
There was a problem hiding this comment.
This code can't replace the DB scheme in the DSN. eg: dsn: "${DB_SCHEME}://admin:1234@localhost:3306/dbname" will fail with invalid DSN structure: parse "999000://admin:1234@localhost:3306/dbname": first path segment in URL cannot contain colon.
It also can't replace env vars that span more than one part the DSN. eg: dsn: "${DSN}" with the DSN env var set to mysql://admin:1234@localhost:3306/dbname will return a DSN of ./mysql://admin:1234@localhost:3306/dbname.
Anthoer example is with the env var DB_ENDPOINT set to "localhost:3306/dbname" and a dsn in the config set to mysql://admin:password@${DB_ENDPOINT}. It evaluates to mysql://admin:password@localhost:3306%2Fdbname instead of mysql://admin:password@localhost:3306/dbname.
If anyone uses env vars in their configs like this, this PR will break their connector.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.