Skip to content

Commit 66e5705

Browse files
authored
fix(tables): match unique JSON values exactly and validate row writes against the live schema (#8394)
* fix(tables): match unique JSON values exactly and validate row writes against the live schema - Unique checks and the upsert conflict probe matched JSON objects and arrays by containment, so `{"a":1}` counted as a duplicate of `{"a":1,"b":2}` and upsert could overwrite a row that only contained its target. They now also require exact jsonb equality, keeping containment as the GIN-indexed leading clause; JSON values take per-value locks like other types - Row writes validated against a schema snapshot read before their transaction, so a concurrent make-unique, make-required, retype, or column delete could commit violating data. Each write now reads the live schema under the table's schema lock (shared) through `user_table_schema_for_write`, in the statement it already runs first, and validates against it - The background update runner derives each batch's patch from the raw payload against the live schema, through the same helper as the inline bulk update, and refuses unique and required-null patches under the lock * fix(tables): install the schema guard for db:push, refit rows from raw input - user_table_schema_for_write moves from Drizzle migration 0391 to script migration 0026, which db:push runs too. A db:push database (local dev, the CI push provision) never applied 0391, so every guarded row write failed there. The journal ends at 0390 again; the function body and its comments are unchanged. - A writer that finds the schema moved rebuilds the row from the caller's raw input instead of the value it coerced against its snapshot, so a "007" sent while a column changed from number to text is stored as "007", not "7". This covers insert, upsert, update, batch update and the import batch. - The refit re-checks the row's size, which a coercion to a wider type can grow past the limit after the pre-lock check passed. * fix(tables): re-check update batches under a moved schema, own-key reads - The background update runner re-reads a batch's rows inside the batch transaction and re-validates them merged with the re-derived patch when the schema moved since the page was checked, as the inline bulk update does; an unchanged schema adds no query. updatePageByIds takes an async per-batch hook with the transaction for this. - batchInsertRowsWithTx returns the definition it validated against, and batchInsertRows dispatches its insert triggers with it. - Row cells and patch keys are read as own properties (Object.hasOwn), so a legacy column keyed by a prototype name such as `constructor` is not seen in rows or patches that do not hold it. - The long schema-wait test asserts the write is waiting on the schema lock before the holder commits. * fix(tables): read the remaining row cells by own key in bulk validation, replace dedupe, and the upsert probe
1 parent 07c3ffe commit 66e5705

24 files changed

Lines changed: 2107 additions & 428 deletions

‎apps/sim/background/table-update.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
import { task } from '@trigger.dev/sdk'
1+
import { AbortTaskRunError, task } from '@trigger.dev/sdk'
22
import {
33
markTableUpdateFailed,
44
runTableUpdate,
55
type TableUpdatePayload,
6+
UpdatePatchRejectedError,
67
} from '@/lib/table/update-runner'
78

89
/**
@@ -19,7 +20,8 @@ export interface TableUpdateTaskPayload extends Omit<TableUpdatePayload, 'cutoff
1920
* worker keysets by id with a `created_at <= cutoff` floor and the JSONB-merge patch is idempotent
2021
* (re-applying the same patch to an already-patched row is a no-op), so a retried attempt re-walks
2122
* and re-applies whatever remains. The `table_jobs` ownership gate stops a retried run that lost
22-
* the job within one page.
23+
* the job within one page. A patch the table's schema refuses aborts without a retry: the retry
24+
* would read the same schema.
2325
*/
2426
export const tableUpdateTask = task({
2527
id: 'table-update',
@@ -30,7 +32,12 @@ export const tableUpdateTask = task({
3032
concurrencyLimit: 10,
3133
},
3234
run: async (payload: TableUpdateTaskPayload) => {
33-
await runTableUpdate({ ...payload, cutoff: new Date(payload.cutoff) })
35+
try {
36+
await runTableUpdate({ ...payload, cutoff: new Date(payload.cutoff) })
37+
} catch (error) {
38+
if (error instanceof UpdatePatchRejectedError) throw new AbortTaskRunError(error.message)
39+
throw error
40+
}
3441
},
3542
onFailure: async ({ payload, error }) => {
3643
await markTableUpdateFailed(payload.tableId, payload.jobId, error)

‎apps/sim/lib/table/application/copilot-bulk-rows.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import {
3333
import { assertRowDelete, assertRowUpdate, patchColumnIds } from '@/lib/table/mutation-locks'
3434
import { createExactEmptyTableRowSecretProvenance } from '@/lib/table/rows/secret-provenance'
3535
import { markTableUpdateFailed, runTableUpdate } from '@/lib/table/update-runner'
36+
import { uniqueColumnsInPatch } from '@/lib/table/validation'
3637

3738
const logger = createLogger('CopilotBulkRowsApplication')
3839

@@ -199,9 +200,7 @@ export const copilotUpdateRowsByFilter = defineAuthorizedTableUseCase({
199200
validateLimit(input.limit)
200201
const idData = rowDataNameToId(input.data, buildIdByName(context.table.schema))
201202
const filter = tablePredicateNamesToFilter(input.filter, context.table)
202-
const patchTouchesUnique = context.table.schema.columns.some(
203-
(column) => column.unique === true && (column.id ?? column.name) in idData
204-
)
203+
const patchTouchesUnique = uniqueColumnsInPatch(context.table.schema, idData).length > 0
205204
const inlineEligible =
206205
input.limit !== undefined && input.limit <= TABLE_LIMITS.MAX_BULK_OPERATION_SIZE
207206

‎apps/sim/lib/table/bulk-update-concurrency.test.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
2+
import { tableRowsLiveSchemaMock } from '@sim/testing/mocks/table-rows-live-schema.mock'
23
import {
34
tableRowsSecretProvenanceMock,
45
tableRowsSecretProvenanceMockFns,
@@ -22,21 +23,25 @@ vi.mock('@/lib/table/rows/ordering', () => ({
2223

2324
vi.mock('@/lib/table/rows/secret-provenance', () => tableRowsSecretProvenanceMock)
2425

26+
vi.mock('@/lib/table/rows/live-schema', () => tableRowsLiveSchemaMock)
27+
2528
vi.mock('@/lib/table/sql', () => ({
2629
buildFilterClause: vi.fn(() => sql`true`),
2730
buildPredicateClause: vi.fn(() => sql`true`),
2831
buildSortClause: vi.fn(() => sql`true`),
2932
escapeLikePattern: vi.fn((value: string) => value),
30-
fieldPredicate: vi.fn(() => sql`true`),
33+
uniqueValuePredicate: vi.fn(() => sql`true`),
3134
}))
3235

3336
vi.mock('@/lib/table/trigger', () => tableTriggerMock)
3437

35-
vi.mock('@/lib/table/validation', () => ({
38+
vi.mock('@/lib/table/validation', async (importOriginal) => ({
39+
cellOf: (await importOriginal<typeof import('@/lib/table/validation')>()).cellOf,
3640
validateRowSize: hoisted.validateRowSize,
3741
coerceRowToSchema: hoisted.coerceRowToSchema,
3842
coerceRowValues: vi.fn(),
3943
getUniqueColumns: vi.fn(() => []),
44+
uniqueColumnsInPatch: vi.fn(() => []),
4045
checkUniqueConstraintsDb: vi.fn(async () => ({ valid: true, errors: [] })),
4146
checkBatchUniqueConstraintsDb: vi.fn(async () => ({ valid: true, errors: [] })),
4247
}))

‎apps/sim/lib/table/import-data.ts‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { CSV_MAX_BATCH_SIZE } from '@/lib/table/import'
1515
import { assertRowDelete, assertRowInsert, assertSchemaMutable } from '@/lib/table/mutation-locks'
1616
import { nKeysBetween } from '@/lib/table/order-key'
1717
import type { DbTransaction } from '@/lib/table/planner'
18+
import { lockLiveTableSchema, refitRowToSchema, withLiveSchema } from '@/lib/table/rows/live-schema'
1819
import {
1920
acquireRowOrderLock,
2021
guardBatch,
@@ -64,9 +65,10 @@ export interface BulkImportBatch {
6465
* `runWorkflowColumn`** (a 1M-row import must not dispatch a workflow run per row).
6566
* Append and replace imports run this against the live table, so other writers can
6667
* race it: the batch holds the table's unique columns exclusively while it checks and
67-
* inserts. `row_count` is maintained set-based by the statement-level
68-
* trigger. There is no surrounding transaction and no rollback: each batch commits on
69-
* its own, so committed batches persist even if a later batch fails.
68+
* inserts, and checks the rows against the schema it reads under the schema lock, which
69+
* may have changed since the job resolved the table. `row_count` is maintained set-based
70+
* by the statement-level trigger. There is no surrounding transaction and no rollback:
71+
* each batch commits on its own, so committed batches persist even if a later batch fails.
7072
*
7173
* Throws on row-size/schema/unique violations or if the statement-level trigger rejects
7274
* the batch for crossing `max_rows`; the caller marks the import failed.
@@ -82,6 +84,7 @@ export async function bulkInsertImportBatch(
8284
// the caller's snapshot too would reject a since-cleared lock.
8385
if (!revalidate) assertRowInsert(table)
8486

87+
const rawRows = data.rows.map((row) => ({ ...row }))
8588
for (let i = 0; i < data.rows.length; i++) {
8689
const sizeValidation = validateRowSize(data.rows[i])
8790
if (!sizeValidation.valid) {
@@ -119,14 +122,23 @@ export async function bulkInsertImportBatch(
119122
}))
120123

121124
const inserted = await db.transaction(async (trx) => {
122-
await guardBatch(trx, data.tableId, revalidate)
123-
if (getUniqueColumns(table.schema).length > 0) {
125+
const fresh = await guardBatch(trx, data.tableId, revalidate)
126+
const live = fresh ? withLiveSchema(table, fresh.schema) : await lockLiveTableSchema(trx, table)
127+
if (live !== table) {
128+
for (let i = 0; i < data.rows.length; i++) {
129+
const refit = refitRowToSchema(data.rows[i], rawRows[i], table.schema, live.schema, 'null')
130+
if (!refit.valid) {
131+
throw new OrchestrationError('validation', `Row ${i + 1}: ${refit.errors.join(', ')}`)
132+
}
133+
}
134+
}
135+
if (getUniqueColumns(live.schema).length > 0) {
124136
// The whole-table unique lock, not per-value: a batch is far more values than the value-lock cap.
125-
await lockUniqueColumns(trx, table)
137+
await lockUniqueColumns(trx, live)
126138
const uniqueResult = await checkBatchUniqueConstraintsDb(
127139
data.tableId,
128140
data.rows,
129-
table.schema,
141+
live.schema,
130142
trx
131143
)
132144
if (!uniqueResult.valid) {
@@ -314,7 +326,7 @@ export async function importAppendRows(
314326
generateId().slice(0, 8),
315327
{ uniqueColumnsLocked: true }
316328
)
317-
inserted.push(...batchInserted)
329+
inserted.push(...batchInserted.rows)
318330
}
319331
return { inserted, table: working }
320332
})

‎apps/sim/lib/table/rows/bulk-update-patch-validation.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
* filter matches rows or none.
55
*/
66
import { resetDbChainMock } from '@sim/testing'
7+
import { tableRowsLiveSchemaMock } from '@sim/testing/mocks/table-rows-live-schema.mock'
78
import {
89
tableRowsSecretProvenanceMock,
910
tableRowsSecretProvenanceMockFns,
@@ -24,12 +25,14 @@ vi.mock('@/lib/table/rows/ordering', () => ({
2425

2526
vi.mock('@/lib/table/rows/secret-provenance', () => tableRowsSecretProvenanceMock)
2627

28+
vi.mock('@/lib/table/rows/live-schema', () => tableRowsLiveSchemaMock)
29+
2730
vi.mock('@/lib/table/sql', () => ({
2831
buildFilterClause: vi.fn(() => sql`true`),
2932
buildPredicateClause: vi.fn(() => sql`true`),
3033
buildSortClause: vi.fn(() => sql`true`),
3134
escapeLikePattern: vi.fn((value: string) => value),
32-
fieldPredicate: vi.fn(() => sql`true`),
35+
uniqueValuePredicate: vi.fn(() => sql`true`),
3336
}))
3437

3538
vi.mock('@/lib/table/trigger', () => tableTriggerMock)
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/**
2+
* Keeps row writes on the table's live schema rather than the definition their caller resolved
3+
* before the write transaction opened.
4+
*
5+
* Schema changes hold the table's schema lock exclusively (`withLockedTable`) while they check the
6+
* stored rows against the new schema: a column made unique is scanned for duplicates, one made
7+
* required for empty cells. A write validated against an older definition skips the check the new
8+
* schema adds, and if it commits after that scan, the column ends up holding the duplicate or the
9+
* empty cell. Each write transaction therefore takes the schema lock shared and reads the schema
10+
* under it, then validates against that: a schema change waits for writes already in flight, and a
11+
* write that waited sees the change.
12+
*/
13+
14+
import { compareStrings } from '@sim/utils/string'
15+
import { sql } from 'drizzle-orm'
16+
import { canonicalJson } from '@/lib/api/cursor-binding'
17+
import { OrchestrationError } from '@/lib/core/orchestration/types'
18+
import { getColumnId } from '@/lib/table/column-keys'
19+
import type { DbTransaction } from '@/lib/table/planner'
20+
import { type TableTxTimeouts, tableTxTimeoutSettings } from '@/lib/table/tx'
21+
import type { RowData, TableDefinition, TableSchema, ValidationResult } from '@/lib/table/types'
22+
import {
23+
coerceRowToSchema,
24+
type PatchedKeys,
25+
type UncoercibleValuePolicy,
26+
validateRowSize,
27+
} from '@/lib/table/validation'
28+
29+
/** Compares schemas by content, whatever order their columns are listed in. */
30+
function schemaFingerprint(schema: TableSchema): string {
31+
const columns = [...schema.columns].sort((a, b) => compareStrings(getColumnId(a), getColumnId(b)))
32+
return canonicalJson({ ...schema, columns })
33+
}
34+
35+
/** `table` itself when `schema` matches its schema, else a copy carrying `schema`. */
36+
export function withLiveSchema(table: TableDefinition, schema: TableSchema): TableDefinition {
37+
return schemaFingerprint(schema) === schemaFingerprint(table.schema)
38+
? table
39+
: { ...table, schema }
40+
}
41+
42+
/**
43+
* Takes the table's schema lock shared and reads its live schema, and returns the definition to
44+
* validate and write against: `table` itself when its schema is still current, else a copy carrying
45+
* the live schema. Call it first in the transaction, before its other locks, passing the
46+
* transaction's `timeouts` (see `setTableTxTimeouts`) in place of a separate timeouts statement.
47+
*
48+
* One statement: `user_table_schema_for_write` (script migration 0026) takes the lock and then
49+
* reads the schema. It is VOLATILE, so under READ COMMITTED its read takes a fresh snapshot and sees
50+
* a schema change that committed while the lock waited. The timeouts are applied in a subquery the call
51+
* reads from, first. The lock waits as long as the transaction's `statement_timeout` allows, not
52+
* its shorter `lock_timeout`, which the function leaves as it found it for the locks that follow.
53+
*/
54+
export async function lockLiveTableSchema(
55+
trx: DbTransaction,
56+
table: TableDefinition,
57+
timeouts?: TableTxTimeouts
58+
): Promise<TableDefinition> {
59+
const read = sql`SELECT user_table_schema_for_write(${table.id}) AS schema`
60+
const [live] = await trx.execute<{ schema: TableSchema | null }>(
61+
timeouts ? sql`${read} FROM (SELECT ${tableTxTimeoutSettings(timeouts)}) AS settings` : read
62+
)
63+
if (!live?.schema) throw new OrchestrationError('not_found', 'Table not found')
64+
return withLiveSchema(table, live.schema)
65+
}
66+
67+
/**
68+
* Removes, in place, the cells of columns `snapshot` defines and `live` no longer does. A column
69+
* delete reclaims its cells in the background, so a cell written after that pass would stay behind.
70+
*/
71+
export function dropDeletedColumns(
72+
rows: readonly RowData[],
73+
snapshot: TableSchema,
74+
live: TableSchema
75+
): void {
76+
const liveIds = new Set(live.columns.map(getColumnId))
77+
const deleted = snapshot.columns.map(getColumnId).filter((id) => !liveIds.has(id))
78+
if (deleted.length === 0) return
79+
for (const row of rows) {
80+
for (const id of deleted) delete row[id]
81+
}
82+
}
83+
84+
/**
85+
* Rebuilds `row`, in place, for `live` once the schema has moved since `snapshot`: from `raw`, the
86+
* row as the caller wrote it before coercing it against `snapshot`, so a value that schema would
87+
* have reshaped (`"007"` read as a number) reaches the live column as it was sent. Then drops the
88+
* cells of deleted columns, coerces and validates against `live` exactly as the write first did,
89+
* and re-checks the row's size, which a coercion to a wider type can grow.
90+
*/
91+
export function refitRowToSchema(
92+
row: RowData,
93+
raw: RowData,
94+
snapshot: TableSchema,
95+
live: TableSchema,
96+
policy?: UncoercibleValuePolicy,
97+
patchedKeys?: PatchedKeys
98+
): ValidationResult {
99+
for (const key of Object.keys(row)) delete row[key]
100+
Object.assign(row, raw)
101+
dropDeletedColumns([row], snapshot, live)
102+
const result = coerceRowToSchema(row, live, policy, patchedKeys)
103+
return result.valid ? validateRowSize(row) : result
104+
}

‎apps/sim/lib/table/rows/ordering.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -333,10 +333,12 @@ export async function insertOrderedRow(params: {
333333
/** Proof the caller asserted the insert lock (see `mutation-locks.ts`). */
334334
proof: MutationProof<'insert'>
335335
/**
336-
* Runs first in the transaction, before the row-order lock: the caller's unique-value locks and
337-
* unique check (see `unique-locks.ts`), so the check sees any concurrent insert of the same value.
336+
* Opens the transaction in place of the default timeouts, before the row-order lock: the
337+
* caller's schema guard (see `live-schema.ts`), which applies the timeouts, then its unique-value
338+
* locks and unique check (see `unique-locks.ts`), so the check sees any concurrent insert of the
339+
* same value.
338340
*/
339-
assertUnique?: (trx: DbTransaction) => Promise<void>
341+
validate?: (trx: DbTransaction) => Promise<void>
340342
}): Promise<{
341343
id: string
342344
data: RowData
@@ -358,8 +360,8 @@ export async function insertOrderedRow(params: {
358360
secretProvenance,
359361
} = params
360362
const [row] = await db.transaction(async (trx) => {
361-
await setTableTxTimeouts(trx)
362-
await params.assertUnique?.(trx)
363+
if (params.validate) await params.validate(trx)
364+
else await setTableTxTimeouts(trx)
363365
await acquireRowOrderLock(trx, tableId)
364366

365367
// Resolve the authoritative order key from neighbor ids when given, else from the requested
@@ -670,17 +672,29 @@ export async function deletePageByIds(
670672
return deleted
671673
}
672674

675+
/** The patch one update batch writes, or `null` when it writes nothing. */
676+
export interface PagePatch {
677+
patchJson: string
678+
secretProvenance: TableRowSecretProvenanceWrite
679+
}
680+
673681
/**
674682
* Applies a JSONB-merge patch (`data || patchJson`) to a page of row ids, committed in
675683
* UPDATE_BATCH_SIZE chunks (each its own transaction, 60s timeout) so a large background update
676-
* makes incremental, resumable progress. Returns the number of rows updated.
684+
* makes incremental, resumable progress. Each batch takes its patch from `prepare`, called inside
685+
* the batch's transaction with the definition `revalidate` read there and the batch's row ids, so a
686+
* caller can derive it, and check the rows it merges into, against the live schema. Returns the
687+
* number of rows updated.
677688
*/
678689
export async function updatePageByIds(
679690
tableId: string,
680691
workspaceId: string,
681692
rowIds: string[],
682-
patchJson: string,
683-
secretProvenance: TableRowSecretProvenanceWrite,
693+
prepare: (
694+
trx: DbTransaction,
695+
table: TableDefinition | undefined,
696+
batch: string[]
697+
) => Promise<PagePatch | null>,
684698
/** Proof the caller asserted the update lock (see `mutation-locks.ts`). */
685699
_proof: MutationProof<'update'>,
686700
/** Re-asserts the lock inside each batch transaction. See {@link guardBatch}. */
@@ -692,15 +706,19 @@ export async function updatePageByIds(
692706
const batch = rowIds.slice(i, i + TABLE_LIMITS.UPDATE_BATCH_SIZE)
693707
const rows = await db.transaction(async (trx) => {
694708
await setTableTxTimeouts(trx, { statementMs: 60_000 })
695-
await guardBatch(trx, tableId, revalidate)
709+
const patch = await prepare(trx, await guardBatch(trx, tableId, revalidate), batch)
710+
if (!patch) return []
696711
return mutateTableRowsWithSecretProvenance(trx, {
697-
rows: batch.map((rowId) => ({ rowId, provenance: secretProvenance })),
712+
rows: batch.map((rowId) => ({ rowId, provenance: patch.secretProvenance })),
698713
rowState: 'existing',
699714
mode: 'merge',
700715
mutate: async () => {
701716
const rows = await trx
702717
.update(userTableRows)
703-
.set({ data: sql`${userTableRows.data} || ${patchJson}::jsonb`, updatedAt: now })
718+
.set({
719+
data: sql`${userTableRows.data} || ${patch.patchJson}::jsonb`,
720+
updatedAt: now,
721+
})
704722
.where(
705723
and(
706724
eq(userTableRows.tableId, tableId),

0 commit comments

Comments
 (0)