Skip to content

Commit fe78d61

Browse files
committed
perf(table): project only the job field the row count needs
Both job reads selected the whole payload jsonb, but mapJobRow reads exactly one number out of it, and only for a running delete. The payload also carries the delete job's filter and an unbounded excludeRowIds array, and the latest non-export job is read on essentially every table request — a table that once ran a large delete would ship that id list on every read, forever. LatestJobRow.payload becomes doomedCount, extracted in SQL. Both readers share JOB_PROJECTION so one edit reaches the batch DISTINCT ON and the correlated subquery alike; the compile-time constraint widens to Column | SQL rather than being dropped. Behaviour is identical. `->` keeps the value jsonb, which postgres-js decodes through its built-in JSON.parse handler, so it arrives as a number with no boundary coercion. A null payload, a payload without the key, a non-object payload and an explicit JSON null all collapse to the same `?? 0` the previous optional chain produced. Sized honestly before claiming a win: payloads are small in practice today, so this is defensive rather than impactful — it removes an unbounded growth path, not a measured cost. Verified against a real Postgres, not just the mocked driver: the generated correlated subquery returns doomedCount 12 for a delete job, null for an import job, and a null row for a table with no job. Also from the review pass: re-homes the strictWrite explanation onto rowWriteOptions, where six {@link} references now point; records why replaceProjectedWireRows carries no keying discriminator; notes the one case the uniqueness-narrowing invariant does not cover; pins the lax id-wire passthrough with a test; and renames a parameter that misled once only its keys were read.
1 parent 0c9827b commit fe78d61

9 files changed

Lines changed: 230 additions & 117 deletions

File tree

