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..43a2c1cc --- /dev/null +++ b/testdata/diff/default_privilege/auto_grant_idempotent/new.sql @@ -0,0 +1,32 @@ +-- https://github.com/pgschema/pgschema/issues/250 +-- +-- 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 + 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 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; + +-- 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 above 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..678f8522 --- /dev/null +++ b/testdata/diff/default_privilege/auto_grant_idempotent/old.sql @@ -0,0 +1,36 @@ +-- https://github.com/pgschema/pgschema/issues/250 +-- +-- 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 + 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 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; + +-- Table and sequence +CREATE TABLE users ( + id serial PRIMARY KEY, + name text NOT NULL +); + +-- 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; 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.