[BB-1827] address expand dsn edge cases - #97
Conversation
WalkthroughRefactors DSN environment-variable expansion to use collision-avoidant string sentinels ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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 (2)
pkg/database/database.go (1)
275-286: Redundant environment variable lookup.The entire-DSN special case performs a second
os.LookupEnvcall afterextractPlaceholdersalready looked up the variable and stored the error. SinceextractPlaceholdersreturns an error when the variable is missing, this code path would never be reached for missing variables.Consider simplifying by using the already-extracted mapping:
// Special case: if the entire DSN is a single variable (e.g., "${DSN}"), // just return the expanded value directly without parsing matches := DSNREnvRegex.FindAllString(dsn, -1) if len(matches) == 1 && strings.TrimSpace(dsn) == strings.TrimSpace(matches[0]) { - // Get the variable name - varName := matches[0][2 : len(matches[0])-1] - value, exists := os.LookupEnv(varName) - if !exists { - return "", fmt.Errorf("environment variable %s is not set", varName) - } - return value, nil + // The single placeholder was replaced with a single sentinel + // Return the mapped value directly + for _, value := range mapping { + return value, nil + } }pkg/database/database_test.go (1)
620-669: Test cases use old numeric sentinel format.The
Test_expandWithMappingtest cases still use bare numeric strings like"999000"as mapping keys. While this works (the function just does string replacement), it doesn't reflect how the function is actually used with the new_PH-999XXX_format.Consider updating the test cases to use the actual sentinel format for consistency:
{ name: "Single sentinel", - input: "user:999000", + input: "user:_PH-999000_", mapping: map[string]string{ - "999000": "secret", + "_PH-999000_": "secret", }, want: "user:secret", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/database/database.go(5 hunks)pkg/database/database_test.go(5 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
🪛 GitHub Check: go-lint
pkg/database/database.go
[failure] 292-292:
offBy1: Index() can return -1; maybe you wanted to do sentinelDSN[:schemeEnd+1] (gocritic)
⏰ 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 (7)
pkg/database/database.go (5)
71-101: LGTM! Sentinel format change improves collision avoidance.The new
_PH-999XXX_format with prefix and suffix is more robust than bare numeric sentinels. The counter-based approach ensures uniqueness within a single DSN.
103-112: LGTM!The
expandWithMappingfunction correctly replaces all sentinel occurrences with their mapped values.
151-174: LGTM! Good handling of embedded paths in host values.The logic correctly splits host values containing paths (e.g.,
localhost:3306/dbname) and prepends the extracted path to the existing URL path.
288-300: Potential panic if scheme sentinel is the entire DSN without://.The static analysis correctly flags that
strings.Indexcan return -1. IfsentinelDSNcontains://but starts with a sentinel that doesn't have://immediately after (edge case), or if the logic is reached via an unexpected path, slicing withschemeEndcould cause issues. However, the condition on line 290 guards this.More critically: if
schemeSentinel(line 292) extracts content before://, but the sentinel itself contains://in its value, the replacement on line 297 would be incorrect.Consider adding a test case where the scheme environment variable value contains
://to verify correct behavior:{ name: "Scheme value contains colon-slash-slash", env: map[string]string{ "DB_SCHEME": "mysql://injected", }, dsn: "${DB_SCHEME}://admin:1234@localhost:3306/dbname", want: "", // or define expected behavior wantErr: true, // should this be rejected? }
213-254: No action needed: IPv6 handling is correct for valid RFC 3986 URLs.The
expandPortSentinelfunction correctly identifies the port separator for bracketed IPv6 addresses like[::1]:_PH-999000_because the last colon appears after the closing bracket. Unbracketed IPv6 addresses (e.g.,::1:_PH-999000_) are invalid according to RFC 3986 and would fail validation at theurl.Parsecall immediately following this function (line 306), making this a non-issue for the function's intended use.pkg/database/database_test.go (2)
287-295: LGTM! Good test coverage for port placeholder.This test case validates the
expandPortSentinelfunctionality with both password and port placeholders in the same DSN, including a literal pattern (db-999001) that could collide with old numeric sentinels.
553-582: LGTM! Sentinel format updates align with implementation.The test expectations correctly reflect the new
_PH-999XXX_sentinel format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/database/database.go (2)
86-98: Misleading comment on error path.The comment at line 88 states the sentinel is returned "to allow URL parsing for better error messages," but since the function returns early at line 96-98 when
err != nil, URL parsing never occurs on error. The sentinel return is necessary forReplaceAllStringFuncto build the result string, but the comment suggests a different purpose.Consider updating the comment:
if !exists { err = errors.Join(err, fmt.Errorf("environment variable %s is not set", varName)) - return sentinel // Return sentinel anyway to allow URL parsing for better error messages + return sentinel // Return placeholder so ReplaceAllStringFunc can complete; err causes early exit below }
213-254: Implementation is correct; consider documenting the side effect more prominently.The port sentinel expansion logic correctly handles:
- IPv6 addresses (uses
LastIndexto find port separator after])- Userinfo with colons (checks
colonIndex > atIndex)- Various DSN structures
However, the function modifies the
mappingparameter in place (line 248). While this is noted in the inline comment, the function signature and doc comment don't make this mutation clear to callers.Consider updating the doc comment:
// expandPortSentinel expands the port sentinel in the DSN before URL parsing. // Ports must be numeric for URL parsing, so we detect and expand port sentinels // (which use _PH-999000_ format) before passing to url.Parse. -// The mapping is modified in place to remove the port sentinel. +// Note: This function mutates the mapping parameter, removing the consumed port sentinel. func expandPortSentinel(sentinelDSN string, mapping map[string]string) string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/database/database.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.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 (4)
pkg/database/database.go (4)
103-112: LGTM!The comment update accurately documents the sentinel format, and the function logic remains correct.
159-172: LGTM with one clarification.The path handling logic is correct. When an environment variable contains both host and path (e.g.,
localhost:3306/dbname), the path is properly extracted and prepended to any existing path.One edge case worth noting: if the existing
parsedUrl.Pathdoesn't start with/, the concatenation at line 167 could produce unexpected results. However,url.Parseshould always produce paths starting with/for absolute URLs, so this is likely safe.
275-286: Correct handling of single-variable DSN; minor redundancy noted.This special case correctly handles DSNs that are entirely a single environment variable (e.g.,
${FULL_DSN}), avoiding unnecessary URL parsing.Minor observation: the
os.LookupEnvat line 281 is redundant sinceextractPlaceholdersalready looked up the value and stored it inmapping. SincesentinelDSNequals the sentinel when the DSN is a single variable, you could usemapping[sentinelDSN]instead. However, this is a minor optimization and the current approach is clear and correct.
288-305: LGTM!The scheme sentinel expansion (lines 288-302) and port sentinel expansion (line 305) correctly handle edge cases where these components are environment variables. Expanding these before
url.Parseensures the URL parser receives valid scheme and port formats.
| matches := DSNREnvRegex.FindAllString(dsn, -1) | ||
| if len(matches) == 1 && strings.TrimSpace(dsn) == strings.TrimSpace(matches[0]) { | ||
| // Get the variable name | ||
| varName := matches[0][2 : len(matches[0])-1] |
There was a problem hiding this comment.
would we want to use the trimmed version of matches since we're trimming in the comparison?
| if strings.HasPrefix(portPart, "_PH-") && strings.HasSuffix(portPart, "_") { | ||
| if value, ok := mapping[portPart]; ok { | ||
| // Replace port sentinel with its value before parsing | ||
| beforePort := sentinelDSN[:schemeEnd+3+colonIndex+1] |
There was a problem hiding this comment.
Maybe make a nicely named variable for schemeEnd+3 since it's repeated in multiple places and give maybe an example of what's being tested here in a comment. I don't actually ever see schemeEnd ever being used without adding the :// length to it so maybe just add it when you create that variable.
3 participants
Description
Useful links:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.