From 23b40446846fe25ca81eda38059316362dced5d3 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sat, 17 Jan 2026 04:17:10 -0800 Subject: [PATCH 1/2] fix: skip revoking privileges covered by default privileges (#250) When default privileges are set for a role, objects created by that role automatically inherit those privileges. Previously, pgschema would incorrectly generate REVOKE statements for these auto-granted privileges because they appeared in the current state but not explicitly in the desired state SQL files. This fix adds logic to check if a privilege being dropped is covered by a matching default privilege in the desired state. If the privilege's object type, grantee, grant option, and privilege types are all covered by a default privilege, the REVOKE is skipped. Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 13 +++- internal/diff/privilege.go | 62 +++++++++++++++++++ .../auto_grant_idempotent/diff.sql | 0 .../auto_grant_idempotent/new.sql | 24 +++++++ .../auto_grant_idempotent/old.sql | 26 ++++++++ .../auto_grant_idempotent/plan.json | 9 +++ .../auto_grant_idempotent/plan.sql | 0 .../auto_grant_idempotent/plan.txt | 1 + 8 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 testdata/diff/default_privilege/auto_grant_idempotent/diff.sql create mode 100644 testdata/diff/default_privilege/auto_grant_idempotent/new.sql create mode 100644 testdata/diff/default_privilege/auto_grant_idempotent/old.sql create mode 100644 testdata/diff/default_privilege/auto_grant_idempotent/plan.json create mode 100644 testdata/diff/default_privilege/auto_grant_idempotent/plan.sql create mode 100644 testdata/diff/default_privilege/auto_grant_idempotent/plan.txt diff --git a/internal/diff/diff.go b/internal/diff/diff.go index f5dceb40..7b69943c 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1104,10 +1104,21 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { } } + // Collect default privileges from the desired state (new IR) + // These are used to filter out privileges that are covered by default privileges + var newDefaultPrivileges []*ir.DefaultPrivilege + for _, dbSchema := range newIR.Schemas { + newDefaultPrivileges = append(newDefaultPrivileges, dbSchema.DefaultPrivileges...) + } + // Find dropped privileges (in old but not matched) + // Skip privileges that are covered by default privileges in the desired state for fullKey, p := range oldPrivs { if !matchedOld[fullKey] { - diff.droppedPrivileges = append(diff.droppedPrivileges, p) + // Check if this privilege is covered by a default privilege + if !isPrivilegeCoveredByDefaultPrivileges(p, newDefaultPrivileges) { + diff.droppedPrivileges = append(diff.droppedPrivileges, p) + } } } diff --git a/internal/diff/privilege.go b/internal/diff/privilege.go index c523f8b4..e763a364 100644 --- a/internal/diff/privilege.go +++ b/internal/diff/privilege.go @@ -294,3 +294,65 @@ func privilegesEqual(old, new *ir.Privilege) bool { func (d *privilegeDiff) GetObjectName() string { return d.New.GetObjectKey() } + +// isPrivilegeCoveredByDefaultPrivileges checks if an explicit privilege is covered +// by default privileges in the desired state. This is used to avoid generating +// spurious REVOKE statements for privileges that are auto-granted via default privileges. +// See https://github.com/pgschema/pgschema/issues/250 +func isPrivilegeCoveredByDefaultPrivileges(p *ir.Privilege, defaultPrivileges []*ir.DefaultPrivilege) bool { + for _, dp := range defaultPrivileges { + // Match object types (TABLE -> TABLES, SEQUENCE -> SEQUENCES, etc.) + if !privilegeObjectTypeMatchesDefault(p.ObjectType, dp.ObjectType) { + continue + } + + // Match grantee + if p.Grantee != dp.Grantee { + continue + } + + // Match grant option + if p.WithGrantOption != dp.WithGrantOption { + continue + } + + // Check if all privilege types are covered by the default privilege + if privilegesCoveredBy(p.Privileges, dp.Privileges) { + return true + } + } + return false +} + +// privilegeObjectTypeMatchesDefault checks if a privilege object type matches +// a default privilege object type (e.g., TABLE matches TABLES) +func privilegeObjectTypeMatchesDefault(privType ir.PrivilegeObjectType, defaultType ir.DefaultPrivilegeObjectType) bool { + switch privType { + case ir.PrivilegeObjectTypeTable: + return defaultType == ir.DefaultPrivilegeObjectTypeTables + case ir.PrivilegeObjectTypeSequence: + return defaultType == ir.DefaultPrivilegeObjectTypeSequences + case ir.PrivilegeObjectTypeFunction: + return defaultType == ir.DefaultPrivilegeObjectTypeFunctions + case ir.PrivilegeObjectTypeProcedure: + return defaultType == ir.DefaultPrivilegeObjectTypeFunctions // Procedures use FUNCTIONS default + case ir.PrivilegeObjectTypeType: + return defaultType == ir.DefaultPrivilegeObjectTypeTypes + default: + return false + } +} + +// privilegesCoveredBy checks if all privileges in 'privs' are covered by 'coveringPrivs' +func privilegesCoveredBy(privs, coveringPrivs []string) bool { + coveringSet := make(map[string]bool) + for _, p := range coveringPrivs { + coveringSet[p] = true + } + for _, p := range privs { + if !coveringSet[p] { + return false + } + } + return true +} diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/diff.sql b/testdata/diff/default_privilege/auto_grant_idempotent/diff.sql new file mode 100644 index 00000000..e69de29b diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/new.sql b/testdata/diff/default_privilege/auto_grant_idempotent/new.sql new file mode 100644 index 00000000..b0381dbb --- /dev/null +++ b/testdata/diff/default_privilege/auto_grant_idempotent/new.sql @@ -0,0 +1,24 @@ +-- https://github.com/pgschema/pgschema/issues/250 +-- Desired state: same default privileges, same table, no explicit grants + +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_role') THEN + CREATE ROLE app_role; + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'owner_role') THEN + CREATE ROLE owner_role; + END IF; +END $$; + +-- Default privileges (same as old state) +ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_role; +ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT USAGE ON SEQUENCES TO app_role; + +-- Table and sequence (same as old state) +CREATE TABLE users ( + id serial PRIMARY KEY, + name text NOT NULL +); + +-- No explicit grants - covered by default privileges diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/old.sql b/testdata/diff/default_privilege/auto_grant_idempotent/old.sql new file mode 100644 index 00000000..494c4a08 --- /dev/null +++ b/testdata/diff/default_privilege/auto_grant_idempotent/old.sql @@ -0,0 +1,26 @@ +-- https://github.com/pgschema/pgschema/issues/250 +-- Current state: default privileges + explicit grants that match them + +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_role') THEN + CREATE ROLE app_role; + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'owner_role') THEN + CREATE ROLE owner_role; + END IF; +END $$; + +-- Default privileges +ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_role; +ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT USAGE ON SEQUENCES TO app_role; + +-- Table and sequence +CREATE TABLE users ( + id serial PRIMARY KEY, + name text NOT NULL +); + +-- Explicit grants that match the default privileges above +GRANT SELECT, INSERT, UPDATE, DELETE ON TABLE users TO app_role; +GRANT USAGE ON SEQUENCE users_id_seq TO app_role; diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/plan.json b/testdata/diff/default_privilege/auto_grant_idempotent/plan.json new file mode 100644 index 00000000..937ac588 --- /dev/null +++ b/testdata/diff/default_privilege/auto_grant_idempotent/plan.json @@ -0,0 +1,9 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.6.1", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "16119adc01274eb2d4d4fdb177740d5176d82ff4330f7605e3c08a5926b8e734" + }, + "groups": null +} diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/plan.sql b/testdata/diff/default_privilege/auto_grant_idempotent/plan.sql new file mode 100644 index 00000000..e69de29b diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/plan.txt b/testdata/diff/default_privilege/auto_grant_idempotent/plan.txt new file mode 100644 index 00000000..241994af --- /dev/null +++ b/testdata/diff/default_privilege/auto_grant_idempotent/plan.txt @@ -0,0 +1 @@ +No changes detected. From 766b095edfc8562ae196a2fb50910cf3a9e63abd Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Sat, 17 Jan 2026 04:30:33 -0800 Subject: [PATCH 2/2] docs: improve test case documentation for default privilege coverage Address PR review feedback by adding detailed comments explaining: - The test simulates auto-granted privileges using explicit GRANTs - Why this approach is used (testing diff logic, not PostgreSQL behavior) - What scenario the test represents (re-applying same schema) Co-Authored-By: Claude Opus 4.5 --- .../auto_grant_idempotent/new.sql | 14 +++++++++++--- .../auto_grant_idempotent/old.sql | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/new.sql b/testdata/diff/default_privilege/auto_grant_idempotent/new.sql index b0381dbb..43a2c1cc 100644 --- a/testdata/diff/default_privilege/auto_grant_idempotent/new.sql +++ b/testdata/diff/default_privilege/auto_grant_idempotent/new.sql @@ -1,5 +1,13 @@ -- https://github.com/pgschema/pgschema/issues/250 --- Desired state: same default privileges, same table, no explicit grants +-- +-- Test: Privileges covered by default privileges should not be revoked. +-- +-- This represents the desired state as written in the user's SQL files. +-- The user declares default privileges and creates objects, but does NOT +-- include explicit GRANTs because they expect PostgreSQL to auto-grant them. +-- +-- The diff should NOT generate REVOKE statements because the privileges +-- in old.sql are covered by the default privileges defined here. DO $$ BEGIN @@ -11,7 +19,7 @@ BEGIN END IF; END $$; --- Default privileges (same as old state) +-- Default privileges for owner_role (same as old state) ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_role; ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT USAGE ON SEQUENCES TO app_role; @@ -21,4 +29,4 @@ CREATE TABLE users ( name text NOT NULL ); --- No explicit grants - covered by default privileges +-- No explicit GRANTs - covered by default privileges above diff --git a/testdata/diff/default_privilege/auto_grant_idempotent/old.sql b/testdata/diff/default_privilege/auto_grant_idempotent/old.sql index 494c4a08..678f8522 100644 --- a/testdata/diff/default_privilege/auto_grant_idempotent/old.sql +++ b/testdata/diff/default_privilege/auto_grant_idempotent/old.sql @@ -1,5 +1,14 @@ -- https://github.com/pgschema/pgschema/issues/250 --- Current state: default privileges + explicit grants that match them +-- +-- Test: Privileges covered by default privileges should not be revoked. +-- +-- This test simulates the scenario where: +-- 1. Default privileges are configured for owner_role +-- 2. Objects were created by owner_role, so privileges were auto-granted +-- 3. The explicit GRANTs below represent those auto-granted privileges +-- +-- In a real database, these grants would be created automatically by PostgreSQL +-- when owner_role creates objects. Here we simulate this by adding explicit GRANTs. DO $$ BEGIN @@ -11,7 +20,7 @@ BEGIN END IF; END $$; --- Default privileges +-- Default privileges for owner_role ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO app_role; ALTER DEFAULT PRIVILEGES FOR ROLE owner_role IN SCHEMA public GRANT USAGE ON SEQUENCES TO app_role; @@ -21,6 +30,7 @@ CREATE TABLE users ( name text NOT NULL ); --- Explicit grants that match the default privileges above +-- Simulate auto-granted privileges (what PostgreSQL would grant automatically +-- when owner_role creates objects with the above default privileges) GRANT SELECT, INSERT, UPDATE, DELETE ON TABLE users TO app_role; GRANT USAGE ON SEQUENCE users_id_seq TO app_role;