fix: preserve nested function calls in RLS policy expressions (#377)#378
fix: preserve nested function calls in RLS policy expressions (#377)#378
Conversation
normalizeExpressionParentheses had a regex that incorrectly matched nested function calls like unnest(get_user_assigned_projects()) as redundantly-wrapped single function calls, stripping the outer parens and merging tokens into invalid SQL (unnestget_user_assigned_projects()). Remove the broken regex. The parenthesized form that pg_get_expr returns (e.g., (current_setting(...))::integer) is valid SQL and needs no unwrapping. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes a bug in Confidence Score: 5/5Safe to merge — targeted removal of a broken regex with no functional regressions. The only change to production logic is removing a regex that was provably wrong for nested function calls. The No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User schema SQL\ne.g. USING (id IN (SELECT unnest(f()) AS unnest))"] --> B["Embedded PG → pg_get_expr\nreturns canonical form with parens"]
C["Applied DB schema"] --> D["Target PG → pg_get_expr\nreturns canonical form with parens"]
B --> E["normalizePolicyExpression(expr, schema)"]
D --> F["normalizePolicyExpression(expr, schema)"]
E --> G["Strip same-schema qualifiers"]
F --> H["Strip same-schema qualifiers"]
G --> I["normalizeExpressionParentheses\n① Wrap if not already wrapped\n② Remove redundant text::text casts"]
H --> J["normalizeExpressionParentheses\n① Wrap if not already wrapped\n② Remove redundant text::text casts"]
I --> K["Compare normalised forms"]
J --> K
K --> L{Equal?}
L -- Yes --> M["No diff — policy unchanged"]
L -- No --> N["Emit ALTER / DROP+CREATE policy"]
style I fill:#d4edda,stroke:#28a745
style J fill:#d4edda,stroke:#28a745
Reviews (1): Last reviewed commit: "fix: preserve nested function calls in R..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func TestNormalizeExpressionParentheses(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "simple expression gets wrapped", | ||
| input: "tenant_id = 1", | ||
| expected: "(tenant_id = 1)", | ||
| }, | ||
| { | ||
| name: "already parenthesized", | ||
| input: "(tenant_id = 1)", | ||
| expected: "(tenant_id = 1)", | ||
| }, | ||
| { | ||
| name: "nested function call preserved", | ||
| input: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", | ||
| expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))", | ||
| }, | ||
| { | ||
| name: "simple function call preserved", | ||
| input: "(has_scope('admin'))", | ||
| expected: "(has_scope('admin'))", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := normalizeExpressionParentheses(tt.input) | ||
| if result != tt.expected { | ||
| t.Errorf("normalizeExpressionParentheses(%q) = %q, want %q", tt.input, result, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new TestNormalizeExpressionParentheses tests verify the fix for nested function calls and basic wrapping, but there's no test case for the expression pattern that motivated the old regex — the type-cast form (current_setting('app.current_tenant'))::integer.
Adding a case like this would document that the cast suffix is preserved and guard against a future regression if someone tries to "clean up" the function by re-adding a similar regex:
{
name: "function call with type cast preserved",
input: "(tenant_id = (current_setting('app.current_tenant'))::integer)",
expected: "(tenant_id = (current_setting('app.current_tenant'))::integer)",
},There was a problem hiding this comment.
Pull request overview
Fixes a regression in RLS policy expression normalization where nested function calls could be mangled into invalid SQL during plan generation (e.g., unnest(get_user_assigned_projects()) becoming unnestget_user_assigned_projects()), by removing an unsafe regex-based “unwrapping” step and adding targeted test coverage.
Changes:
- Removed the regex that attempted to strip “redundant” parentheses around function calls in
normalizeExpressionParentheses. - Added unit tests covering
normalizeExpressionParenthesesandnormalizePolicyExpressionfor nested function call preservation. - Added/updated diff fixtures to ensure nested function calls in unchanged policies don’t trigger policy diffs or invalid emitted SQL.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ir/normalize.go | Removes the problematic regex that could corrupt nested function calls in policy expressions. |
| ir/normalize_test.go | Adds unit tests validating correct normalization behavior for nested function calls and schema-qualifier stripping. |
| testdata/diff/create_policy/issue_377_nested_function_in_policy/old.sql | New fixture reproducing the nested function policy expression scenario from #377. |
| testdata/diff/create_policy/issue_377_nested_function_in_policy/new.sql | New fixture with a non-policy table change to ensure the unchanged policy doesn’t produce diffs/invalid SQL. |
| testdata/diff/create_policy/issue_377_nested_function_in_policy/diff.sql | Expected diff SQL (table column add only). |
| testdata/diff/create_policy/issue_377_nested_function_in_policy/plan.txt | Expected plan output confirming only the table change is planned. |
| testdata/diff/create_policy/issue_377_nested_function_in_policy/plan.sql | Expected plan SQL (table column add only). |
| testdata/diff/create_policy/issue_377_nested_function_in_policy/plan.json | Expected plan JSON for the new fixture. |
| testdata/diff/create_policy/add_policy/plan.txt | Updates expected DDL formatting to preserve pg_get_expr-style parenthesized function calls with casts. |
| testdata/diff/create_policy/add_policy/plan.sql | Same as above for SQL output. |
| testdata/diff/create_policy/add_policy/plan.json | Same as above for JSON output. |
| testdata/diff/create_policy/add_policy/diff.sql | Updates expected diff SQL formatting accordingly. |
| testdata/diff/create_policy/alter_policy_using/plan.json | Updates source fingerprint hash due to normalization/output changes. |
| testdata/diff/create_policy/alter_policy_roles/plan.json | Updates source fingerprint hash due to normalization/output changes. |
| testdata/diff/create_policy/disable_rls/plan.json | Updates source fingerprint hash due to normalization/output changes. |
| testdata/diff/create_policy/drop_policy/plan.json | Updates source fingerprint hash due to normalization/output changes. |
| testdata/diff/create_policy/force_rls/plan.json | Updates source fingerprint hash due to normalization/output changes. |
| testdata/diff/create_policy/remove_force_rls/plan.json | Updates source fingerprint hash due to normalization/output changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
normalizeExpressionParentheseshad a regex that treated nested function calls (e.g.,unnest(get_user_assigned_projects())) as redundantly-wrapped single function calls, stripping the outer parens and producing invalid SQL (unnestget_user_assigned_projects())pg_get_expr(e.g.,(current_setting(...))::integer) is valid SQL and needs no unwrappingFixes #377
Test plan
normalizeExpressionParenthesesandnormalizePolicyExpressionconfirming nested function calls are preservedcreate_policy/issue_377_nested_function_in_policyPGSCHEMA_TEST_FILTER="create_policy/" go test -v ./cmd -run TestPlanAndApply)go test -v ./ir/...)🤖 Generated with Claude Code