From f27c7d6efc627965970736f89b98333c053823c7 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 11:49:33 -0700 Subject: [PATCH] fix(v2): give the keyset cursor's timestamp an explicit SQL type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handing back the `nextCursor` from any timestamp-sorted v2 list and passing it straight in returned 500. The keyset compares millisecond-truncated timestamps on both sides, and the bound cursor value went out as a bare placeholder — which Postgres types as `unknown`. `date_trunc` is overloaded across `timestamp`, `timestamptz`, and `interval`, so `date_trunc(unknown, unknown)` matched no single candidate and the statement failed outright. The value was already validated; it just carried no type. Cast it to the column's own SQL type inside `timestampKey`, so all twelve call sites across six modules inherit the fix. Derived from the column rather than hardcoded, which keeps a `timestamptz` column's offset honoured too. The millisecond truncation is unchanged — it is what stops the page's own last row being re-admitted. --- .agents/skills/v2-api-conventions/SKILL.md | 11 +++++-- .claude/commands/v2-api-conventions.md | 11 +++++-- .cursor/commands/v2-api-conventions.md | 11 +++++-- apps/sim/lib/api/list-query.test.ts | 35 +++++++++++++++++++--- apps/sim/lib/api/list-query.ts | 12 ++++++-- 5 files changed, 65 insertions(+), 15 deletions(-) diff --git a/.agents/skills/v2-api-conventions/SKILL.md b/.agents/skills/v2-api-conventions/SKILL.md index 346c9206ad9..276bbd14436 100644 --- a/.agents/skills/v2-api-conventions/SKILL.md +++ b/.agents/skills/v2-api-conventions/SKILL.md @@ -16,12 +16,13 @@ failure (always) { "error": { "code": "...", "message": "...", "details"?: Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML. -That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local: +That promise is worth stating as a rule because it has been broken five separate ways, each time by a route or a builder taking a shortcut that looked local: - `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`. - A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered. - `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page. - Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`. +- Handing back a `nextCursor` from any timestamp-sorted list and passing it straight in returned **500**. The value was validated and bound — but bound with no SQL type, into `date_trunc`, which is overloaded, so Postgres could resolve no overload. Validation was never the missing half; the type was. Each was one line. The rules below are the generalisations. @@ -62,7 +63,11 @@ Two of these carry real design weight: **404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose. -**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. +**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped three times — a fractional `limit` reaching `LIMIT 2.5`, a plain `HEAD` tripping the builder's method guard, and a keyset cursor's timestamp reaching `date_trunc` as an untyped placeholder — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. + +**Validating a value is only half of it; the value also has to reach SQL with a type.** A bound parameter arrives as `unknown` and takes its type from context. Against a typed column (`sort_order > $1`) that inference always succeeds, which is why the gap stays invisible almost everywhere — but as an argument to an overloaded function it can resolve to nothing at all. So: **if a bound value is an argument to a SQL function rather than one side of a comparison, write its type down** (`lib/api/list-query.ts`, `timestampKey`, casts from the column). + +And this class survives a green test suite — `keysetAfter` returned well-formed SQL and every assertion passed; only Postgres's parser rejected it. When a change alters the *shape* of generated SQL rather than its values, execute it somewhere before believing the suite. **Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So: @@ -182,7 +187,7 @@ Run this against any new or changed v2 endpoint. - [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`. - [ ] Route uses a shared builder; no hand-built `NextResponse.json`. - [ ] Query and body schemas are `.strict()`. -- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer. +- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer, and that any bound value passed as an argument to a SQL function carries an explicit type. - [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`. - [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. - [ ] Keyset sorts end in a unique `id` key. diff --git a/.claude/commands/v2-api-conventions.md b/.claude/commands/v2-api-conventions.md index b19c991c0e5..f2ce200b413 100644 --- a/.claude/commands/v2-api-conventions.md +++ b/.claude/commands/v2-api-conventions.md @@ -15,12 +15,13 @@ failure (always) { "error": { "code": "...", "message": "...", "details"?: Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML. -That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local: +That promise is worth stating as a rule because it has been broken five separate ways, each time by a route or a builder taking a shortcut that looked local: - `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`. - A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered. - `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page. - Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`. +- Handing back a `nextCursor` from any timestamp-sorted list and passing it straight in returned **500**. The value was validated and bound — but bound with no SQL type, into `date_trunc`, which is overloaded, so Postgres could resolve no overload. Validation was never the missing half; the type was. Each was one line. The rules below are the generalisations. @@ -61,7 +62,11 @@ Two of these carry real design weight: **404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose. -**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. +**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped three times — a fractional `limit` reaching `LIMIT 2.5`, a plain `HEAD` tripping the builder's method guard, and a keyset cursor's timestamp reaching `date_trunc` as an untyped placeholder — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. + +**Validating a value is only half of it; the value also has to reach SQL with a type.** A bound parameter arrives as `unknown` and takes its type from context. Against a typed column (`sort_order > $1`) that inference always succeeds, which is why the gap stays invisible almost everywhere — but as an argument to an overloaded function it can resolve to nothing at all. So: **if a bound value is an argument to a SQL function rather than one side of a comparison, write its type down** (`lib/api/list-query.ts`, `timestampKey`, casts from the column). + +And this class survives a green test suite — `keysetAfter` returned well-formed SQL and every assertion passed; only Postgres's parser rejected it. When a change alters the *shape* of generated SQL rather than its values, execute it somewhere before believing the suite. **Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So: @@ -181,7 +186,7 @@ Run this against any new or changed v2 endpoint. - [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`. - [ ] Route uses a shared builder; no hand-built `NextResponse.json`. - [ ] Query and body schemas are `.strict()`. -- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer. +- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer, and that any bound value passed as an argument to a SQL function carries an explicit type. - [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`. - [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. - [ ] Keyset sorts end in a unique `id` key. diff --git a/.cursor/commands/v2-api-conventions.md b/.cursor/commands/v2-api-conventions.md index f1e3dad2bd4..d114966e6f5 100644 --- a/.cursor/commands/v2-api-conventions.md +++ b/.cursor/commands/v2-api-conventions.md @@ -10,12 +10,13 @@ failure (always) { "error": { "code": "...", "message": "...", "details"?: Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML. -That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local: +That promise is worth stating as a rule because it has been broken five separate ways, each time by a route or a builder taking a shortcut that looked local: - `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`. - A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered. - `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page. - Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`. +- Handing back a `nextCursor` from any timestamp-sorted list and passing it straight in returned **500**. The value was validated and bound — but bound with no SQL type, into `date_trunc`, which is overloaded, so Postgres could resolve no overload. Validation was never the missing half; the type was. Each was one line. The rules below are the generalisations. @@ -56,7 +57,11 @@ Two of these carry real design weight: **404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose. -**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. +**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped three times — a fractional `limit` reaching `LIMIT 2.5`, a plain `HEAD` tripping the builder's method guard, and a keyset cursor's timestamp reaching `date_trunc` as an untyped placeholder — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface. + +**Validating a value is only half of it; the value also has to reach SQL with a type.** A bound parameter arrives as `unknown` and takes its type from context. Against a typed column (`sort_order > $1`) that inference always succeeds, which is why the gap stays invisible almost everywhere — but as an argument to an overloaded function it can resolve to nothing at all. So: **if a bound value is an argument to a SQL function rather than one side of a comparison, write its type down** (`lib/api/list-query.ts`, `timestampKey`, casts from the column). + +And this class survives a green test suite — `keysetAfter` returned well-formed SQL and every assertion passed; only Postgres's parser rejected it. When a change alters the *shape* of generated SQL rather than its values, execute it somewhere before believing the suite. **Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So: @@ -176,7 +181,7 @@ Run this against any new or changed v2 endpoint. - [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`. - [ ] Route uses a shared builder; no hand-built `NextResponse.json`. - [ ] Query and body schemas are `.strict()`. -- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer. +- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer, and that any bound value passed as an argument to a SQL function carries an explicit type. - [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`. - [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them. - [ ] Keyset sorts end in a unique `id` key. diff --git a/apps/sim/lib/api/list-query.test.ts b/apps/sim/lib/api/list-query.test.ts index 4da992aa2f4..51dedd00025 100644 --- a/apps/sim/lib/api/list-query.test.ts +++ b/apps/sim/lib/api/list-query.test.ts @@ -105,13 +105,24 @@ describe('timestampKey', () => { ) }) - it('truncates the bound cursor value to match, binding it through the column encoder', () => { + it('casts the bound cursor value so date_trunc has a resolvable overload', () => { const { sql: text, params } = render(createdKey.bind('2024-01-01T00:00:00.123Z')!) - expect(text).toBe(`date_trunc('milliseconds', $1)`) + expect(text).toBe(`date_trunc('milliseconds', cast($1 as timestamp))`) expect(params).toEqual(['2024-01-01T00:00:00.123Z']) }) + it('takes the cast from the column, so a timestamptz column keeps its offset', () => { + const zoned = pgTable('zoned', { + at: timestamp('at', { withTimezone: true }).notNull(), + }) + const zonedKey = timestampKey<{ at: Date }>(zoned.at, (r) => r.at) + + expect(render(zonedKey.bind('2024-01-01T00:00:00.123Z')!).sql).toBe( + `date_trunc('milliseconds', cast($1 as timestamp with time zone))` + ) + }) + it('rejects a cursor value that is not a parseable timestamp', () => { expect(createdKey.bind('not-a-date')).toBeNull() expect(createdKey.bind(1700000000000)).toBeNull() @@ -173,13 +184,29 @@ describe('keysetAfter', () => { ) expect(text).toBe( - `(date_trunc('milliseconds', "thing"."created_at") > date_trunc('milliseconds', $1) or ` + - `(date_trunc('milliseconds', "thing"."created_at") = date_trunc('milliseconds', $2) and ` + + `(date_trunc('milliseconds', "thing"."created_at") > date_trunc('milliseconds', cast($1 as timestamp)) or ` + + `(date_trunc('milliseconds', "thing"."created_at") = date_trunc('milliseconds', cast($2 as timestamp)) and ` + `"thing"."id" > $3))` ) expect(params).toEqual(['2024-01-01T00:00:00.123Z', '2024-01-01T00:00:00.123Z', 'file-7']) }) + /** + * Pins the class, not the instance: `date_trunc` is today's only wrapping, so + * what this catches is a future key that wraps its bound value untyped. + */ + it('leaves no bound value bare inside a function call', () => { + const { sql: text } = render( + keysetAfter( + [numberKey(thing.size, () => 0), createdKey, idKey], + [7, '2024-01-01T00:00:00.123Z', 'file-7'], + 'asc' + )! + ) + + expect(text).not.toMatch(/[a-z_]+\((?:[^()]*,)?\s*\$\d+\s*\)/i) + }) + /** A caller controls the cursor's contents, so a bad value is a 400, not a 500 from SQL. */ it('refuses a cursor carrying a value its key cannot hold', () => { expect(keysetAfter([createdKey, idKey], ['not-a-date', 'file-7'], 'asc')).toBeNull() diff --git a/apps/sim/lib/api/list-query.ts b/apps/sim/lib/api/list-query.ts index d45d28dacf0..e45560a78fc 100644 --- a/apps/sim/lib/api/list-query.ts +++ b/apps/sim/lib/api/list-query.ts @@ -109,6 +109,15 @@ export function numberKey(column: SQLWrapper, read: (row: Row) => number): * `date_trunc` rules out an index-ordered scan, but none of the timestamp * columns sorted here are indexed, so it costs nothing today. Adding an index * to serve one of these sorts means indexing this same expression. + * + * The column serializes the `Date` (drizzle's own timestamp encoder) and also + * supplies the cast. A bare placeholder arrives as `unknown` and `date_trunc` is + * overloaded (`timestamp`, `timestamptz`, `interval`), so it resolves to no + * overload and 500s on page two — this is the only key whose placeholder sits + * inside a function rather than against a typed column. Taking the type from the + * column rather than a literal keeps a `timestamptz` correct, and reading it + * inside `bind` keeps the module-scope sort maps from touching the column at + * import time. */ export function timestampKey(column: Column, read: (row: Row) => Date): KeysetKey { return { @@ -118,8 +127,7 @@ export function timestampKey(column: Column, read: (row: Row) => Date): Key if (typeof value !== 'string') return null const date = new Date(value) if (Number.isNaN(date.getTime())) return null - // Bound through the column so drizzle's own timestamp encoder serializes it. - return sql`date_trunc('milliseconds', ${sql.param(date, column)})` + return sql`date_trunc('milliseconds', cast(${sql.param(date, column)} as ${sql.raw(column.getSQLType())}))` }, } }