fix: return 409 on unique constraint violations for virtual keys, teams, customers, and MCP clients - #4539
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 13 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughHTTP error handling in governance, MCP, and provider key code is updated so that database unique-constraint violations now return HTTP 409 Conflict with specific, domain-appropriate messages. Config store library code maps ChangesUnique Constraint → HTTP 409 Conflict
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5Safe to merge. The change is a targeted error-handling fix with no new code paths, no schema changes, and no mutations to core business logic. All affected store methods use parseGormError with %w, so errors.Is correctly traverses the error chain. The CreateMCPClientConfig change closes the one gap where the application-level duplicate check was not wrapped. Both the pre-check path and the concurrent-insert (TOCTOU) path are now correctly mapped to ErrAlreadyExists before reaching the handlers. Previous issues raised in review threads — raw error exposure in 409 messages, inconsistent check ordering, and the IsUniqueConstraintError/parseGormError mismatch — are all resolved. No regressions were identified across the five changed files. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[HTTP Handler\ncreate/update request] --> B{badRequestError?}
B -- yes --> C[400 Bad Request]
B -- no --> D{errors.Is ErrAlreadyExists?}
D -- yes --> E[409 Conflict\nsanitized message]
D -- no --> F[500 Internal Server Error]
subgraph Store Layer
G[CreateVirtualKey / CreateTeam\nCreateCustomer / CreateProviderKey] --> H[parseGormError]
H -- unique constraint --> I[fmt.Errorf ... %w ErrAlreadyExists]
J[CreateMCPClientConfig] --> K{name exists pre-check}
K -- found --> L[fmt.Errorf ... %w ErrAlreadyExists]
K -- not found --> M[tx.Create]
M -- constraint error --> H
end
I --> D
L --> D
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[HTTP Handler\ncreate/update request] --> B{badRequestError?}
B -- yes --> C[400 Bad Request]
B -- no --> D{errors.Is ErrAlreadyExists?}
D -- yes --> E[409 Conflict\nsanitized message]
D -- no --> F[500 Internal Server Error]
subgraph Store Layer
G[CreateVirtualKey / CreateTeam\nCreateCustomer / CreateProviderKey] --> H[parseGormError]
H -- unique constraint --> I[fmt.Errorf ... %w ErrAlreadyExists]
J[CreateMCPClientConfig] --> K{name exists pre-check}
K -- found --> L[fmt.Errorf ... %w ErrAlreadyExists]
K -- not found --> M[tx.Create]
M -- constraint error --> H
end
I --> D
L --> D
Reviews (9): Last reviewed commit: "fix: return 409 for Conflict errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@transports/bifrost-http/handlers/governance.go`:
- Around line 1862-1868: The error checking order in this handler is incorrect
and causes substring-based misclassification. Move the `badRequestError` type
check (using errors.As) before the `IsUniqueConstraintError` check, since
IsUniqueConstraintError uses substring matching and can incorrectly classify
validation errors containing "duplicate entry" text as HTTP 409 instead of the
correct HTTP 400. Reorder the conditions so that badRequestError is unwrapped
and handled first with its SendError call returning HTTP 400, followed by the
IsUniqueConstraintError check for HTTP 409.
🪄 Autofix (Beta)
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: 4d7cda9b-929d-45eb-a92d-864cfa926a8b
📒 Files selected for processing (2)
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.go
2d30ab1 to
f5459d0
Compare
a0fbaa9 to
1d7940b
Compare
f5459d0 to
9737aa4
Compare
1d7940b to
2a6e01a
Compare
2a6e01a to
15aea50
Compare
9737aa4 to
a8c2da7
Compare
15aea50 to
a2cebf4
Compare
fe22630 to
9612bce
Compare
a2cebf4 to
7fd0013
Compare
9612bce to
5812813
Compare
5812813 to
4f10557
Compare
4f10557 to
4dd6889
Compare
00e721d to
a8114ec
Compare
4dd6889 to
62b6764
Compare
62b6764 to
e462f62
Compare
a8114ec to
1aef72e
Compare
Merge activity
|
The base branch was changed.
2 participants
Summary
Duplicate resource creation (virtual keys, teams, customers, MCP clients) was returning a generic 500 Internal Server Error instead of a meaningful conflict response. This PR ensures that unique constraint violations from the database are surfaced to callers as HTTP 409 Conflict with descriptive error messages.
Changes
IsUniqueConstraintErrorhelper is now consistently used across all these handlers rather than ad-hoc string matching.Type of change
Affected areas
How to test
Attempt to create a virtual key, team, customer, or MCP client with a name that already exists in the system. The API should return a 409 Conflict response with a descriptive error message instead of a 500 Internal Server Error.
go test ./...Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines