Skip to content

fix(adapter-pg): preserve error.constraint on 23505 unique violations#29634

Open
tsushanth wants to merge 2 commits into
prisma:mainfrom
tsushanth:adapter-pg-23505-constraint
Open

fix(adapter-pg): preserve error.constraint on 23505 unique violations#29634
tsushanth wants to merge 2 commits into
prisma:mainfrom
tsushanth:adapter-pg-23505-constraint

Conversation

@tsushanth

Copy link
Copy Markdown

Closes #29495.

Summary

@prisma/adapter-pg's mapDriverError treated error.constraint (populated by node-postgres from PG_DIAG_CONSTRAINT_NAME) asymmetrically between unique-violation (23505) and foreign-key-violation (23503) branches:

  • case '23503' already falls back to { index: error.constraint } when error.column isn't populated.
  • case '23505' 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 (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 parsed fields. The constraint name is the only signal that disambiguates which one fired, and callers shouldn't have to regex-match originalMessage (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:

case '23505': {
  let constraint: { fields: string[] } | { index: string } | undefined
  if (error.constraint) {
    constraint = { index: error.constraint }
  } else {
    const fields = error.detail?.match(/Key \(([^)]+)\)/)?.at(1)?.split(', ')
    if (fields !== undefined) {
      constraint = { fields }
    }
  }
  return { kind: 'UniqueConstraintViolation', constraint }
}

MappedError.UniqueConstraintViolation already 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.ts pass.

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>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d0388b78-edfd-4e2e-9526-309e820f83f7

📥 Commits

Reviewing files that changed from the base of the PR and between 11de871 and e201c7b.

📒 Files selected for processing (1)
  • packages/adapter-pg/src/errors.ts

Summary by CodeRabbit

  • Bug Fixes

    • More accurate handling of PostgreSQL unique constraint violations: when a constraint name is present it is used, otherwise referenced columns are derived; original error details are preserved for clearer messages.
  • Tests

    • Expanded tests to cover different unique-constraint error formats and ensure consistent conversion behavior.

Walkthrough

This PR updates mapDriverError in the PostgreSQL adapter to use error.constraint as a constraint index identifier when present, falling back to column-name parsing from error.detail for 23505 (unique constraint violations). This matches the existing behavior for 23503 (foreign key violations) and allows callers to distinguish partial unique indexes with overlapping columns. Test coverage expands to verify three scenarios: detail-only parsing, constraint-only index usage, and constraint precedence when both are present.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main change: preserving error.constraint on 23505 unique violations in the PostgreSQL adapter.
Description check ✅ Passed Description is well-related to the changeset, explaining the asymmetry issue, why it matters, the fix, and test coverage.
Linked Issues check ✅ Passed Code changes fully satisfy #29495 requirements: error.constraint is preserved as {index} when present, falls back to {fields} from detail parsing, matching 23503 behavior and aligning with adapter-mysql [#29495].
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the 23505 unique constraint violation handling in adapter-pg per #29495; no unrelated changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 value

Consider adding edge case tests.

The current test coverage is solid for the main scenarios. Consider adding tests for edge cases to improve robustness:

  1. Empty constraint string: constraint: '' (currently would return { index: '' })
  2. Malformed detail: detail: 'Invalid format' (regex won't match, constraint would be undefined)
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 918cf5d and 11de871.

📒 Files selected for processing (2)
  • packages/adapter-pg/src/__tests__/errors.test.ts
  • packages/adapter-pg/src/errors.ts
Comment thread packages/adapter-pg/src/errors.ts Outdated
…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.
@tsushanth

Copy link
Copy Markdown
Author

Thanks @coderabbitai, good DRY catch. Pushed e201c7b extracting parseKeyFields so the regex lives in one place and both the 23505 detail-only branch and the 23502 case read at the intent layer. Existing tests at errors.test.ts:36, 47, 62, 77 already exercise both paths, so behaviour is unchanged.

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 constraint is parsed as undefined upstream), the malformed-detail and both-absent cases collapse into the same constraint: undefined branch already covered by the existing tests, and fields !== undefined already guards the consumers. Adding them would test the regex/optional-chain language semantics rather than the adapter contract.

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set chat.allow_non_org_members: true in your configuration.

@daltino daltino left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 detail and constraint present" case — verify that detail takes precedence and constraint is ignored.
  • No test for the "neither detail nor constraint present" case — what does the function return? This is currently untested for 23505, whereas 23503 presumably 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) ⚠️ Missing
Test: neither present ⚠️ Missing
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 nahrinoda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants