Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/sim/app/api/v1/logs/filters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ export function buildLogFilters(filters: LogFilters): SQL<unknown> {
const cursorDate = new Date(filters.cursor.startedAt)
if (filters.order === 'desc') {
conditions.push(
sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) < (${cursorDate}, ${filters.cursor.id})`
sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) < (${sql.param(cursorDate, workflowExecutionLogs.startedAt)}, ${filters.cursor.id})`
)
} else {
conditions.push(
sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) > (${cursorDate}, ${filters.cursor.id})`
sql`(${workflowExecutionLogs.startedAt}, ${workflowExecutionLogs.id}) > (${sql.param(cursorDate, workflowExecutionLogs.startedAt)}, ${filters.cursor.id})`
)
}
}
Expand Down
14 changes: 7 additions & 7 deletions apps/sim/lib/execution/payloads/large-value-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,9 +390,9 @@ async function pruneStaleReferences(
batchSize: number,
dbClient: LargeValueMetadataClient
): Promise<number> {
// `IN ()` is a syntax error; callers chunk a non-empty list, but never rely on that.
// Empty input is a valid no-op, and `IN ()` is a syntax error whose failure the
// cleanup job swallows — keep these total rather than relying on the caller.
if (workspaceIds.length === 0) return 0

const rows = await dbClient.execute<{ count: number }>(sql`
WITH deleted AS (
DELETE FROM ${executionLargeValueReferences} AS ref
Expand Down Expand Up @@ -434,9 +434,9 @@ async function pruneDeletedParentDependencies(
batchSize: number,
dbClient: LargeValueMetadataClient
): Promise<number> {
// `IN ()` is a syntax error; callers chunk a non-empty list, but never rely on that.
// Empty input is a valid no-op, and `IN ()` is a syntax error whose failure the
// cleanup job swallows — keep these total rather than relying on the caller.
if (workspaceIds.length === 0) return 0

const rows = await dbClient.execute<{ count: number }>(sql`
WITH deleted AS (
DELETE FROM ${executionLargeValueDependencies} AS dependency
Expand Down Expand Up @@ -472,9 +472,9 @@ async function pruneDeletedLargeValueTombstones(
batchSize: number,
dbClient: LargeValueMetadataClient
): Promise<number> {
// `IN ()` is a syntax error; callers chunk a non-empty list, but never rely on that.
// Empty input is a valid no-op, and `IN ()` is a syntax error whose failure the
// cleanup job swallows — keep these total rather than relying on the caller.
if (workspaceIds.length === 0) return 0

const rows = await dbClient.execute<{ count: number }>(sql`
WITH deleted AS (
DELETE FROM ${executionLargeValues} AS value
Expand All @@ -483,7 +483,7 @@ async function pruneDeletedLargeValueTombstones(
FROM ${executionLargeValues} AS value
WHERE value.workspace_id IN ${workspaceIds}
AND value.deleted_at IS NOT NULL
AND value.deleted_at < ${deletedBefore}
AND value.deleted_at < ${sql.param(deletedBefore, executionLargeValues.deletedAt)}
AND NOT EXISTS (
SELECT 1
FROM ${executionLargeValueDependencies} AS dependency
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,13 @@ describe('unreferencedLargeValuePredicate SQL', () => {
// `status = IN (...)` is a syntax error Postgres only reports at execution time.
expect(text).not.toMatch(/=\s*IN\s*\(/)
})

it('never binds an array as a parameter', () => {
// The invariant that matters: the app pools set `fetch_types: false`, so an
// array bound as one parameter fails at execution even though the rendered
// SQL (`ANY($1::text[])`) is valid. Only the params can reveal it.
for (const param of params) {
expect(Array.isArray(param)).toBe(false)
}
})
})
6 changes: 5 additions & 1 deletion apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,16 @@ describe('pruneLargeValueMetadata SQL', () => {
}
})

it('never binds an array as a parameter', async () => {
it('binds only scalar parameters', async () => {
const statements = await renderPruneStatements([...ids])

for (const { params } of statements) {
for (const param of params) {
// Arrays fail under fetch_types: false (no serializer, 22P02); Date
// objects fail in postgres-js's unsafe path outright
// (ERR_INVALID_ARG_TYPE) — both must be pre-encoded to strings.
expect(Array.isArray(param)).toBe(false)
expect(param instanceof Date).toBe(false)
}
}
})
Expand Down
54 changes: 54 additions & 0 deletions apps/sim/lib/webhooks/polling/config-sql.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* @vitest-environment node
*/

// Renders the real key-removal expression against the real drizzle dialect.
// utils.test.ts mocks drizzle wholesale, so its assertions passed against BOTH
// previously-broken forms (`- ${keys}::text[]` and `- ${sql.param(keys)}::text[]`).
// Only a real render can tell them apart, and only the params reveal an array bind:
// the app pools set `fetch_types: false` (packages/db/db.ts), which leaves
// postgres-js unable to serialize an array bound as a single parameter.
import { describe, expect, it, vi } from 'vitest'

vi.unmock('drizzle-orm')
vi.unmock('@sim/db')
vi.unmock('@sim/db/schema')

process.env.DATABASE_URL ??= 'postgresql://user:pass@localhost:5432/test'

const { sql } = await import('drizzle-orm')
const { PgDialect } = await import('drizzle-orm/pg-core')

/** Mirrors the expression built in `updateWebhookProviderConfig`. */
function renderKeyRemoval(removedKeys: string[]) {
const merged = sql`COALESCE("provider_config"::jsonb, '{}'::jsonb) || ${JSON.stringify({ a: 1 })}::jsonb`
return new PgDialect().sqlToQuery(
sql`(${merged}) - ARRAY[${sql.join(
removedKeys.map((key) => sql`${key}`),
sql`, `
)}]::text[]`
)
}

describe('provider-config key removal SQL', () => {
// Production only ever removes one key (google-drive's pageToken reset), so the
// single-element case is the one that actually runs.
for (const keys of [['pageToken'], ['a', 'b'], ['a', 'b', 'c']]) {
it(`binds ${keys.length} key(s) as scalars inside an ARRAY constructor`, () => {
const { sql: text, params } = renderKeyRemoval(keys)

expect(text).toContain(`ARRAY[${keys.map((_, i) => `$${i + 2}`).join(', ')}]::text[]`)
expect(params).toEqual([JSON.stringify({ a: 1 }), ...keys])
for (const param of params) {
expect(Array.isArray(param)).toBe(false)
}
})
}

it('never renders a row constructor cast to text[]', () => {
// `- ($1, $2)::text[]` — what interpolating the raw JS array produced. Postgres
// rejects it: "cannot cast type record to text[]" (and 22P02 at one element).
const { sql: text } = renderKeyRemoval(['a', 'b'])
expect(text).not.toMatch(/\(\$\d+(, \$\d+)+\)::text\[\]/)
})
})
37 changes: 26 additions & 11 deletions packages/db/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,33 @@ if (!connectionString) {
}

/**
* `fetch_types: false` skips postgres.js's `pg_catalog.pg_type` roundtrip, which
* it otherwise runs on every new connection before that connection's first query.
* It builds array parsers only — scalar types (including `jsonb` and pgvector)
* are unaffected — and Drizzle already parses this schema's `text[]` columns
* itself, so the fetch is pure connection-setup latency.
* `fetch_types: false` skips postgres.js's `pg_catalog.pg_type` roundtrip, which it
* otherwise runs on every new connection before that connection's first query.
*
* The one behavior this changes: a raw `db.execute` that projects an array-typed
* column yields the wire form `'{a,b}'` rather than `['a','b']`, since nothing maps
* the result. Verified by differential test that Drizzle-typed selects and
* `.returning()` are unaffected — `PgArray.mapFromDriverValue` parses the wire form
* itself, and writes never reach the array serializer because Drizzle serializes
* arrays before binding.
* That roundtrip populates the array type map, which postgres.js uses in BOTH
* directions, for every array type — not just `text[]`. Disabling it has two
* consequences on every client built from these options (the primary, the replica,
* and every `dbFor()` sub-pool):
*
* 1. Reads: a raw `db.execute` projecting an array column yields the wire form
* `'{a,b}'`, not `['a','b']`. Drizzle-typed selects and `.returning()` are
* unaffected — `PgArray.mapFromDriverValue` parses the wire form itself.
* 2. Writes: a JS array bound as a SINGLE parameter fails at execution with
* `22P02 Array value must start with "{"` — there is no serializer to build the
* literal, and neither `prepare: false` nor `sql.array()` avoids it. Drizzle-typed
* column writes ARE safe (`PgArray.mapToDriverValue` stringifies first), as are
* `inArray`/`${jsArray}` in a drizzle `sql` template (both expand to scalar binds).
* Raw binds are NOT: never write `sql.param(someArray)`. Pass an expanded list
* (`IN ${ids}`) or an explicit `ARRAY[${sql.join(...)}]::text[]` of scalars.
*
* Scalar types — including `jsonb`, pgvector, enums and ranges — are unaffected.
*
* Note postgres.js's OWN `sql` tag has the opposite semantics: it binds `${array}`
* as one array parameter. `packages/db/scripts/migrate.ts` deliberately omits
* `fetch_types` for that reason; see the note there before sharing these options.
*
* Pinned by apps/sim/lib/execution/payloads/prune-metadata-sql.test.ts, which renders
* the real statements and asserts no bind parameter is an array.
*/
const poolOptions = {
prepare: false,
Expand Down
8 changes: 8 additions & 0 deletions packages/db/scripts/migrate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ const hasDirectMigrationUrl = Boolean(process.env.MIGRATION_DATABASE_URL)
* `max_lifetime: null` pins the session for the whole run: the postgres-js
* default recycles the connection after 30–60 min, silently dropping the
* session advisory lock and `SET`s.
*
* Deliberately does NOT set `fetch_types: false`, unlike the app pools in
* `packages/db/db.ts`. Script migrations bind JS arrays directly through
* postgres-js's own `sql` tag (`= ANY(${ids}::text[])`, `unnest(${ids}::text[])`),
* and that binding is powered by the `pg_catalog.pg_type` fetch this option would
* skip. Copying the app's options here for consistency fails every registered
* script migration with `22P02`, aborting the migration container and blocking
* startup on every deploy and every fresh self-hosted install.
*/
const client = postgres(url, {
max: 1,
Expand Down
29 changes: 23 additions & 6 deletions packages/testing/src/mocks/database.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,29 @@ export function createMockSql() {
}),
})

// Binds a value as a single parameter — drizzle's escape hatch for passing an
// array to Postgres as an array rather than expanding it into a value list.
sqlFn.param = (value: any) => ({
value,
toSQL: () => ({ sql: '?', params: [value] }),
})
// Binds a value as a single parameter. Rejecting arrays here is the only place
// the unit suite can see this bug class: the app pools set `fetch_types: false`
// (packages/db/db.ts), which leaves postgres-js with no array serializer, so an
// array bound as ONE parameter fails at execution with `22P02`. Rendered SQL
// looks perfect (`ANY($1::text[])`), so no assertion on query text can catch it.
sqlFn.param = (value: any, encoder?: any) => {
if (encoder === undefined && Array.isArray(value)) {
throw new Error(
'sql.param(array) binds an array as one parameter, which fails under ' +
'fetch_types: false (packages/db/db.ts). Interpolate the array directly ' +
'for an expanded IN list, or build an ARRAY[...]::text[] constructor of ' +
'scalar binds via sql.join.'
)
}
if (encoder === undefined && value instanceof Date) {
throw new Error(
'sql.param(date) without an encoder reaches postgres-js as a Date object, ' +
'which its unsafe path cannot serialize (ERR_INVALID_ARG_TYPE). Bind ' +
'through the matching column: sql.param(date, table.timestampColumn).'
)
}
return { value, toSQL: () => ({ sql: '?', params: [value] }) }
}

return sqlFn
}
Expand Down
Loading