fix(adapter-pg): preserve error.constraint on 23505 unique violations#29634
fix(adapter-pg): preserve error.constraint on 23505 unique violations#29634tsushanth wants to merge 2 commits into
Conversation
Closes prisma#29495. `mapDriverError` treated `error.constraint` asymmetrically: the 23503 (foreign-key) branch falls back to `{ index: error.constraint }` when `error.column` isn't populated, but the 23505 (unique) branch ignored `error.constraint` entirely and only emitted `{ fields }` parsed out of `error.detail`. `@prisma/adapter-mysql` already preserves the constraint name as `constraint.index` on 1062, so Postgres was the outlier. For partial unique indexes the parsed column list is ambiguous — two unique constraints on the same table can share key columns (e.g. a full unique on `(user_id, account_id)` and a partial unique on `(account_id) WHERE role = 'owner'`). Both surface as 23505 with the same parsed `fields`, so callers couldn't tell which constraint fired without regex-matching `originalMessage` (not public API per prisma#28281). Prefer `error.constraint` (mapped to `{ index }`, matching the 23503 shape) when it's set, falling back to the existing `{ fields }` parse when it isn't. `MappedError.UniqueConstraintViolation` already accepts `{ index: string }` in `@prisma/driver-adapter-utils`, so this is a non-breaking widening of what callers may see. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis PR updates 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/adapter-pg/src/__tests__/errors.test.ts (1)
36-74: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding edge case tests.
The current test coverage is solid for the main scenarios. Consider adding tests for edge cases to improve robustness:
- Empty constraint string:
constraint: ''(currently would return{ index: '' })- Malformed detail:
detail: 'Invalid format'(regex won't match, constraint would beundefined)- Neither constraint nor detail present: Both absent (constraint would be
undefined)These are unlikely scenarios with real PostgreSQL errors, but testing them would verify the implementation handles unexpected input gracefully.
🧪 Example edge case tests
it('should handle UniqueConstraintViolation (23505) with neither constraint nor detail', () => { const error = { code: '23505', message: 'msg', severity: 'ERROR' } expect(convertDriverError(error)).toEqual({ kind: 'UniqueConstraintViolation', constraint: undefined, originalCode: error.code, originalMessage: error.message, }) }) it('should handle UniqueConstraintViolation (23505) with malformed detail', () => { const error = { code: '23505', message: 'msg', severity: 'ERROR', detail: 'Invalid format' } expect(convertDriverError(error)).toEqual({ kind: 'UniqueConstraintViolation', constraint: undefined, originalCode: error.code, originalMessage: error.message, }) })🤖 Prompt for 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. In `@packages/adapter-pg/src/__tests__/errors.test.ts` around lines 36 - 74, Add edge-case tests for convertDriverError handling of 23505 unique-constraint errors: add cases where constraint is an empty string (constraint: ''), where detail is malformed/unparseable (detail: 'Invalid format'), and where neither constraint nor detail is present (no constraint/detail properties); each test should assert the returned object has kind 'UniqueConstraintViolation', constraint is undefined (or absent) for the malformed/none cases and index '' for the empty-string constraint case, and originalCode/originalMessage mirror the input error.
🤖 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 `@packages/adapter-pg/src/errors.ts`:
- Around line 83-86: Extract the duplicated regex/detail parsing into a small
helper (e.g., parseErrorDetailFields or extractKeyFields) that accepts
error.detail and returns the array of field names or undefined, then replace the
inline logic in both the 23505 handling (currently using /Key \(([^)]+)\)/ and
.split(', ')) and the 23502 handling to call that helper; ensure the helper
handles undefined detail and trims field names before returning.
---
Outside diff comments:
In `@packages/adapter-pg/src/__tests__/errors.test.ts`:
- Around line 36-74: Add edge-case tests for convertDriverError handling of
23505 unique-constraint errors: add cases where constraint is an empty string
(constraint: ''), where detail is malformed/unparseable (detail: 'Invalid
format'), and where neither constraint nor detail is present (no
constraint/detail properties); each test should assert the returned object has
kind 'UniqueConstraintViolation', constraint is undefined (or absent) for the
malformed/none cases and index '' for the empty-string constraint case, and
originalCode/originalMessage mirror the input error.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b93bcbea-2ded-4e4a-abb2-e1b6fc19301c
📒 Files selected for processing (2)
packages/adapter-pg/src/__tests__/errors.test.tspackages/adapter-pg/src/errors.ts
…arsing CodeRabbit pointed out (prisma#29634 review) that the same optional-chain regex to pull field names out of error.detail was inline at both the 23505 and 23502 sites. Hoist it into a small helper so the regex lives in one place and both call sites read at the intent layer.
|
Thanks @coderabbitai, good DRY catch. Pushed e201c7b extracting On the outside-diff edge-case suggestion (empty constraint string, malformed detail, neither present): I'd rather not add those. The constraint-empty-string case is unreachable through node-pg (an empty |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
daltino
left a comment
There was a problem hiding this comment.
Great fix — the asymmetry between 23503 and 23505 was a real bug and the PR addresses it cleanly. The extracted parseKeyFields helper is a nice bonus since it eliminates duplicated regex logic. A few things worth discussing before merging:
Review
Correctness
The core logic looks right. Falling back to { index: error.constraint } when detail is absent (or the regex doesn't match) mirrors the existing 23503 branch and is the correct behaviour — error.constraint is always populated by libpq when the error comes from a constraint violation.
One subtle concern worth double-checking: what happens when both detail and constraint are present? The current precedence (detail → constraint) seems intentional, since detail gives us the actual column names rather than just the constraint name. Make sure this is documented in a comment so the next reader doesn't wonder why we're ignoring constraint in that branch.
case '23505': {
// Prefer detail (gives column names); fall back to constraint name.
const fields = parseKeyFields(error.detail) ?? (error.constraint ? [error.constraint] : undefined)
...
}Edge Cases
1. Composite key with spaces in column names
parseKeyFields splits on ', '. A column name containing a comma (unusual but technically valid in quoted identifiers) would silently produce wrong field names. This is an edge case Postgres itself handles weirdly, so it may not be worth solving now, but a comment acknowledging the limitation would be helpful.
2. detail present but regex doesn't match
If detail exists but has an unexpected format (e.g., a translated locale message or a future Postgres change to the format), parseKeyFields returns undefined and the code falls back to constraint. That's good defensive behaviour — just confirm the fallback path is exercised by a test.
3. Both detail and constraint absent
What does the returned object look like when both are undefined? If fields ends up undefined, is { fields: undefined } or {} returned? Check that downstream consumers handle the missing fields key gracefully.
Test Coverage
The new test for constraint-only is great. A couple of gaps:
- No test for the "both
detailandconstraintpresent" case — verify thatdetailtakes precedence andconstraintis ignored. - No test for the "neither
detailnorconstraintpresent" case — what does the function return? This is currently untested for23505, whereas23503presumably has similar gaps. Consider adding it. - The renamed existing test (
with detail only) is a nice clarification — good call.
it('should handle UniqueConstraintViolation (23505) with both detail and constraint', () => {
const error = {
code: '23505',
message: 'msg',
severity: 'ERROR',
detail: 'Key (email)=(foo@bar.com) already exists.',
constraint: 'users_email_key',
}
expect(convertDriverError(error)).toEqual({
kind: 'UniqueConstraintViolation',
fields: ['email'], // detail wins
})
})Code Style
parseKeyFields is a good extraction. One small nit: the name is slightly generic — parseDetailFields or parseUniqueViolationFields would tie it more clearly to its purpose. Not a blocker at all.
The optional-chaining chain:
detail?.match(...)?.at(1)?.split(', ')is idiomatic modern TypeScript and readable. ✅
Performance
Not a concern here — this is only called on error paths.
Summary
| Area | Status |
|---|---|
| Core bug fix | ✅ Correct |
parseKeyFields extraction |
✅ Nice cleanup |
| Fallback precedence (detail → constraint) | ✅ Mirrors 23503 |
| Test: constraint-only path | ✅ Added |
| Test: both present (detail wins) | |
| Test: neither present | |
| Composite key with spaces comment | 💬 Nice-to-have |
Overall this is a solid, minimal fix. Add the two missing test cases and optionally a short comment about precedence, and it's ready to merge. 🎉
nahrinoda
left a comment
There was a problem hiding this comment.
Small correction on @daltino's review — the precedence is constraint → detail, not detail → constraint. The code checks error.constraint first:
if (error.constraint) {
constraint = { index: error.constraint }
} else {
const fields = parseKeyFields(error.detail)
...
}This is the right call for the partial unique index case the PR describes — two constraints sharing the same key columns can only be disambiguated by name, so the constraint name should take precedence over parsed column names.
The "both present" test is also already included ('with both constraint and detail (constraint wins)') and correctly pins this precedence. So daltino's suggested test would actually fail against the current code since it asserts fields: ['email'] (detail wins).
The "neither present" case (no constraint, no detail) is still untested — constraint would be undefined, giving { kind: 'UniqueConstraintViolation', constraint: undefined }. Might be worth a one-liner test if the author wants completeness, but not a blocker.
Clean fix overall. The parseKeyFields extraction and the 23503/23505 symmetry are solid.
Closes #29495.
Summary
@prisma/adapter-pg'smapDriverErrortreatederror.constraint(populated by node-postgres fromPG_DIAG_CONSTRAINT_NAME) asymmetrically between unique-violation (23505) and foreign-key-violation (23503) branches:case '23503'already falls back to{ index: error.constraint }whenerror.columnisn't populated.case '23505'ignorederror.constraintentirely and only emitted{ fields }parsed out oferror.detail.@prisma/adapter-mysqlalready preserves the constraint name asconstraint.indexon 1062 (see #29344), so Postgres was the outlier here.Why it matters
For partial unique indexes, the parsed column list is ambiguous — two unique constraints on the same table can share key columns. For example, a full unique on
(user_id, account_id)and a partial unique on(account_id) WHERE role = 'owner'both surface as 23505 with the same parsedfields. The constraint name is the only signal that disambiguates which one fired, and callers shouldn't have to regex-matchoriginalMessage(not public API per #28281) to know.Fix
Prefer
error.constraint(mapped to{ index }, matching the 23503 shape) when it's set, falling back to the existing{ fields }parse when it isn't:MappedError.UniqueConstraintViolationalready accepts{ index: string }in@prisma/driver-adapter-utils, so this is a non-breaking widening of what callers may already see from the MySQL adapter.Tests
Three tests under
src/__tests__/errors.test.ts:should handle UniqueConstraintViolation (23505) with detail only— existing behaviour, unchanged.should handle UniqueConstraintViolation (23505) with constraint— new branch.should handle UniqueConstraintViolation (23505) with both constraint and detail (constraint wins)— pins the precedence rule.All 35 tests in
errors.test.tspass.