apps/sim/lib/table/__tests__/update-row.test.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,10 @@ describe('batchUpdateRows — per-row partial merge', () => {
558558
* The safety argument is that a merge cannot newly violate uniqueness on a
559559
* column it leaves alone: that value is the one already stored, and it
560560
* satisfied the constraint when it was written.
561+
*
562+
* The one case that does not cover is a unique constraint added to a column
563+
* that already held duplicates — such a row is no longer blocked from edits
564+
* elsewhere in it, which is the intended outcome.
561565
*/
562566
describe('updateRow — uniqueness probe scoping', () => {
563567
beforeEach(() => {

apps/sim/lib/table/application/context.test.ts

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,23 @@ async function withUnhandledRejectionWatch(body: () => Promise<void>): Promise<u
4747
return seen
4848
}
4949

50+
/**
51+
* Holds the table load open so a test can observe what the resolver does before
52+
* the table arrives. `release` resolves it with the canonical table.
53+
*/
54+
function deferTableLoad(): { release: () => void } {
55+
let releaseTable: (table: unknown) => void = () => {}
56+
getTableById.mockImplementationOnce(
57+
() =>
58+
new Promise((resolve) => {
59+
releaseTable = resolve
60+
})
61+
)
62+
return {
63+
release: () => releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' }),
64+
}
65+
}
66+
5067
describe('table application context', () => {
5168
beforeEach(() => {
5269
vi.clearAllMocks()
@@ -73,13 +90,7 @@ describe('table application context', () => {
7390
})
7491

7592
it('starts the workspace load without waiting for the table when a workspace is asserted', async () => {
76-
let releaseTable: (table: unknown) => void = () => {}
77-
getTableById.mockImplementationOnce(
78-
() =>
79-
new Promise((resolve) => {
80-
releaseTable = resolve
81-
})
82-
)
93+
const { release } = deferTableLoad()
8394

8495
const pending = resolveActiveTableContext({
8596
tableId: 'table-1',
@@ -90,26 +101,20 @@ describe('table application context', () => {
90101

91102
expect(loadWorkspace).toHaveBeenCalledWith('workspace-1')
92103

93-
releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' })
104+
release()
94105
await expect(pending).resolves.toMatchObject({ tableId: 'table-1', workspaceId: 'workspace-1' })
95106
})
96107

97108
it('waits for the table before loading a workspace when none is asserted', async () => {
98-
let releaseTable: (table: unknown) => void = () => {}
99-
getTableById.mockImplementationOnce(
100-
() =>
101-
new Promise((resolve) => {
102-
releaseTable = resolve
103-
})
104-
)
109+
const { release } = deferTableLoad()
105110

106111
const pending = resolveActiveTableContext({ tableId: 'table-1' })
107112
await Promise.resolve()
108113
await Promise.resolve()
109114

110115
expect(loadWorkspace).not.toHaveBeenCalled()
111116

112-
releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' })
117+
release()
113118
await expect(pending).resolves.toMatchObject({ tableId: 'table-1', workspaceId: 'workspace-1' })
114119
expect(loadWorkspace).toHaveBeenCalledWith('workspace-1')
115120
})

apps/sim/lib/table/application/rows.test.ts

Lines changed: 51 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,22 @@ const TABLE: TableDefinition = {
172172

173173
const PRINCIPAL = { kind: 'session' as const, userId: 'user-1', sessionId: 'session-1' }
174174

175+
/**
176+
* The active-table context every row command resolves before it does any work.
177+
* Pass a variant table when a test needs a different schema — the surrounding
178+
* workspace scope is the same for every command under test.
179+
*/
180+
function contextFor(table: TableDefinition = TABLE) {
181+
return {
182+
tableId: table.id,
183+
table,
184+
workspaceId: table.workspaceId,
185+
workspaceOrganizationId: 'organization-1',
186+
allowPersonalApiKeys: true,
187+
billedAccountUserId: 'billing-owner-1',
188+
}
189+
}
190+
175191
describe('table predicate translation', () => {
176192
it('maps invalid run filters to the shared row validation error', () => {
177193
expect(() =>
@@ -210,14 +226,7 @@ describe('replaceProjectedWireRows application command', () => {
210226
beforeEach(() => {
211227
vi.clearAllMocks()
212228
mockResolvePermission.mockResolvedValue('write')
213-
mockResolveContext.mockResolvedValue({
214-
tableId: TABLE.id,
215-
table: TABLE,
216-
workspaceId: TABLE.workspaceId,
217-
workspaceOrganizationId: 'organization-1',
218-
allowPersonalApiKeys: true,
219-
billedAccountUserId: 'billing-owner-1',
220-
})
229+
mockResolveContext.mockResolvedValue(contextFor())
221230
mockAssertRowCapacity.mockResolvedValue(10_000)
222231
mockWithLockedTable.mockImplementation(
223232
async (_tableId: string, run: (table: TableDefinition, trx: unknown) => unknown) =>
@@ -457,14 +466,7 @@ describe('replaceTableRows application use case', () => {
457466
beforeEach(() => {
458467
vi.clearAllMocks()
459468
mockResolvePermission.mockResolvedValue('write')
460-
mockResolveContext.mockResolvedValue({
461-
tableId: TABLE.id,
462-
table: TABLE,
463-
workspaceId: TABLE.workspaceId,
464-
workspaceOrganizationId: 'organization-1',
465-
allowPersonalApiKeys: true,
466-
billedAccountUserId: 'billing-owner-1',
467-
})
469+
mockResolveContext.mockResolvedValue(contextFor())
468470
mockReplaceRowsPrimitive.mockResolvedValue({ deletedCount: 2, insertedCount: 1 })
469471
})
470472

@@ -570,14 +572,7 @@ describe('row query and upsert application semantics', () => {
570572
beforeEach(() => {
571573
vi.clearAllMocks()
572574
mockResolvePermission.mockResolvedValue('write')
573-
mockResolveContext.mockResolvedValue({
574-
tableId: TABLE.id,
575-
table: TABLE,
576-
workspaceId: TABLE.workspaceId,
577-
workspaceOrganizationId: 'organization-1',
578-
allowPersonalApiKeys: true,
579-
billedAccountUserId: 'billing-owner-1',
580-
})
575+
mockResolveContext.mockResolvedValue(contextFor())
581576
})
582577

583578
it('rejects a malformed POST query cursor before querying storage', async () => {
@@ -831,14 +826,7 @@ describe('table row write secret provenance defaulting', () => {
831826
beforeEach(() => {
832827
vi.clearAllMocks()
833828
mockResolvePermission.mockResolvedValue('write')
834-
mockResolveContext.mockResolvedValue({
835-
tableId: TABLE.id,
836-
table: TABLE,
837-
workspaceId: TABLE.workspaceId,
838-
workspaceOrganizationId: 'organization-1',
839-
allowPersonalApiKeys: true,
840-
billedAccountUserId: 'billing-owner-1',
841-
})
829+
mockResolveContext.mockResolvedValue(contextFor())
842830
mockValidateRowData.mockResolvedValue({ valid: true })
843831
mockValidateBatchRows.mockResolvedValue({ valid: true })
844832
mockInsertRow.mockResolvedValue(ROW)
@@ -898,14 +886,7 @@ describe('table row write secret provenance defaulting', () => {
898886
...TABLE,
899887
schema: { columns: [{ id: 'column_name', name: 'name', type: 'string' }] },
900888
}
901-
mockResolveContext.mockResolvedValue({
902-
tableId: TABLE.id,
903-
table: filterableTable,
904-
workspaceId: TABLE.workspaceId,
905-
workspaceOrganizationId: 'organization-1',
906-
allowPersonalApiKeys: true,
907-
billedAccountUserId: 'billing-owner-1',
908-
})
889+
mockResolveContext.mockResolvedValue(contextFor(filterableTable))
909890

910891
await updateTableRows.execute({
911892
principal: PRINCIPAL,
@@ -995,14 +976,7 @@ describe('unknown column names under strictWrite', () => {
995976
beforeEach(() => {
996977
vi.clearAllMocks()
997978
mockResolvePermission.mockResolvedValue('write')
998-
mockResolveContext.mockResolvedValue({
999-
tableId: TABLE.id,
1000-
table: TABLE,
1001-
workspaceId: TABLE.workspaceId,
1002-
workspaceOrganizationId: 'organization-1',
1003-
allowPersonalApiKeys: true,
1004-
billedAccountUserId: 'billing-owner-1',
1005-
})
979+
mockResolveContext.mockResolvedValue(contextFor())
1006980
mockValidateRowData.mockResolvedValue({ valid: true })
1007981
mockValidateBatchRows.mockResolvedValue({ valid: true })
1008982
mockInsertRow.mockResolvedValue({ id: 'row-1', data: {} })
@@ -1161,14 +1135,7 @@ describe('row data keying', () => {
11611135
beforeEach(() => {
11621136
vi.clearAllMocks()
11631137
mockResolvePermission.mockResolvedValue('write')
1164-
mockResolveContext.mockResolvedValue({
1165-
tableId: TABLE.id,
1166-
table: TABLE,
1167-
workspaceId: TABLE.workspaceId,
1168-
workspaceOrganizationId: 'organization-1',
1169-
allowPersonalApiKeys: true,
1170-
billedAccountUserId: 'billing-owner-1',
1171-
})
1138+
mockResolveContext.mockResolvedValue(contextFor())
11721139
mockAssertRowCapacity.mockResolvedValue(10_000)
11731140
mockCreateSecretProvenance.mockReturnValue({ complete: true, columns: {} })
11741141
mockIsScopeCompatible.mockReturnValue(true)
@@ -1215,6 +1182,31 @@ describe('row data keying', () => {
12151182
)
12161183
})
12171184

1185+
it('persists an unrecognised key on the lax id wire, unlike the name wire', async () => {
1186+
// The asymmetry a non-strict id-keyed caller sees, pinned deliberately: the
1187+
// name path drops what it cannot resolve, the id path stores what it is
1188+
// given. This is what the grid does today via the identity `dataIn` in
1189+
// `row-wire.ts`, so the discriminator preserved it rather than changing it.
1190+
// Closing it is a behaviour change and belongs with the route migration.
1191+
await updateTableRow.execute({
1192+
principal: PRINCIPAL,
1193+
input: {
1194+
tableId: TABLE.id,
1195+
rowId: 'row-1',
1196+
data: { 'column-name': 'Ada', 'no-such-column': 'x' },
1197+
strictWrite: false,
1198+
dataKeying: 'ids',
1199+
},
1200+
})
1201+
1202+
expect(mockUpdateRow).toHaveBeenCalledWith(
1203+
expect.objectContaining({ data: { 'column-name': 'Ada', 'no-such-column': 'x' } }),
1204+
TABLE,
1205+
expect.any(String),
1206+
expect.anything()
1207+
)
1208+
})
1209+
12181210
it('translates a name-keyed write to storage ids', async () => {
12191211
await updateTableRow.execute({
12201212
principal: PRINCIPAL,
@@ -1255,14 +1247,9 @@ describe('row data keying', () => {
12551247
// Two production tables still carry pre-backfill columns with no `id`.
12561248
// Their storage key is the name, so a strict id-keyed write naming one must
12571249
// be accepted, not refused as unknown.
1258-
mockResolveContext.mockResolvedValue({
1259-
tableId: TABLE.id,
1260-
table: { ...TABLE, schema: { columns: [{ name: 'legacy', type: 'string' }] } },
1261-
workspaceId: TABLE.workspaceId,
1262-
workspaceOrganizationId: 'organization-1',
1263-
allowPersonalApiKeys: true,
1264-
billedAccountUserId: 'billing-owner-1',
1265-
})
1250+
mockResolveContext.mockResolvedValue(
1251+
contextFor({ ...TABLE, schema: { columns: [{ name: 'legacy', type: 'string' }] } })
1252+
)
12661253

12671254
await expect(
12681255
updateTableRow.execute({

apps/sim/lib/table/application/rows.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,19 @@ interface TableScopedInput {
8383
requestId?: string
8484
}
8585

86-
/** The write policy `strictWrite` selects, for the row-service primitives. */
86+
/**
87+
* The write policy `strictWrite` selects, for the row-service primitives.
88+
*
89+
* `strictWrite` means the calling surface publishes the stricter `/api/v2` write
90+
* contract: a row naming a column the table does not have is refused rather than
91+
* having that key dropped, and a value the column's type cannot coerce is
92+
* answered with a 400 rather than stored as `null`.
93+
*
94+
* Absent — every first-party surface, and the only behavior any of them has ever
95+
* had: the workspace grid, the internal `/api/table` routes, `/api/v1`, the
96+
* Copilot table tools, and the executor's Table block all drop the unknown key
97+
* and blank the uncoercible cell.
98+
*/
8799
function rowWriteOptions(input: { strictWrite: boolean }): RowWriteOptions {
88100
return input.strictWrite ? { uncoercibleValues: 'reject' } : {}
89101
}
@@ -729,7 +741,16 @@ function projectedRowsSecretProvenance(
729741
})
730742
}
731743

732-
/** Atomically validates name-keyed projected rows against the locked schema and replaces the table. */
744+
/**
745+
* Atomically validates name-keyed projected rows against the locked schema and
746+
* replaces the table.
747+
*
748+
* Deliberately carries no {@link TableRowDataKeying}: unlike the six generic
749+
* write use cases this one is not surface-agnostic. Its resolved-secret gate and
750+
* its "row matches no column" check both compare by `column.name` (see
751+
* {@link projectedRowsForTable}), and its only caller is Copilot's
752+
* `Function.execute` output — keys a model can only have written as names.
753+
*/
733754
export const replaceProjectedWireRows = defineAuthorizedTableUseCase({
734755
operation: tableOperations.replaceRows,
735756
resolveContext: ({ input }: { input: ReplaceProjectedWireRowsInput }) =>

apps/sim/lib/table/column-keys.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,11 @@ export function rowDataNameToId(data: RowData, idByName: Map<string, string>): R
207207
* row, and letting it through would reinstate the silent drop for exactly the
208208
* callers most likely to believe they had written something.
209209
*/
210-
export function unknownColumnNames(data: RowData, idByName: ReadonlyMap<string, string>): string[] {
211-
return Object.keys(data).filter((name) => !idByName.has(name))
210+
export function unknownColumnNames(
211+
data: RowData,
212+
knownKeys: ReadonlyMap<string, unknown>
213+
): string[] {
214+
return Object.keys(data).filter((key) => !knownKeys.has(key))
212215
}
213216

214217
/**

0 commit comments

Comments
 (0)