diff --git a/cmd/issue_449_integration_test.go b/cmd/issue_449_integration_test.go new file mode 100644 index 00000000..c44305a2 --- /dev/null +++ b/cmd/issue_449_integration_test.go @@ -0,0 +1,96 @@ +package cmd + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/pgplex/pgschema/testutil" +) + +// TestIssue449RepeatPlanIdempotency verifies that a plan generated immediately after a +// successful apply reports no changes (https://github.com/pgplex/pgschema/issues/449). +// +// Two distinct drift sources are covered: +// 1. CHECK constraints casting to a type in the managed (non-public) schema: the current +// state rendered the cast schema-qualified while the desired state stripped the +// qualifier, producing a perpetual DROP/ADD/VALIDATE cycle (also issue #445). +// 2. RLS policy expressions on a table whose name equals the managed schema name: the +// same-schema qualifier stripping mistook table-qualified column references +// (profiles.id) for schema qualifiers and removed them from the current state only. +func TestIssue449RepeatPlanIdempotency(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() + + // applyThenReplan applies the desired state to the given schema and returns the SQL + // of a plan generated immediately afterwards. An idempotent plan returns "". + applyThenReplan := func(t *testing.T, schema, desiredSQL string) string { + t.Helper() + + if _, err := conn.ExecContext(ctx, "CREATE SCHEMA IF NOT EXISTS "+schema); err != nil { + t.Fatalf("Failed to create schema %s: %v", schema, err) + } + + desiredStateFile := filepath.Join(t.TempDir(), "desired.sql") + if err := os.WriteFile(desiredStateFile, []byte(desiredSQL), 0644); err != nil { + t.Fatalf("Failed to write desired state file: %v", err) + } + + if err := applySchemaChanges(host, port, dbname, user, password, schema, desiredStateFile); err != nil { + t.Fatalf("Failed to apply desired state: %v", err) + } + + replanOutput, err := generatePlanSQLFormatted(host, port, dbname, user, password, schema, desiredStateFile) + if err != nil { + t.Fatalf("Failed to generate repeat plan: %v", err) + } + return replanOutput + } + + t.Run("check_constraint_same_schema_type_cast", func(t *testing.T) { + replan := applyThenReplan(t, "app449", ` + CREATE TYPE item_status AS ENUM ('active', 'disabled'); + + CREATE TABLE items ( + id integer PRIMARY KEY, + status item_status NOT NULL, + CONSTRAINT items_status_check CHECK (status <> 'disabled'::item_status) + ); + `) + if replan != "" { + t.Errorf("Expected no changes on repeat plan after apply, but got:\n%s", replan) + } + }) + + t.Run("policy_schema_name_equals_table_name", func(t *testing.T) { + replan := applyThenReplan(t, "profiles", ` + CREATE TABLE profiles ( + id uuid PRIMARY KEY, + user_id uuid NOT NULL + ); + + CREATE TABLE profile_media ( + id uuid PRIMARY KEY, + profile_id uuid NOT NULL REFERENCES profiles (id) + ); + + ALTER TABLE profile_media ENABLE ROW LEVEL SECURITY; + + CREATE POLICY "Users can read their own media" ON profile_media + FOR SELECT + USING (profile_id IN (SELECT id FROM profiles)); + `) + if replan != "" { + t.Errorf("Expected no changes on repeat plan after apply, but got:\n%s", replan) + } + }) +} diff --git a/ir/normalize.go b/ir/normalize.go index 8afb20e2..1ee4db99 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -82,9 +82,9 @@ func normalizeTable(table *Table) { normalizeColumn(column, table.Schema) } - // Normalize policies (pass table schema for context - Issue #220) + // Normalize policies for _, policy := range table.Policies { - normalizePolicy(policy, table.Schema) + normalizePolicy(policy) } // Normalize triggers @@ -204,8 +204,7 @@ func normalizeDefaultValue(value string, tableSchema string) string { } // normalizePolicy normalizes RLS policy representation -// tableSchema is used to strip same-schema qualifiers from function calls (Issue #220) -func normalizePolicy(policy *RLSPolicy, tableSchema string) { +func normalizePolicy(policy *RLSPolicy) { if policy == nil { return } @@ -215,8 +214,8 @@ func normalizePolicy(policy *RLSPolicy, tableSchema string) { // Normalize expressions by removing extra whitespace // For policy expressions, we want to preserve parentheses as they are part of the expected format - policy.Using = normalizePolicyExpression(policy.Using, tableSchema) - policy.WithCheck = normalizePolicyExpression(policy.WithCheck, tableSchema) + policy.Using = normalizePolicyExpression(policy.Using) + policy.WithCheck = normalizePolicyExpression(policy.WithCheck) } // normalizePolicyRoles normalizes policy roles for consistent comparison @@ -243,8 +242,14 @@ func normalizePolicyRoles(roles []string) []string { // normalizePolicyExpression normalizes policy expressions (USING/WITH CHECK clauses) // It preserves parentheses as they are part of the expected format for policies -// tableSchema is used to strip same-schema qualifiers from function calls and table references (Issue #220, #224) -func normalizePolicyExpression(expr string, tableSchema string) string { +// +// Same-schema qualifiers no longer need stripping here: GetRLSPoliciesForSchema renders +// expressions with search_path set to the table's own schema, so same-schema references +// (Issue #220, #224) come out unqualified on both the current and desired side, while +// cross-schema references (Issue #427) stay qualified. Regex-stripping was also unsafe: +// when the schema name equals a table name, it mangled table-qualified column +// references like profiles.id (Issue #449). +func normalizePolicyExpression(expr string) string { if expr == "" { return expr } @@ -253,22 +258,6 @@ func normalizePolicyExpression(expr string, tableSchema string) string { expr = strings.TrimSpace(expr) expr = regexp.MustCompile(`\s+`).ReplaceAllString(expr, " ") - // Strip same-schema qualifiers from function calls (Issue #220) - // This matches PostgreSQL's behavior where same-schema qualifiers are stripped - // Example: tenant1.auth_uid() -> auth_uid() (when tableSchema is "tenant1") - // util.get_status() -> util.get_status() (preserved, different schema) - if tableSchema != "" && strings.Contains(expr, tableSchema+".") { - prefix := tableSchema + "." - pattern := regexp.MustCompile(regexp.QuoteMeta(prefix) + `([a-zA-Z_][a-zA-Z0-9_]*)\(`) - expr = pattern.ReplaceAllString(expr, `${1}(`) - - // Strip same-schema qualifiers from table references (Issue #224) - // Matches schema.identifier followed by whitespace, comma, closing paren, or end of string - // Example: public.users -> users (when tableSchema is "public") - tablePattern := regexp.MustCompile(regexp.QuoteMeta(prefix) + `([a-zA-Z_][a-zA-Z0-9_]*)(\s|,|\)|$)`) - expr = tablePattern.ReplaceAllString(expr, `${1}${2}`) - } - // Handle all parentheses normalization (adding required ones, removing unnecessary ones) expr = normalizeExpressionParentheses(expr) diff --git a/ir/normalize_test.go b/ir/normalize_test.go index 73ee9f2e..17828657 100644 --- a/ir/normalize_test.go +++ b/ir/normalize_test.go @@ -361,30 +361,32 @@ func TestNormalizeExpressionParentheses(t *testing.T) { func TestNormalizePolicyExpression(t *testing.T) { tests := []struct { - name string - expr string - tableSchema string - expected string + name string + expr string + expected string }{ { - name: "nested function call in policy expression", - expr: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", - tableSchema: "public", - expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", + name: "nested function call in policy expression", + expr: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", + expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", }, { - name: "schema-qualified nested function call", - expr: "(id IN ( SELECT unnest(public.get_user_assigned_projects()) AS unnest))", - tableSchema: "public", - expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", + // Schema qualifiers are preserved as rendered: GetRLSPoliciesForSchema sets + // search_path to the table's schema, so qualifiers only appear for + // cross-schema references and must survive normalization (issue #427). + // Table-qualified column references (profiles.id when the schema is also + // named profiles) must survive too (issue #449). + name: "qualified references preserved", + expr: "(id IN ( SELECT profiles.id FROM profiles WHERE (profiles.user_id = auth.uid())))", + expected: "(id IN ( SELECT profiles.id FROM profiles WHERE (profiles.user_id = auth.uid())))", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := normalizePolicyExpression(tt.expr, tt.tableSchema) + result := normalizePolicyExpression(tt.expr) if result != tt.expected { - t.Errorf("normalizePolicyExpression(%q, %q) = %q, want %q", tt.expr, tt.tableSchema, result, tt.expected) + t.Errorf("normalizePolicyExpression(%q) = %q, want %q", tt.expr, result, tt.expected) } }) } diff --git a/ir/queries/queries.sql b/ir/queries/queries.sql index cca8e8d4..c1c099a5 100644 --- a/ir/queries/queries.sql +++ b/ir/queries/queries.sql @@ -325,8 +325,8 @@ SELECT COALESCE(fcl.relname, '') AS foreign_table_name, COALESCE(fa.attname, '') AS foreign_column_name, COALESCE(fa.attnum, 0) AS foreign_ordinal_position, - CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, - CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition, + cd.check_clause, + cd.exclusion_definition, CASE c.confdeltype WHEN 'a' THEN 'NO ACTION' WHEN 'r' THEN 'RESTRICT' @@ -355,6 +355,16 @@ LEFT JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = ANY(c.conkey) LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)] +LEFT JOIN LATERAL ( + SELECT + -- Render with search_path set to the table's own schema so same-schema + -- type/function references come out unqualified while cross-schema + -- references stay qualified. This matches how the desired state renders + -- in its temporary schema, keeping both sides comparable (issue #449). + set_config('search_path', quote_ident(n.nspname), true) AS dummy, + CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, + CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition +) cd ON true WHERE n.nspname NOT IN ('information_schema', 'pg_catalog', 'pg_toast') AND n.nspname NOT LIKE 'pg_temp_%' AND n.nspname NOT LIKE 'pg_toast_temp_%' @@ -769,13 +779,12 @@ ORDER BY n.nspname, c.relname; -- GetRLSPolicies retrieves all row level security policies -- This replicates the pg_policies system view but computes the qual/with_check --- expressions with a forced search_path so cross-schema references stay qualified: --- 1. set_config sets search_path to only pg_catalog --- 2. pg_get_expr then uses that search_path and includes schema qualifiers for any --- function or table reference outside pg_catalog. Same-schema qualifiers are stripped --- later by normalizePolicyExpression. Without this, references to objects on the --- session search_path (e.g. Supabase's auth.uid()) are emitted unqualified and the --- generated CREATE POLICY fails on apply. (Issue #427) +-- expressions with a forced search_path so qualification is deterministic: +-- 1. set_config sets search_path to only the table's own schema +-- 2. pg_get_expr then renders same-schema references unqualified while cross-schema +-- references (e.g. Supabase's auth.uid(), issue #427) stay qualified. Without this, +-- rendering depends on the session search_path and the current/desired states +-- compare differently (issue #449). -- name: GetRLSPolicies :many SELECT n.nspname AS schemaname, @@ -805,7 +814,7 @@ JOIN pg_catalog.pg_class c ON c.oid = pol.polrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace LEFT JOIN LATERAL ( SELECT - set_config('search_path', 'pg_catalog', true) AS dummy, + set_config('search_path', quote_ident(n.nspname), true) AS dummy, pg_get_expr(pol.polqual, pol.polrelid) AS qual, pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check ) e ON true @@ -862,7 +871,12 @@ JOIN pg_catalog.pg_class c ON c.oid = pol.polrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace LEFT JOIN LATERAL ( SELECT - set_config('search_path', 'pg_catalog', true) AS dummy, + -- Render with search_path set to the table's own schema so same-schema + -- references come out unqualified while cross-schema references (e.g. + -- Supabase's auth.uid(), issue #427) stay qualified. This matches how the + -- desired state renders in its temporary schema, so both sides compare + -- equal without regex-stripping qualifiers afterwards (issue #449). + set_config('search_path', quote_ident(n.nspname), true) AS dummy, pg_get_expr(pol.polqual, pol.polrelid) AS qual, pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check ) e ON true @@ -970,8 +984,8 @@ SELECT COALESCE(fcl.relname, '') AS foreign_table_name, COALESCE(fa.attname, '') AS foreign_column_name, COALESCE(fa.attnum, 0) AS foreign_ordinal_position, - CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, - CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition, + cd.check_clause, + cd.exclusion_definition, CASE c.confdeltype WHEN 'a' THEN 'NO ACTION' WHEN 'r' THEN 'RESTRICT' @@ -1004,6 +1018,16 @@ LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)] LEFT JOIN pg_index i ON i.indexrelid = c.conindid +LEFT JOIN LATERAL ( + SELECT + -- Render with search_path set to the table's own schema so same-schema + -- type/function references come out unqualified while cross-schema + -- references stay qualified. This matches how the desired state renders + -- in its temporary schema, keeping both sides comparable (issue #449). + set_config('search_path', quote_ident(n.nspname), true) AS dummy, + CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, + CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition +) cd ON true WHERE n.nspname = $1 -- Skip internal per-partition FK rows (conparentid != 0) that PostgreSQL -- creates when a FK references a partitioned table. pg_dump omits these; diff --git a/ir/queries/queries.sql.go b/ir/queries/queries.sql.go index cf066330..82f4b943 100644 --- a/ir/queries/queries.sql.go +++ b/ir/queries/queries.sql.go @@ -861,8 +861,8 @@ SELECT COALESCE(fcl.relname, '') AS foreign_table_name, COALESCE(fa.attname, '') AS foreign_column_name, COALESCE(fa.attnum, 0) AS foreign_ordinal_position, - CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, - CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition, + cd.check_clause, + cd.exclusion_definition, CASE c.confdeltype WHEN 'a' THEN 'NO ACTION' WHEN 'r' THEN 'RESTRICT' @@ -891,6 +891,16 @@ LEFT JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = ANY(c.conkey) LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)] +LEFT JOIN LATERAL ( + SELECT + -- Render with search_path set to the table's own schema so same-schema + -- type/function references come out unqualified while cross-schema + -- references stay qualified. This matches how the desired state renders + -- in its temporary schema, keeping both sides comparable (issue #449). + set_config('search_path', quote_ident(n.nspname), true) AS dummy, + CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, + CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition +) cd ON true WHERE n.nspname NOT IN ('information_schema', 'pg_catalog', 'pg_toast') AND n.nspname NOT LIKE 'pg_temp_%' AND n.nspname NOT LIKE 'pg_toast_temp_%' @@ -986,8 +996,8 @@ SELECT COALESCE(fcl.relname, '') AS foreign_table_name, COALESCE(fa.attname, '') AS foreign_column_name, COALESCE(fa.attnum, 0) AS foreign_ordinal_position, - CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, - CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition, + cd.check_clause, + cd.exclusion_definition, CASE c.confdeltype WHEN 'a' THEN 'NO ACTION' WHEN 'r' THEN 'RESTRICT' @@ -1020,6 +1030,16 @@ LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)] LEFT JOIN pg_index i ON i.indexrelid = c.conindid +LEFT JOIN LATERAL ( + SELECT + -- Render with search_path set to the table's own schema so same-schema + -- type/function references come out unqualified while cross-schema + -- references stay qualified. This matches how the desired state renders + -- in its temporary schema, keeping both sides comparable (issue #449). + set_config('search_path', quote_ident(n.nspname), true) AS dummy, + CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause, + CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition +) cd ON true WHERE n.nspname = $1 -- Skip internal per-partition FK rows (conparentid != 0) that PostgreSQL -- creates when a FK references a partitioned table. pg_dump omits these; @@ -2403,7 +2423,7 @@ JOIN pg_catalog.pg_class c ON c.oid = pol.polrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace LEFT JOIN LATERAL ( SELECT - set_config('search_path', 'pg_catalog', true) AS dummy, + set_config('search_path', quote_ident(n.nspname), true) AS dummy, pg_get_expr(pol.polqual, pol.polrelid) AS qual, pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check ) e ON true @@ -2427,13 +2447,12 @@ type GetRLSPoliciesRow struct { // GetRLSPolicies retrieves all row level security policies // This replicates the pg_policies system view but computes the qual/with_check -// expressions with a forced search_path so cross-schema references stay qualified: -// 1. set_config sets search_path to only pg_catalog -// 2. pg_get_expr then uses that search_path and includes schema qualifiers for any -// function or table reference outside pg_catalog. Same-schema qualifiers are stripped -// later by normalizePolicyExpression. Without this, references to objects on the -// session search_path (e.g. Supabase's auth.uid()) are emitted unqualified and the -// generated CREATE POLICY fails on apply. (Issue #427) +// expressions with a forced search_path so qualification is deterministic: +// 1. set_config sets search_path to only the table's own schema +// 2. pg_get_expr then renders same-schema references unqualified while cross-schema +// references (e.g. Supabase's auth.uid(), issue #427) stay qualified. Without this, +// rendering depends on the session search_path and the current/desired states +// compare differently (issue #449). func (q *Queries) GetRLSPolicies(ctx context.Context) ([]GetRLSPoliciesRow, error) { rows, err := q.db.QueryContext(ctx, getRLSPolicies) if err != nil { @@ -2495,7 +2514,12 @@ JOIN pg_catalog.pg_class c ON c.oid = pol.polrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace LEFT JOIN LATERAL ( SELECT - set_config('search_path', 'pg_catalog', true) AS dummy, + -- Render with search_path set to the table's own schema so same-schema + -- references come out unqualified while cross-schema references (e.g. + -- Supabase's auth.uid(), issue #427) stay qualified. This matches how the + -- desired state renders in its temporary schema, so both sides compare + -- equal without regex-stripping qualifiers afterwards (issue #449). + set_config('search_path', quote_ident(n.nspname), true) AS dummy, pg_get_expr(pol.polqual, pol.polrelid) AS qual, pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check ) e ON true