Fix Postgres password command DSN parsing#4531
Conversation
Confidence Score: 5/5Safe to merge — the change is a narrowly scoped quoting fix with no behavioral impact on the happy path and a direct regression test for the broken scenario. The escaping logic is correct (backslash escaped before single quote to avoid double-escaping), all six DSN fields are covered, and the password_command flow is unaffected because OptionBeforeConnect overwrites the placeholder empty password at connection time. The new tests validate the exact failing scenario and the two escaping edge cases via pgx.ParseConfig round-trips. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'main' into codex/pg-passwo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dani29
force-pushed
the
codex/pg-password-command-dsn
branch
from
8f82f05 to
58753f8
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbitBug Fixes
Walkthrough
ChangesDSN Value Quoting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dani29
force-pushed
the
codex/pg-password-command-dsn
branch
from
58753f8 to
65fdae4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/postgresconn/postgresconn_test.go (1)
48-62: ⚡ Quick winAdd a focused assertion for quote/backslash escaping paths.
This test covers IAM username + empty password parsing, but it doesn’t directly exercise the new
'and\escaping branches inquoteLibpqValue.Suggested test addition
func TestBuildDSNQuotesValuesForPasswordCommandParsing(t *testing.T) { cfg := validConfig() cfg.Host = schemas.NewEnvVar("127.0.0.1") cfg.User = schemas.NewEnvVar("service-account@example-project.iam") cfg.Password = schemas.NewEnvVar("") cfg.PasswordCommand = &PasswordCommandConfig{Command: "printf", Args: []string{"unused-iam-auth"}} pgxConfig, err := pgx.ParseConfig(BuildDSN(cfg)) require.NoError(t, err) require.Equal(t, "127.0.0.1", pgxConfig.Host) require.Equal(t, "service-account@example-project.iam", pgxConfig.User) require.Equal(t, "", pgxConfig.Password) require.Equal(t, "bifrost", pgxConfig.Database) } + +func TestBuildDSNEscapesSpecialChars(t *testing.T) { + cfg := validConfig() + cfg.User = schemas.NewEnvVar(`service\acct`) + cfg.DBName = schemas.NewEnvVar(`bifro'st`) + + pgxConfig, err := pgx.ParseConfig(BuildDSN(cfg)) + + require.NoError(t, err) + require.Equal(t, `service\acct`, pgxConfig.User) + require.Equal(t, `bifro'st`, pgxConfig.Database) +}As per coding guidelines,
framework/**tests should cover edge cases and failure paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/postgresconn/postgresconn_test.go` around lines 48 - 62, The test TestBuildDSNQuotesValuesForPasswordCommandParsing does not directly exercise the quote and backslash escaping branches in the quoteLibpqValue function. Add test assertions that explicitly test configuration values containing single quotes and backslashes (such as in the Host, User, or other DSN fields) to verify that BuildDSN properly escapes these characters and that pgx.ParseConfig correctly interprets the escaped values in the resulting DSN string.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/postgresconn/postgresconn_test.go`:
- Around line 48-62: The test TestBuildDSNQuotesValuesForPasswordCommandParsing
does not directly exercise the quote and backslash escaping branches in the
quoteLibpqValue function. Add test assertions that explicitly test configuration
values containing single quotes and backslashes (such as in the Host, User, or
other DSN fields) to verify that BuildDSN properly escapes these characters and
that pgx.ParseConfig correctly interprets the escaped values in the resulting
DSN string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 413c9786-8ce8-48a4-ba69-f4aed75fcf90
📒 Files selected for processing (2)
framework/postgresconn/postgresconn.goframework/postgresconn/postgresconn_test.go
|
cc @akshaydeo, another small follow-up :-( Looks like command parsing is problematic with password command. |
|
|
|
fixes #4564 |
3 participants
Summary
Verification