[FE-4070] fix(studio): allow adding expressions to RLS policies - #48700
Conversation
… them A policy created via SQL without a USING or WITH CHECK clause stores null for that field, and the policy editor's diff logic skipped null fields entirely, so adding an expression through the dashboard was silently dropped. The diff now treats stored null as empty and is branched by command so INSERT policies never emit a USING clause. Saving with no changes no longer sends an empty transaction, and clearing an existing WITH CHECK expression shows an error instead of generating invalid SQL.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
| ) | ||
| } | ||
|
|
||
| const payload = generateUpdatePolicyPayload(selectedPolicy, { |
There was a problem hiding this comment.
Review context: the old inline diff here had three quirks this replaces. (1) selectedPolicy.definition !== null && … meant a policy whose definition was null (created via SQL without USING) could never gain one — the FE-4070 bug. (2) The null !== undefined comparisons on check assigned a present-but-undefined key, so an untouched form still passed the Object.keys(payload).length check and sent a literal BEGIN; COMMIT; to the user database. (3) With the null guard removed, the definition branch would have fired for INSERT policies too (editor one holds the check expression there), emitting an invalid USING clause — hence the diff is now branched by command inside the util.
📝 WalkthroughWalkthroughThe policy editor now validates update-specific expression rules, generates payloads containing only changed fields, handles command-specific expression mapping, and submits executable SQL fragments. Unit tests cover unchanged, changed, empty, ChangesPolicy update payload
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PolicyEditorPanel
participant generateUpdatePolicyPayload
participant UpdateMutation
PolicyEditorPanel->>PolicyEditorPanel: validate policy changes
PolicyEditorPanel->>generateUpdatePolicyPayload: generate changed payload
generateUpdatePolicyPayload-->>PolicyEditorPanel: return changed fields
generateUpdatePolicyPayload->>UpdateMutation: submit executable SQL fragments
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/studio/components/interfaces/Database/Policies/PolicyEditorPanel/index.tsx`:
- Around line 237-244: The issue is that when updating a policy with an absent
clause (null definition or check), the undefined values are included in the
payload calculation, preventing the no-op branch from triggering for valid
updates like name or role changes. Move the validation for attempted clause
removal to occur before calling generateUpdatePolicyPayload, so it rejects only
when an existing clause is being explicitly removed. This allows name-only and
role-only updates to reach the no-op branch correctly when no payload changes
exist, while preserving the required-expression validation for the create path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98fe4103-b5c4-493b-82af-fc8241c2fc2c
📒 Files selected for processing (3)
apps/studio/components/interfaces/Database/Policies/PolicyEditorPanel/PolicyEditorPanel.utils.test.tsapps/studio/components/interfaces/Database/Policies/PolicyEditorPanel/PolicyEditorPanel.utils.tsapps/studio/components/interfaces/Database/Policies/PolicyEditorPanel/index.tsx
🎭 Playwright Test Results (tanstack)
Details
Flaky testsFeatures › queue-table-operations.spec.ts › Queue Table Operations - queue identity fixes › deleting a newly inserted row removes it from pending changes Skipped testsFeatures › auth-users.spec.ts › should show web3 users as enabled when the matching web3 provider is enabled |
🎭 Playwright Test Results (next)
Details
Flaky testsFeatures › realtime-inspector.spec.ts › Realtime Inspector › Basic Inspector UI › channel selection popover opens and works Skipped testsFeatures › auth-users.spec.ts › should show web3 users as enabled when the matching web3 provider is enabled |
Updates to a policy whose definition/check is null (created without that clause) were blocked by the create-path required-expression validation, so rename-only and role-only saves errored. The update path now only rejects removing an existing clause, since ALTER POLICY can replace an expression but not remove it.
The unsaved-changes check compared the form's lowercase command against 'INSERT', which never matched, so it diffed the hidden second editor as the check expression for INSERT policies and closing an untouched INSERT policy editor prompted about unsaved changes.
| definition: editorOneFormattedValue, | ||
| check: command === 'INSERT' ? editorOneFormattedValue : editorTwoFormattedValue, | ||
| check: | ||
| selectedPolicy.command === 'INSERT' |
There was a problem hiding this comment.
Review context: pre-existing bug spotted while auditing this PR for regressions (predates the branch — traces back to #47433). The form command from useWatch is always lowercase ('insert'), so the old command === 'INSERT' comparison never matched. For INSERT policies the dirty check therefore diffed the hidden second editor (always empty) against the stored check expression, so closing an untouched INSERT policy editor showed a spurious "Unsaved changes" prompt. Comparing against selectedPolicy.command (uppercase, and the actual source of truth for an existing policy) fixes it; command also drops out of the useCallback deps since it is no longer read. Verified in the browser: an untouched INSERT policy editor now closes without the prompt.
Braintrust eval report
|
2 participants
A table RLS policy created via SQL without a
USING/WITH CHECKclause storesnullfor that field, and the policy editor's payload diff skippednullfields entirely — so adding an expression later through the dashboard closed the panel as if saved but persisted nothing. This fixes the diff so those policies are editable, and cleans up adjacent issues in the same code path.Changed:
PolicyEditorPanel's submit handler into a puregenerateUpdatePolicyPayload()inPolicyEditorPanel.utils.ts. A storednulldefinition/check now counts as empty, so typing an expression into a previously empty editor produces a payload field. The diff is branched by command so INSERT policies only ever emitWITH CHECK, never an invalidUSINGclause.nullclause is valid, so rename-only and role-only saves on such policies work; the update path instead rejects attempts to clear an existingUSING/WITH CHECKexpression with an inline error (ALTER POLICYcan only replace an expression, not remove it).undefinedpayload key, which sent a literalBEGIN; COMMIT;to the user's database.'INSERT'(never matched), which made closing an untouched INSERT policy editor prompt about unsaved changes. It now comparesselectedPolicy.command.Added:
PolicyEditorPanel.utils.test.ts— 11 unit tests covering null→value transitions for definition and check, INSERT command mapping, value→value updates, no-op saves, and empty-value handling.To test
create policy "p1" on <table> for delete to authenticated;(noUSINGclause), then editp1in Database → Policies, add aUSINGexpression, and save. Confirm viaselect pg_get_expr(polqual, polrelid) from pg_policy where polname = 'p1'that the expression persisted.create policy "p2" on <table> for insert to authenticated;, then add aWITH CHECKexpression via the editor and confirmpolwithcheckis set (andpolqualstays null).p1(still without aUSINGexpression? recreate it if you added one), rename the policy without touching the expression editors — the rename should save successfully.USINGexpression, change it, and confirm the new expression persists (regression).policy-updatenetwork request.USING(orWITH CHECK) expression, clear that editor and save — an inline error should appear and no request should fire.WITH CHECKexpression, change nothing, and close the panel — it should close without an "Unsaved changes" prompt.Summary by CodeRabbit
Bug Fixes
USINGorWITH CHECKexpressions where unsupported.Tests