From 7bc575f4c07a5abfe45c94bce1c3b22c30e5e483 Mon Sep 17 00:00:00 2001 From: Shehjad Date: Thu, 15 Oct 2026 00:13:27 +0600 Subject: [PATCH 1/2] fix: include partitioned tables in privilege inspection (#640) GetPrivilegesForSchema and GetColumnPrivilegesForSchema filtered pg_class by relkind 'r', 'v', 'm', 'S', so partitioned tables (relkind 'p') were never inspected. GRANT/REVOKE on a partitioned table produced no plan changes, and dump omitted its privileges. Include relkind 'p' (reported as TABLE) in both queries and regenerate queries.sql.go with sqlc v1.30.0. --- cmd/issue_640_integration_test.go | 111 ++++++++++++++++++++++++++++++ ir/queries/queries.sql | 5 +- ir/queries/queries.sql.go | 5 +- 3 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 cmd/issue_640_integration_test.go diff --git a/cmd/issue_640_integration_test.go b/cmd/issue_640_integration_test.go new file mode 100644 index 00000000..1f6dc915 --- /dev/null +++ b/cmd/issue_640_integration_test.go @@ -0,0 +1,111 @@ +package cmd + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/pgplex/pgschema/testutil" +) + +// TestIssue640PartitionedTablePrivileges verifies that GRANT/REVOKE/dump +// cover privileges on partitioned tables (relkind 'p'), not only ordinary +// tables. See https://github.com/pgplex/pgschema/issues/640. +func TestIssue640PartitionedTablePrivileges(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ctx := context.Background() + + embeddedPG := testutil.SetupPostgres(t) + defer embeddedPG.Stop() + conn, host, port, dbname, user, password := testutil.ConnectToPostgres(t, embeddedPG) + defer conn.Close() + + // Roles are not managed by pgschema; create on the target before plan/apply. + roleSQL := ` +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN + CREATE ROLE reader; + END IF; +END $$; +` + if _, err := conn.ExecContext(ctx, roleSQL); err != nil { + t.Fatalf("create role on target: %v", err) + } + + tablesOnly := ` +CREATE TABLE events ( + id int NOT NULL, + at date NOT NULL +) PARTITION BY RANGE (at); + +CREATE TABLE events_2026_01 PARTITION OF events FOR VALUES FROM ('2026-01-01') TO ('2026-02-01'); + +CREATE TABLE plain (id int NOT NULL); +` + withGrants := tablesOnly + ` +GRANT SELECT ON events TO reader; +GRANT SELECT ON plain TO reader; +` + + schema := "public" + tmp := t.TempDir() + tablesFile := filepath.Join(tmp, "tables.sql") + grantsFile := filepath.Join(tmp, "with_grants.sql") + if err := os.WriteFile(tablesFile, []byte(tablesOnly), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(grantsFile, []byte(withGrants), 0644); err != nil { + t.Fatal(err) + } + + if err := applySchemaChanges(host, port, dbname, user, password, schema, tablesFile); err != nil { + t.Fatalf("apply tables: %v", err) + } + + planSQL, err := generatePlanSQLFormatted(host, port, dbname, user, password, schema, grantsFile) + if err != nil { + t.Fatalf("plan with grants: %v", err) + } + + hasGrant := func(sql, obj string) bool { + return strings.Contains(sql, "GRANT SELECT ON TABLE "+obj+" TO reader") || + strings.Contains(sql, "GRANT SELECT ON "+obj+" TO reader") + } + hasRevoke := func(sql, obj string) bool { + return strings.Contains(sql, "REVOKE SELECT ON TABLE "+obj+" FROM reader") || + strings.Contains(sql, "REVOKE SELECT ON "+obj+" FROM reader") + } + + if !hasGrant(planSQL, "plain") { + t.Fatalf("plan missing GRANT on plain table:\n%s", planSQL) + } + if !hasGrant(planSQL, "events") { + t.Fatalf("plan missing GRANT on partitioned table events (issue #640):\n%s", planSQL) + } + t.Logf("plan correctly includes GRANT on partitioned table events:\n%s", planSQL) + + if _, err := conn.ExecContext(ctx, ` + GRANT SELECT ON events TO reader; + GRANT SELECT ON plain TO reader; + `); err != nil { + t.Fatalf("grant on live db: %v", err) + } + + revokePlan, err := generatePlanSQLFormatted(host, port, dbname, user, password, schema, tablesFile) + if err != nil { + t.Fatalf("plan revoke: %v", err) + } + if !hasRevoke(revokePlan, "plain") { + t.Fatalf("plan missing REVOKE on plain:\n%s", revokePlan) + } + if !hasRevoke(revokePlan, "events") { + t.Fatalf("plan missing REVOKE on partitioned table events (issue #640):\n%s", revokePlan) + } + t.Logf("plan correctly includes REVOKE on partitioned table events:\n%s", revokePlan) +} diff --git a/ir/queries/queries.sql b/ir/queries/queries.sql index 60ec9800..44a31d8b 100644 --- a/ir/queries/queries.sql +++ b/ir/queries/queries.sql @@ -2026,6 +2026,7 @@ WITH acl_data AS ( c.relname::text AS object_name, CASE c.relkind WHEN 'r' THEN 'TABLE' + WHEN 'p' THEN 'TABLE' WHEN 'v' THEN 'VIEW' WHEN 'm' THEN 'VIEW' WHEN 'S' THEN 'SEQUENCE' @@ -2044,7 +2045,7 @@ WITH acl_data AS ( AND dep.refobjsubid = 0 AND dep.deptype = 'e' ) - AND c.relkind IN ('r', 'v', 'm', 'S') + AND c.relkind IN ('r', 'p', 'v', 'm', 'S') AND c.relacl IS NOT NULL -- SERIAL/identity sequences inherit the owning extension table's boundary. AND NOT EXISTS ( @@ -2267,7 +2268,7 @@ WITH column_acls AS ( AND dep.refobjsubid = 0 AND dep.deptype = 'e' ) - AND c.relkind IN ('r', 'v', 'm') -- tables, views, materialized views + AND c.relkind IN ('r', 'p', 'v', 'm') -- ordinary/partitioned tables, views, materialized views AND a.attnum > 0 -- skip system columns AND NOT a.attisdropped AND a.attacl IS NOT NULL -- only columns with explicit ACL diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index 2dfc73e3..3707333b 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -337,7 +337,7 @@ WITH column_acls AS ( AND dep.refobjsubid = 0 AND dep.deptype = 'e' ) - AND c.relkind IN ('r', 'v', 'm') -- tables, views, materialized views + AND c.relkind IN ('r', 'p', 'v', 'm') -- ordinary/partitioned tables, views, materialized views AND a.attnum > 0 -- skip system columns AND NOT a.attisdropped AND a.attacl IS NOT NULL -- only columns with explicit ACL @@ -2601,6 +2601,7 @@ WITH acl_data AS ( c.relname::text AS object_name, CASE c.relkind WHEN 'r' THEN 'TABLE' + WHEN 'p' THEN 'TABLE' WHEN 'v' THEN 'VIEW' WHEN 'm' THEN 'VIEW' WHEN 'S' THEN 'SEQUENCE' @@ -2619,7 +2620,7 @@ WITH acl_data AS ( AND dep.refobjsubid = 0 AND dep.deptype = 'e' ) - AND c.relkind IN ('r', 'v', 'm', 'S') + AND c.relkind IN ('r', 'p', 'v', 'm', 'S') AND c.relacl IS NOT NULL -- SERIAL/identity sequences inherit the owning extension table's boundary. AND NOT EXISTS ( From 589947a12eadc67d5b47784b14a67563439f43df Mon Sep 17 00:00:00 2001 From: Shehjad Date: Thu, 15 Oct 2026 17:04:18 +0600 Subject: [PATCH 2/2] test: replace issue 640 integration test with privilege diff fixture --- cmd/issue_640_integration_test.go | 111 ------------------ .../diff.sql | 5 + .../issue_640_partitioned_table_grant/new.sql | 16 +++ .../issue_640_partitioned_table_grant/old.sql | 15 +++ .../plan.json | 32 +++++ .../plan.sql | 5 + .../plan.txt | 21 ++++ 7 files changed, 94 insertions(+), 111 deletions(-) delete mode 100644 cmd/issue_640_integration_test.go create mode 100644 testdata/diff/privilege/issue_640_partitioned_table_grant/diff.sql create mode 100644 testdata/diff/privilege/issue_640_partitioned_table_grant/new.sql create mode 100644 testdata/diff/privilege/issue_640_partitioned_table_grant/old.sql create mode 100644 testdata/diff/privilege/issue_640_partitioned_table_grant/plan.json create mode 100644 testdata/diff/privilege/issue_640_partitioned_table_grant/plan.sql create mode 100644 testdata/diff/privilege/issue_640_partitioned_table_grant/plan.txt diff --git a/cmd/issue_640_integration_test.go b/cmd/issue_640_integration_test.go deleted file mode 100644 index 1f6dc915..00000000 --- a/cmd/issue_640_integration_test.go +++ /dev/null @@ -1,111 +0,0 @@ -package cmd - -import ( - "context" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/pgplex/pgschema/testutil" -) - -// TestIssue640PartitionedTablePrivileges verifies that GRANT/REVOKE/dump -// cover privileges on partitioned tables (relkind 'p'), not only ordinary -// tables. See https://github.com/pgplex/pgschema/issues/640. -func TestIssue640PartitionedTablePrivileges(t *testing.T) { - if testing.Short() { - t.Skip("Skipping integration test in short mode") - } - - ctx := context.Background() - - embeddedPG := testutil.SetupPostgres(t) - defer embeddedPG.Stop() - conn, host, port, dbname, user, password := testutil.ConnectToPostgres(t, embeddedPG) - defer conn.Close() - - // Roles are not managed by pgschema; create on the target before plan/apply. - roleSQL := ` -DO $$ -BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN - CREATE ROLE reader; - END IF; -END $$; -` - if _, err := conn.ExecContext(ctx, roleSQL); err != nil { - t.Fatalf("create role on target: %v", err) - } - - tablesOnly := ` -CREATE TABLE events ( - id int NOT NULL, - at date NOT NULL -) PARTITION BY RANGE (at); - -CREATE TABLE events_2026_01 PARTITION OF events FOR VALUES FROM ('2026-01-01') TO ('2026-02-01'); - -CREATE TABLE plain (id int NOT NULL); -` - withGrants := tablesOnly + ` -GRANT SELECT ON events TO reader; -GRANT SELECT ON plain TO reader; -` - - schema := "public" - tmp := t.TempDir() - tablesFile := filepath.Join(tmp, "tables.sql") - grantsFile := filepath.Join(tmp, "with_grants.sql") - if err := os.WriteFile(tablesFile, []byte(tablesOnly), 0644); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(grantsFile, []byte(withGrants), 0644); err != nil { - t.Fatal(err) - } - - if err := applySchemaChanges(host, port, dbname, user, password, schema, tablesFile); err != nil { - t.Fatalf("apply tables: %v", err) - } - - planSQL, err := generatePlanSQLFormatted(host, port, dbname, user, password, schema, grantsFile) - if err != nil { - t.Fatalf("plan with grants: %v", err) - } - - hasGrant := func(sql, obj string) bool { - return strings.Contains(sql, "GRANT SELECT ON TABLE "+obj+" TO reader") || - strings.Contains(sql, "GRANT SELECT ON "+obj+" TO reader") - } - hasRevoke := func(sql, obj string) bool { - return strings.Contains(sql, "REVOKE SELECT ON TABLE "+obj+" FROM reader") || - strings.Contains(sql, "REVOKE SELECT ON "+obj+" FROM reader") - } - - if !hasGrant(planSQL, "plain") { - t.Fatalf("plan missing GRANT on plain table:\n%s", planSQL) - } - if !hasGrant(planSQL, "events") { - t.Fatalf("plan missing GRANT on partitioned table events (issue #640):\n%s", planSQL) - } - t.Logf("plan correctly includes GRANT on partitioned table events:\n%s", planSQL) - - if _, err := conn.ExecContext(ctx, ` - GRANT SELECT ON events TO reader; - GRANT SELECT ON plain TO reader; - `); err != nil { - t.Fatalf("grant on live db: %v", err) - } - - revokePlan, err := generatePlanSQLFormatted(host, port, dbname, user, password, schema, tablesFile) - if err != nil { - t.Fatalf("plan revoke: %v", err) - } - if !hasRevoke(revokePlan, "plain") { - t.Fatalf("plan missing REVOKE on plain:\n%s", revokePlan) - } - if !hasRevoke(revokePlan, "events") { - t.Fatalf("plan missing REVOKE on partitioned table events (issue #640):\n%s", revokePlan) - } - t.Logf("plan correctly includes REVOKE on partitioned table events:\n%s", revokePlan) -} diff --git a/testdata/diff/privilege/issue_640_partitioned_table_grant/diff.sql b/testdata/diff/privilege/issue_640_partitioned_table_grant/diff.sql new file mode 100644 index 00000000..a2f79ee9 --- /dev/null +++ b/testdata/diff/privilege/issue_640_partitioned_table_grant/diff.sql @@ -0,0 +1,5 @@ +REVOKE INSERT ON TABLE events FROM reader; + +GRANT SELECT ON TABLE events TO reader; + +GRANT UPDATE (at) ON TABLE events TO reader; diff --git a/testdata/diff/privilege/issue_640_partitioned_table_grant/new.sql b/testdata/diff/privilege/issue_640_partitioned_table_grant/new.sql new file mode 100644 index 00000000..dffb4df0 --- /dev/null +++ b/testdata/diff/privilege/issue_640_partitioned_table_grant/new.sql @@ -0,0 +1,16 @@ +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN + CREATE ROLE reader; + END IF; +END $$; + +CREATE TABLE events ( + id int NOT NULL, + at date NOT NULL +) PARTITION BY RANGE (at); + +CREATE TABLE events_2026_01 PARTITION OF events FOR VALUES FROM ('2026-01-01') TO ('2026-02-01'); + +GRANT SELECT ON events TO reader; +GRANT UPDATE (at) ON events TO reader; diff --git a/testdata/diff/privilege/issue_640_partitioned_table_grant/old.sql b/testdata/diff/privilege/issue_640_partitioned_table_grant/old.sql new file mode 100644 index 00000000..be37dcab --- /dev/null +++ b/testdata/diff/privilege/issue_640_partitioned_table_grant/old.sql @@ -0,0 +1,15 @@ +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'reader') THEN + CREATE ROLE reader; + END IF; +END $$; + +CREATE TABLE events ( + id int NOT NULL, + at date NOT NULL +) PARTITION BY RANGE (at); + +CREATE TABLE events_2026_01 PARTITION OF events FOR VALUES FROM ('2026-01-01') TO ('2026-02-01'); + +GRANT INSERT ON events TO reader; diff --git a/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.json b/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.json new file mode 100644 index 00000000..3feb2e03 --- /dev/null +++ b/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.json @@ -0,0 +1,32 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.13.1", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "c3d943f90b64cc5a2c206d2e949b575b0f2b0b4d6c2daf9cd3d525764d33eb36" + }, + "groups": [ + { + "steps": [ + { + "sql": "REVOKE INSERT ON TABLE events FROM reader;", + "type": "privilege", + "operation": "alter", + "path": "privileges.TABLE.events.reader" + }, + { + "sql": "GRANT SELECT ON TABLE events TO reader;", + "type": "privilege", + "operation": "alter", + "path": "privileges.TABLE.events.reader" + }, + { + "sql": "GRANT UPDATE (at) ON TABLE events TO reader;", + "type": "column_privilege", + "operation": "create", + "path": "column_privileges.TABLE.events.at.reader" + } + ] + } + ] +} diff --git a/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.sql b/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.sql new file mode 100644 index 00000000..a2f79ee9 --- /dev/null +++ b/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.sql @@ -0,0 +1,5 @@ +REVOKE INSERT ON TABLE events FROM reader; + +GRANT SELECT ON TABLE events TO reader; + +GRANT UPDATE (at) ON TABLE events TO reader; diff --git a/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.txt b/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.txt new file mode 100644 index 00000000..e88315e0 --- /dev/null +++ b/testdata/diff/privilege/issue_640_partitioned_table_grant/plan.txt @@ -0,0 +1,21 @@ +Plan: 1 to add, 2 to modify. + +Summary by type: + privileges: 2 to modify + column privileges: 1 to add + +Privileges: + ~ reader + ~ reader + +Column privileges: + + reader + +DDL to be executed: +-------------------------------------------------- + +REVOKE INSERT ON TABLE events FROM reader; + +GRANT SELECT ON TABLE events TO reader; + +GRANT UPDATE (at) ON TABLE events TO reader;