From 687decb2bb0c2430968a50d9941ad78453875546 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Tue, 27 Jan 2026 07:10:40 -0800 Subject: [PATCH] fix: missing parentheses in CREATE POLICY USING clause output (#259) PostgreSQL's CREATE POLICY syntax requires parentheses around USING and WITH CHECK expressions. When pg_get_expr returns simple function calls without outer parentheses (e.g., is_admin()), pgschema was outputting invalid SQL like `USING is_admin()` instead of `USING (is_admin())`. Added ensureParentheses() helper to wrap expressions in parentheses if not already wrapped. Co-Authored-By: Claude Opus 4.5 --- internal/diff/policy.go | 22 +++++++++++++++---- .../diff/create_policy/add_policy/diff.sql | 1 + .../diff/create_policy/add_policy/new.sql | 10 +++++++++ .../diff/create_policy/add_policy/old.sql | 3 +++ .../diff/create_policy/add_policy/plan.json | 8 ++++++- .../diff/create_policy/add_policy/plan.sql | 2 ++ .../diff/create_policy/add_policy/plan.txt | 3 +++ 7 files changed, 44 insertions(+), 5 deletions(-) diff --git a/internal/diff/policy.go b/internal/diff/policy.go index 2c145d4a..a4ef6dc7 100644 --- a/internal/diff/policy.go +++ b/internal/diff/policy.go @@ -105,12 +105,12 @@ func generatePolicySQL(policy *ir.RLSPolicy, targetSchema string) string { // Add USING clause if present if policy.Using != "" { - policyStmt += fmt.Sprintf(" USING %s", policy.Using) + policyStmt += fmt.Sprintf(" USING %s", ensureParentheses(policy.Using)) } // Add WITH CHECK clause if present if policy.WithCheck != "" { - policyStmt += fmt.Sprintf(" WITH CHECK %s", policy.WithCheck) + policyStmt += fmt.Sprintf(" WITH CHECK %s", ensureParentheses(policy.WithCheck)) } return policyStmt + ";" @@ -142,12 +142,12 @@ func generateAlterPolicySQL(old, new *ir.RLSPolicy, targetSchema string) string // Add USING clause if it changed if usingChange { - alterStmt += fmt.Sprintf(" USING %s", new.Using) + alterStmt += fmt.Sprintf(" USING %s", ensureParentheses(new.Using)) } // Add WITH CHECK clause if it changed if withCheckChange { - alterStmt += fmt.Sprintf(" WITH CHECK %s", new.WithCheck) + alterStmt += fmt.Sprintf(" WITH CHECK %s", ensureParentheses(new.WithCheck)) } // If no changes detected, this shouldn't happen @@ -158,6 +158,20 @@ func generateAlterPolicySQL(old, new *ir.RLSPolicy, targetSchema string) string return alterStmt + ";" } +// ensureParentheses wraps an expression in parentheses if not already wrapped. +// PostgreSQL's CREATE POLICY and ALTER POLICY syntax requires USING (expr) and WITH CHECK (expr). +// pg_get_expr may return expressions without outer parentheses (e.g., simple function calls). +func ensureParentheses(expr string) string { + if expr == "" { + return expr + } + expr = strings.TrimSpace(expr) + if strings.HasPrefix(expr, "(") && strings.HasSuffix(expr, ")") { + return expr + } + return "(" + expr + ")" +} + // roleListsEqualCaseInsensitive compares two role lists for equality (case-insensitive) // PostgreSQL role names are case-insensitive by default func roleListsEqualCaseInsensitive(oldRoles, newRoles []string) bool { diff --git a/testdata/diff/create_policy/add_policy/diff.sql b/testdata/diff/create_policy/add_policy/diff.sql index 5e0a116e..f3227297 100644 --- a/testdata/diff/create_policy/add_policy/diff.sql +++ b/testdata/diff/create_policy/add_policy/diff.sql @@ -1,6 +1,7 @@ ALTER TABLE orders ENABLE ROW LEVEL SECURITY; CREATE POLICY orders_user_access ON orders FOR SELECT TO PUBLIC USING (user_id IN ( SELECT users.id FROM users)); CREATE POLICY "UserPolicy" ON users TO PUBLIC USING (tenant_id = current_setting('app.current_tenant')::integer); +CREATE POLICY admin_only ON users FOR DELETE TO PUBLIC USING (is_admin()); CREATE POLICY "my-policy" ON users FOR INSERT TO PUBLIC WITH CHECK ((role)::text = 'user'); CREATE POLICY "select" ON users FOR SELECT TO PUBLIC USING (true); CREATE POLICY user_tenant_isolation ON users FOR UPDATE TO PUBLIC USING (tenant_id = current_setting('app.current_tenant')::integer); diff --git a/testdata/diff/create_policy/add_policy/new.sql b/testdata/diff/create_policy/add_policy/new.sql index e61a347a..3ccbf283 100644 --- a/testdata/diff/create_policy/add_policy/new.sql +++ b/testdata/diff/create_policy/add_policy/new.sql @@ -11,12 +11,22 @@ CREATE TABLE orders ( total NUMERIC(10,2) ); +-- Function to check if user is admin (for Issue #259) +CREATE FUNCTION is_admin() RETURNS boolean LANGUAGE sql AS $$ SELECT true $$; + -- RLS is enabled with multiple policies demonstrating quoting scenarios ALTER TABLE users ENABLE ROW LEVEL SECURITY; -- RLS on orders with policy referencing users table (Issue #224) ALTER TABLE orders ENABLE ROW LEVEL SECURITY; +-- Policy with function call in USING clause (Issue #259) +-- Tests that parentheses are correctly preserved around function calls +CREATE POLICY admin_only ON users + FOR DELETE + TO PUBLIC + USING (is_admin()); + -- Policy with reserved word name (requires quoting) CREATE POLICY "select" ON users FOR SELECT diff --git a/testdata/diff/create_policy/add_policy/old.sql b/testdata/diff/create_policy/add_policy/old.sql index 91fb3453..b2516019 100644 --- a/testdata/diff/create_policy/add_policy/old.sql +++ b/testdata/diff/create_policy/add_policy/old.sql @@ -11,5 +11,8 @@ CREATE TABLE orders ( total NUMERIC(10,2) ); +-- Function to check if user is admin (for Issue #259) +CREATE FUNCTION is_admin() RETURNS boolean LANGUAGE sql AS $$ SELECT true $$; + -- RLS is enabled but no policies exist yet ALTER TABLE users ENABLE ROW LEVEL SECURITY; diff --git a/testdata/diff/create_policy/add_policy/plan.json b/testdata/diff/create_policy/add_policy/plan.json index e48e496b..5f2c3f0e 100644 --- a/testdata/diff/create_policy/add_policy/plan.json +++ b/testdata/diff/create_policy/add_policy/plan.json @@ -3,7 +3,7 @@ "pgschema_version": "1.6.1", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "9323772d9678bd1630383ff088214914f1c01c427086930540c96be45e4be387" + "hash": "261f1966678419184a6d17b5ad2c09d590b77fff9dbe01fd56849d53f3079c8e" }, "groups": [ { @@ -26,6 +26,12 @@ "operation": "create", "path": "public.users.UserPolicy" }, + { + "sql": "CREATE POLICY admin_only ON users FOR DELETE TO PUBLIC USING (is_admin());", + "type": "table.policy", + "operation": "create", + "path": "public.users.admin_only" + }, { "sql": "CREATE POLICY \"my-policy\" ON users FOR INSERT TO PUBLIC WITH CHECK ((role)::text = 'user');", "type": "table.policy", diff --git a/testdata/diff/create_policy/add_policy/plan.sql b/testdata/diff/create_policy/add_policy/plan.sql index 4d87fa18..e054fda1 100644 --- a/testdata/diff/create_policy/add_policy/plan.sql +++ b/testdata/diff/create_policy/add_policy/plan.sql @@ -4,6 +4,8 @@ CREATE POLICY orders_user_access ON orders FOR SELECT TO PUBLIC USING (user_id I CREATE POLICY "UserPolicy" ON users TO PUBLIC USING (tenant_id = current_setting('app.current_tenant')::integer); +CREATE POLICY admin_only ON users FOR DELETE TO PUBLIC USING (is_admin()); + CREATE POLICY "my-policy" ON users FOR INSERT TO PUBLIC WITH CHECK ((role)::text = 'user'); CREATE POLICY "select" ON users FOR SELECT TO PUBLIC USING (true); diff --git a/testdata/diff/create_policy/add_policy/plan.txt b/testdata/diff/create_policy/add_policy/plan.txt index 4ce51f08..4e760a23 100644 --- a/testdata/diff/create_policy/add_policy/plan.txt +++ b/testdata/diff/create_policy/add_policy/plan.txt @@ -9,6 +9,7 @@ Tables: + orders (rls) ~ users + UserPolicy (policy) + + admin_only (policy) + my-policy (policy) + select (policy) + user_tenant_isolation (policy) @@ -22,6 +23,8 @@ CREATE POLICY orders_user_access ON orders FOR SELECT TO PUBLIC USING (user_id I CREATE POLICY "UserPolicy" ON users TO PUBLIC USING (tenant_id = current_setting('app.current_tenant')::integer); +CREATE POLICY admin_only ON users FOR DELETE TO PUBLIC USING (is_admin()); + CREATE POLICY "my-policy" ON users FOR INSERT TO PUBLIC WITH CHECK ((role)::text = 'user'); CREATE POLICY "select" ON users FOR SELECT TO PUBLIC USING (true);