diff --git a/.changeset/unique-violation-409.md b/.changeset/unique-violation-409.md new file mode 100644 index 000000000..710b08429 --- /dev/null +++ b/.changeset/unique-violation-409.md @@ -0,0 +1,50 @@ +--- +"@aws-blocks/bb-distributed-data": minor +"@aws-blocks/bb-data": minor +"@aws-blocks/blocks": patch +--- + +fix(data): map duplicate-key unique-constraint violations to JSON-RPC 409 (Conflict) instead of 500 + +A duplicate-key / unique-constraint violation (SQLSTATE `23505`, +`UniqueConstraintViolation`) now surfaces to clients as JSON-RPC error +**code 409 (Conflict)** instead of a generic **500**. Previously the engine +translators set `error.name` on the raw driver error and re-threw a plain named +`Error`; the JSON-RPC serializer maps any non-`ApiError` to 500, so a routine, +expected duplicate-key conflict was indistinguishable from an internal server +error. This mirrors the `40001`/OCC → 409 mapping already established for these +Blocks. + +Each affected conflict is now an `ApiError` with `status: 409`, so on the client +`error.status === 409`. The structured `error.name` is preserved end-to-end, so +`isBlocksError(e, DatabaseErrors.UniqueConstraintViolation)` (and the +`DistributedDatabaseErrors` equivalent) keeps matching by name on both server and +client; the typed error constants are unchanged. + +- `@aws-blocks/bb-data` — a duplicate-key violation (SQLSTATE `23505`, + `DatabaseErrors.UniqueConstraintViolation`) across the PGlite, pg-client, and + Data API engines (both the SQLState-parsed and message-matched Data API paths), + routed through a shared `uniqueConstraintConflict()` helper. +- `@aws-blocks/bb-distributed-data` — a DSQL duplicate-key violation (SQLSTATE + `23505`, `DistributedDatabaseErrors.UniqueConstraintViolation`) in + `translateDsqlError`, in both the mock and real engines. + +The conflict is **not** flagged `retriable`: a duplicate key is deterministic, so +a blind retry of the same insert fails identically (unlike the `40001` +serialization failures, which stay retriable). The client-visible message is a +fixed, stable string; the raw driver text (which can name columns / constraints) +is retained only as `cause` for server-side diagnostics. Genuine infrastructure +errors (`ConnectionFailed`, `QueryFailed`) are unchanged and correctly stay 500, +and the `SerializationFailure`/OCC paths are untouched. + +This is a `minor` bump. Both data packages are pre-1.0, where `minor` is this +repo's signal for a change that can alter existing behavior: callers that +branched on `error.status === 500` for these conflicts (or on the JSON-RPC error +code) will now see `409`. Code that matches conflicts by name via +`isBlocksError` — the documented pattern — is unaffected. + +`@aws-blocks/blocks` gets a `patch` bump because it re-exports `bb-data` and +`bb-distributed-data` (satisfies the umbrella publish guard); no umbrella source +changed. + +Fixes #508. diff --git a/packages/bb-data/DESIGN.md b/packages/bb-data/DESIGN.md index f9e6aa488..6728456dd 100644 --- a/packages/bb-data/DESIGN.md +++ b/packages/bb-data/DESIGN.md @@ -54,7 +54,7 @@ Error translation happens at the engine layer, not the database layer. Each engi | pg error code | DatabaseErrors name | |---------------|-------------------| | `40001` | `SerializationFailure` (→ `ApiError` 409, retriable) | -| `23505` | `UniqueConstraintViolation` | +| `23505` | `UniqueConstraintViolation` (→ `ApiError` 409, not retriable) | | `08xxx` | `ConnectionFailed` | | (other) | `QueryFailed` | @@ -64,6 +64,8 @@ The `DatabaseBase` subclass only adds `TransactionFailed` naming for errors that An OCC serialization failure (SQLSTATE `40001`) is translated to an `ApiError` with status **409 (Conflict)**, retriable, preserving the `SerializationFailure` name so the JSON-RPC serializer emits 409 instead of a generic 500. This mapping is verified by translator/engine **unit tests** (`pg-error-translator.test.ts`, `data-api-engine.test.ts`), not an over-the-wire e2e test: the single-connection PGlite mock has no conflict-injection hook and cannot deterministically produce a `40001` serialization conflict over the wire, so unit tests are the verification ceiling for this Block. (The same holds for DistributedDatabase: its `40001` → 409 mapping is likewise covered by translator/engine unit tests only, not an over-the-wire e2e test — consistent with `bb-distributed-data/DESIGN.md`.) +A duplicate-key unique-constraint violation (SQLSTATE `23505`) is likewise translated to an `ApiError` with status **409 (Conflict)** — via the shared `uniqueConstraintConflict()` helper across the PGlite, pg-client, and Data API engines (both the SQLState-parsed and message-matched Data API paths) — preserving the `UniqueConstraintViolation` name. It is **not** retriable: a duplicate key is deterministic, so a blind retry of the same insert fails identically. Unlike `40001`, a `23505` conflict **is** deterministically inducible in the PGlite mock (which enforces the PK constraint), so this mapping is additionally verified by an **over-the-wire e2e test** (`test-apps/comprehensive/test/database.test.ts`) asserting `error.status === 409` on the client — per AGENTS.md §11 for a serialization/behavior-affecting change. The raw driver text is retained only as `cause` (server-side); the client message is a fixed string. + ## RLS Implementation `withRLS(context)` returns an `RLSScopedDatabase` that wraps every operation in a transaction with PostgreSQL session variables (using the standard `request.jwt.claims` / role convention): diff --git a/packages/bb-data/README.md b/packages/bb-data/README.md index 5f26d04b9..e8e170acb 100644 --- a/packages/bb-data/README.md +++ b/packages/bb-data/README.md @@ -214,7 +214,8 @@ try { await db.execute(sql`INSERT INTO users (id, email) VALUES (${id}, ${email})`); } catch (e: unknown) { if (isBlocksError(e, DatabaseErrors.UniqueConstraintViolation)) { - // Duplicate key — email already exists + // Duplicate key — email already exists. Serialized as HTTP 409 (Conflict), + // not retriable (a blind retry of the same insert fails identically). } if (isBlocksError(e, DatabaseErrors.QueryFailed)) { // General query failure (syntax error, missing table, etc.) diff --git a/packages/bb-data/src/engines/data-api-engine.test.ts b/packages/bb-data/src/engines/data-api-engine.test.ts index a7914a360..0c9f44305 100644 --- a/packages/bb-data/src/engines/data-api-engine.test.ts +++ b/packages/bb-data/src/engines/data-api-engine.test.ts @@ -3,6 +3,7 @@ import { test } from 'node:test'; import assert from 'node:assert'; +import { ApiError } from '@aws-blocks/core'; import { DataApiEngine, toField, fromField } from './data-api-engine.js'; import { DatabaseErrors } from '../errors.js'; @@ -160,7 +161,7 @@ test('execute returns rowCount', async () => { // --- error translation --- -test('BadRequestException with unique constraint maps to UniqueConstraintViolation', async () => { +test('BadRequestException with unique constraint maps to UniqueConstraintViolation (ApiError 409)', async () => { const engine = createEngine({ ExecuteStatementCommand: () => { const err = new Error('duplicate key value violates unique constraint'); @@ -170,14 +171,17 @@ test('BadRequestException with unique constraint maps to UniqueConstraintViolati }); await assert.rejects( () => engine.execute('INSERT INTO t VALUES (1)'), - (err: Error) => { + (err: unknown) => { + assert.ok(err instanceof ApiError, 'expected an ApiError'); + assert.strictEqual(err.status, 409, 'duplicate key must be 409, not 500'); assert.strictEqual(err.name, DatabaseErrors.UniqueConstraintViolation); + assert.notStrictEqual(err.retriable, true); return true; } ); }); -test('non-BadRequestException with unique constraint message maps to UniqueConstraintViolation', async () => { +test('non-BadRequestException with unique constraint message maps to UniqueConstraintViolation (ApiError 409)', async () => { const engine = createEngine({ ExecuteStatementCommand: () => { const err = new Error('ERROR: duplicate key value violates unique constraint "t_pkey"; SQLState: 23505'); @@ -187,7 +191,9 @@ test('non-BadRequestException with unique constraint message maps to UniqueConst }); await assert.rejects( () => engine.execute('INSERT INTO t VALUES (1)'), - (err: Error) => { + (err: unknown) => { + assert.ok(err instanceof ApiError, 'expected an ApiError'); + assert.strictEqual(err.status, 409); assert.strictEqual(err.name, DatabaseErrors.UniqueConstraintViolation); return true; } @@ -330,9 +336,9 @@ test('serialization failure surfaced on CommitTransaction (SQLState 40001) is cl ); }); -test('Data API unique violation (DatabaseErrorException, SQLState 23505) maps to UniqueConstraintViolation', async () => { +test('Data API unique violation (DatabaseErrorException, SQLState 23505) maps to UniqueConstraintViolation (ApiError 409)', async () => { // Confirms the real exception name is DatabaseErrorException (not BadRequestException), - // and code-based classification keeps unique-violation mapping working. + // and code-based classification maps unique-violation to a 409 ApiError. const engine = createEngine({ ExecuteStatementCommand: () => { const err = new Error( @@ -344,8 +350,11 @@ test('Data API unique violation (DatabaseErrorException, SQLState 23505) maps to }); await assert.rejects( () => engine.execute('INSERT INTO t VALUES ($1)', ['dup']), - (err: Error) => { + (err: unknown) => { + assert.ok(err instanceof ApiError, 'expected an ApiError'); + assert.strictEqual(err.status, 409, 'duplicate key must be 409, not 500'); assert.strictEqual(err.name, DatabaseErrors.UniqueConstraintViolation); + assert.notStrictEqual(err.retriable, true); return true; }, ); diff --git a/packages/bb-data/src/engines/data-api-engine.ts b/packages/bb-data/src/engines/data-api-engine.ts index 79e7d22af..81ebf8d12 100644 --- a/packages/bb-data/src/engines/data-api-engine.ts +++ b/packages/bb-data/src/engines/data-api-engine.ts @@ -10,7 +10,7 @@ import { type Field, } from '@aws-sdk/client-rds-data'; import type { DatabaseEngine, TransactionHandle } from '@aws-blocks/data-common'; -import { DatabaseErrors, TRANSIENT_DATA_API_ERROR_NAMES, wrapError, serializationConflict } from '../errors.js'; +import { DatabaseErrors, TRANSIENT_DATA_API_ERROR_NAMES, wrapError, serializationConflict, uniqueConstraintConflict } from '../errors.js'; /** * Translate `$1`, `$2`, ... placeholders to `:p1`, `:p2`, ... for Data API. @@ -106,14 +106,19 @@ function translateError(e: unknown): never { // error as cause. Matches the PGlite / pg-client engine paths. throw serializationConflict(e); } else if (code === '23505') { - e.name = DatabaseErrors.UniqueConstraintViolation; + // Duplicate key: surface as a 409 (Conflict), not a generic 500. Not + // retriable. Matches the PGlite / pg-client engine paths. + throw uniqueConstraintConflict(e); } else if (code.startsWith('08')) { e.name = DatabaseErrors.ConnectionFailed; } else { e.name = DatabaseErrors.QueryFailed; } } else if (/unique constraint|duplicate key/i.test(msg)) { - e.name = DatabaseErrors.UniqueConstraintViolation; + // Data API errors without a parseable SQLState still carry the driver's + // unique-violation text — map to the same 409 (Conflict) as the + // SQLState-parsed path above. + throw uniqueConstraintConflict(e); } else if (TRANSIENT_DATA_API_ERROR_NAMES.has(e.name)) { e.name = DatabaseErrors.ConnectionFailed; } else { diff --git a/packages/bb-data/src/engines/pg-error-translator.test.ts b/packages/bb-data/src/engines/pg-error-translator.test.ts index 26f59b85e..f61bd0d3e 100644 --- a/packages/bb-data/src/engines/pg-error-translator.test.ts +++ b/packages/bb-data/src/engines/pg-error-translator.test.ts @@ -3,10 +3,11 @@ /** * Unit tests for the shared PostgreSQL error translator (used by PgClientEngine - * and PGliteEngine). Focus: an OCC / serialization-failure conflict (SQLSTATE - * 40001) must surface as an ApiError with HTTP status 409 (Conflict), not a - * generic 500, while preserving the SerializationFailure name and flagging the - * conflict retriable. + * and PGliteEngine). Focus: conflict codes must surface as an ApiError with an + * HTTP status (409 Conflict), not a generic 500 — an OCC / serialization-failure + * conflict (SQLSTATE 40001, retriable) and a duplicate-key / unique-constraint + * violation (SQLSTATE 23505, not retriable) — while preserving the standardized + * error name so `isBlocksError` keeps matching. */ import { test } from 'node:test'; import assert from 'node:assert'; @@ -28,14 +29,18 @@ test('translatePgError: serialization failure (40001) → ApiError status 409, r ); }); -test('translatePgError: unique violation (23505) → UniqueConstraintViolation (unchanged, non-ApiError)', () => { - const err = Object.assign(new Error('duplicate key'), { code: '23505' }); +test('translatePgError: unique violation (23505) → ApiError status 409, name preserved, not retriable', () => { + const err = Object.assign(new Error('duplicate key value violates unique constraint "t_pkey"'), { code: '23505' }); assert.throws( () => translatePgError(err, 'PgClientEngine'), (e: unknown) => { - assert.ok(e instanceof Error); - assert.strictEqual((e as Error).name, DatabaseErrors.UniqueConstraintViolation); - assert.ok(!(e instanceof ApiError), 'unique violation should not be remapped to an ApiError'); + assert.ok(e instanceof ApiError, 'expected an ApiError'); + assert.strictEqual(e.status, 409); + assert.strictEqual(e.name, DatabaseErrors.UniqueConstraintViolation); + assert.notStrictEqual(e.retriable, true, 'a duplicate-key retry fails identically → not retriable'); + // Raw driver error is retained server-side as `cause`, not leaked into the message. + assert.strictEqual(e.cause, err); + assert.notStrictEqual(e.message, err.message); return true; }, ); diff --git a/packages/bb-data/src/engines/pg-error-translator.ts b/packages/bb-data/src/engines/pg-error-translator.ts index 5afab3bf5..8374542c3 100644 --- a/packages/bb-data/src/engines/pg-error-translator.ts +++ b/packages/bb-data/src/engines/pg-error-translator.ts @@ -1,7 +1,7 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import { DatabaseErrors, wrapError, serializationConflict } from '../errors.js'; +import { DatabaseErrors, wrapError, serializationConflict, uniqueConstraintConflict } from '../errors.js'; /** PostgreSQL error code for unique constraint violations. */ const PG_UNIQUE_VIOLATION = '23505'; @@ -30,8 +30,11 @@ export function translatePgError(e: unknown, engineName: string): never { throw serializationConflict(e); } if (code === PG_UNIQUE_VIOLATION) { - e.name = DatabaseErrors.UniqueConstraintViolation; - } else if (code && code.startsWith(PG_CONNECTION_EXCEPTION_CLASS)) { + // Duplicate key: surface as a 409 (Conflict), not a generic 500. Not + // retriable — a blind retry of the same insert fails identically. + throw uniqueConstraintConflict(e); + } + if (code && code.startsWith(PG_CONNECTION_EXCEPTION_CLASS)) { e.name = DatabaseErrors.ConnectionFailed; } else { e.name = DatabaseErrors.QueryFailed; diff --git a/packages/bb-data/src/engines/pglite-engine.test.ts b/packages/bb-data/src/engines/pglite-engine.test.ts index 4a83e80b7..8ee1e9767 100644 --- a/packages/bb-data/src/engines/pglite-engine.test.ts +++ b/packages/bb-data/src/engines/pglite-engine.test.ts @@ -3,6 +3,7 @@ import { test, afterEach } from 'node:test'; import assert from 'node:assert'; +import { ApiError } from '@aws-blocks/core'; import { PGlite } from '@electric-sql/pglite'; import { PGliteEngine } from './pglite-engine.js'; import { DatabaseErrors } from '../errors.js'; @@ -78,13 +79,16 @@ test('execute returns rowCount for DELETE', async () => { // --- Core: error translation --- -test('duplicate key throws UniqueConstraintViolation', async () => { +test('duplicate key throws UniqueConstraintViolation as ApiError status 409', async () => { await setup(); await engine.execute("INSERT INTO t (id, value) VALUES ('a', 'one')"); await assert.rejects( () => engine.execute("INSERT INTO t (id, value) VALUES ('a', 'dupe')"), - (err: Error) => { + (err: unknown) => { + assert.ok(err instanceof ApiError, 'expected an ApiError'); + assert.strictEqual(err.status, 409, 'duplicate key must be 409, not 500'); assert.strictEqual(err.name, DatabaseErrors.UniqueConstraintViolation); + assert.notStrictEqual(err.retriable, true, 'duplicate key is not retriable'); return true; } ); @@ -154,7 +158,9 @@ test('error translation works within transactions', async () => { const handle = await engine.beginTransaction(); await assert.rejects( () => engine.executeInTransaction(handle, "INSERT INTO t (id, value) VALUES ('a', 'dupe')"), - (err: Error) => { + (err: unknown) => { + assert.ok(err instanceof ApiError, 'expected an ApiError'); + assert.strictEqual(err.status, 409); assert.strictEqual(err.name, DatabaseErrors.UniqueConstraintViolation); return true; } diff --git a/packages/bb-data/src/engines/pglite-engine.ts b/packages/bb-data/src/engines/pglite-engine.ts index fbc863678..c17e0e5b6 100644 --- a/packages/bb-data/src/engines/pglite-engine.ts +++ b/packages/bb-data/src/engines/pglite-engine.ts @@ -6,7 +6,7 @@ import { randomUUID } from 'node:crypto'; import { existsSync, mkdirSync, readdirSync, renameSync, unlinkSync } from 'node:fs'; import { join } from 'node:path'; import { initializePgliteWithRetry, type DatabaseEngine, type TransactionHandle } from '@aws-blocks/data-common'; -import { DatabaseErrors, wrapError, serializationConflict } from '../errors.js'; +import { DatabaseErrors, wrapError, serializationConflict, uniqueConstraintConflict } from '../errors.js'; /** PostgreSQL error code for unique constraint violations. */ const PG_UNIQUE_VIOLATION = '23505'; @@ -45,8 +45,11 @@ function translateError(e: unknown): never { throw serializationConflict(e); } if (code === PG_UNIQUE_VIOLATION) { - e.name = DatabaseErrors.UniqueConstraintViolation; - } else if (code && code.startsWith(PG_CONNECTION_EXCEPTION_CLASS)) { + // Duplicate key: surface as a 409 (Conflict), not a generic 500. Not + // retriable — a blind retry of the same insert fails identically. + throw uniqueConstraintConflict(e); + } + if (code && code.startsWith(PG_CONNECTION_EXCEPTION_CLASS)) { e.name = DatabaseErrors.ConnectionFailed; } else { e.name = DatabaseErrors.QueryFailed; diff --git a/packages/bb-data/src/errors.ts b/packages/bb-data/src/errors.ts index dc8458bb0..9f0ba2dff 100644 --- a/packages/bb-data/src/errors.ts +++ b/packages/bb-data/src/errors.ts @@ -39,6 +39,31 @@ export function serializationConflict(cause: Error): ApiError { }); } +/** + * Build the 409 ApiError for a unique-constraint / duplicate-key violation + * (SQLSTATE 23505). Maps to HTTP 409 (Conflict) so the JSON-RPC serializer emits + * code 409 instead of a generic 500, preserves the `UniqueConstraintViolation` + * name so `isBlocksError(e, DatabaseErrors.UniqueConstraintViolation)` keeps + * matching on both server and client, and keeps the original engine error as + * `cause` (server-side). Shared by every engine translator (PGlite, pg-client, + * Data API) so all paths produce an identically shaped 409. + * + * Unlike {@link serializationConflict}, this is NOT flagged retriable: a + * duplicate key is a deterministic constraint failure, so a blind retry of the + * same insert fails identically (ApiError defaults `retriable` to `false`). + * + * The client-visible message is a fixed, stable string; the raw driver text + * (which can name columns / constraint identifiers and varies by engine) is + * retained only as `cause` for server-side diagnostics, never interpolated into + * the message. + */ +export function uniqueConstraintConflict(cause: Error): ApiError { + return new ApiError('The item violates a unique constraint', 409, { + name: DatabaseErrors.UniqueConstraintViolation, + cause, + }); +} + const knownErrors = new Set(Object.values(DatabaseErrors)); /** diff --git a/packages/bb-distributed-data/DESIGN.md b/packages/bb-distributed-data/DESIGN.md index a5fc89c5c..7b94c3be6 100644 --- a/packages/bb-distributed-data/DESIGN.md +++ b/packages/bb-distributed-data/DESIGN.md @@ -47,7 +47,7 @@ bb-distributed-data (this package) - PGlite wrapped with a validation layer - `validateStatement()` rejects unsupported SQL before execution - `TransactionTracker` enforces DDL/DML separation and 3,000-row limit -- `simulateConflict()` test helper for OCC unit testing (mock-only hook, absent from the deployed AWS surface). The `40001`→409 serialization-conflict mapping is covered by translator/engine **unit tests** (mirroring `bb-data`), not an over-the-wire e2e test: `simulateConflict()` is a mock-only trigger with no counterpart on the deployed runtime, and there is no deterministic way to raise a genuine `40001` conflict over the JSON-RPC wire in local e2e. +- `simulateConflict()` test helper for OCC unit testing (mock-only hook, absent from the deployed AWS surface). The `40001`→409 serialization-conflict mapping is covered by translator/engine **unit tests** (mirroring `bb-data`), not an over-the-wire e2e test: `simulateConflict()` is a mock-only trigger with no counterpart on the deployed runtime, and there is no deterministic way to raise a genuine `40001` conflict over the JSON-RPC wire in local e2e. A duplicate-key `23505`→409 `UniqueConstraintViolation` conflict (not retriable) is different: the DSQL mock enforces the primary-key constraint, so it **is** deterministically inducible over the wire and is additionally covered by an over-the-wire e2e test (`test-apps/comprehensive/test/dsql.test.ts`), not a mock-only hook. - Error translation matches production behavior ## Validation Layer @@ -128,7 +128,7 @@ Happens in the engine layer (same pattern as bb-data engines): | pg error code | DistributedDatabaseErrors name | |---------------|------------------------| | `40001` | `SerializationFailure` | -| `23505` | `UniqueConstraintViolation` | +| `23505` | `UniqueConstraintViolation` (→ `ApiError` 409, not retriable) | | `08xxx` | `ConnectionFailed` | | (other) | `QueryFailed` | diff --git a/packages/bb-distributed-data/README.md b/packages/bb-distributed-data/README.md index 8830cb894..73bf54b09 100644 --- a/packages/bb-distributed-data/README.md +++ b/packages/bb-distributed-data/README.md @@ -161,7 +161,8 @@ try { // (Conflict), retriable — safe to retry. } if (isBlocksError(e, DistributedDatabaseErrors.UniqueConstraintViolation)) { - // Duplicate key + // Duplicate key — serialized as HTTP 409 (Conflict), not retriable + // (a blind retry of the same insert fails identically). } if (isBlocksError(e, DistributedDatabaseErrors.QueryFailed)) { // General query failure diff --git a/packages/bb-distributed-data/src/errors.test.ts b/packages/bb-distributed-data/src/errors.test.ts index e3bd1dead..cc3b6dbad 100644 --- a/packages/bb-distributed-data/src/errors.test.ts +++ b/packages/bb-distributed-data/src/errors.test.ts @@ -43,12 +43,18 @@ test('translateDsqlError: serialization failure (40001) → SerializationFailure ); }); -test('translateDsqlError: unique violation (23505) → UniqueConstraintViolation', () => { - const err = Object.assign(new Error('duplicate key'), { code: PG_UNIQUE_VIOLATION }); +test('translateDsqlError: unique violation (23505) → ApiError status 409, name preserved, not retriable', () => { + const err = Object.assign(new Error('duplicate key value violates unique constraint "dsql_items_pkey"'), { code: PG_UNIQUE_VIOLATION }); assert.throws( () => translateDsqlError(err), - (e: Error) => { + (e: unknown) => { + assert.ok(e instanceof ApiError, 'expected an ApiError'); + assert.equal(e.status, 409); assert.equal(e.name, DistributedDatabaseErrors.UniqueConstraintViolation); + assert.notEqual(e.retriable, true, 'a duplicate-key retry fails identically → not retriable'); + assert.equal(e.message, 'The item violates a unique constraint'); + // Raw driver error retained server-side as `cause`, not leaked into the message. + assert.equal(e.cause, err); return true; } ); diff --git a/packages/bb-distributed-data/src/errors.ts b/packages/bb-distributed-data/src/errors.ts index edb0ad374..d9cdd4cf7 100644 --- a/packages/bb-distributed-data/src/errors.ts +++ b/packages/bb-distributed-data/src/errors.ts @@ -51,7 +51,20 @@ export function translateDsqlError(e: Error): never { retriable: true, }); } else if (code === PG_UNIQUE_VIOLATION) { - e.name = DistributedDatabaseErrors.UniqueConstraintViolation; + // A duplicate-key / unique-constraint violation (SQLSTATE 23505) is a + // Conflict, not an InternalServerError: throw an ApiError with status 409 + // so the JSON-RPC serializer emits code 409 instead of a generic 500. + // Preserve the UniqueConstraintViolation name so isBlocksError() keeps + // matching on both server and client, and keep the original error as + // `cause` (server-side). Unlike the 40001 branch above, this is NOT + // retriable — a duplicate key is deterministic, so a blind retry of the + // same insert fails identically (ApiError defaults retriable=false). The + // client-visible message is a fixed, stable string; the raw driver text + // (which can name columns / constraints) is retained only as `cause`. + throw new ApiError('The item violates a unique constraint', 409, { + name: DistributedDatabaseErrors.UniqueConstraintViolation, + cause: e, + }); } else if (code && code.startsWith(PG_CONNECTION_EXCEPTION_CLASS)) { e.name = DistributedDatabaseErrors.ConnectionFailed; } else { diff --git a/test-apps/comprehensive/test/database.test.ts b/test-apps/comprehensive/test/database.test.ts index 3f906ea16..793c9fd6c 100644 --- a/test-apps/comprehensive/test/database.test.ts +++ b/test-apps/comprehensive/test/database.test.ts @@ -3,6 +3,8 @@ import { test, describe } from 'node:test'; import assert from 'node:assert'; +import { isBlocksError, ApiError } from '@aws-blocks/core'; +import { DatabaseErrors } from '@aws-blocks/bb-data'; import type { api as apiType } from 'aws-blocks'; // Compile-time type assertion helpers. `Equal` is the standard invariant @@ -105,6 +107,27 @@ export function databaseTests(getApi: () => typeof apiType) { await api.dbDelete(id); }); + // Duplicate-key (SQLSTATE 23505) UniqueConstraintViolation must serialize to + // JSON-RPC 409 (Conflict) over the wire — reconstructed client-side as an + // ApiError with status 409 — not a generic 500. The error name is preserved + // so isBlocksError still matches. The PGlite mock enforces the PK constraint, + // so this exercises the real translate path locally. (issue #508) + test('Database - duplicate insert returns status 409 over the wire', async () => { + const api = getApi(); + const id = `t-409-${Date.now().toString(36)}`; + await api.dbInsert(id, 'first', 1); + try { + await api.dbInsert(id, 'dup', 2); + assert.fail('Expected a conflict error'); + } catch (e) { + assert.ok(e instanceof ApiError, `Expected ApiError, got ${e}`); + assert.strictEqual(e.status, 409, 'duplicate key must be 409, not 500'); + assert.ok(isBlocksError(e, DatabaseErrors.UniqueConstraintViolation)); + assert.notStrictEqual(e.retriable, true, 'duplicate key is not retriable'); + } + await api.dbDelete(id); + }); + // Kysely transactions must be atomic on the real engine. test('Database - Kysely transaction commits transfer', async () => { const api = getApi(); diff --git a/test-apps/comprehensive/test/dsql.test.ts b/test-apps/comprehensive/test/dsql.test.ts index 332393c98..c7c9b3aa0 100644 --- a/test-apps/comprehensive/test/dsql.test.ts +++ b/test-apps/comprehensive/test/dsql.test.ts @@ -3,6 +3,8 @@ import { test, describe } from 'node:test'; import assert from 'node:assert'; +import { isBlocksError, ApiError } from '@aws-blocks/core'; +import { DistributedDatabaseErrors } from '@aws-blocks/bb-distributed-data'; import type { api as apiType } from 'aws-blocks'; // Compile-time type assertion helpers (same pattern as database.test.ts). @@ -102,6 +104,27 @@ export function dsqlTests(getApi: () => typeof apiType) { await api.dsqlDelete(id); }); + // Duplicate-key (SQLSTATE 23505) UniqueConstraintViolation must serialize to + // JSON-RPC 409 (Conflict) over the wire — reconstructed client-side as an + // ApiError with status 409 — not a generic 500. The error name is preserved + // so isBlocksError still matches. The DSQL mock enforces the PK constraint, + // so this exercises the real translate path locally. (issue #508) + test('DSQL - duplicate insert returns status 409 over the wire', async () => { + const api = getApi(); + const id = `d-409-${Date.now().toString(36)}`; + await api.dsqlInsert(id, 'first', 1); + try { + await api.dsqlInsert(id, 'dup', 2); + assert.fail('Expected a conflict error'); + } catch (e) { + assert.ok(e instanceof ApiError, `Expected ApiError, got ${e}`); + assert.strictEqual(e.status, 409, 'duplicate key must be 409, not 500'); + assert.ok(isBlocksError(e, DistributedDatabaseErrors.UniqueConstraintViolation)); + assert.notStrictEqual(e.retriable, true, 'duplicate key is not retriable'); + } + await api.dsqlDelete(id); + }); + test('DSQL - rejects FOREIGN KEY at query time', async () => { const api = getApi(); const result = await api.dsqlRejectForeignKey();