Skip to content

fix: use target database user for embedded postgres in plan command (#303) #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tianzhou merged 3 commits into from
Mar 4, 2026
+63 −2
Merged

fix: use target database user for embedded postgres in plan command (#303) #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62639ae
fix: use target database user for embedded postgres in plan command (…
tianzhou Mar 4, 2026
9970824
fix: add guard against empty config.User in CreateEmbeddedPostgresFor…
tianzhou Mar 4, 2026
691d56d
fix: use GRANT SELECT instead of GRANT ALL in test for cross-version …
tianzhou Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions cmd/plan/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,19 @@ func CreateDesiredStateProvider(config *PlanConfig) (postgres.DesiredStateProvid
// CreateEmbeddedPostgresForPlan creates a temporary embedded PostgreSQL instance
// for validating the desired state schema. The instance should be stopped by the caller.
func CreateEmbeddedPostgresForPlan(config *PlanConfig, pgVersion postgres.PostgresVersion) (*postgres.EmbeddedPostgres, error) {
// Start embedded PostgreSQL with matching version
if config.User == "" {
return nil, fmt.Errorf("target database user must not be empty when creating embedded postgres")
}

// Start embedded PostgreSQL with matching version.
// Use the target database username so that role references match between
// the desired state (embedded) and current state (target database).
// This ensures ALTER DEFAULT PRIVILEGES FOR ROLE <user> works correctly
// and that implicit owner roles match the target database. (issue #303)
embeddedConfig := &postgres.EmbeddedPostgresConfig{
Version: pgVersion,
Database: "pgschema_temp",
Username: "pgschema",
Username: config.User,
Password: "pgschema",
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior change (embedded DB username now derived from config.User) isn’t directly covered by a regression test that would have failed before this fix. Consider adding an integration test for plan that uses a desired-state file containing ALTER DEFAULT PRIVILEGES FOR ROLE <target user> ... (and creates any required roles), and asserts the plan command succeeds; this would fail when the embedded username was hardcoded to pgschema and pass with this change.

Suggested change
Password: "pgschema",
Password: config.Password,

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@tianzhou tianzhou Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The password suggestion is incorrect. The embedded postgres is a fresh local instance — its password is independent of the target database. The username matters because PostgreSQL uses it for role ownership attribution (pg_get_userbyid(defaclrole)). The password has no semantic meaning here; it is just auth to a throwaway instance.

Regarding test coverage: valid observation. The integration test uses sharedEmbeddedPG as the provider (bypassing CreateEmbeddedPostgresForPlan), so it would not have caught the hardcoded username directly. However, a true end-to-end test spinning up a separate embedded PG with a mismatched user would be complex and slow. The diff test verifies FOR ROLE works correctly at the IR level.

}
Comment on lines 221 to 226
Copy link

@greptile-apps greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No guard against empty config.User in programmatic use

CreateEmbeddedPostgresForPlan is an exported function, and config.User is passed directly as the embedded postgres Username. In the CLI path this is safe because PreRunE validates the --user flag. However, if this function is ever called programmatically with an empty config.User, the embedded-postgres library will either fail to start or silently fall back to a default username — breaking the role-matching invariant this fix establishes.

Consider adding a defensive guard before constructing embeddedConfig:

if config.User == "" {
    return nil, fmt.Errorf("target database user must not be empty when creating embedded postgres")
}

Copy link
Contributor Author

@tianzhou tianzhou Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 9937ab6.

embeddedPG, err := postgres.StartEmbeddedPostgres(embeddedConfig)
Expand Down
1 change: 1 addition & 0 deletions testdata/diff/default_privilege/issue_303_for_role/diff.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO demouser;
10 changes: 10 additions & 0 deletions testdata/diff/default_privilege/issue_303_for_role/new.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
-- Create roles for testing
DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'demouser') THEN
CREATE ROLE demouser;
END IF;
END $$;

-- Grant default privileges with explicit FOR ROLE (issue #303)
ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO demouser;
9 changes: 9 additions & 0 deletions testdata/diff/default_privilege/issue_303_for_role/old.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-- Create roles for testing
DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'demouser') THEN
CREATE ROLE demouser;
END IF;
END $$;

-- No default privileges configured
20 changes: 20 additions & 0 deletions testdata/diff/default_privilege/issue_303_for_role/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"version": "1.0.0",
"pgschema_version": "1.7.3",
"created_at": "1970-01-01T00:00:00Z",
"source_fingerprint": {
"hash": "965b1131737c955e24c7f827c55bd78e4cb49a75adfd04229e0ba297376f5085"
},
"groups": [
{
"steps": [
{
"sql": "ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO demouser;",
"type": "default_privilege",
"operation": "create",
"path": "default_privileges.testuser.TABLES.demouser"
}
]
}
]
}
1 change: 1 addition & 0 deletions testdata/diff/default_privilege/issue_303_for_role/plan.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO demouser;
12 changes: 12 additions & 0 deletions testdata/diff/default_privilege/issue_303_for_role/plan.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
Plan: 1 to add.

Summary by type:
default privileges: 1 to add

Default privileges:
+ demouser

DDL to be executed:
--------------------------------------------------

ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO demouser;