Make policy names globally unique #4969
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
- Add migration to change unique constraint from (user_id, name) to (name) - Update upsert_policy to reject duplicate names with 409 Conflict - Deduping of existing duplicates was already run in prod Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
nishu-builder
force-pushed
the
nishad/unique-policy-names
branch
from
a3ef9bb to
8cfe0d6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cfe0d607d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| INSERT INTO policies (name, user_id, attributes) | ||
| VALUES (%s, %s, %s) | ||
| ON CONFLICT (user_id, name) DO NOTHING | ||
| RETURNING id | ||
| """, |
There was a problem hiding this comment.
Because the pre-check is separate from the INSERT, two concurrent requests can both pass the SELECT and then one will hit the new unique constraint on name. That raises a database unique-violation exception (not a ValueError), which bubbles up as a 500 instead of the intended 409, especially in stats_routes where only ValueError is caught. This regression is new because the previous ON CONFLICT DO NOTHING path avoided exceptions; you may need to catch the database exception here or restore ON CONFLICT to keep the API returning 409 on races.
Useful? React with 👍 / 👎.
| else: | ||
| raise ValueError(f"Policy {name} not found") | ||
| if row is None: | ||
| raise ValueError(f"Failed to create policy {name}") |
There was a problem hiding this comment.
I agree with codex that races are a (admittedly very minor) concern; and I'll also add that insert -> select on conflict is more performant in the "happy path", so generally it's better to try and fallback for better error reporting.
I don't think either of these concerns matter in this case, we don't have that many policies to optimize this. But I do think that the try/fallback strategy similar to the previous version is better in general.
Merge activity
|
This is so that metta://policy/xxx:vZ is unique ## Summary - Add migration to change unique constraint from (user_id, name) to (name) - Update upsert_policy to reject duplicate names with 409 Conflict - Deduping of existing duplicates was already run in prod ## Test plan - [x] Deduped existing duplicates in prod manually - [ ] Deploy and verify migration runs
This is so that metta://policy/xxx:vZ is unique
Summary
Test plan