From e8f17b2a2488d9f1809087b0619b02a83e67ad8d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:06:52 -0700 Subject: [PATCH 01/12] test(table): characterize the single-row route before migrating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-row surface (GET/PATCH/DELETE) had no route-level tests despite being the hottest table write path. Pin the behavior it emits today so the migration onto the shared internal route builder is verifiable rather than hopeful. Covers status codes, body shapes, ISO-8601 timestamp serialization, the access level each method demands, collaborator invocation, and the dual-caller wire keying — session callers pass column ids through untouched while internal-JWT callers translate names to ids in both directions. Verified to fail: mutating the wire translator, the deleted count, and the workspace-ownership guard each turn the corresponding tests red. --- .../[tableId]/rows/[rowId]/route.test.ts | 411 ++++++++++++++++++ 1 file changed, 411 insertions(+) create mode 100644 apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts new file mode 100644 index 00000000000..414dc8d903e --- /dev/null +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts @@ -0,0 +1,411 @@ +/** + * @vitest-environment node + * + * Characterization tests for the single-row surface. + * + * These pin the wire behavior this route emits TODAY — status codes, body + * shapes, date serialization, and which collaborators are invoked — so the + * route can be migrated onto the shared internal route builder without + * silently changing what clients observe. They intentionally assert the + * existing contract rather than an idealized one. + */ +import { hybridAuthMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TableDefinition } from '@/lib/table' + +const { + mockCheckAccess, + mockUpdateRow, + mockPerformDeleteTableRow, + mockSignalTableRowsChangedByActor, +} = vi.hoisted(() => ({ + mockCheckAccess: vi.fn(), + mockUpdateRow: vi.fn(), + mockPerformDeleteTableRow: vi.fn(), + mockSignalTableRowsChangedByActor: vi.fn(), +})) + +vi.mock('@/app/api/table/utils', async () => { + const { NextResponse } = await import('next/server') + return { + checkAccess: mockCheckAccess, + accessError: (result: { status: number }) => + NextResponse.json({ error: 'Access denied' }, { status: result.status }), + orchestrationErrorResponse: (error: unknown) => + (error as { __orchestrated?: boolean })?.__orchestrated + ? NextResponse.json({ error: 'Orchestration failed' }, { status: 409 }) + : null, + orchestrationOutcomeErrorResponse: (_outcome: unknown, message: string) => + NextResponse.json({ error: message }, { status: 400 }), + tableLockErrorResponse: (error: unknown) => + (error as { __locked?: boolean })?.__locked + ? NextResponse.json({ error: 'Table is locked' }, { status: 423 }) + : null, + } +}) + +vi.mock('@/lib/table', async () => { + const columnKeys = await import('@/lib/table/column-keys') + return { ...columnKeys, updateRow: mockUpdateRow } +}) + +vi.mock('@/lib/table/orchestration', () => ({ + performDeleteTableRow: mockPerformDeleteTableRow, +})) + +vi.mock('@/lib/table/events', () => ({ + signalTableRowsChangedByActor: mockSignalTableRowsChangedByActor, +})) + +import { DELETE, GET, PATCH } from '@/app/api/table/[tableId]/rows/[rowId]/route' + +const TABLE_ID = 'tbl_1' +const ROW_ID = 'row_1' +const WORKSPACE_ID = 'workspace-1' +const CREATED_AT = new Date('2024-01-01T00:00:00.000Z') +const UPDATED_AT = new Date('2024-02-02T00:00:00.000Z') + +function buildTable(overrides: Partial = {}): TableDefinition { + return { + id: TABLE_ID, + name: 'People', + description: null, + schema: { + columns: [ + { id: 'col_aaa', name: 'Name', type: 'string' }, + { id: 'col_bbb', name: 'Age', type: 'number' }, + ], + }, + metadata: null, + rowCount: 0, + maxRows: 100, + workspaceId: WORKSPACE_ID, + createdBy: 'user-1', + archivedAt: null, + createdAt: CREATED_AT, + updatedAt: UPDATED_AT, + ...overrides, + } as TableDefinition +} + +function buildStoredRow() { + return { + id: ROW_ID, + data: { col_aaa: 'Ada', col_bbb: 36 }, + position: 0, + createdAt: CREATED_AT, + updatedAt: UPDATED_AT, + } +} + +function authAs(authType: 'session' | 'internal_jwt' = 'session') { + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType, + }) +} + +function unauthenticated() { + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: false }) +} + +function routeContext() { + return { params: Promise.resolve({ tableId: TABLE_ID, rowId: ROW_ID }) } +} + +function getRequest(workspaceId: string | null = WORKSPACE_ID) { + const url = new URL(`http://localhost/api/table/${TABLE_ID}/rows/${ROW_ID}`) + if (workspaceId !== null) url.searchParams.set('workspaceId', workspaceId) + return new NextRequest(url, { method: 'GET' }) +} + +function bodyRequest(method: 'PATCH' | 'DELETE', body: unknown) { + return new NextRequest(`http://localhost/api/table/${TABLE_ID}/rows/${ROW_ID}`, { + method, + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }) +} + +beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + authAs() + mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) +}) + +describe('GET /api/table/[tableId]/rows/[rowId]', () => { + it('returns 401 when the caller is not authenticated', async () => { + unauthenticated() + + const response = await GET(getRequest(), routeContext()) + + expect(response.status).toBe(401) + await expect(response.json()).resolves.toEqual({ error: 'Authentication required' }) + expect(mockCheckAccess).not.toHaveBeenCalled() + }) + + it('returns 400 when workspaceId is absent from the query string', async () => { + const response = await GET(getRequest(null), routeContext()) + + expect(response.status).toBe(400) + expect(mockCheckAccess).not.toHaveBeenCalled() + }) + + it('propagates the access decision when the caller lacks read access', async () => { + mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) + + const response = await GET(getRequest(), routeContext()) + + expect(response.status).toBe(403) + expect(mockCheckAccess).toHaveBeenCalledWith(TABLE_ID, 'user-1', 'read') + }) + + it('returns 400 when the asserted workspace does not own the table', async () => { + const response = await GET(getRequest('workspace-other'), routeContext()) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ error: 'Invalid workspace ID' }) + }) + + it('returns 404 when the row does not exist', async () => { + queueTableRows(schemaMock.userTableRows, []) + + const response = await GET(getRequest(), routeContext()) + + expect(response.status).toBe(404) + await expect(response.json()).resolves.toEqual({ error: 'Row not found' }) + }) + + it('returns the row with ISO-8601 timestamps under data.row', async () => { + queueTableRows(schemaMock.userTableRows, [buildStoredRow()]) + + const response = await GET(getRequest(), routeContext()) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ + success: true, + data: { + row: { + id: ROW_ID, + data: { col_aaa: 'Ada', col_bbb: 36 }, + position: 0, + createdAt: CREATED_AT.toISOString(), + updatedAt: UPDATED_AT.toISOString(), + }, + }, + }) + }) +}) + +describe('PATCH /api/table/[tableId]/rows/[rowId]', () => { + const patchBody = { workspaceId: WORKSPACE_ID, data: { col_aaa: 'Grace' } } + + beforeEach(() => { + mockUpdateRow.mockResolvedValue({ + ...buildStoredRow(), + data: { col_aaa: 'Grace', col_bbb: 36 }, + }) + }) + + it('returns 401 when the caller is not authenticated', async () => { + unauthenticated() + + const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(response.status).toBe(401) + expect(mockUpdateRow).not.toHaveBeenCalled() + }) + + it('returns 400 when the body fails contract validation', async () => { + const response = await PATCH( + bodyRequest('PATCH', { workspaceId: WORKSPACE_ID }), + routeContext() + ) + + expect(response.status).toBe(400) + expect(mockUpdateRow).not.toHaveBeenCalled() + }) + + it('requires write access, not read access', async () => { + mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) + + const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(response.status).toBe(403) + expect(mockCheckAccess).toHaveBeenCalledWith(TABLE_ID, 'user-1', 'write') + }) + + it('returns 400 when the asserted workspace does not own the table', async () => { + const response = await PATCH( + bodyRequest('PATCH', { ...patchBody, workspaceId: 'workspace-other' }), + routeContext() + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ error: 'Invalid workspace ID' }) + expect(mockUpdateRow).not.toHaveBeenCalled() + }) + + it('returns the updated row and the success message', async () => { + const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ + success: true, + data: { + row: { + id: ROW_ID, + data: { col_aaa: 'Grace', col_bbb: 36 }, + position: 0, + createdAt: CREATED_AT.toISOString(), + updatedAt: UPDATED_AT.toISOString(), + }, + message: 'Row updated successfully', + }, + }) + }) + + it('passes the acting user and the column-keyed patch to updateRow', async () => { + await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(mockUpdateRow).toHaveBeenCalledTimes(1) + const [input, table] = mockUpdateRow.mock.calls[0] + expect(input).toMatchObject({ + tableId: TABLE_ID, + rowId: ROW_ID, + workspaceId: WORKSPACE_ID, + actorUserId: 'user-1', + data: { col_aaa: 'Grace' }, + }) + expect(table.id).toBe(TABLE_ID) + }) + + it('translates column names to ids for an internal JWT caller', async () => { + authAs('internal_jwt') + + await PATCH( + bodyRequest('PATCH', { workspaceId: WORKSPACE_ID, data: { Name: 'Grace' } }), + routeContext() + ) + + expect(mockUpdateRow.mock.calls[0][0]).toMatchObject({ data: { col_aaa: 'Grace' } }) + }) + + it('returns column names to an internal JWT caller', async () => { + authAs('internal_jwt') + + const response = await PATCH( + bodyRequest('PATCH', { workspaceId: WORKSPACE_ID, data: { Name: 'Grace' } }), + routeContext() + ) + + const body = await response.json() + expect(body.data.row.data).toEqual({ Name: 'Grace', Age: 36 }) + }) + + it('signals open collaborators that the row changed', async () => { + await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(mockSignalTableRowsChangedByActor).toHaveBeenCalledWith(TABLE_ID, undefined) + }) + + it('forwards the originating tab id so that tab ignores its own broadcast', async () => { + const request = new NextRequest(`http://localhost/api/table/${TABLE_ID}/rows/${ROW_ID}`, { + method: 'PATCH', + headers: { 'content-type': 'application/json', 'x-sim-client-id': 'tab-42' }, + body: JSON.stringify(patchBody), + }) + + await PATCH(request, routeContext()) + + expect(mockSignalTableRowsChangedByActor).toHaveBeenCalledWith(TABLE_ID, 'tab-42') + }) + + it('projects a classified orchestration failure instead of a generic 500', async () => { + mockUpdateRow.mockRejectedValue(Object.assign(new Error('conflict'), { __orchestrated: true })) + + const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(response.status).toBe(409) + }) + + it('falls back to 500 for an unclassified failure', async () => { + mockUpdateRow.mockRejectedValue(new Error('boom')) + + const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) + + expect(response.status).toBe(500) + await expect(response.json()).resolves.toEqual({ error: 'Failed to update row' }) + }) +}) + +describe('DELETE /api/table/[tableId]/rows/[rowId]', () => { + const deleteBody = { workspaceId: WORKSPACE_ID } + + beforeEach(() => { + mockPerformDeleteTableRow.mockResolvedValue({ success: true }) + }) + + it('returns 401 when the caller is not authenticated', async () => { + unauthenticated() + + const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + + expect(response.status).toBe(401) + expect(mockPerformDeleteTableRow).not.toHaveBeenCalled() + }) + + it('requires write access', async () => { + mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) + + const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + + expect(response.status).toBe(403) + expect(mockCheckAccess).toHaveBeenCalledWith(TABLE_ID, 'user-1', 'write') + }) + + it('returns 400 when the asserted workspace does not own the table', async () => { + const response = await DELETE( + bodyRequest('DELETE', { workspaceId: 'workspace-other' }), + routeContext() + ) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ error: 'Invalid workspace ID' }) + expect(mockPerformDeleteTableRow).not.toHaveBeenCalled() + }) + + it('reports a deleted count of one on success', async () => { + const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ + success: true, + data: { message: 'Row deleted successfully', deletedCount: 1 }, + }) + expect(mockSignalTableRowsChangedByActor).toHaveBeenCalledWith(TABLE_ID, undefined) + }) + + it('projects an unsuccessful delete outcome as a client error', async () => { + mockPerformDeleteTableRow.mockResolvedValue({ success: false }) + + const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ error: 'Failed to delete row' }) + expect(mockSignalTableRowsChangedByActor).not.toHaveBeenCalled() + }) + + it('projects a table lock failure ahead of the generic handler', async () => { + mockPerformDeleteTableRow.mockRejectedValue( + Object.assign(new Error('locked'), { __locked: true }) + ) + + const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + + expect(response.status).toBe(423) + }) +}) From fc43edc061b35514ead7d312cc0dc23a0cb096af Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:19:26 -0700 Subject: [PATCH 02/12] feat(table): model wire keying and actor attribution on row write use cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The row write use cases assumed every caller speaks column names. That holds for /api/v2, /api/v1 and the Copilot tools, but not for the first-party grid or the internal /api/table routes, which address cells by stable storage id. Feeding id-keyed data through the name remap drops every key it does not recognise — a storage id names no column name — so the write would store nothing and still report success. Make the wire an explicit, required property of the input rather than an assumption. `dataKeying: 'names' | 'ids'` sits alongside `strictWrite` and is required for the same reason: a new write surface must state which contract it publishes. Strictness now means the same thing on either wire — an unknown column id is refused exactly as an unknown column name already was. Single-row writes also gain optional actor attribution, so the acting tab can skip refetching its own write. It is optional and absent by default, so every existing caller keeps broadcasting to all subscribers as before. Only the single-row create, update and delete paths accept it; a batch write is not reconciled locally by the actor and must still refetch. The attribution pin moves with the behaviour: what selects the actor-scoped signal is no longer which file calls it but which surface supplies an actor, so that is now what the test pins. Verified to fail: ignoring the keying discriminator, and dropping actor attribution, each turn the corresponding tests red. --- .../[tableId]/rows/[rowId]/route.test.ts | 1 + .../v2/tables/[tableId]/rows/[rowId]/route.ts | 1 + .../v2/tables/[tableId]/rows/route.test.ts | 2 + .../app/api/v2/tables/[tableId]/rows/route.ts | 3 + .../[tableId]/rows/upsert/route.test.ts | 1 + .../v2/tables/[tableId]/rows/upsert/route.ts | 1 + .../copilot/tools/server/table/user-table.ts | 3 + apps/sim/lib/table/application/rows.test.ts | 240 +++++++++++++++++- apps/sim/lib/table/application/rows.ts | 138 ++++++++-- apps/sim/lib/table/events.attribution.test.ts | 54 +++- 10 files changed, 408 insertions(+), 36 deletions(-) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts index 387473a3935..08e8a510620 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts @@ -132,6 +132,7 @@ describe('/api/v2/tables/[tableId]/rows/[rowId]', () => { assertedWorkspaceId: WORKSPACE_ID, data: { name: 'Ada' }, strictWrite: true, + dataKeying: 'names', }, request: req, }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts index 465577e099a..fe3ce419375 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts @@ -42,6 +42,7 @@ export const PATCH = defineV2JsonRoute({ assertedWorkspaceId: body.workspaceId, data: body.data, strictWrite: true, + dataKeying: 'names' as const, }), useCase: updateTableRow, present: ({ table, row }) => ({ diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts index 1f0b8a3b5e7..bb5ccea0a76 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts @@ -190,6 +190,7 @@ describe('/api/v2/tables/[tableId]/rows', () => { // or a value the column cannot hold is a 400, not a dropped key or a // nulled cell. Every first-party surface leaves this unset. strictWrite: true, + dataKeying: 'names', }, request: single, }) @@ -207,6 +208,7 @@ describe('/api/v2/tables/[tableId]/rows', () => { assertedWorkspaceId: WORKSPACE_ID, rows: [{ name: 'Ada' }], strictWrite: true, + dataKeying: 'names', }, request: batch, }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts index 4727a7bba6a..1c5382064d8 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts @@ -71,6 +71,7 @@ export const POST = defineV2JsonRoute({ assertedWorkspaceId: body.workspaceId, rows: body.rows, strictWrite: true, + dataKeying: 'names' as const, } : { kind: 'single' as const, @@ -80,6 +81,7 @@ export const POST = defineV2JsonRoute({ afterRowId: body.afterRowId, beforeRowId: body.beforeRowId, strictWrite: true, + dataKeying: 'names' as const, }, useCase: createTableRows, present: (result) => { @@ -108,6 +110,7 @@ export const PATCH = defineV2JsonRoute({ data: body.data, limit: body.limit, strictWrite: true, + dataKeying: 'names' as const, }), useCase: updateTableRows, present: ({ affectedCount, affectedRowIds }) => ({ diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.test.ts index e17f2760632..29451b0db62 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.test.ts @@ -103,6 +103,7 @@ describe('POST /api/v2/tables/[tableId]/rows/upsert', () => { data: { email: 'ada@example.com' }, conflictTarget: 'email', strictWrite: true, + dataKeying: 'names', }, request, }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts index 26550374d43..ff66079c7d5 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts @@ -21,6 +21,7 @@ export const POST = defineV2JsonRoute({ data: body.data, conflictTarget: body.conflictTarget, strictWrite: true, + dataKeying: 'names' as const, }), useCase: upsertTableRow, present: ({ table, row, operation }) => ({ diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.ts b/apps/sim/lib/copilot/tools/server/table/user-table.ts index da3fee1d8ce..c43fca7fafc 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.ts @@ -286,6 +286,7 @@ export const userTableServerTool: BaseServerTool tableId: args.tableId, assertedWorkspaceId: workspaceId, strictWrite: false, + dataKeying: 'names', data: args.data, position: args.position as number | undefined, secretProvenance: createExactEmptyTableRowSecretProvenance(args.data), @@ -329,6 +330,7 @@ export const userTableServerTool: BaseServerTool tableId: args.tableId, assertedWorkspaceId: workspaceId, strictWrite: false, + dataKeying: 'names', rows: sourceRows, secretProvenance: sourceRows.map(createExactEmptyTableRowSecretProvenance), }, @@ -476,6 +478,7 @@ export const userTableServerTool: BaseServerTool tableId: args.tableId, assertedWorkspaceId: workspaceId, strictWrite: false, + dataKeying: 'names', rowId: args.rowId, data: args.data, secretProvenance: createExactEmptyTableRowSecretProvenance(args.data), diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index 2f8c9325562..7b6dc6f8ef9 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -19,6 +19,7 @@ const { mockResolveContext, mockResolvePermission, mockSignalRowsChanged, + mockSignalRowsChangedByActor, mockUpsertRow, mockWithLockedTable, mockInsertRow, @@ -41,6 +42,7 @@ const { mockResolveContext: vi.fn(), mockResolvePermission: vi.fn(), mockSignalRowsChanged: vi.fn(), + mockSignalRowsChangedByActor: vi.fn(), mockUpsertRow: vi.fn(), mockWithLockedTable: vi.fn(), mockInsertRow: vi.fn(), @@ -137,10 +139,12 @@ vi.mock('@/lib/table/application/context', () => ({ vi.mock('@/lib/table/events', () => ({ signalTableRowsChanged: mockSignalRowsChanged, + signalTableRowsChangedByActor: mockSignalRowsChangedByActor, })) import { createTableRows, + deleteTableRow, deleteTableRows, listTableRows, ProjectedWireRowsValidationError, @@ -504,7 +508,12 @@ describe('replaceTableRows application use case', () => { await expect( replaceTableRows.execute({ principal: PRINCIPAL, - input: { tableId: TABLE.id, rows: [{ name: 'Ada', unknown: 'x' }], strictWrite: true }, + input: { + tableId: TABLE.id, + rows: [{ name: 'Ada', unknown: 'x' }], + strictWrite: true, + dataKeying: 'names', + }, }) ).rejects.toThrow(/Row 1: Unknown column: unknown/) expect(mockReplaceRowsPrimitive).not.toHaveBeenCalled() @@ -792,6 +801,7 @@ describe('row query and upsert application semantics', () => { requestId: 'request-1', data: { name: 'Ada' }, conflictTarget: 'name', + dataKeying: 'names', }, }) @@ -1010,7 +1020,13 @@ describe('unknown column names under strictWrite', () => { await expect( createTableRows.execute({ principal: PRINCIPAL, - input: { kind: 'single', tableId: TABLE.id, data: { nosuchcol: 'x' }, strictWrite: true }, + input: { + kind: 'single', + tableId: TABLE.id, + data: { nosuchcol: 'x' }, + strictWrite: true, + dataKeying: 'names', + }, }) ).rejects.toThrow(/Unknown column: nosuchcol/) expect(mockInsertRow).not.toHaveBeenCalled() @@ -1040,6 +1056,7 @@ describe('unknown column names under strictWrite', () => { tableId: TABLE.id, data: { zzz: 'x', qqq: 'y' }, strictWrite: true, + dataKeying: 'names', }, }) ).rejects.toThrow(/Unknown columns: zzz, qqq/) @@ -1054,6 +1071,7 @@ describe('unknown column names under strictWrite', () => { tableId: TABLE.id, rows: [{ name: 'Ada' }, { zzz: 'x' }], strictWrite: true, + dataKeying: 'names', }, }) ).rejects.toThrow(/Row 2: Unknown column: zzz/) @@ -1069,6 +1087,7 @@ describe('unknown column names under strictWrite', () => { filter: { all: [{ field: 'name', op: 'eq', value: 'Ada' }] }, data: { zzz: 'x' }, strictWrite: true, + dataKeying: 'names', }, }) ).rejects.toThrow(/Unknown column: zzz/) @@ -1079,7 +1098,13 @@ describe('unknown column names under strictWrite', () => { await expect( updateTableRow.execute({ principal: PRINCIPAL, - input: { tableId: TABLE.id, rowId: 'row-1', data: { zzz: 'x' }, strictWrite: true }, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { zzz: 'x' }, + strictWrite: true, + dataKeying: 'names', + }, }) ).rejects.toThrow(/Unknown column: zzz/) expect(mockUpdateRow).not.toHaveBeenCalled() @@ -1107,7 +1132,13 @@ describe('unknown column names under strictWrite', () => { it('carries the strict value policy to the primitive, and nothing without it', async () => { await createTableRows.execute({ principal: PRINCIPAL, - input: { kind: 'single', tableId: TABLE.id, data: { name: 'Ada' }, strictWrite: true }, + input: { + kind: 'single', + tableId: TABLE.id, + data: { name: 'Ada' }, + strictWrite: true, + dataKeying: 'names', + }, }) expect(mockInsertRow).toHaveBeenLastCalledWith(expect.anything(), TABLE, expect.any(String), { uncoercibleValues: 'reject', @@ -1120,3 +1151,204 @@ describe('unknown column names under strictWrite', () => { expect(mockInsertRow).toHaveBeenLastCalledWith(expect.anything(), TABLE, expect.any(String), {}) }) }) + +/** + * The two wires a table write can arrive on. `/api/v2`, `/api/v1` and the + * Copilot tools publish column names; the first-party grid and the internal + * `/api/table` routes publish stable storage ids. + * + * The failure this guards is silent: the name remap drops what it does not + * recognise, and a storage id names no column *name*, so an id-keyed write sent + * down the name path stores nothing while reporting success. + */ +describe('row data keying', () => { + beforeEach(() => { + vi.clearAllMocks() + mockResolvePermission.mockResolvedValue('write') + mockResolveContext.mockResolvedValue({ + tableId: TABLE.id, + table: TABLE, + workspaceId: TABLE.workspaceId, + workspaceOrganizationId: 'organization-1', + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + mockAssertRowCapacity.mockResolvedValue(10_000) + mockCreateSecretProvenance.mockReturnValue({ complete: true, columns: {} }) + mockIsScopeCompatible.mockReturnValue(true) + }) + + it('stores an id-keyed write exactly as given', async () => { + await updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { 'column-name': 'Ada' }, + strictWrite: false, + dataKeying: 'ids', + }, + }) + + expect(mockUpdateRow).toHaveBeenCalledWith( + expect.objectContaining({ data: { 'column-name': 'Ada' } }), + TABLE, + expect.any(String), + expect.anything() + ) + }) + + it('does not silently drop an id-keyed write, which the name path would', async () => { + await updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { 'column-name': 'Ada' }, + strictWrite: false, + dataKeying: 'names', + }, + }) + + // Pins the hazard itself: the same payload on the name wire stores nothing. + expect(mockUpdateRow).toHaveBeenCalledWith( + expect.objectContaining({ data: {} }), + TABLE, + expect.any(String), + expect.anything() + ) + }) + + it('translates a name-keyed write to storage ids', async () => { + await updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { name: 'Ada' }, + strictWrite: false, + dataKeying: 'names', + }, + }) + + expect(mockUpdateRow).toHaveBeenCalledWith( + expect.objectContaining({ data: { 'column-name': 'Ada' } }), + TABLE, + expect.any(String), + expect.anything() + ) + }) + + it('refuses an unknown column id when the caller writes strictly', async () => { + await expect( + updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { 'column-nope': 'x' }, + strictWrite: true, + dataKeying: 'ids', + }, + }) + ).rejects.toThrow(/Unknown column: column-nope/) + expect(mockUpdateRow).not.toHaveBeenCalled() + }) + + it('names every unknown id at once, as the name wire does', async () => { + await expect( + createTableRows.execute({ + principal: PRINCIPAL, + input: { + kind: 'batch', + tableId: TABLE.id, + rows: [{ 'column-name': 'Ada' }, { zzz: 'x', qqq: 'y' }], + strictWrite: true, + dataKeying: 'ids', + }, + }) + ).rejects.toThrow(/Row 2: Unknown columns: zzz, qqq/) + }) +}) + +/** + * Naming the acting tab lets that tab skip refetching its own write. Only the + * single-row paths accept an actor — see `events.attribution.test.ts` for why, + * and for the pinned list of surfaces allowed to supply one. + */ +describe('row change attribution', () => { + beforeEach(() => { + vi.clearAllMocks() + mockResolvePermission.mockResolvedValue('write') + mockResolveContext.mockResolvedValue({ + tableId: TABLE.id, + table: TABLE, + workspaceId: TABLE.workspaceId, + workspaceOrganizationId: 'organization-1', + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + mockAssertRowCapacity.mockResolvedValue(10_000) + mockCreateSecretProvenance.mockReturnValue({ complete: true, columns: {} }) + mockIsScopeCompatible.mockReturnValue(true) + }) + + it('names the acting tab on a single-row update', async () => { + await updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { name: 'Ada' }, + strictWrite: false, + dataKeying: 'names', + actorClientId: 'tab-42', + }, + }) + + expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, 'tab-42') + expect(mockSignalRowsChanged).not.toHaveBeenCalled() + }) + + it('broadcasts to everyone when the surface cannot name a tab', async () => { + await updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { name: 'Ada' }, + strictWrite: false, + dataKeying: 'names', + }, + }) + + expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, undefined) + }) + + it('names the acting tab on a single-row delete', async () => { + await deleteTableRow.execute({ + principal: PRINCIPAL, + input: { tableId: TABLE.id, rowId: 'row-1', actorClientId: 'tab-42' }, + }) + + expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, 'tab-42') + }) + + it('broadcasts a batch insert to everyone even though a tab is named', async () => { + await createTableRows.execute({ + principal: PRINCIPAL, + input: { + kind: 'batch', + tableId: TABLE.id, + rows: [{ name: 'Ada' }, { name: 'Grace' }], + strictWrite: false, + dataKeying: 'names', + }, + }) + + // A batch write is not reconciled locally by the acting tab, so it must + // refetch like everyone else. + expect(mockSignalRowsChanged).toHaveBeenCalledWith(TABLE.id) + expect(mockSignalRowsChangedByActor).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index 5185c9b0c52..f06587e419e 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -46,7 +46,7 @@ import { assertRowCapacity, notifyTableRowUsage } from '@/lib/table/billing' import { buildIdByName, unknownColumnNames } from '@/lib/table/column-keys' import { columnTypeOf } from '@/lib/table/column-types' import { TableQueryValidationError } from '@/lib/table/errors' -import { signalTableRowsChanged } from '@/lib/table/events' +import { signalTableRowsChanged, signalTableRowsChangedByActor } from '@/lib/table/events' import { predicateToFilter } from '@/lib/table/query-builder/converters' import { validatePredicate, @@ -155,22 +155,75 @@ function assertKnownColumnNames( ) } -function namedDataToStorage(data: RowData, table: TableDefinition, strict = false): RowData { +/** + * Which column keying a write's row data arrives in. + * + * `'names'` — the caller publishes column **names** on its wire: `/api/v2`, + * `/api/v1`, and the Copilot table tools, where a name is what the caller (or + * the model) can read off a row. Names are remapped to storage ids here. + * + * `'ids'` — the caller already speaks stable storage column **ids**: the + * first-party grid and the internal `/api/table` routes, which hold the schema + * they rendered and address cells by id. + * + * Required so a new write surface must state which contract it publishes. The + * name remap drops keys it does not recognise, and a storage id names no column + * *name*, so feeding id-keyed data through the name path silently drops every + * cell and reports the write as successful. + */ +export type TableRowDataKeying = 'names' | 'ids' + +/** + * Refuses a wire row naming a column id the table does not have. The id-keyed + * counterpart of {@link assertKnownColumnNames}, and applied on the same + * `strictWrite` condition, so strictness means the same thing on either wire. + */ +function assertKnownColumnIds(data: RowData, table: TableDefinition, rowLabel?: string): void { + const ids = new Set(table.schema.columns.map((column) => column.id)) + const unknown = Object.keys(data).filter((key) => !ids.has(key)) + if (unknown.length === 0) return + const where = rowLabel ? `${rowLabel}: ` : '' + throw new TableRowsValidationError( + `${where}Unknown column${unknown.length > 1 ? 's' : ''}: ${unknown.join(', ')}` + ) +} + +/** + * Normalizes one wire row to storage keying. See {@link TableRowDataKeying}. + */ +function rowDataToStorage( + data: RowData, + table: TableDefinition, + keying: TableRowDataKeying, + strict = false, + rowLabel?: string +): RowData { + if (keying === 'ids') { + if (strict) assertKnownColumnIds(data, table, rowLabel) + return data + } const idByName = buildIdByName(table.schema) - if (strict) assertKnownColumnNames(data, idByName) + if (strict) assertKnownColumnNames(data, idByName, rowLabel) return rowDataNameToId(data, idByName) } /** - * {@link namedDataToStorage} over a batch. The name index is built once for the + * {@link rowDataToStorage} over a batch. The name index is built once for the * whole batch rather than per row — these paths run over up to * `MAX_BATCH_INSERT_SIZE` rows. */ -function namedRowsToStorage( +function rowsToStorage( rows: readonly RowData[], table: TableDefinition, + keying: TableRowDataKeying, strict = false ): RowData[] { + if (keying === 'ids') { + return rows.map((row, index) => { + if (strict) assertKnownColumnIds(row, table, `Row ${index + 1}`) + return row + }) + } const idByName = buildIdByName(table.schema) return rows.map((row, index) => { if (strict) assertKnownColumnNames(row, idByName, `Row ${index + 1}`) @@ -420,7 +473,17 @@ export const readTableRow = defineAuthorizedTableUseCase({ interface CreateSingleTableRowInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean + /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ + dataKeying: TableRowDataKeying kind: 'single' + /** + * Tab that caused this write, when the calling surface knows it. Lets that + * tab skip its own refetch — see {@link signalTableRowsChangedByActor}, whose + * soundness condition (the caller's hook reconciles the write locally across + * every cached rows query) is why only the single-row paths accept this. + * Omitted by every surface that cannot name a tab, which broadcasts to all. + */ + actorClientId?: string data: RowData position?: number afterRowId?: string @@ -431,6 +494,8 @@ interface CreateSingleTableRowInput extends TableScopedInput { interface CreateBatchTableRowsInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean + /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ + dataKeying: TableRowDataKeying kind: 'batch' rows: RowData[] orderKeys?: string[] @@ -458,7 +523,7 @@ export const createTableRows = defineAuthorizedTableUseCase({ ) { throw new TableRowsValidationError('Position must be 0 or greater') } - const data = namedDataToStorage(input.data, context.table, input.strictWrite) + const data = rowDataToStorage(input.data, context.table, input.dataKeying, input.strictWrite) const writeOptions = rowWriteOptions(input) await throwValidationResponse( await validateRowData({ @@ -496,7 +561,7 @@ export const createTableRows = defineAuthorizedTableUseCase({ if (input.orderKeys && input.orderKeys.length !== input.rows.length) { throw new TableRowsValidationError('orderKeys must align one-to-one with rows') } - const rows = namedRowsToStorage(input.rows, context.table, input.strictWrite) + const rows = rowsToStorage(input.rows, context.table, input.dataKeying, input.strictWrite) const batchWriteOptions = rowWriteOptions(input) await throwValidationResponse( await validateBatchRows({ @@ -521,9 +586,16 @@ export const createTableRows = defineAuthorizedTableUseCase({ ) return { kind: 'batch', table: context.table, rows: created } }, - afterSuccess: ({ context, result }) => { + afterSuccess: ({ context, input, result }) => { const affected = result.kind === 'single' ? 1 : result.rows.length - if (affected > 0) signalTableRowsChanged(context.tableId) + if (affected === 0) return + // Only the single-row path carries an actor; a batch insert genuinely needs + // the originating tab to refetch, so it broadcasts to everyone. + if (input.kind === 'single') { + signalTableRowsChangedByActor(context.tableId, input.actorClientId) + return + } + signalTableRowsChanged(context.tableId) }, }) @@ -532,6 +604,8 @@ const MAX_REPLACE_TABLE_ROWS = 10_000 export interface ReplaceTableRowsInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean + /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ + dataKeying: TableRowDataKeying rows: RowData[] secretProvenance?: Array } @@ -551,7 +625,7 @@ export const replaceTableRows = defineAuthorizedTableUseCase({ throw new TableRowsValidationError('Secret provenance must align one-to-one with rows') } - const rows = namedRowsToStorage(input.rows, context.table, input.strictWrite) + const rows = rowsToStorage(input.rows, context.table, input.dataKeying, input.strictWrite) const result = await replaceTableRowsPrimitive( { tableId: context.tableId, @@ -742,10 +816,20 @@ export const replaceProjectedWireRows = defineAuthorizedTableUseCase({ export interface UpdateTableRowInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean + /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ + dataKeying: TableRowDataKeying rowId: string data: RowData secretProvenance?: TableRowSecretProvenanceWrite includePersistedSecretProvenance?: boolean + /** + * Tab that caused this write, when the calling surface knows it. Lets that + * tab skip its own refetch — see {@link signalTableRowsChangedByActor}, whose + * soundness condition (the caller's hook reconciles the write locally across + * every cached rows query) is why only the single-row paths accept this. + * Omitted by every surface that cannot name a tab, which broadcasts to all. + */ + actorClientId?: string } export interface UpdateTableRowResult extends TableResult { @@ -758,7 +842,7 @@ export const updateTableRow = defineAuthorizedTableUseCase({ operation: tableOperations.updateRow, resolveContext: ({ input }: { input: UpdateTableRowInput }) => resolveActiveTableContext(input), async execute({ principal, input, context }): Promise { - const data = namedDataToStorage(input.data, context.table, input.strictWrite) + const data = rowDataToStorage(input.data, context.table, input.dataKeying, input.strictWrite) const row = await updateRow( { tableId: context.tableId, @@ -785,14 +869,16 @@ export const updateTableRow = defineAuthorizedTableUseCase({ ), } }, - afterSuccess: ({ context, result }) => { - if (result.changed) signalTableRowsChanged(context.tableId) + afterSuccess: ({ context, input, result }) => { + if (result.changed) signalTableRowsChangedByActor(context.tableId, input.actorClientId) }, }) export interface UpdateTableRowsInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean + /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ + dataKeying: TableRowDataKeying filter: TablePredicate data: RowData limit?: number @@ -809,7 +895,7 @@ export const updateTableRows = defineAuthorizedTableUseCase({ if (input.limit !== undefined) { requireIntegerInRange(input.limit, 1, TABLE_LIMITS.MAX_BULK_OPERATION_SIZE, 'Limit') } - const data = namedDataToStorage(input.data, context.table, input.strictWrite) + const data = rowDataToStorage(input.data, context.table, input.dataKeying, input.strictWrite) const result = await updateRowsByFilter( context.table, { @@ -834,6 +920,14 @@ export const updateTableRows = defineAuthorizedTableUseCase({ export interface DeleteTableRowInput extends TableScopedInput { rowId: string + /** + * Tab that caused this write, when the calling surface knows it. Lets that + * tab skip its own refetch — see {@link signalTableRowsChangedByActor}, whose + * soundness condition (the caller's hook reconciles the write locally across + * every cached rows query) is why only the single-row paths accept this. + * Omitted by every surface that cannot name a tab, which broadcasts to all. + */ + actorClientId?: string } export interface DeleteTableRowResult extends TableResult { @@ -847,7 +941,8 @@ export const deleteTableRow = defineAuthorizedTableUseCase({ await deleteRow(context.table, input.rowId, requestId(input)) return { table: context.table, deletedRowId: input.rowId } }, - afterSuccess: ({ context }) => signalTableRowsChanged(context.tableId), + afterSuccess: ({ context, input }) => + signalTableRowsChangedByActor(context.tableId, input.actorClientId), }) export type DeleteTableRowsInput = TableScopedInput & @@ -918,6 +1013,8 @@ export const deleteTableRows = defineAuthorizedTableUseCase({ export interface UpsertTableRowInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean + /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ + dataKeying: TableRowDataKeying data: RowData conflictTarget?: string secretProvenance?: TableRowSecretProvenanceWrite @@ -932,10 +1029,13 @@ export const upsertTableRow = defineAuthorizedTableUseCase({ operation: tableOperations.upsertRow, resolveContext: ({ input }: { input: UpsertTableRowInput }) => resolveActiveTableContext(input), async execute({ principal, input, context }): Promise { - const conflictTarget = input.conflictTarget - ? (buildIdByName(context.table.schema).get(input.conflictTarget) ?? input.conflictTarget) - : undefined - const data = namedDataToStorage(input.data, context.table, input.strictWrite) + // An id-keyed caller already names the storage column; only a name-keyed + // one needs the lookup, and its miss falls through as before. + const conflictTarget = + input.conflictTarget && input.dataKeying !== 'ids' + ? (buildIdByName(context.table.schema).get(input.conflictTarget) ?? input.conflictTarget) + : input.conflictTarget + const data = rowDataToStorage(input.data, context.table, input.dataKeying, input.strictWrite) const result = await upsertRow( { tableId: context.tableId, diff --git a/apps/sim/lib/table/events.attribution.test.ts b/apps/sim/lib/table/events.attribution.test.ts index e0f8c9ee448..2f96c90ca52 100644 --- a/apps/sim/lib/table/events.attribution.test.ts +++ b/apps/sim/lib/table/events.attribution.test.ts @@ -9,16 +9,26 @@ import { describe, expect, it } from 'vitest' * `signalTableRowsChangedByActor` lets the acting tab skip its own refetch, which is only sound * where that tab's mutation hook already applies the server's answer to every cached rows query. * That invariant lives in `hooks/queries/tables.ts` — nothing in the type system ties it to the - * call site, so a well-meaning fourth call would silently strand that client on stale rows. + * call site, so a well-meaning extra call would silently strand that client on stale rows. * - * This pins the allowlist. If you are here because it failed: adding a call means proving the - * calling route's client hook reconciles locally, then adding it below. Removing one is always safe. + * Two lists are pinned, because the single-row paths now signal from inside their application use + * case rather than from the route. The call itself is no longer the decision: the use case is + * shared with `/api/v2` and Copilot, and it degrades to a broadcast whenever no actor is named. + * What actually selects the behavior is which surface supplies `actorClientId`, so that is pinned + * too and is the list to scrutinise. + * + * If you are here because it failed: adding a supplier means proving that surface's client hook + * reconciles the write locally across every cached rows query. Removing one is always safe. */ const ATTRIBUTED_CALL_SITES = [ 'app/api/table/[tableId]/rows/route.ts', 'app/api/table/[tableId]/rows/[rowId]/route.ts', + 'lib/table/application/rows.ts', ] as const +/** Surfaces that name the acting tab. See the note above — this is the real allowlist. */ +const ACTOR_SUPPLYING_SURFACES = [] as const + const APP_ROOT = join(import.meta.dirname, '../..') /** Declares the function; matching its own definition would say nothing about call sites. */ const DECLARING_MODULE = 'lib/table/events.ts' @@ -32,17 +42,35 @@ async function* walk(dir: string): AsyncGenerator { } } +async function filesContaining(needle: string, skip: (relative: string) => boolean = () => false) { + const found: string[] = [] + for await (const file of walk(APP_ROOT)) { + const source = await readFile(file, 'utf8') + if (!source.includes(needle)) continue + const relative = file.slice(APP_ROOT.length + 1) + if (skip(relative)) continue + found.push(relative) + } + return found.sort() +} + describe('signalTableRowsChangedByActor call sites', () => { it('is called only where the acting tab reconciles the write locally', async () => { - const callers: string[] = [] - for await (const file of walk(APP_ROOT)) { - const source = await readFile(file, 'utf8') - if (!source.includes('signalTableRowsChangedByActor(')) continue - const relative = file.slice(APP_ROOT.length + 1) - if (relative === DECLARING_MODULE) continue - callers.push(relative) - } - - expect(callers.sort()).toEqual([...ATTRIBUTED_CALL_SITES].sort()) + const callers = await filesContaining( + 'signalTableRowsChangedByActor(', + (relative) => relative === DECLARING_MODULE + ) + + expect(callers).toEqual([...ATTRIBUTED_CALL_SITES].sort()) + }) + + it('is given an actor only by surfaces whose client hook reconciles locally', async () => { + const suppliers = await filesContaining( + 'actorClientId:', + // Declares the field rather than supplying one. + (relative) => relative === 'lib/table/application/rows.ts' + ) + + expect(suppliers).toEqual([...ACTOR_SUPPLYING_SURFACES].sort()) }) }) From 1dbecd4a61f68fc3d8021a54fabab96eaf846097 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:34:32 -0700 Subject: [PATCH 03/12] perf(table): remove two round trips from every single-row update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single-cell PATCH spends far more time in sequential round trips to a remote Postgres than in the UPDATE it issues. Prepared statements are disabled for PgBouncer transaction mode, so every await is a full Parse/Bind/Execute. Two of them were avoidable. getRowById issued the row lookup and its executions sidecar in series, but the sidecar is keyed on the row id the caller already supplied, so it never depended on the lookup. Issuing both together makes it one round trip. A miss now pays one redundant sidecar read, which is the rare path and costs no extra wall time. The uniqueness probe ran whenever the table had any unique column, passing the fully merged row, so editing an unrelated cell re-probed every unique column — its own transaction plus one query per column. It is now scoped to the columns the patch actually writes. A merge cannot newly violate uniqueness on a column it leaves alone: that value is the one already stored, and it satisfied the constraint when it was written. Sized before changing: few tables declare a unique column, but write traffic concentrates in the ones that do, so this is the larger of the two savings in practice. Verified to fail: reverting the probe scoping turns the covering test red. --- apps/sim/lib/api/contracts/tables.ts | 16 ++++ .../lib/table/__tests__/update-row.test.ts | 95 +++++++++++++++++++ apps/sim/lib/table/rows/service.ts | 44 ++++++--- 3 files changed, 141 insertions(+), 14 deletions(-) diff --git a/apps/sim/lib/api/contracts/tables.ts b/apps/sim/lib/api/contracts/tables.ts index 0ed44ed4543..9cd4217c5f7 100644 --- a/apps/sim/lib/api/contracts/tables.ts +++ b/apps/sim/lib/api/contracts/tables.ts @@ -1433,6 +1433,22 @@ export const upsertTableRowContract = defineRouteContract({ }, }) +/** + * Reads one row. The sibling of {@link updateTableRowContract} and + * {@link deleteTableRowContract}, which take their workspace scope from a body; + * a GET has none, so it is asserted on the query string instead. + */ +export const getTableRowContract = defineRouteContract({ + method: 'GET', + path: '/api/table/[tableId]/rows/[rowId]', + params: tableRowParamsSchema, + query: getTableQuerySchema, + response: { + mode: 'json', + schema: successResponseSchema(z.object({ row: tableRowSchema })), + }, +}) + export const updateTableRowContract = defineRouteContract({ method: 'PATCH', path: '/api/table/[tableId]/rows/[rowId]', diff --git a/apps/sim/lib/table/__tests__/update-row.test.ts b/apps/sim/lib/table/__tests__/update-row.test.ts index cc2ed28df81..172a5e03c10 100644 --- a/apps/sim/lib/table/__tests__/update-row.test.ts +++ b/apps/sim/lib/table/__tests__/update-row.test.ts @@ -548,3 +548,98 @@ describe('batchUpdateRows — per-row partial merge', () => { expect(values).not.toContain(JSON.stringify({ name: 'Alice', age: 31 })) }) }) + +/** + * The uniqueness probe opens its own transaction and queries once per unique + * column, so on a table that has any unique column it used to cost several + * round trips on every edit — including edits nowhere near one. It is now + * scoped to the columns the patch actually writes. + * + * The safety argument is that a merge cannot newly violate uniqueness on a + * column it leaves alone: that value is the one already stored, and it + * satisfied the constraint when it was written. + */ +describe('updateRow — uniqueness probe scoping', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + dbChainMockFns.limit.mockResolvedValue([EXISTING_ROW]) + dbChainMockFns.returning.mockResolvedValue([ + { id: EXISTING_ROW.id, updatedAt: PERSISTED_UPDATED_AT }, + ]) + }) + + it('does not probe when the patch touches no unique column', async () => { + const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') + vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) + + await updateRow( + { tableId: 'tbl-1', rowId: 'row-1', data: { age: 31 }, workspaceId: 'ws-1' }, + TABLE, + 'req-1' + ) + + expect(checkUniqueConstraintsDb).not.toHaveBeenCalled() + }) + + it('still probes when the patch touches a unique column', async () => { + const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') + vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) + + await updateRow( + { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, + TABLE, + 'req-1' + ) + + expect(checkUniqueConstraintsDb).toHaveBeenCalledTimes(1) + }) + + it('probes against the merged row, so the excluded row is still the one being edited', async () => { + const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') + vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) + + await updateRow( + { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, + TABLE, + 'req-1' + ) + + expect(checkUniqueConstraintsDb).toHaveBeenCalledWith( + 'tbl-1', + { name: 'Grace', age: 30 }, + TABLE.schema, + 'row-1' + ) + }) + + it('surfaces a duplicate on a column the patch does write', async () => { + const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') + vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) + vi.mocked(checkUniqueConstraintsDb).mockResolvedValueOnce({ + valid: false, + errors: ['Duplicate value for name'], + }) + + await expect( + updateRow( + { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, + TABLE, + 'req-1' + ) + ).rejects.toThrow(/Duplicate value for name/) + }) + + it('does not probe on a table with no unique columns at all', async () => { + const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') + vi.mocked(getUniqueColumns).mockReturnValue([]) + + await updateRow( + { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, + TABLE, + 'req-1' + ) + + expect(checkUniqueConstraintsDb).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index 4ed64ae8824..bfc4f80f1ac 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1422,22 +1422,28 @@ export async function getRowById( rowId: string, workspaceId: string ): Promise { - const results = await db - .select() - .from(userTableRows) - .where( - and( - eq(userTableRows.id, rowId), - eq(userTableRows.tableId, tableId), - eq(userTableRows.workspaceId, workspaceId) + // The executions sidecar is keyed on the row id the caller already gave us, so + // it does not depend on the row lookup — issuing both together makes this one + // round trip instead of two. A miss pays one redundant sidecar read, which is + // the rare path and costs no extra wall time. + const [results, executions] = await Promise.all([ + db + .select() + .from(userTableRows) + .where( + and( + eq(userTableRows.id, rowId), + eq(userTableRows.tableId, tableId), + eq(userTableRows.workspaceId, workspaceId) + ) ) - ) - .limit(1) + .limit(1), + loadExecutionsForRow(db, rowId), + ]) if (results.length === 0) return null const row = results[0] - const executions = await loadExecutionsForRow(db, row.id) return { id: row.id, data: row.data as RowData, @@ -1604,9 +1610,19 @@ export async function updateRow( ) } - // Check unique constraints using optimized database query - const uniqueColumns = getUniqueColumns(table.schema) - if (uniqueColumns.length > 0) { + // Check unique constraints using optimized database query. + // + // Scoped to the columns this patch actually writes. A merge cannot newly + // violate uniqueness on a column it leaves alone: that value is the one + // already stored, and it satisfied the constraint when it was written. The + // probe opens its own transaction and queries once per unique column, so on a + // table that has any unique column this was several round trips on every + // edit, including edits nowhere near one. + const patchedColumnIds = new Set(Object.keys(data.data)) + const patchedUniqueColumns = getUniqueColumns(table.schema).filter((column) => + patchedColumnIds.has(getColumnId(column)) + ) + if (patchedUniqueColumns.length > 0) { const uniqueValidation = await checkUniqueConstraintsDb( data.tableId, mergedData, From 80ce6ae3b75003eac8fb96e8ca5aaff7a1794f2b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:43:32 -0700 Subject: [PATCH 04/12] perf(table): start the workspace load with the table load when a workspace is asserted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveActiveTableContext ran two sequential round trips: load the table, then load the workspace it turned out to live in. When the caller asserts a workspace the second input is already in hand, so both can start together. What makes that safe is unchanged: requireTable still compares the table's canonical workspaceId against the assertion and reports a mismatch as not_found. The table outcome is inspected first and unconditionally, so any path that returns has proven the assertion equal to the canonical id, and a failing workspace load can never replace the concealing not_found. A final identity check on the loaded context restates the invariant at the point of return, so even with the first check removed the function cannot hand back a foreign workspace. Promise.allSettled keeps the discarded branch from surfacing as an unhandled rejection. With no asserted workspace the path stays sequential — the table load is what reveals which workspace to load, so there is nothing to start early. One existing assertion changed: it required the workspace load not to have been issued yet on a mismatched assertion, which is internal sequencing rather than caller-observable behaviour and is definitionally untrue once the loads overlap. The observable half is kept, and two timing tests now cover the sequencing directly. Verified to fail: removing either mismatch check, reading the workspace outcome first, and swapping allSettled for bare awaits each turn the corresponding tests red. --- .../sim/lib/table/application/context.test.ts | 162 +++++++++++++++++- apps/sim/lib/table/application/context.ts | 41 ++++- 2 files changed, 193 insertions(+), 10 deletions(-) diff --git a/apps/sim/lib/table/application/context.test.ts b/apps/sim/lib/table/application/context.test.ts index d1b8ff08a66..62d07678516 100644 --- a/apps/sim/lib/table/application/context.test.ts +++ b/apps/sim/lib/table/application/context.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { getTableById, loadWorkspace } = vi.hoisted(() => ({ getTableById: vi.fn(), @@ -16,6 +16,37 @@ vi.mock('@/lib/workspaces/application/workspace-context', () => ({ import { resolveActiveTableContext } from '@/lib/table/application/context' +const WORKSPACE_ONE = { + workspaceId: 'workspace-1', + workspaceOrganizationId: 'organization-1', + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-user-1', +} + +const WORKSPACE_TWO = { + workspaceId: 'workspace-2', + workspaceOrganizationId: 'organization-2', + allowPersonalApiKeys: false, + billedAccountUserId: 'billing-user-2', +} + +/** Runs `body` while capturing any unhandled promise rejection it provokes. */ +async function withUnhandledRejectionWatch(body: () => Promise): Promise { + const seen: unknown[] = [] + const onUnhandled = (reason: unknown) => { + seen.push(reason) + } + process.on('unhandledRejection', onUnhandled) + try { + await body() + await new Promise((resolve) => setImmediate(resolve)) + await new Promise((resolve) => setImmediate(resolve)) + } finally { + process.off('unhandledRejection', onUnhandled) + } + return seen +} + describe('table application context', () => { beforeEach(() => { vi.clearAllMocks() @@ -24,12 +55,13 @@ describe('table application context', () => { workspaceId: 'workspace-1', name: 'Contacts', }) - loadWorkspace.mockResolvedValue({ - workspaceId: 'workspace-1', - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-user-1', - }) + loadWorkspace.mockImplementation(async (workspaceId: string) => + workspaceId === 'workspace-1' ? WORKSPACE_ONE : WORKSPACE_TWO + ) + }) + + afterEach(() => { + vi.useRealTimers() }) it('derives workspace scope from the canonical active table', async () => { @@ -44,13 +76,106 @@ describe('table application context', () => { expect(loadWorkspace).toHaveBeenCalledWith('workspace-1') }) - it('conceals an asserted cross-workspace table before workspace resolution', async () => { + it('starts the workspace load without waiting for the table when a workspace is asserted', async () => { + let releaseTable: (table: unknown) => void = () => {} + getTableById.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseTable = resolve + }) + ) + + const pending = resolveActiveTableContext({ + tableId: 'table-1', + assertedWorkspaceId: 'workspace-1', + }) + await Promise.resolve() + await Promise.resolve() + + expect(loadWorkspace).toHaveBeenCalledWith('workspace-1') + + releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' }) + await expect(pending).resolves.toMatchObject({ tableId: 'table-1', workspaceId: 'workspace-1' }) + }) + + it('waits for the table before loading a workspace when none is asserted', async () => { + let releaseTable: (table: unknown) => void = () => {} + getTableById.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseTable = resolve + }) + ) + + const pending = resolveActiveTableContext({ tableId: 'table-1' }) + await Promise.resolve() + await Promise.resolve() + + expect(loadWorkspace).not.toHaveBeenCalled() + + releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' }) + await expect(pending).resolves.toMatchObject({ tableId: 'table-1', workspaceId: 'workspace-1' }) + expect(loadWorkspace).toHaveBeenCalledWith('workspace-1') + }) + + it('conceals an asserted cross-workspace table as not found', async () => { await expect( resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-2' }) ).rejects.toMatchObject({ code: 'not_found', message: 'Table not found' }) + }) + + it('conceals a table that does not exist at all', async () => { + getTableById.mockResolvedValueOnce(null) + + await expect( + resolveActiveTableContext({ tableId: 'missing', assertedWorkspaceId: 'workspace-1' }) + ).rejects.toMatchObject({ code: 'not_found', message: 'Table not found' }) + }) + + it('conceals a missing table with no asserted workspace', async () => { + getTableById.mockResolvedValueOnce(null) + + await expect(resolveActiveTableContext({ tableId: 'missing' })).rejects.toMatchObject({ + code: 'not_found', + message: 'Table not found', + }) expect(loadWorkspace).not.toHaveBeenCalled() }) + it('surfaces not_found rather than a failing workspace load on a mismatched assertion', async () => { + const failure = new Error('workspace database unavailable') + loadWorkspace.mockRejectedValueOnce(failure) + + const unhandled = await withUnhandledRejectionWatch(async () => { + await expect( + resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-2' }) + ).rejects.toMatchObject({ code: 'not_found', message: 'Table not found' }) + }) + + expect(unhandled).toEqual([]) + }) + + it('surfaces not_found rather than a failing table load on a matched assertion', async () => { + const failure = new Error('table database unavailable') + getTableById.mockRejectedValueOnce(failure) + + const unhandled = await withUnhandledRejectionWatch(async () => { + await expect( + resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-1' }) + ).rejects.toBe(failure) + }) + + expect(unhandled).toEqual([]) + }) + + it('refuses a workspace context that is not the canonical workspace of the table', async () => { + loadWorkspace.mockResolvedValueOnce(WORKSPACE_TWO) + + await expect( + resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-1' }) + ).rejects.toMatchObject({ code: 'not_found', message: 'Table not found' }) + }) + it('fails when the canonical workspace is unavailable', async () => { loadWorkspace.mockResolvedValueOnce(null) @@ -60,10 +185,31 @@ describe('table application context', () => { }) }) + it('fails when the canonical workspace is unavailable on the asserted path', async () => { + loadWorkspace.mockResolvedValueOnce(null) + + await expect( + resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-1' }) + ).rejects.toMatchObject({ code: 'not_found', message: 'Workspace not found' }) + }) + it('propagates canonical workspace database failures', async () => { const failure = new Error('workspace database unavailable') loadWorkspace.mockRejectedValueOnce(failure) await expect(resolveActiveTableContext({ tableId: 'table-1' })).rejects.toBe(failure) }) + + it('propagates canonical workspace database failures on the asserted path', async () => { + const failure = new Error('workspace database unavailable') + loadWorkspace.mockRejectedValueOnce(failure) + + const unhandled = await withUnhandledRejectionWatch(async () => { + await expect( + resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-1' }) + ).rejects.toBe(failure) + }) + + expect(unhandled).toEqual([]) + }) }) diff --git a/apps/sim/lib/table/application/context.ts b/apps/sim/lib/table/application/context.ts index 71e71de49b9..7bedaa7bfaa 100644 --- a/apps/sim/lib/table/application/context.ts +++ b/apps/sim/lib/table/application/context.ts @@ -32,12 +32,49 @@ async function requireTable(tableId: string, workspaceId: string | undefined) { return table } +/** + * Loads the canonical context a table use case authorizes against. + * + * When the caller asserts a workspace, the workspace load no longer has to wait for the table + * load: the asserted id is already in hand, so both round trips start together. What makes that + * safe is that {@link requireTable} still compares the table's canonical `workspaceId` against + * the assertion and reports any mismatch as `not_found`. The table outcome is inspected first and + * unconditionally, so on every path that returns, the assertion has been *proven* equal to the + * canonical workspace id — the context handed back is the table's own workspace, never a + * workspace the caller merely named. A mismatch throws before the workspace outcome is read, so a + * failing workspace load can never replace the concealing `not_found`. `Promise.allSettled` keeps + * the branch that is thrown away from surfacing as an unhandled rejection. A final identity check + * on the loaded context restates the invariant at the point of return, so the value handed back is + * only ever a context whose own `workspaceId` is the table's. + * + * Without an asserted id there is nothing to start early — the table load is what reveals which + * workspace to load — so that path stays sequential. + */ export async function resolveActiveTableContext(input: { tableId: string assertedWorkspaceId?: string }): Promise { - const table = await requireTable(input.tableId, input.assertedWorkspaceId) - const workspaceContext = await resolveTableWorkspaceContext(table.workspaceId) + const { tableId, assertedWorkspaceId } = input + + if (assertedWorkspaceId === undefined) { + const table = await requireTable(tableId, undefined) + const workspaceContext = await resolveTableWorkspaceContext(table.workspaceId) + return { ...workspaceContext, tableId: table.id, table } + } + + const [tableOutcome, workspaceOutcome] = await Promise.allSettled([ + requireTable(tableId, assertedWorkspaceId), + resolveTableWorkspaceContext(assertedWorkspaceId), + ]) + + if (tableOutcome.status === 'rejected') throw tableOutcome.reason + if (workspaceOutcome.status === 'rejected') throw workspaceOutcome.reason + + const table = tableOutcome.value + const workspaceContext = workspaceOutcome.value + if (workspaceContext.workspaceId !== table.workspaceId) { + throw new OrchestrationError('not_found', 'Table not found') + } return { ...workspaceContext, tableId: table.id, table } } From d77dbba36dfbaff15b9e5296480ca860c637d601 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:45:42 -0700 Subject: [PATCH 05/12] perf(table): read a table and its latest job in one round trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getTableById issued the table SELECT and then awaited latestJobForTable, so every table request paid two sequential round trips. With prepared statements disabled for PgBouncer transaction mode every await is a full round trip, and this loader is on essentially every table route. The job read cannot be skipped: a table's reported rowCount is the stored count minus the job's pendingDeleteRemaining, so dropping it would overstate the count during a pending delete and could wrongly reject inserts as over capacity. An opt-out flag would have made that a caller's trap. Instead the job is read in the same statement, as a correlated jsonb subquery in the select list — the select-list form of a LEFT JOIN LATERAL, which is what drizzle can type here. Output is unchanged for every input. latestJobForTable is deleted rather than left dangling: getTableById was its only caller, and keeping it would have carried a third copy of the exports-excluded / newest-started_at / limit-one rule. mapJobRow is now exported so the batch path and the lateral share one implementation of the doomedCount and pendingDeleteRemaining logic. The batch DISTINCT ON path used by the list endpoint is untouched. Verified to fail: dropping the export filter, reversing or re-keying the sort, dropping the limit, dropping the correlation, loosening either doomedCount condition, and removing the rowCount subtraction each turn tests red. Dropping the lateral from the projection initially survived, because the shared db mock returns queued rows regardless of predicate; a projection assertion now covers it. --- apps/sim/lib/api/list-convention.test.ts | 3 +- apps/sim/lib/table/jobs/service.test.ts | 110 ++++++++++++++ apps/sim/lib/table/jobs/service.ts | 76 ++++++---- apps/sim/lib/table/service.test.ts | 181 ++++++++++++++++++++++- apps/sim/lib/table/service.ts | 16 +- 5 files changed, 352 insertions(+), 34 deletions(-) create mode 100644 apps/sim/lib/table/jobs/service.test.ts diff --git a/apps/sim/lib/api/list-convention.test.ts b/apps/sim/lib/api/list-convention.test.ts index 77453b972c1..255f6edd28b 100644 --- a/apps/sim/lib/api/list-convention.test.ts +++ b/apps/sim/lib/api/list-convention.test.ts @@ -37,7 +37,8 @@ vi.mock('@/lib/table/billing', () => ({ })) vi.mock('@/lib/table/jobs/service', () => ({ EMPTY_JOB_FIELDS: {}, - latestJobForTable: vi.fn(async () => null), + latestNonExportJobJson: vi.fn(() => null), + mapJobRow: vi.fn(() => ({})), latestJobsForTables: vi.fn(async () => new Map()), })) vi.mock('@/lib/table/events', () => ({ appendTableEvent: vi.fn() })) diff --git a/apps/sim/lib/table/jobs/service.test.ts b/apps/sim/lib/table/jobs/service.test.ts new file mode 100644 index 00000000000..a34efbdbbbe --- /dev/null +++ b/apps/sim/lib/table/jobs/service.test.ts @@ -0,0 +1,110 @@ +/** + * @vitest-environment node + */ +import { schemaMock } from '@sim/testing' +import { describe, expect, it } from 'vitest' +import { + EMPTY_JOB_FIELDS, + type LatestJobRow, + latestNonExportJobJson, + mapJobRow, +} from '@/lib/table/jobs/service' + +function job(overrides: Partial): LatestJobRow { + return { + id: 'job-1', + type: 'delete', + status: 'running', + rowsProcessed: 0, + error: null, + payload: null, + ...overrides, + } +} + +describe('mapJobRow', () => { + it('returns the empty fields when the table has no job row', () => { + expect(mapJobRow(null)).toEqual(EMPTY_JOB_FIELDS) + expect(mapJobRow(undefined)).toEqual(EMPTY_JOB_FIELDS) + }) + + it('projects a running delete job and its remaining doomed rows', () => { + expect(mapJobRow(job({ rowsProcessed: 4, payload: { doomedCount: 10 } }))).toEqual({ + jobStatus: 'running', + jobId: 'job-1', + jobType: 'delete', + jobError: null, + jobRowsProcessed: 4, + pendingDeleteRemaining: 6, + }) + }) + + it('ignores doomedCount once the delete job is terminal', () => { + expect( + mapJobRow(job({ status: 'ready', rowsProcessed: 4, payload: { doomedCount: 10 } })) + .pendingDeleteRemaining + ).toBe(0) + }) + + it('ignores doomedCount for a running job that is not a delete', () => { + expect( + mapJobRow(job({ type: 'import', rowsProcessed: 4, payload: { doomedCount: 10 } })) + .pendingDeleteRemaining + ).toBe(0) + }) + + it('treats a missing doomedCount as zero and never goes negative', () => { + expect(mapJobRow(job({ rowsProcessed: 4 })).pendingDeleteRemaining).toBe(0) + expect( + mapJobRow(job({ rowsProcessed: 25, payload: { doomedCount: 10 } })).pendingDeleteRemaining + ).toBe(0) + }) + + it('carries a failed job error through', () => { + expect(mapJobRow(job({ status: 'failed', error: 'boom' }))).toMatchObject({ + jobStatus: 'failed', + jobError: 'boom', + }) + }) +}) + +/** + * The lateral is a raw `sql` fragment, so the mocked drizzle `sql` tag is the only + * place its text is observable — and the text IS the contract (`getTableById` would + * otherwise silently return a different job than `latestJobsForTables` does). + */ +function renderLateral(): { text: string; values: unknown[] } { + // double-cast-allowed: the mocked drizzle `sql` tag exposes the raw template parts + const fragment = latestNonExportJobJson(schemaMock.userTableDefinitions.id) as unknown as { + strings: string[] + values: unknown[] + } + return { text: fragment.strings.join(' ? ').replace(/\s+/g, ' '), values: fragment.values } +} + +describe('latestNonExportJobJson', () => { + it('excludes export jobs', () => { + expect(renderLateral().text).toContain("<> 'export'") + }) + + it('takes the single newest job by started_at', () => { + const { text, values } = renderLateral() + expect(text).toContain('order by ? desc') + expect(text).toContain('limit 1') + expect(values).toContain(schemaMock.tableJobs.startedAt) + }) + + it('correlates the subquery to the outer table id', () => { + const { text, values } = renderLateral() + expect(text).toContain('where ? = ?') + expect(values).toContain(schemaMock.tableJobs.tableId) + expect(values).toContain(schemaMock.userTableDefinitions.id) + }) + + it('projects every field mapJobRow reads', () => { + const { text } = renderLateral() + for (const key of ['id', 'type', 'status', 'rowsProcessed', 'error', 'payload']) { + expect(text).toContain(`'${key}',`) + } + }) +}) diff --git a/apps/sim/lib/table/jobs/service.ts b/apps/sim/lib/table/jobs/service.ts index c5f16114f8b..367e4b2a6ce 100644 --- a/apps/sim/lib/table/jobs/service.ts +++ b/apps/sim/lib/table/jobs/service.ts @@ -13,8 +13,8 @@ import { db } from '@sim/db' import { tableJobs, userTableDefinitions, userTableRows } from '@sim/db/schema' +import type { Column, SQL } from 'drizzle-orm' import { and, asc, desc, eq, gt, inArray, ne, or, sql } from 'drizzle-orm' -import type { DbOrTx } from '@/lib/db/types' import { pendingDeleteMask } from '@/lib/table/rows/pending-delete-mask' import type { RowData, @@ -25,7 +25,7 @@ import type { } from '@/lib/table/types' /** Job fields projected onto a {@link TableDefinition}, derived from its latest `table_jobs` row. */ -interface DerivedJobFields { +export interface DerivedJobFields { jobStatus: TableDefinition['jobStatus'] jobId: string | null jobType: TableDefinition['jobType'] @@ -49,18 +49,22 @@ export const EMPTY_JOB_FIELDS: DerivedJobFields = { pendingDeleteRemaining: 0, } -function mapJobRow( - row: - | { - id: string - type: string - status: string - rowsProcessed: number - error: string | null - payload: unknown - } - | undefined -): DerivedJobFields { +/** + * The shape every latest-job read produces, whether it comes back as query columns + * (the batch `DISTINCT ON`) or as one jsonb object (the correlated lateral folded + * into the table SELECT). The single source of truth for the doomed-count rule is + * {@link mapJobRow} — never re-derive `pendingDeleteRemaining` at a call site. + */ +export interface LatestJobRow { + id: string + type: string + status: string + rowsProcessed: number + error: string | null + payload: unknown +} + +export function mapJobRow(row: LatestJobRow | null | undefined): DerivedJobFields { if (!row) return EMPTY_JOB_FIELDS const doomedCount = row.type === 'delete' && row.status === 'running' @@ -86,22 +90,36 @@ const JOB_PROJECTION = { } as const /** - * The latest job for one table (the running one if present, else the most recent terminal). - * Exports are excluded: they're read-only, run concurrently with other jobs, and have their own - * client surface — surfacing one here would clobber the import/delete/backfill status the tray - * and SSE consumer derive from these fields. + * The latest non-export job for one table, as a single jsonb value correlated to + * `outerTableId` — i.e. a `LEFT JOIN LATERAL (... LIMIT 1) ON true` expressed in the + * select list, which is the form drizzle can type without `leftJoinLateral`. + * + * It exists so {@link getTableById} stays ONE database round trip. With prepared + * statements disabled (PgBouncer transaction mode) every extra `await` is a full + * round trip, and `getTableById` is on essentially every table request. The job row + * cannot simply be skipped: a table's reported `rowCount` is the stored count minus + * this job's `pendingDeleteRemaining`, so the count and the job row are one read. + * + * Semantics match the batch {@link latestJobsForTables} exactly — exports excluded + * (they run concurrently and have their own client surface), newest `started_at` + * first, one row. `NULL` when the table has no such job; feed the result straight to + * {@link mapJobRow}. */ -export async function latestJobForTable( - tableId: string, - executor: DbOrTx = db -): Promise { - const [row] = await executor - .select(JOB_PROJECTION) - .from(tableJobs) - .where(and(eq(tableJobs.tableId, tableId), ne(tableJobs.type, 'export'))) - .orderBy(desc(tableJobs.startedAt)) - .limit(1) - return mapJobRow(row) +export function latestNonExportJobJson(outerTableId: Column | SQL): SQL { + return sql`( + select jsonb_build_object( + 'id', ${tableJobs.id}, + 'type', ${tableJobs.type}, + 'status', ${tableJobs.status}, + 'rowsProcessed', ${tableJobs.rowsProcessed}, + 'error', ${tableJobs.error}, + 'payload', ${tableJobs.payload} + ) + from ${tableJobs} + where ${tableJobs.tableId} = ${outerTableId} and ${tableJobs.type} <> 'export' + order by ${tableJobs.startedAt} desc + limit 1 + )` } /** Latest non-export job per table for a batch of ids, via `DISTINCT ON (table_id)`. */ diff --git a/apps/sim/lib/table/service.test.ts b/apps/sim/lib/table/service.test.ts index 5774caea7ab..f7b276a2ed7 100644 --- a/apps/sim/lib/table/service.test.ts +++ b/apps/sim/lib/table/service.test.ts @@ -1,8 +1,15 @@ /** * @vitest-environment node */ -import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { + dbChainMockFns, + hasMockCondition, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { DbOrTx } from '@/lib/db/types' import type { TableSchema } from '@/lib/table/types' vi.mock('@/lib/realtime/notify', () => ({ @@ -14,7 +21,7 @@ vi.mock('@/lib/table/billing', () => ({ notifyTableRowUsage: vi.fn(), })) -import { createTable } from '@/lib/table/service' +import { createTable, getTableById } from '@/lib/table/service' const WORKSPACE_ID = '6fc7631d-88cd-46f8-9f0a-d4764daef7f8' @@ -96,3 +103,173 @@ describe('createTable schema invariants', () => { expect(dbChainMockFns.insert).toHaveBeenCalled() }) }) + +const TABLE_ID = '0f2b1a4a-1e0e-4b4a-9a0f-0a2b3c4d5e6f' + +/** A `user_table_definitions` row as the folded SELECT returns it. */ +function definitionRow(overrides: Record = {}) { + return { + id: TABLE_ID, + name: 'contacts', + description: null, + schema: { columns: [{ id: 'col_email', name: 'email', type: 'string' }] }, + metadata: null, + maxRows: 10000, + workspaceId: WORKSPACE_ID, + folderId: null, + createdBy: 'user-1', + archivedAt: null, + createdAt: new Date('2026-01-01T00:00:00Z'), + updatedAt: new Date('2026-01-02T00:00:00Z'), + rowCount: 100, + latestJob: null, + schemaLocked: false, + insertLocked: false, + updateLocked: false, + deleteLocked: false, + ...overrides, + } +} + +/** + * The job row is folded into the table SELECT as a lateral, so these cover both that + * one query still carries every job field and that `rowCount` stays adjusted by a + * running delete — the reason the two reads cannot be split apart. + */ +describe('getTableById job derivation', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + it('reads the table and its latest job in a single query', async () => { + queueTableRows(schemaMock.userTableDefinitions, [definitionRow()]) + + const table = await getTableById(TABLE_ID) + + expect(table).toMatchObject({ id: TABLE_ID, rowCount: 100 }) + expect(table).toMatchObject({ + jobStatus: null, + jobId: null, + jobType: null, + jobError: null, + jobRowsProcessed: 0, + }) + expect(table).not.toHaveProperty('pendingDeleteRemaining') + expect(dbChainMockFns.select).toHaveBeenCalledTimes(1) + // double-cast-allowed: the mocked drizzle `sql` tag exposes the raw template parts + const projected = dbChainMockFns.select.mock.calls[0][0] as unknown as { + latestJob?: { strings: string[]; values: unknown[] } + } + expect(projected.latestJob?.strings.join(' ? ')).toContain("<> 'export'") + expect(projected.latestJob?.values).toContain(schemaMock.userTableDefinitions.id) + }) + + it("reduces rowCount by a running delete job's remaining doomed rows", async () => { + queueTableRows(schemaMock.userTableDefinitions, [ + definitionRow({ + latestJob: { + id: 'job-1', + type: 'delete', + status: 'running', + rowsProcessed: 4, + error: null, + payload: { doomedCount: 10 }, + }, + }), + ]) + + const table = await getTableById(TABLE_ID) + + expect(table).toMatchObject({ + rowCount: 94, + jobId: 'job-1', + jobType: 'delete', + jobStatus: 'running', + jobRowsProcessed: 4, + }) + }) + + it('leaves rowCount alone for a running job that is not a delete', async () => { + queueTableRows(schemaMock.userTableDefinitions, [ + definitionRow({ + latestJob: { + id: 'job-2', + type: 'import', + status: 'running', + rowsProcessed: 4, + error: null, + payload: { doomedCount: 10 }, + }, + }), + ]) + + expect(await getTableById(TABLE_ID)).toMatchObject({ rowCount: 100, jobType: 'import' }) + }) + + it('leaves rowCount alone once the delete job is terminal', async () => { + queueTableRows(schemaMock.userTableDefinitions, [ + definitionRow({ + latestJob: { + id: 'job-3', + type: 'delete', + status: 'ready', + rowsProcessed: 4, + error: null, + payload: { doomedCount: 10 }, + }, + }), + ]) + + expect(await getTableById(TABLE_ID)).toMatchObject({ rowCount: 100, jobStatus: 'ready' }) + }) + + it('filters out archived tables unless includeArchived is set', async () => { + queueTableRows(schemaMock.userTableDefinitions, [definitionRow()]) + await getTableById(TABLE_ID) + expect( + hasMockCondition( + dbChainMockFns.where.mock.calls[0][0], + (node) => + node.type === 'isNull' && node.column === schemaMock.userTableDefinitions.archivedAt + ) + ).toBe(true) + + const archivedAt = new Date('2026-01-03T00:00:00Z') + queueTableRows(schemaMock.userTableDefinitions, [definitionRow({ archivedAt })]) + const archived = await getTableById(TABLE_ID, { includeArchived: true }) + + expect(archived).toMatchObject({ archivedAt }) + expect( + hasMockCondition( + dbChainMockFns.where.mock.calls[1][0], + (node) => + node.type === 'isNull' && node.column === schemaMock.userTableDefinitions.archivedAt + ) + ).toBe(false) + }) + + it('runs the single query on a supplied transaction executor', async () => { + const limit = vi.fn().mockResolvedValue([ + definitionRow({ + latestJob: { + id: 'job-4', + type: 'delete', + status: 'running', + rowsProcessed: 1, + error: null, + payload: { doomedCount: 5 }, + }, + }), + ]) + const select = vi.fn(() => ({ from: () => ({ where: () => ({ limit }) }) })) + const tx = { select } as unknown as DbOrTx + + const table = await getTableById(TABLE_ID, { tx }) + + expect(table).toMatchObject({ rowCount: 96, jobId: 'job-4' }) + expect(select).toHaveBeenCalledTimes(1) + expect(limit).toHaveBeenCalledWith(1) + expect(dbChainMockFns.select).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/table/service.ts b/apps/sim/lib/table/service.ts index 95d3ff94939..42aa16fc4a9 100644 --- a/apps/sim/lib/table/service.ts +++ b/apps/sim/lib/table/service.ts @@ -38,7 +38,12 @@ import { assertRowCapacity, notifyTableRowUsage } from '@/lib/table/billing' import { generateColumnId, getColumnId, withGeneratedColumnIds } from '@/lib/table/column-keys' import { COLUMN_TYPES, NAME_PATTERN, TABLE_LIMITS } from '@/lib/table/constants' import { appendTableEvent } from '@/lib/table/events' -import { EMPTY_JOB_FIELDS, latestJobForTable, latestJobsForTables } from '@/lib/table/jobs/service' +import { + EMPTY_JOB_FIELDS, + latestJobsForTables, + latestNonExportJobJson, + mapJobRow, +} from '@/lib/table/jobs/service' import { assertSchemaMutable, TableLockedError } from '@/lib/table/mutation-locks' import { nKeysBetween } from '@/lib/table/order-key' import type { DbTransaction } from '@/lib/table/planner' @@ -168,6 +173,12 @@ function applyColumnOrderToSchema( /** * Gets a table by ID with full details. * + * One round trip: the table's latest non-export job comes back in the same SELECT as + * a correlated lateral ({@link latestNonExportJobJson}) rather than a second query. + * The two cannot be separated — the reported `rowCount` is the stored count minus the + * running delete job's `pendingDeleteRemaining`, so dropping the job read would + * overstate the count mid-delete and let over-capacity inserts through. + * * @param tableId - Table ID to fetch * @returns Table definition or null if not found */ @@ -192,6 +203,7 @@ export async function getTableById( createdAt: userTableDefinitions.createdAt, updatedAt: userTableDefinitions.updatedAt, rowCount: userTableDefinitions.rowCount, + latestJob: latestNonExportJobJson(userTableDefinitions.id), ...LOCK_SELECT, }) .from(userTableDefinitions) @@ -206,7 +218,7 @@ export async function getTableById( const table = results[0] const metadata = (table.metadata as TableMetadata) ?? null - const { pendingDeleteRemaining, ...jobFields } = await latestJobForTable(tableId, executor) + const { pendingDeleteRemaining, ...jobFields } = mapJobRow(table.latestJob) return { id: table.id, name: table.name, From bca66ea1832e8730de0043dd8e071da5d6ef6c4d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:46:07 -0700 Subject: [PATCH 06/12] chore(table): drop the unused single-row GET contract Authored as groundwork for migrating the internal row routes onto the shared builder, which is not in this change. An exported contract nothing consumes is dead code, so it lands with the migration that needs it instead. --- apps/sim/lib/api/contracts/tables.ts | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/apps/sim/lib/api/contracts/tables.ts b/apps/sim/lib/api/contracts/tables.ts index 9cd4217c5f7..0ed44ed4543 100644 --- a/apps/sim/lib/api/contracts/tables.ts +++ b/apps/sim/lib/api/contracts/tables.ts @@ -1433,22 +1433,6 @@ export const upsertTableRowContract = defineRouteContract({ }, }) -/** - * Reads one row. The sibling of {@link updateTableRowContract} and - * {@link deleteTableRowContract}, which take their workspace scope from a body; - * a GET has none, so it is asserted on the query string instead. - */ -export const getTableRowContract = defineRouteContract({ - method: 'GET', - path: '/api/table/[tableId]/rows/[rowId]', - params: tableRowParamsSchema, - query: getTableQuerySchema, - response: { - mode: 'json', - schema: successResponseSchema(z.object({ row: tableRowSchema })), - }, -}) - export const updateTableRowContract = defineRouteContract({ method: 'PATCH', path: '/api/table/[tableId]/rows/[rowId]', From 1536278f8d0597350686a863b2a0411f2f8c966b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 19:49:40 -0700 Subject: [PATCH 07/12] fix(table): resolve strict id-keyed columns through getColumnId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit assertKnownColumnIds read column.id directly, but a column id is optional — pre-backfill columns have none and are stored under their name, which is why getColumnId exists and is what every other consumer of the schema uses. Such columns still exist, so a strict id-keyed write naming one would have been refused as unknown. Latent today: no surface yet combines dataKeying 'ids' with strictWrite. Fixed before one does. Verified to fail: reading column.id turns the covering test red. --- apps/sim/lib/table/application/rows.test.ts | 27 +++++++++++++++++++++ apps/sim/lib/table/application/rows.ts | 7 ++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index 7b6dc6f8ef9..661b98f985b 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -1255,6 +1255,33 @@ describe('row data keying', () => { expect(mockUpdateRow).not.toHaveBeenCalled() }) + it('accepts a legacy column that has no id and is stored under its name', async () => { + // Two production tables still carry pre-backfill columns with no `id`. + // Their storage key is the name, so a strict id-keyed write naming one must + // be accepted, not refused as unknown. + mockResolveContext.mockResolvedValue({ + tableId: TABLE.id, + table: { ...TABLE, schema: { columns: [{ name: 'legacy', type: 'string' }] } }, + workspaceId: TABLE.workspaceId, + workspaceOrganizationId: 'organization-1', + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + }) + + await expect( + updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { legacy: 'x' }, + strictWrite: true, + dataKeying: 'ids', + }, + }) + ).resolves.toBeDefined() + }) + it('names every unknown id at once, as the name wire does', async () => { await expect( createTableRows.execute({ diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index f06587e419e..2d00f9b9049 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -43,7 +43,7 @@ import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized import { resolveActiveTableContext } from '@/lib/table/application/context' import { tableOperations } from '@/lib/table/application/operations' import { assertRowCapacity, notifyTableRowUsage } from '@/lib/table/billing' -import { buildIdByName, unknownColumnNames } from '@/lib/table/column-keys' +import { buildIdByName, getColumnId, unknownColumnNames } from '@/lib/table/column-keys' import { columnTypeOf } from '@/lib/table/column-types' import { TableQueryValidationError } from '@/lib/table/errors' import { signalTableRowsChanged, signalTableRowsChangedByActor } from '@/lib/table/events' @@ -179,7 +179,10 @@ export type TableRowDataKeying = 'names' | 'ids' * `strictWrite` condition, so strictness means the same thing on either wire. */ function assertKnownColumnIds(data: RowData, table: TableDefinition, rowLabel?: string): void { - const ids = new Set(table.schema.columns.map((column) => column.id)) + // `getColumnId`, not `column.id`: a legacy pre-backfill column has no id and is + // stored under its name, so reading the raw field would reject a write that + // every other consumer of the schema accepts. + const ids = new Set(table.schema.columns.map((column) => getColumnId(column))) const unknown = Object.keys(data).filter((key) => !ids.has(key)) if (unknown.length === 0) return const where = rowLabel ? `${rowLabel}: ` : '' From 465cea2bd6138670e7075c2399b08c38123d432d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 20:01:42 -0700 Subject: [PATCH 08/12] refactor(table): apply review findings from the quality pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four parallel reviews (reuse, simplification, efficiency, altitude) converged on the same set. Applied: Removed actorClientId entirely. It had no supplier anywhere in the repo, so every call reached signalTableRowsChangedByActor(id, undefined), which is byte-identical to the broadcast it replaced — three optional fields, three verbatim doc blocks and a pin test asserting the empty set, all inert. It belongs with the route migration that supplies an actor. The attribution pin is restored to its original form. The uniqueness probe was only half-narrowed: the patched column list was computed and then discarded, and the probe re-derived every unique column from the full schema. It now receives only the columns the patch touched, so a table with several unique columns runs one query instead of all of them. assertKnownColumnIds hand-rolled an id index and duplicated the sibling assert's message verbatim. It now reuses buildColumnNameById — which already keys by getColumnId, so the legacy pre-backfill column case is handled by the shared helper rather than by a special case here — and both asserts share one message builder. The job field list had become two copies, one drizzle-checked and one an unchecked sql cast that could silently return undefined for a renamed field. The lateral now derives its jsonb pairs from JOB_PROJECTION, which satisfies Record. That compile-time guarantee replaces the runtime drift test it makes redundant. Also: hoisted the id index out of the batch loop to match the names path, dropped a never-supplied parameter, narrowed an over-broad parameter type, removed a dead timer cleanup and its now-unused import, replaced dynamic re-imports with the static one already present, hoisted a repeated stub, and documented why filters need no keying counterpart and how laxness differs between the two wires. Verified to fail: removing a field from JOB_PROJECTION breaks the build in two places. --- .../lib/table/__tests__/update-row.test.ts | 37 ++++-- .../sim/lib/table/application/context.test.ts | 6 +- apps/sim/lib/table/application/rows.test.ts | 86 -------------- apps/sim/lib/table/application/rows.ts | 112 +++++++----------- apps/sim/lib/table/column-keys.ts | 2 +- apps/sim/lib/table/events.attribution.test.ts | 54 ++------- apps/sim/lib/table/jobs/service.test.ts | 10 +- apps/sim/lib/table/jobs/service.ts | 25 ++-- apps/sim/lib/table/rows/service.ts | 6 +- 9 files changed, 105 insertions(+), 233 deletions(-) diff --git a/apps/sim/lib/table/__tests__/update-row.test.ts b/apps/sim/lib/table/__tests__/update-row.test.ts index 172a5e03c10..e7472eb40fd 100644 --- a/apps/sim/lib/table/__tests__/update-row.test.ts +++ b/apps/sim/lib/table/__tests__/update-row.test.ts @@ -14,7 +14,7 @@ import { upsertRow, } from '@/lib/table/rows/service' import type { TableDefinition } from '@/lib/table/types' -import { getUniqueColumns } from '@/lib/table/validation' +import { checkUniqueConstraintsDb, getUniqueColumns } from '@/lib/table/validation' // Capacity is exercised in billing.test.ts; here it's a no-op so the timeout-scaling // suites can use large synthetic row counts without tripping the plan limit. @@ -563,6 +563,9 @@ describe('updateRow — uniqueness probe scoping', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() + // The common case: one unique column. The two tests that need a different + // shape override this. + vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) dbChainMockFns.limit.mockResolvedValue([EXISTING_ROW]) dbChainMockFns.returning.mockResolvedValue([ { id: EXISTING_ROW.id, updatedAt: PERSISTED_UPDATED_AT }, @@ -570,9 +573,6 @@ describe('updateRow — uniqueness probe scoping', () => { }) it('does not probe when the patch touches no unique column', async () => { - const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') - vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) - await updateRow( { tableId: 'tbl-1', rowId: 'row-1', data: { age: 31 }, workspaceId: 'ws-1' }, TABLE, @@ -583,9 +583,6 @@ describe('updateRow — uniqueness probe scoping', () => { }) it('still probes when the patch touches a unique column', async () => { - const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') - vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) - await updateRow( { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, TABLE, @@ -596,25 +593,40 @@ describe('updateRow — uniqueness probe scoping', () => { }) it('probes against the merged row, so the excluded row is still the one being edited', async () => { - const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') - vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) - await updateRow( { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, TABLE, 'req-1' ) + // The merged row is what gets probed, the excluded row is the one being + // edited, and the schema is narrowed to the unique columns this patch + // touched — the probe issues one query per column it is handed. expect(checkUniqueConstraintsDb).toHaveBeenCalledWith( 'tbl-1', { name: 'Grace', age: 30 }, - TABLE.schema, + { ...TABLE.schema, columns: [{ name: 'name', type: 'string', unique: true }] }, 'row-1' ) }) + it('hands the probe only the unique columns the patch touched', async () => { + vi.mocked(getUniqueColumns).mockReturnValue([ + { name: 'name', type: 'string', unique: true }, + { name: 'email', type: 'string', unique: true }, + ]) + + await updateRow( + { tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' }, + TABLE, + 'req-1' + ) + + const schemaArg = vi.mocked(checkUniqueConstraintsDb).mock.calls[0][2] + expect(schemaArg.columns).toEqual([{ name: 'name', type: 'string', unique: true }]) + }) + it('surfaces a duplicate on a column the patch does write', async () => { - const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }]) vi.mocked(checkUniqueConstraintsDb).mockResolvedValueOnce({ valid: false, @@ -631,7 +643,6 @@ describe('updateRow — uniqueness probe scoping', () => { }) it('does not probe on a table with no unique columns at all', async () => { - const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation') vi.mocked(getUniqueColumns).mockReturnValue([]) await updateRow( diff --git a/apps/sim/lib/table/application/context.test.ts b/apps/sim/lib/table/application/context.test.ts index 62d07678516..8ffe66c6196 100644 --- a/apps/sim/lib/table/application/context.test.ts +++ b/apps/sim/lib/table/application/context.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' const { getTableById, loadWorkspace } = vi.hoisted(() => ({ getTableById: vi.fn(), @@ -60,10 +60,6 @@ describe('table application context', () => { ) }) - afterEach(() => { - vi.useRealTimers() - }) - it('derives workspace scope from the canonical active table', async () => { await expect( resolveActiveTableContext({ tableId: 'table-1', assertedWorkspaceId: 'workspace-1' }) diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index 661b98f985b..2424fb0a7da 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -19,7 +19,6 @@ const { mockResolveContext, mockResolvePermission, mockSignalRowsChanged, - mockSignalRowsChangedByActor, mockUpsertRow, mockWithLockedTable, mockInsertRow, @@ -42,7 +41,6 @@ const { mockResolveContext: vi.fn(), mockResolvePermission: vi.fn(), mockSignalRowsChanged: vi.fn(), - mockSignalRowsChangedByActor: vi.fn(), mockUpsertRow: vi.fn(), mockWithLockedTable: vi.fn(), mockInsertRow: vi.fn(), @@ -139,12 +137,10 @@ vi.mock('@/lib/table/application/context', () => ({ vi.mock('@/lib/table/events', () => ({ signalTableRowsChanged: mockSignalRowsChanged, - signalTableRowsChangedByActor: mockSignalRowsChangedByActor, })) import { createTableRows, - deleteTableRow, deleteTableRows, listTableRows, ProjectedWireRowsValidationError, @@ -1297,85 +1293,3 @@ describe('row data keying', () => { ).rejects.toThrow(/Row 2: Unknown columns: zzz, qqq/) }) }) - -/** - * Naming the acting tab lets that tab skip refetching its own write. Only the - * single-row paths accept an actor — see `events.attribution.test.ts` for why, - * and for the pinned list of surfaces allowed to supply one. - */ -describe('row change attribution', () => { - beforeEach(() => { - vi.clearAllMocks() - mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) - mockAssertRowCapacity.mockResolvedValue(10_000) - mockCreateSecretProvenance.mockReturnValue({ complete: true, columns: {} }) - mockIsScopeCompatible.mockReturnValue(true) - }) - - it('names the acting tab on a single-row update', async () => { - await updateTableRow.execute({ - principal: PRINCIPAL, - input: { - tableId: TABLE.id, - rowId: 'row-1', - data: { name: 'Ada' }, - strictWrite: false, - dataKeying: 'names', - actorClientId: 'tab-42', - }, - }) - - expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, 'tab-42') - expect(mockSignalRowsChanged).not.toHaveBeenCalled() - }) - - it('broadcasts to everyone when the surface cannot name a tab', async () => { - await updateTableRow.execute({ - principal: PRINCIPAL, - input: { - tableId: TABLE.id, - rowId: 'row-1', - data: { name: 'Ada' }, - strictWrite: false, - dataKeying: 'names', - }, - }) - - expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, undefined) - }) - - it('names the acting tab on a single-row delete', async () => { - await deleteTableRow.execute({ - principal: PRINCIPAL, - input: { tableId: TABLE.id, rowId: 'row-1', actorClientId: 'tab-42' }, - }) - - expect(mockSignalRowsChangedByActor).toHaveBeenCalledWith(TABLE.id, 'tab-42') - }) - - it('broadcasts a batch insert to everyone even though a tab is named', async () => { - await createTableRows.execute({ - principal: PRINCIPAL, - input: { - kind: 'batch', - tableId: TABLE.id, - rows: [{ name: 'Ada' }, { name: 'Grace' }], - strictWrite: false, - dataKeying: 'names', - }, - }) - - // A batch write is not reconciled locally by the acting tab, so it must - // refetch like everyone else. - expect(mockSignalRowsChanged).toHaveBeenCalledWith(TABLE.id) - expect(mockSignalRowsChangedByActor).not.toHaveBeenCalled() - }) -}) diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index 2d00f9b9049..a1dd9385a82 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -43,10 +43,10 @@ import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized import { resolveActiveTableContext } from '@/lib/table/application/context' import { tableOperations } from '@/lib/table/application/operations' import { assertRowCapacity, notifyTableRowUsage } from '@/lib/table/billing' -import { buildIdByName, getColumnId, unknownColumnNames } from '@/lib/table/column-keys' +import { buildColumnNameById, buildIdByName, unknownColumnNames } from '@/lib/table/column-keys' import { columnTypeOf } from '@/lib/table/column-types' import { TableQueryValidationError } from '@/lib/table/errors' -import { signalTableRowsChanged, signalTableRowsChangedByActor } from '@/lib/table/events' +import { signalTableRowsChanged } from '@/lib/table/events' import { predicateToFilter } from '@/lib/table/query-builder/converters' import { validatePredicate, @@ -81,17 +81,6 @@ interface TableScopedInput { tableId: string assertedWorkspaceId?: string requestId?: string - /** - * Whether the calling surface publishes the stricter `/api/v2` write contract: - * a row naming a column the table does not have is refused rather than having - * that key dropped, and a value the column's type cannot coerce is answered - * with a 400 rather than stored as `null`. - * - * Absent — every first-party surface, and the only behavior any of them has - * ever had: the workspace grid, the internal `/api/table` routes, `/api/v1`, - * the Copilot table tools, and the executor's Table block all drop the - * unknown key and blank the uncoercible cell. Read-only use cases ignore it. - */ } /** The write policy `strictWrite` selects, for the row-service primitives. */ @@ -133,7 +122,7 @@ function actorUserId( /** * Refuses a wire row naming a column the table does not have. Applied only to a - * `strictWrite` caller — see {@link TableScopedInput.strictWrite}. + * `strictWrite` caller — see {@link rowWriteOptions}. * * The name→id remap drops unrecognised keys, so without this an insert of * `{"nosuchcol":"x"}` created an empty row under a 201, and a patch of @@ -147,7 +136,14 @@ function assertKnownColumnNames( idByName: ReadonlyMap, rowLabel?: string ): void { - const unknown = unknownColumnNames(data, idByName) + assertNoUnknownColumns(unknownColumnNames(data, idByName), rowLabel) +} + +/** + * Refuses a wire row naming a column the table does not have, on either wire. + * Shared so the two keyings cannot drift in how they name the offending keys. + */ +function assertNoUnknownColumns(unknown: string[], rowLabel?: string): void { if (unknown.length === 0) return const where = rowLabel ? `${rowLabel}: ` : '' throw new TableRowsValidationError( @@ -170,48 +166,54 @@ function assertKnownColumnNames( * name remap drops keys it does not recognise, and a storage id names no column * *name*, so feeding id-keyed data through the name path silently drops every * cell and reports the write as successful. + * + * Row data needs this and filters, sorts and predicates do not: their + * translators pass an unrecognised field through unchanged (`idByName.get(key) + * ?? key`), so they are already correct under either keying. Only the row-data + * remap is lossy, which is why only it carries a discriminator. */ export type TableRowDataKeying = 'names' | 'ids' /** - * Refuses a wire row naming a column id the table does not have. The id-keyed - * counterpart of {@link assertKnownColumnNames}, and applied on the same - * `strictWrite` condition, so strictness means the same thing on either wire. + * The id-keyed counterpart of {@link assertKnownColumnNames}, applied on the same + * `strictWrite` condition so strictness means the same thing on either wire. + * + * `buildColumnNameById` keys by {@link getColumnId}, so a legacy pre-backfill + * column — which has no id and is stored under its name — is recognised rather + * than reported unknown. */ function assertKnownColumnIds(data: RowData, table: TableDefinition, rowLabel?: string): void { - // `getColumnId`, not `column.id`: a legacy pre-backfill column has no id and is - // stored under its name, so reading the raw field would reject a write that - // every other consumer of the schema accepts. - const ids = new Set(table.schema.columns.map((column) => getColumnId(column))) - const unknown = Object.keys(data).filter((key) => !ids.has(key)) - if (unknown.length === 0) return - const where = rowLabel ? `${rowLabel}: ` : '' - throw new TableRowsValidationError( - `${where}Unknown column${unknown.length > 1 ? 's' : ''}: ${unknown.join(', ')}` + assertNoUnknownColumns( + unknownColumnNames(data, buildColumnNameById(table.schema.columns)), + rowLabel ) } /** * Normalizes one wire row to storage keying. See {@link TableRowDataKeying}. + * + * Note the asymmetry a lax (non-`strictWrite`) caller sees: the name path drops + * keys naming no column, while the id path stores what it is given. That + * matches what each wire did before this discriminator existed, and only + * `strictWrite` makes the two agree. */ function rowDataToStorage( data: RowData, table: TableDefinition, keying: TableRowDataKeying, - strict = false, - rowLabel?: string + strict = false ): RowData { if (keying === 'ids') { - if (strict) assertKnownColumnIds(data, table, rowLabel) + if (strict) assertKnownColumnIds(data, table) return data } const idByName = buildIdByName(table.schema) - if (strict) assertKnownColumnNames(data, idByName, rowLabel) + if (strict) assertKnownColumnNames(data, idByName) return rowDataNameToId(data, idByName) } /** - * {@link rowDataToStorage} over a batch. The name index is built once for the + * {@link rowDataToStorage} over a batch. Either index is built once for the * whole batch rather than per row — these paths run over up to * `MAX_BATCH_INSERT_SIZE` rows. */ @@ -222,8 +224,10 @@ function rowsToStorage( strict = false ): RowData[] { if (keying === 'ids') { + if (!strict) return [...rows] + const nameById = buildColumnNameById(table.schema.columns) return rows.map((row, index) => { - if (strict) assertKnownColumnIds(row, table, `Row ${index + 1}`) + assertNoUnknownColumns(unknownColumnNames(row, nameById), `Row ${index + 1}`) return row }) } @@ -479,14 +483,6 @@ interface CreateSingleTableRowInput extends TableScopedInput { /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ dataKeying: TableRowDataKeying kind: 'single' - /** - * Tab that caused this write, when the calling surface knows it. Lets that - * tab skip its own refetch — see {@link signalTableRowsChangedByActor}, whose - * soundness condition (the caller's hook reconciles the write locally across - * every cached rows query) is why only the single-row paths accept this. - * Omitted by every surface that cannot name a tab, which broadcasts to all. - */ - actorClientId?: string data: RowData position?: number afterRowId?: string @@ -589,16 +585,9 @@ export const createTableRows = defineAuthorizedTableUseCase({ ) return { kind: 'batch', table: context.table, rows: created } }, - afterSuccess: ({ context, input, result }) => { + afterSuccess: ({ context, result }) => { const affected = result.kind === 'single' ? 1 : result.rows.length - if (affected === 0) return - // Only the single-row path carries an actor; a batch insert genuinely needs - // the originating tab to refetch, so it broadcasts to everyone. - if (input.kind === 'single') { - signalTableRowsChangedByActor(context.tableId, input.actorClientId) - return - } - signalTableRowsChanged(context.tableId) + if (affected > 0) signalTableRowsChanged(context.tableId) }, }) @@ -825,14 +814,6 @@ export interface UpdateTableRowInput extends TableScopedInput { data: RowData secretProvenance?: TableRowSecretProvenanceWrite includePersistedSecretProvenance?: boolean - /** - * Tab that caused this write, when the calling surface knows it. Lets that - * tab skip its own refetch — see {@link signalTableRowsChangedByActor}, whose - * soundness condition (the caller's hook reconciles the write locally across - * every cached rows query) is why only the single-row paths accept this. - * Omitted by every surface that cannot name a tab, which broadcasts to all. - */ - actorClientId?: string } export interface UpdateTableRowResult extends TableResult { @@ -872,8 +853,8 @@ export const updateTableRow = defineAuthorizedTableUseCase({ ), } }, - afterSuccess: ({ context, input, result }) => { - if (result.changed) signalTableRowsChangedByActor(context.tableId, input.actorClientId) + afterSuccess: ({ context, result }) => { + if (result.changed) signalTableRowsChanged(context.tableId) }, }) @@ -923,14 +904,6 @@ export const updateTableRows = defineAuthorizedTableUseCase({ export interface DeleteTableRowInput extends TableScopedInput { rowId: string - /** - * Tab that caused this write, when the calling surface knows it. Lets that - * tab skip its own refetch — see {@link signalTableRowsChangedByActor}, whose - * soundness condition (the caller's hook reconciles the write locally across - * every cached rows query) is why only the single-row paths accept this. - * Omitted by every surface that cannot name a tab, which broadcasts to all. - */ - actorClientId?: string } export interface DeleteTableRowResult extends TableResult { @@ -944,8 +917,7 @@ export const deleteTableRow = defineAuthorizedTableUseCase({ await deleteRow(context.table, input.rowId, requestId(input)) return { table: context.table, deletedRowId: input.rowId } }, - afterSuccess: ({ context, input }) => - signalTableRowsChangedByActor(context.tableId, input.actorClientId), + afterSuccess: ({ context }) => signalTableRowsChanged(context.tableId), }) export type DeleteTableRowsInput = TableScopedInput & diff --git a/apps/sim/lib/table/column-keys.ts b/apps/sim/lib/table/column-keys.ts index fc92ac7e9fb..8ee9620c3bc 100644 --- a/apps/sim/lib/table/column-keys.ts +++ b/apps/sim/lib/table/column-keys.ts @@ -188,7 +188,7 @@ export function remapViewConfigColumnRefs( * drop — a key that survives to here unrecognised is a cell the caller asked to * write and the table never stored. Callers on a surface that can answer the * client check {@link unknownColumnNames} first; see - * `namedDataToStorage` in `application/rows.ts`. + * `rowDataToStorage` in `application/rows.ts`. */ export function rowDataNameToId(data: RowData, idByName: Map): RowData { const out: RowData = {} diff --git a/apps/sim/lib/table/events.attribution.test.ts b/apps/sim/lib/table/events.attribution.test.ts index 2f96c90ca52..e0f8c9ee448 100644 --- a/apps/sim/lib/table/events.attribution.test.ts +++ b/apps/sim/lib/table/events.attribution.test.ts @@ -9,26 +9,16 @@ import { describe, expect, it } from 'vitest' * `signalTableRowsChangedByActor` lets the acting tab skip its own refetch, which is only sound * where that tab's mutation hook already applies the server's answer to every cached rows query. * That invariant lives in `hooks/queries/tables.ts` — nothing in the type system ties it to the - * call site, so a well-meaning extra call would silently strand that client on stale rows. + * call site, so a well-meaning fourth call would silently strand that client on stale rows. * - * Two lists are pinned, because the single-row paths now signal from inside their application use - * case rather than from the route. The call itself is no longer the decision: the use case is - * shared with `/api/v2` and Copilot, and it degrades to a broadcast whenever no actor is named. - * What actually selects the behavior is which surface supplies `actorClientId`, so that is pinned - * too and is the list to scrutinise. - * - * If you are here because it failed: adding a supplier means proving that surface's client hook - * reconciles the write locally across every cached rows query. Removing one is always safe. + * This pins the allowlist. If you are here because it failed: adding a call means proving the + * calling route's client hook reconciles locally, then adding it below. Removing one is always safe. */ const ATTRIBUTED_CALL_SITES = [ 'app/api/table/[tableId]/rows/route.ts', 'app/api/table/[tableId]/rows/[rowId]/route.ts', - 'lib/table/application/rows.ts', ] as const -/** Surfaces that name the acting tab. See the note above — this is the real allowlist. */ -const ACTOR_SUPPLYING_SURFACES = [] as const - const APP_ROOT = join(import.meta.dirname, '../..') /** Declares the function; matching its own definition would say nothing about call sites. */ const DECLARING_MODULE = 'lib/table/events.ts' @@ -42,35 +32,17 @@ async function* walk(dir: string): AsyncGenerator { } } -async function filesContaining(needle: string, skip: (relative: string) => boolean = () => false) { - const found: string[] = [] - for await (const file of walk(APP_ROOT)) { - const source = await readFile(file, 'utf8') - if (!source.includes(needle)) continue - const relative = file.slice(APP_ROOT.length + 1) - if (skip(relative)) continue - found.push(relative) - } - return found.sort() -} - describe('signalTableRowsChangedByActor call sites', () => { it('is called only where the acting tab reconciles the write locally', async () => { - const callers = await filesContaining( - 'signalTableRowsChangedByActor(', - (relative) => relative === DECLARING_MODULE - ) - - expect(callers).toEqual([...ATTRIBUTED_CALL_SITES].sort()) - }) - - it('is given an actor only by surfaces whose client hook reconciles locally', async () => { - const suppliers = await filesContaining( - 'actorClientId:', - // Declares the field rather than supplying one. - (relative) => relative === 'lib/table/application/rows.ts' - ) - - expect(suppliers).toEqual([...ACTOR_SUPPLYING_SURFACES].sort()) + const callers: string[] = [] + for await (const file of walk(APP_ROOT)) { + const source = await readFile(file, 'utf8') + if (!source.includes('signalTableRowsChangedByActor(')) continue + const relative = file.slice(APP_ROOT.length + 1) + if (relative === DECLARING_MODULE) continue + callers.push(relative) + } + + expect(callers.sort()).toEqual([...ATTRIBUTED_CALL_SITES].sort()) }) }) diff --git a/apps/sim/lib/table/jobs/service.test.ts b/apps/sim/lib/table/jobs/service.test.ts index a34efbdbbbe..34ea40301b2 100644 --- a/apps/sim/lib/table/jobs/service.test.ts +++ b/apps/sim/lib/table/jobs/service.test.ts @@ -101,10 +101,8 @@ describe('latestNonExportJobJson', () => { expect(values).toContain(schemaMock.userTableDefinitions.id) }) - it('projects every field mapJobRow reads', () => { - const { text } = renderLateral() - for (const key of ['id', 'type', 'status', 'rowsProcessed', 'error', 'payload']) { - expect(text).toContain(`'${key}',`) - } - }) + // No drift test for the projected field list: the fragment derives its + // jsonb pairs from JOB_PROJECTION, which `satisfies Record`. A missing field is a compile error, which is + // stronger than anything asserted here could be. }) diff --git a/apps/sim/lib/table/jobs/service.ts b/apps/sim/lib/table/jobs/service.ts index 367e4b2a6ce..767345bc357 100644 --- a/apps/sim/lib/table/jobs/service.ts +++ b/apps/sim/lib/table/jobs/service.ts @@ -80,6 +80,12 @@ export function mapJobRow(row: LatestJobRow | null | undefined): DerivedJobField } } +/** + * The columns {@link mapJobRow} reads, as one source for both job reads: the + * batch `DISTINCT ON` selects it directly, and {@link latestNonExportJobJson} + * derives its `jsonb_build_object` pairs from it. Adding a field here reaches + * both — the two cannot drift into disagreeing about what a job row is. + */ const JOB_PROJECTION = { id: tableJobs.id, type: tableJobs.type, @@ -87,7 +93,7 @@ const JOB_PROJECTION = { rowsProcessed: tableJobs.rowsProcessed, error: tableJobs.error, payload: tableJobs.payload, -} as const +} as const satisfies Record /** * The latest non-export job for one table, as a single jsonb value correlated to @@ -105,16 +111,15 @@ const JOB_PROJECTION = { * first, one row. `NULL` when the table has no such job; feed the result straight to * {@link mapJobRow}. */ -export function latestNonExportJobJson(outerTableId: Column | SQL): SQL { +export function latestNonExportJobJson(outerTableId: Column): SQL { + // Keys come from JOB_PROJECTION, never from input, so `sql.raw` here cannot + // carry anything a caller controls. + const pairs = Object.entries(JOB_PROJECTION).flatMap(([key, column]) => [ + sql.raw(`'${key}'`), + column, + ]) return sql`( - select jsonb_build_object( - 'id', ${tableJobs.id}, - 'type', ${tableJobs.type}, - 'status', ${tableJobs.status}, - 'rowsProcessed', ${tableJobs.rowsProcessed}, - 'error', ${tableJobs.error}, - 'payload', ${tableJobs.payload} - ) + select jsonb_build_object(${sql.join(pairs, sql`, `)}) from ${tableJobs} where ${tableJobs.tableId} = ${outerTableId} and ${tableJobs.type} <> 'export' order by ${tableJobs.startedAt} desc diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index bfc4f80f1ac..291593471f9 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1626,7 +1626,11 @@ export async function updateRow( const uniqueValidation = await checkUniqueConstraintsDb( data.tableId, mergedData, - table.schema, + // Narrowed to the patched unique columns, not just used as a gate: the + // probe issues one SELECT per unique column it is given, so a table with + // several would otherwise re-check all of them to validate a patch that + // touched one. `schema` is read only for its unique columns here. + { ...table.schema, columns: patchedUniqueColumns }, data.rowId // Exclude current row ) if (!uniqueValidation.valid) { From 0c9827ba55c29cb868fa0f7a909edb4dffc6337b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 20:09:33 -0700 Subject: [PATCH 09/12] test(table): share one table-definition fixture factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildTable was copy-pasted into 13 route test files under app/api/table, each a near-identical TableDefinition literal. A required field added to that type would have failed 13 files individually. packages/testing already owned createTableColumn and createTableRow but no definition factory, and — as it turns out — did not export any of them from the barrel, so they were unreachable from @sim/testing. Adds createTableDefinition beside them and exports all three. The fixture type is a structural stand-in rather than TableDefinition itself, because packages/* must not import from apps/* — the same approach the existing factories in that file already take. No assertion changed. Call sites that varied a field pass it as an override; the four files with several call sites hoist a shared options const and spread it so each call still gets a fresh object. --- .../[tableId]/delete-async/route.test.ts | 35 +++---- .../table/[tableId]/dispatches/route.test.ts | 23 +---- .../[tableId]/export-async/route.test.ts | 29 ++---- .../[tableId]/export/download/route.test.ts | 23 +---- .../api/table/[tableId]/export/route.test.ts | 38 +++---- .../[tableId]/import-async/route.test.ts | 32 +++--- .../api/table/[tableId]/import/route.test.ts | 59 +++++------ .../table/[tableId]/job/cancel/route.test.ts | 23 +---- .../api/table/[tableId]/query/route.test.ts | 38 +++---- .../enrichment/[groupId]/route.test.ts | 23 +---- .../[tableId]/rows/[rowId]/route.test.ts | 47 ++++----- .../table/[tableId]/rows/find/route.test.ts | 31 ++---- .../api/table/[tableId]/rows/route.test.ts | 41 +++----- packages/testing/src/factories/index.ts | 14 +++ .../testing/src/factories/table.factory.ts | 98 ++++++++++++++++++- 15 files changed, 255 insertions(+), 299 deletions(-) diff --git a/apps/sim/app/api/table/[tableId]/delete-async/route.test.ts b/apps/sim/app/api/table/[tableId]/delete-async/route.test.ts index cffb5dc8810..fb875ca66b6 100644 --- a/apps/sim/app/api/table/[tableId]/delete-async/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/delete-async/route.test.ts @@ -1,10 +1,15 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns, resetEnvFlagsMock, setEnvFlags } from '@sim/testing' +import { + createTableDefinition, + hybridAuthMockFns, + resetEnvFlagsMock, + setEnvFlags, + type TableDefinitionFactoryOptions, +} from '@sim/testing' import { NextRequest, NextResponse } from 'next/server' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, @@ -58,22 +63,9 @@ import { POST } from '@/app/api/table/[tableId]/delete-async/route' afterAll(resetEnvFlagsMock) -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [{ name: 'status', type: 'string' }] }, - metadata: null, - rowCount: 1000, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - ...overrides, - } +const TABLE_FIXTURE: TableDefinitionFactoryOptions = { + columns: [{ name: 'status', type: 'string' }], + rowCount: 1000, } function makeRequest(body: unknown, tableId = 'tbl_1') { @@ -99,7 +91,7 @@ describe('POST /api/table/[tableId]/delete-async', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition(TABLE_FIXTURE) }) mockMarkTableJobRunning.mockResolvedValue(true) mockRunTableDelete.mockResolvedValue(undefined) mockTableFilterError.mockReturnValue(null) @@ -168,7 +160,10 @@ describe('POST /api/table/[tableId]/delete-async', () => { }) it('returns 400 when the table is archived', async () => { - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable({ archivedAt: new Date() }) }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ ...TABLE_FIXTURE, archivedAt: new Date() }), + }) const response = await makeRequest(validBody) expect(response.status).toBe(400) expect(mockRunTableDelete).not.toHaveBeenCalled() diff --git a/apps/sim/app/api/table/[tableId]/dispatches/route.test.ts b/apps/sim/app/api/table/[tableId]/dispatches/route.test.ts index 3acddbbe325..b1c08d00328 100644 --- a/apps/sim/app/api/table/[tableId]/dispatches/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/dispatches/route.test.ts @@ -1,10 +1,9 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockListActiveDispatches, mockCountRunningCells } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -27,24 +26,6 @@ vi.mock('@/app/api/table/utils', async () => { import { GET } from '@/app/api/table/[tableId]/dispatches/route' -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [] }, - metadata: null, - rowCount: 0, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - ...overrides, - } -} - function makeRequest(tableId = 'tbl_1') { const req = new NextRequest(`http://localhost:3000/api/table/${tableId}/dispatches`) return GET(req, { params: Promise.resolve({ tableId }) }) @@ -75,7 +56,7 @@ describe('GET /api/table/[tableId]/dispatches', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition() }) mockListActiveDispatches.mockResolvedValue([]) mockCountRunningCells.mockResolvedValue({ byRowId: {}, hasRunning: false }) }) diff --git a/apps/sim/app/api/table/[tableId]/export-async/route.test.ts b/apps/sim/app/api/table/[tableId]/export-async/route.test.ts index 5be95a85ee6..4922d49041c 100644 --- a/apps/sim/app/api/table/[tableId]/export-async/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/export-async/route.test.ts @@ -1,10 +1,9 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockMarkTableJobRunning, mockRunTableExport } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -34,24 +33,6 @@ vi.mock('@/app/api/table/utils', async () => { import { POST } from '@/app/api/table/[tableId]/export-async/route' -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [{ name: 'name', type: 'string' }] }, - metadata: null, - rowCount: 50000, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - ...overrides, - } -} - function makeRequest(body: unknown, tableId = 'tbl_1') { const req = new NextRequest(`http://localhost:3000/api/table/${tableId}/export-async`, { method: 'POST', @@ -71,7 +52,13 @@ describe('POST /api/table/[tableId]/export-async', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ + columns: [{ name: 'name', type: 'string' }], + rowCount: 50000, + }), + }) mockMarkTableJobRunning.mockResolvedValue(true) mockRunTableExport.mockResolvedValue(undefined) }) diff --git a/apps/sim/app/api/table/[tableId]/export/download/route.test.ts b/apps/sim/app/api/table/[tableId]/export/download/route.test.ts index 53ed1a28cf3..b977f5ff9f8 100644 --- a/apps/sim/app/api/table/[tableId]/export/download/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/export/download/route.test.ts @@ -1,10 +1,9 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockGetTableJob, mockGeneratePresignedDownloadUrl } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -27,24 +26,6 @@ vi.mock('@/app/api/table/utils', async () => { import { GET } from '@/app/api/table/[tableId]/export/download/route' -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [] }, - metadata: null, - rowCount: 0, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - ...overrides, - } -} - function makeRequest(query: Record, tableId = 'tbl_1') { const qs = new URLSearchParams(query).toString() const req = new NextRequest(`http://localhost:3000/api/table/${tableId}/export/download?${qs}`) @@ -61,7 +42,7 @@ describe('GET /api/table/[tableId]/export/download', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition() }) mockGetTableJob.mockResolvedValue({ id: 'job_1', type: 'export', diff --git a/apps/sim/app/api/table/[tableId]/export/route.test.ts b/apps/sim/app/api/table/[tableId]/export/route.test.ts index b420b5f97da..4e1dd314b3c 100644 --- a/apps/sim/app/api/table/[tableId]/export/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/export/route.test.ts @@ -1,10 +1,9 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockQueryRows } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -27,27 +26,6 @@ vi.mock('@/lib/table/rows/service', () => ({ import { GET } from '@/app/api/table/[tableId]/export/route' /** Table with an id-native column whose stable id (`col_email`) differs from its display name. */ -function buildTable(): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { - columns: [ - { id: 'col_email', name: 'email', type: 'string' }, - { name: 'legacy', type: 'string' }, // legacy: id == name - ], - }, - metadata: null, - rowCount: 1, - maxRows: 100, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date('2024-01-01'), - updatedAt: new Date('2024-01-01'), - } -} function callGet(format: string) { const req = new NextRequest(`http://localhost:3000/api/table/tbl_1/export?format=${format}`, { @@ -64,7 +42,19 @@ describe('table export route — id→name translation', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ + columns: [ + { id: 'col_email', name: 'email', type: 'string' }, + { name: 'legacy', type: 'string' }, // legacy: id == name + ], + rowCount: 1, + maxRows: 100, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), + }), + }) // Row data is keyed by stable column id (`col_email`), not the display name. mockQueryRows.mockResolvedValue({ rows: [{ id: 'r1', data: { col_email: 'a@b.c', legacy: 'x' }, executions: {}, position: 0 }], diff --git a/apps/sim/app/api/table/[tableId]/import-async/route.test.ts b/apps/sim/app/api/table/[tableId]/import-async/route.test.ts index 271cf2b8ed6..a22bd6b5e7d 100644 --- a/apps/sim/app/api/table/[tableId]/import-async/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/import-async/route.test.ts @@ -1,10 +1,13 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { + createTableDefinition, + hybridAuthMockFns, + type TableDefinitionFactoryOptions, +} from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockMarkTableImporting, mockRunTableImport } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -34,22 +37,8 @@ vi.mock('@/app/api/table/utils', async () => { import { POST } from '@/app/api/table/[tableId]/import-async/route' -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [{ name: 'name', type: 'string' }] }, - metadata: null, - rowCount: 0, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - ...overrides, - } +const TABLE_FIXTURE: TableDefinitionFactoryOptions = { + columns: [{ name: 'name', type: 'string' }], } function makeRequest(body: unknown, tableId = 'tbl_1') { @@ -76,7 +65,7 @@ describe('POST /api/table/[tableId]/import-async', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition(TABLE_FIXTURE) }) mockMarkTableImporting.mockResolvedValue(true) mockRunTableImport.mockResolvedValue(undefined) }) @@ -126,7 +115,10 @@ describe('POST /api/table/[tableId]/import-async', () => { }) it('returns 400 when the target table is archived', async () => { - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable({ archivedAt: new Date() }) }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ ...TABLE_FIXTURE, archivedAt: new Date() }), + }) const response = await makeRequest(validBody) expect(response.status).toBe(400) expect(mockRunTableImport).not.toHaveBeenCalled() diff --git a/apps/sim/app/api/table/[tableId]/import/route.test.ts b/apps/sim/app/api/table/[tableId]/import/route.test.ts index a2689295725..45de3f210ee 100644 --- a/apps/sim/app/api/table/[tableId]/import/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/import/route.test.ts @@ -1,7 +1,11 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { + createTableDefinition, + hybridAuthMockFns, + type TableDefinitionFactoryOptions, +} from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { TableDefinition } from '@/lib/table' @@ -122,27 +126,14 @@ function createFormData( return form } -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { - columns: [ - { name: 'name', type: 'string', required: true }, - { name: 'age', type: 'number' }, - ], - }, - metadata: null, - rowCount: 0, - maxRows: 100, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date('2024-01-01'), - updatedAt: new Date('2024-01-01'), - ...overrides, - } +const TABLE_FIXTURE: TableDefinitionFactoryOptions = { + columns: [ + { name: 'name', type: 'string', required: true }, + { name: 'age', type: 'number' }, + ], + maxRows: 100, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), } /** Additions array the route passed to importAppendRows (2nd positional arg). */ @@ -173,7 +164,7 @@ describe('POST /api/table/[tableId]/import', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition(TABLE_FIXTURE) }) mockImportAppendRows.mockImplementation( async (table: TableDefinition, _additions: unknown, rows: unknown[]) => ({ inserted: rows.map((_, i) => ({ id: `row_${i}` })), @@ -229,7 +220,7 @@ describe('POST /api/table/[tableId]/import', () => { it('returns 400 when the target table is archived', async () => { mockCheckAccess.mockResolvedValueOnce({ ok: true, - table: buildTable({ archivedAt: new Date('2024-01-02') }), + table: createTableDefinition({ ...TABLE_FIXTURE, archivedAt: new Date('2024-01-02') }), }) const response = await callPost(createFormData(createCsvFile('name,age\nAlice,30'))) expect(response.status).toBe(400) @@ -306,7 +297,10 @@ describe('POST /api/table/[tableId]/import', () => { }) it('rejects append when it would exceed the current plan row limit', async () => { - mockCheckAccess.mockResolvedValueOnce({ ok: true, table: buildTable({ rowCount: 99 }) }) + mockCheckAccess.mockResolvedValueOnce({ + ok: true, + table: createTableDefinition({ ...TABLE_FIXTURE, rowCount: 99 }), + }) mockGetMaxRowsPerTable.mockResolvedValueOnce(100) const response = await callPost( createFormData(createCsvFile('name,age\nAlice,30\nBob,40'), { mode: 'append' }) @@ -459,14 +453,13 @@ describe('POST /api/table/[tableId]/import', () => { it('dedupes when sanitized name collides with an existing column', async () => { mockCheckAccess.mockResolvedValueOnce({ ok: true, - table: buildTable({ - schema: { - columns: [ - { name: 'name', type: 'string', required: true }, - { name: 'age', type: 'number' }, - { name: 'email', type: 'string' }, - ], - }, + table: createTableDefinition({ + ...TABLE_FIXTURE, + columns: [ + { name: 'name', type: 'string', required: true }, + { name: 'age', type: 'number' }, + { name: 'email', type: 'string' }, + ], }), }) const response = await callPost( diff --git a/apps/sim/app/api/table/[tableId]/job/cancel/route.test.ts b/apps/sim/app/api/table/[tableId]/job/cancel/route.test.ts index dd0a16b2576..0bc244c2182 100644 --- a/apps/sim/app/api/table/[tableId]/job/cancel/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/job/cancel/route.test.ts @@ -1,10 +1,9 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockMarkJobCanceled, mockGetTableJob, mockAppendTableEvent } = vi.hoisted( () => ({ @@ -31,24 +30,6 @@ vi.mock('@/app/api/table/utils', async () => { import { POST } from '@/app/api/table/[tableId]/job/cancel/route' -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [] }, - metadata: null, - rowCount: 0, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - ...overrides, - } -} - function makeRequest(body: unknown, tableId = 'tbl_1') { const req = new NextRequest(`http://localhost:3000/api/table/${tableId}/job/cancel`, { method: 'POST', @@ -68,7 +49,7 @@ describe('POST /api/table/[tableId]/job/cancel', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition() }) mockMarkJobCanceled.mockResolvedValue(true) mockGetTableJob.mockResolvedValue({ id: 'job_1', diff --git a/apps/sim/app/api/table/[tableId]/query/route.test.ts b/apps/sim/app/api/table/[tableId]/query/route.test.ts index e0857d6589c..9d58e383f7e 100644 --- a/apps/sim/app/api/table/[tableId]/query/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/query/route.test.ts @@ -5,10 +5,9 @@ * (session auth included — the string grammar is name-keyed for every caller), * cursor validation, and the response envelope. */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table/types' const { mockCheckAccess, mockQueryRows, mockGate } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -39,28 +38,6 @@ vi.mock('@/lib/table/rows/service', () => ({ import { encodeCursor } from '@/lib/table/rows/cursor' import { POST } from '@/app/api/table/[tableId]/query/route' -function buildTable(): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { - columns: [ - { id: 'col_aaa', name: 'name', type: 'string' }, - { id: 'col_bbb', name: 'wins', type: 'number' }, - ], - }, - metadata: null, - rowCount: 0, - maxRows: 100, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date('2024-01-01'), - updatedAt: new Date('2024-01-01'), - } -} - function authAs(authType: 'session' | 'internal_jwt') { hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: true, @@ -90,7 +67,18 @@ const EMPTY_RESULT = { describe('POST /api/table/[tableId]/query', () => { beforeEach(() => { vi.clearAllMocks() - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ + columns: [ + { id: 'col_aaa', name: 'name', type: 'string' }, + { id: 'col_bbb', name: 'wins', type: 'number' }, + ], + maxRows: 100, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), + }), + }) mockQueryRows.mockResolvedValue(EMPTY_RESULT) mockGate.mockResolvedValue(null) }) diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts index 8ef809a71a1..cb59859af26 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts @@ -1,10 +1,10 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { EnrichmentRunDetail, TableDefinition } from '@/lib/table' +import type { EnrichmentRunDetail } from '@/lib/table' const { mockCheckAccess, mockLoadEnrichmentDetail } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -25,23 +25,6 @@ vi.mock('@/app/api/table/utils', async () => { import { GET } from '@/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route' -function buildTable(): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [] }, - metadata: null, - rowCount: 1, - maxRows: 1_000_000, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date(), - updatedAt: new Date(), - } -} - function makeRequest(tableId = 'tbl_1', rowId = 'row_1', groupId = 'grp_1') { const req = new NextRequest( `http://localhost:3000/api/table/${tableId}/rows/${rowId}/enrichment/${groupId}` @@ -77,7 +60,7 @@ describe('GET /api/table/[tableId]/rows/[rowId]/enrichment/[groupId]', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition({ rowCount: 1 }) }) }) it('returns the enrichment detail', async () => { diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts index 414dc8d903e..e57151a35e7 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts @@ -9,10 +9,15 @@ * silently changing what clients observe. They intentionally assert the * existing contract rather than an idealized one. */ -import { hybridAuthMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { + createTableDefinition, + hybridAuthMockFns, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, @@ -66,29 +71,6 @@ const WORKSPACE_ID = 'workspace-1' const CREATED_AT = new Date('2024-01-01T00:00:00.000Z') const UPDATED_AT = new Date('2024-02-02T00:00:00.000Z') -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: TABLE_ID, - name: 'People', - description: null, - schema: { - columns: [ - { id: 'col_aaa', name: 'Name', type: 'string' }, - { id: 'col_bbb', name: 'Age', type: 'number' }, - ], - }, - metadata: null, - rowCount: 0, - maxRows: 100, - workspaceId: WORKSPACE_ID, - createdBy: 'user-1', - archivedAt: null, - createdAt: CREATED_AT, - updatedAt: UPDATED_AT, - ...overrides, - } as TableDefinition -} - function buildStoredRow() { return { id: ROW_ID, @@ -133,7 +115,20 @@ beforeEach(() => { vi.clearAllMocks() resetDbChainMock() authAs() - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ + id: TABLE_ID, + columns: [ + { id: 'col_aaa', name: 'Name', type: 'string' }, + { id: 'col_bbb', name: 'Age', type: 'number' }, + ], + maxRows: 100, + workspaceId: WORKSPACE_ID, + createdAt: CREATED_AT, + updatedAt: UPDATED_AT, + }), + }) }) describe('GET /api/table/[tableId]/rows/[rowId]', () => { diff --git a/apps/sim/app/api/table/[tableId]/rows/find/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/find/route.test.ts index 4b892a39340..26ba08d9742 100644 --- a/apps/sim/app/api/table/[tableId]/rows/find/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/rows/find/route.test.ts @@ -1,10 +1,9 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, mockFindRowMatches, mockTableFilterError } = vi.hoisted(() => ({ mockCheckAccess: vi.fn(), @@ -31,24 +30,6 @@ vi.mock('@/lib/table/rows/service', () => ({ import { GET } from '@/app/api/table/[tableId]/rows/find/route' -function buildTable(overrides: Partial = {}): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { columns: [{ name: 'name', type: 'string' }] }, - metadata: null, - rowCount: 0, - maxRows: 100, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date('2024-01-01'), - updatedAt: new Date('2024-01-01'), - ...overrides, - } -} - function callGet( query: Record, { tableId }: { tableId: string } = { tableId: 'tbl_1' } @@ -68,7 +49,15 @@ describe('GET /api/table/[tableId]/rows/find', () => { userId: 'user-1', authType: 'session', }) - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ + ok: true, + table: createTableDefinition({ + columns: [{ name: 'name', type: 'string' }], + maxRows: 100, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), + }), + }) mockTableFilterError.mockReturnValue(null) mockFindRowMatches.mockResolvedValue({ matches: [{ ordinal: 4, rowId: 'row_4', column: 'name' }], diff --git a/apps/sim/app/api/table/[tableId]/rows/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/route.test.ts index 0711127e7a4..b277f2eb83c 100644 --- a/apps/sim/app/api/table/[tableId]/rows/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/rows/route.test.ts @@ -1,10 +1,13 @@ /** * @vitest-environment node */ -import { hybridAuthMockFns } from '@sim/testing' +import { + createTableDefinition, + hybridAuthMockFns, + type TableDefinitionFactoryOptions, +} from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { TableDefinition } from '@/lib/table' const { mockCheckAccess, @@ -59,26 +62,14 @@ vi.mock('@/lib/table/sql', () => ({ import { DELETE, GET, POST, PUT } from '@/app/api/table/[tableId]/rows/route' -function buildTable(): TableDefinition { - return { - id: 'tbl_1', - name: 'People', - description: null, - schema: { - columns: [ - { id: 'col_aaa', name: 'Name', type: 'string' }, - { id: 'col_bbb', name: 'Age', type: 'number' }, - ], - }, - metadata: null, - rowCount: 0, - maxRows: 100, - workspaceId: 'workspace-1', - createdBy: 'user-1', - archivedAt: null, - createdAt: new Date('2024-01-01'), - updatedAt: new Date('2024-01-01'), - } +const TABLE_FIXTURE: TableDefinitionFactoryOptions = { + columns: [ + { id: 'col_aaa', name: 'Name', type: 'string' }, + { id: 'col_bbb', name: 'Age', type: 'number' }, + ], + maxRows: 100, + createdAt: new Date('2024-01-01'), + updatedAt: new Date('2024-01-01'), } function authAs(authType: 'session' | 'internal_jwt') { @@ -109,7 +100,7 @@ function callGet(query: Record) { describe('POST /api/table/[tableId]/rows', () => { beforeEach(() => { vi.clearAllMocks() - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition(TABLE_FIXTURE) }) mockValidateRowData.mockResolvedValue({ valid: true }) mockInsertRow.mockResolvedValue({ id: 'row_1', @@ -166,7 +157,7 @@ describe('POST /api/table/[tableId]/rows', () => { describe('GET /api/table/[tableId]/rows', () => { beforeEach(() => { vi.clearAllMocks() - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition(TABLE_FIXTURE) }) mockQueryRows.mockResolvedValue({ rows: [ { @@ -313,7 +304,7 @@ describe('GET /api/table/[tableId]/rows', () => { describe('PUT/DELETE /api/table/[tableId]/rows — predicate filters', () => { beforeEach(() => { vi.clearAllMocks() - mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() }) + mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition(TABLE_FIXTURE) }) mockUpdateRowsByFilter.mockResolvedValue({ affectedCount: 1, affectedRowIds: ['row_1'] }) mockDeleteRowsByFilter.mockResolvedValue({ affectedCount: 1, affectedRowIds: ['row_1'] }) }) diff --git a/packages/testing/src/factories/index.ts b/packages/testing/src/factories/index.ts index 586f7fea59b..e471c3e24b8 100644 --- a/packages/testing/src/factories/index.ts +++ b/packages/testing/src/factories/index.ts @@ -118,6 +118,20 @@ export { type SerializedConnection, type SerializedWorkflow, } from './serialized-block.factory' +// Table factories +export { + createTableColumn, + createTableDefinition, + createTableRow, + type TableColumnFactoryOptions, + type TableColumnFixture, + type TableColumnType, + type TableDefinitionFactoryOptions, + type TableDefinitionFixture, + type TableLocksFixture, + type TableRowFactoryOptions, + type TableRowFixture, +} from './table.factory' // Tool mock responses export { mockDriveResponses, diff --git a/packages/testing/src/factories/table.factory.ts b/packages/testing/src/factories/table.factory.ts index 020787735ec..70e418618d5 100644 --- a/packages/testing/src/factories/table.factory.ts +++ b/packages/testing/src/factories/table.factory.ts @@ -2,9 +2,18 @@ import { generateShortId } from '@sim/utils/id' const COLUMN_SUFFIX_ALPHABET = 'abcdefghijklmnopqrstuvwxyz0123456789_' -export type TableColumnType = 'string' | 'number' | 'boolean' | 'date' | 'json' +export type TableColumnType = + | 'string' + | 'number' + | 'currency' + | 'boolean' + | 'date' + | 'json' + | 'select' export interface TableColumnFixture { + /** Stable storage key. Absent on legacy columns, where the name is the key. */ + id?: string name: string type: TableColumnType required?: boolean @@ -20,6 +29,7 @@ export interface TableRowFixture { } export interface TableColumnFactoryOptions { + id?: string name?: string type?: TableColumnType required?: boolean @@ -39,6 +49,7 @@ export interface TableRowFactoryOptions { */ export function createTableColumn(options: TableColumnFactoryOptions = {}): TableColumnFixture { return { + id: options.id, name: options.name ?? `column_${generateShortId(6, COLUMN_SUFFIX_ALPHABET)}`, type: options.type ?? 'string', required: options.required, @@ -60,3 +71,88 @@ export function createTableRow(options: TableRowFactoryOptions = {}): TableRowFi updatedAt: options.updatedAt ?? timestamp, } } + +/** Per-table mutation locks. All false means fully unlocked. */ +export interface TableLocksFixture { + schemaLocked: boolean + insertLocked: boolean + updateLocked: boolean + deleteLocked: boolean +} + +/** + * Structural stand-in for `TableDefinition` in `apps/sim/lib/table/types.ts`. + * Declared here rather than imported because `@sim/testing` must not depend on + * `apps/*` (enforced by `scripts/check-monorepo-boundaries.ts`). + */ +export interface TableDefinitionFixture { + id: string + name: string + description: string | null + schema: { columns: TableColumnFixture[] } + metadata: Record | null + rowCount: number + maxRows: number + workspaceId: string + folderId?: string | null + createdBy: string + locks: TableLocksFixture + archivedAt: Date | string | null + createdAt: Date | string + updatedAt: Date | string +} + +export interface TableDefinitionFactoryOptions { + id?: string + name?: string + description?: string | null + /** Shorthand for `schema.columns` — the field call sites vary most. */ + columns?: TableColumnFixture[] + metadata?: Record | null + rowCount?: number + maxRows?: number + workspaceId?: string + folderId?: string | null + createdBy?: string + locks?: TableLocksFixture + archivedAt?: Date | string | null + createdAt?: Date | string + updatedAt?: Date | string +} + +const UNLOCKED_TABLE_LOCKS: TableLocksFixture = { + schemaLocked: false, + insertLocked: false, + updateLocked: false, + deleteLocked: false, +} + +/** + * Creates a table definition fixture with sensible defaults — the shape route + * and service tests hand back from a table lookup. + * + * Callers most often override `columns` (the table's schema), `rowCount`, + * `maxRows`, and `archivedAt`. + */ +export function createTableDefinition( + options: TableDefinitionFactoryOptions = {} +): TableDefinitionFixture { + const timestamp = new Date() + + return { + id: options.id ?? 'tbl_1', + name: options.name ?? 'People', + description: options.description ?? null, + schema: { columns: options.columns ?? [] }, + metadata: options.metadata ?? null, + rowCount: options.rowCount ?? 0, + maxRows: options.maxRows ?? 1_000_000, + workspaceId: options.workspaceId ?? 'workspace-1', + folderId: options.folderId, + createdBy: options.createdBy ?? 'user-1', + locks: options.locks ?? UNLOCKED_TABLE_LOCKS, + archivedAt: options.archivedAt ?? null, + createdAt: options.createdAt ?? timestamp, + updatedAt: options.updatedAt ?? timestamp, + } +} From fe78d61c73bde195f46390bc18847750a0af23f3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 20:12:19 -0700 Subject: [PATCH 10/12] perf(table): project only the job field the row count needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../lib/table/__tests__/update-row.test.ts | 4 + .../sim/lib/table/application/context.test.ts | 37 +++--- apps/sim/lib/table/application/rows.test.ts | 115 ++++++++---------- apps/sim/lib/table/application/rows.ts | 25 +++- apps/sim/lib/table/column-keys.ts | 7 +- apps/sim/lib/table/jobs/service.test.ts | 92 ++++++++++++-- apps/sim/lib/table/jobs/service.ts | 54 +++++--- apps/sim/lib/table/rows/service.ts | 5 + apps/sim/lib/table/service.test.ts | 8 +- 9 files changed, 230 insertions(+), 117 deletions(-) diff --git a/apps/sim/lib/table/__tests__/update-row.test.ts b/apps/sim/lib/table/__tests__/update-row.test.ts index e7472eb40fd..46777d4b185 100644 --- a/apps/sim/lib/table/__tests__/update-row.test.ts +++ b/apps/sim/lib/table/__tests__/update-row.test.ts @@ -558,6 +558,10 @@ describe('batchUpdateRows — per-row partial merge', () => { * The safety argument is that a merge cannot newly violate uniqueness on a * column it leaves alone: that value is the one already stored, and it * satisfied the constraint when it was written. + * + * The one case that does not cover is a unique constraint added to a column + * that already held duplicates — such a row is no longer blocked from edits + * elsewhere in it, which is the intended outcome. */ describe('updateRow — uniqueness probe scoping', () => { beforeEach(() => { diff --git a/apps/sim/lib/table/application/context.test.ts b/apps/sim/lib/table/application/context.test.ts index 8ffe66c6196..5a41c4a48cc 100644 --- a/apps/sim/lib/table/application/context.test.ts +++ b/apps/sim/lib/table/application/context.test.ts @@ -47,6 +47,23 @@ async function withUnhandledRejectionWatch(body: () => Promise): Promise void } { + let releaseTable: (table: unknown) => void = () => {} + getTableById.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseTable = resolve + }) + ) + return { + release: () => releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' }), + } +} + describe('table application context', () => { beforeEach(() => { vi.clearAllMocks() @@ -73,13 +90,7 @@ describe('table application context', () => { }) it('starts the workspace load without waiting for the table when a workspace is asserted', async () => { - let releaseTable: (table: unknown) => void = () => {} - getTableById.mockImplementationOnce( - () => - new Promise((resolve) => { - releaseTable = resolve - }) - ) + const { release } = deferTableLoad() const pending = resolveActiveTableContext({ tableId: 'table-1', @@ -90,18 +101,12 @@ describe('table application context', () => { expect(loadWorkspace).toHaveBeenCalledWith('workspace-1') - releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' }) + release() await expect(pending).resolves.toMatchObject({ tableId: 'table-1', workspaceId: 'workspace-1' }) }) it('waits for the table before loading a workspace when none is asserted', async () => { - let releaseTable: (table: unknown) => void = () => {} - getTableById.mockImplementationOnce( - () => - new Promise((resolve) => { - releaseTable = resolve - }) - ) + const { release } = deferTableLoad() const pending = resolveActiveTableContext({ tableId: 'table-1' }) await Promise.resolve() @@ -109,7 +114,7 @@ describe('table application context', () => { expect(loadWorkspace).not.toHaveBeenCalled() - releaseTable({ id: 'table-1', workspaceId: 'workspace-1', name: 'Contacts' }) + release() await expect(pending).resolves.toMatchObject({ tableId: 'table-1', workspaceId: 'workspace-1' }) expect(loadWorkspace).toHaveBeenCalledWith('workspace-1') }) diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index 2424fb0a7da..23d52dcb87c 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -172,6 +172,22 @@ const TABLE: TableDefinition = { const PRINCIPAL = { kind: 'session' as const, userId: 'user-1', sessionId: 'session-1' } +/** + * The active-table context every row command resolves before it does any work. + * Pass a variant table when a test needs a different schema — the surrounding + * workspace scope is the same for every command under test. + */ +function contextFor(table: TableDefinition = TABLE) { + return { + tableId: table.id, + table, + workspaceId: table.workspaceId, + workspaceOrganizationId: 'organization-1', + allowPersonalApiKeys: true, + billedAccountUserId: 'billing-owner-1', + } +} + describe('table predicate translation', () => { it('maps invalid run filters to the shared row validation error', () => { expect(() => @@ -210,14 +226,7 @@ describe('replaceProjectedWireRows application command', () => { beforeEach(() => { vi.clearAllMocks() mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor()) mockAssertRowCapacity.mockResolvedValue(10_000) mockWithLockedTable.mockImplementation( async (_tableId: string, run: (table: TableDefinition, trx: unknown) => unknown) => @@ -457,14 +466,7 @@ describe('replaceTableRows application use case', () => { beforeEach(() => { vi.clearAllMocks() mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor()) mockReplaceRowsPrimitive.mockResolvedValue({ deletedCount: 2, insertedCount: 1 }) }) @@ -570,14 +572,7 @@ describe('row query and upsert application semantics', () => { beforeEach(() => { vi.clearAllMocks() mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor()) }) it('rejects a malformed POST query cursor before querying storage', async () => { @@ -831,14 +826,7 @@ describe('table row write secret provenance defaulting', () => { beforeEach(() => { vi.clearAllMocks() mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor()) mockValidateRowData.mockResolvedValue({ valid: true }) mockValidateBatchRows.mockResolvedValue({ valid: true }) mockInsertRow.mockResolvedValue(ROW) @@ -898,14 +886,7 @@ describe('table row write secret provenance defaulting', () => { ...TABLE, schema: { columns: [{ id: 'column_name', name: 'name', type: 'string' }] }, } - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: filterableTable, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor(filterableTable)) await updateTableRows.execute({ principal: PRINCIPAL, @@ -995,14 +976,7 @@ describe('unknown column names under strictWrite', () => { beforeEach(() => { vi.clearAllMocks() mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor()) mockValidateRowData.mockResolvedValue({ valid: true }) mockValidateBatchRows.mockResolvedValue({ valid: true }) mockInsertRow.mockResolvedValue({ id: 'row-1', data: {} }) @@ -1161,14 +1135,7 @@ describe('row data keying', () => { beforeEach(() => { vi.clearAllMocks() mockResolvePermission.mockResolvedValue('write') - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: TABLE, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue(contextFor()) mockAssertRowCapacity.mockResolvedValue(10_000) mockCreateSecretProvenance.mockReturnValue({ complete: true, columns: {} }) mockIsScopeCompatible.mockReturnValue(true) @@ -1215,6 +1182,31 @@ describe('row data keying', () => { ) }) + it('persists an unrecognised key on the lax id wire, unlike the name wire', async () => { + // The asymmetry a non-strict id-keyed caller sees, pinned deliberately: the + // name path drops what it cannot resolve, the id path stores what it is + // given. This is what the grid does today via the identity `dataIn` in + // `row-wire.ts`, so the discriminator preserved it rather than changing it. + // Closing it is a behaviour change and belongs with the route migration. + await updateTableRow.execute({ + principal: PRINCIPAL, + input: { + tableId: TABLE.id, + rowId: 'row-1', + data: { 'column-name': 'Ada', 'no-such-column': 'x' }, + strictWrite: false, + dataKeying: 'ids', + }, + }) + + expect(mockUpdateRow).toHaveBeenCalledWith( + expect.objectContaining({ data: { 'column-name': 'Ada', 'no-such-column': 'x' } }), + TABLE, + expect.any(String), + expect.anything() + ) + }) + it('translates a name-keyed write to storage ids', async () => { await updateTableRow.execute({ principal: PRINCIPAL, @@ -1255,14 +1247,9 @@ describe('row data keying', () => { // Two production tables still carry pre-backfill columns with no `id`. // Their storage key is the name, so a strict id-keyed write naming one must // be accepted, not refused as unknown. - mockResolveContext.mockResolvedValue({ - tableId: TABLE.id, - table: { ...TABLE, schema: { columns: [{ name: 'legacy', type: 'string' }] } }, - workspaceId: TABLE.workspaceId, - workspaceOrganizationId: 'organization-1', - allowPersonalApiKeys: true, - billedAccountUserId: 'billing-owner-1', - }) + mockResolveContext.mockResolvedValue( + contextFor({ ...TABLE, schema: { columns: [{ name: 'legacy', type: 'string' }] } }) + ) await expect( updateTableRow.execute({ diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index a1dd9385a82..2f00f7b9696 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -83,7 +83,19 @@ interface TableScopedInput { requestId?: string } -/** The write policy `strictWrite` selects, for the row-service primitives. */ +/** + * The write policy `strictWrite` selects, for the row-service primitives. + * + * `strictWrite` means the calling surface publishes the stricter `/api/v2` write + * contract: a row naming a column the table does not have is refused rather than + * having that key dropped, and a value the column's type cannot coerce is + * answered with a 400 rather than stored as `null`. + * + * Absent — every first-party surface, and the only behavior any of them has ever + * had: the workspace grid, the internal `/api/table` routes, `/api/v1`, the + * Copilot table tools, and the executor's Table block all drop the unknown key + * and blank the uncoercible cell. + */ function rowWriteOptions(input: { strictWrite: boolean }): RowWriteOptions { return input.strictWrite ? { uncoercibleValues: 'reject' } : {} } @@ -729,7 +741,16 @@ function projectedRowsSecretProvenance( }) } -/** Atomically validates name-keyed projected rows against the locked schema and replaces the table. */ +/** + * Atomically validates name-keyed projected rows against the locked schema and + * replaces the table. + * + * Deliberately carries no {@link TableRowDataKeying}: unlike the six generic + * write use cases this one is not surface-agnostic. Its resolved-secret gate and + * its "row matches no column" check both compare by `column.name` (see + * {@link projectedRowsForTable}), and its only caller is Copilot's + * `Function.execute` output — keys a model can only have written as names. + */ export const replaceProjectedWireRows = defineAuthorizedTableUseCase({ operation: tableOperations.replaceRows, resolveContext: ({ input }: { input: ReplaceProjectedWireRowsInput }) => diff --git a/apps/sim/lib/table/column-keys.ts b/apps/sim/lib/table/column-keys.ts index 8ee9620c3bc..5f8b0e7e19e 100644 --- a/apps/sim/lib/table/column-keys.ts +++ b/apps/sim/lib/table/column-keys.ts @@ -207,8 +207,11 @@ export function rowDataNameToId(data: RowData, idByName: Map): R * row, and letting it through would reinstate the silent drop for exactly the * callers most likely to believe they had written something. */ -export function unknownColumnNames(data: RowData, idByName: ReadonlyMap): string[] { - return Object.keys(data).filter((name) => !idByName.has(name)) +export function unknownColumnNames( + data: RowData, + knownKeys: ReadonlyMap +): string[] { + return Object.keys(data).filter((key) => !knownKeys.has(key)) } /** diff --git a/apps/sim/lib/table/jobs/service.test.ts b/apps/sim/lib/table/jobs/service.test.ts index 34ea40301b2..b537f1f9cab 100644 --- a/apps/sim/lib/table/jobs/service.test.ts +++ b/apps/sim/lib/table/jobs/service.test.ts @@ -1,11 +1,12 @@ /** * @vitest-environment node */ -import { schemaMock } from '@sim/testing' -import { describe, expect, it } from 'vitest' +import { dbChainMockFns, resetDbChainMock, schemaMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { EMPTY_JOB_FIELDS, type LatestJobRow, + latestJobsForTables, latestNonExportJobJson, mapJobRow, } from '@/lib/table/jobs/service' @@ -17,7 +18,7 @@ function job(overrides: Partial): LatestJobRow { status: 'running', rowsProcessed: 0, error: null, - payload: null, + doomedCount: null, ...overrides, } } @@ -29,7 +30,7 @@ describe('mapJobRow', () => { }) it('projects a running delete job and its remaining doomed rows', () => { - expect(mapJobRow(job({ rowsProcessed: 4, payload: { doomedCount: 10 } }))).toEqual({ + expect(mapJobRow(job({ rowsProcessed: 4, doomedCount: 10 }))).toEqual({ jobStatus: 'running', jobId: 'job-1', jobType: 'delete', @@ -41,23 +42,19 @@ describe('mapJobRow', () => { it('ignores doomedCount once the delete job is terminal', () => { expect( - mapJobRow(job({ status: 'ready', rowsProcessed: 4, payload: { doomedCount: 10 } })) - .pendingDeleteRemaining + mapJobRow(job({ status: 'ready', rowsProcessed: 4, doomedCount: 10 })).pendingDeleteRemaining ).toBe(0) }) it('ignores doomedCount for a running job that is not a delete', () => { expect( - mapJobRow(job({ type: 'import', rowsProcessed: 4, payload: { doomedCount: 10 } })) - .pendingDeleteRemaining + mapJobRow(job({ type: 'import', rowsProcessed: 4, doomedCount: 10 })).pendingDeleteRemaining ).toBe(0) }) it('treats a missing doomedCount as zero and never goes negative', () => { expect(mapJobRow(job({ rowsProcessed: 4 })).pendingDeleteRemaining).toBe(0) - expect( - mapJobRow(job({ rowsProcessed: 25, payload: { doomedCount: 10 } })).pendingDeleteRemaining - ).toBe(0) + expect(mapJobRow(job({ rowsProcessed: 25, doomedCount: 10 })).pendingDeleteRemaining).toBe(0) }) it('carries a failed job error through', () => { @@ -103,6 +100,77 @@ describe('latestNonExportJobJson', () => { // No drift test for the projected field list: the fragment derives its // jsonb pairs from JOB_PROJECTION, which `satisfies Record`. A missing field is a compile error, which is + // LatestJobRow, Column | SQL>`. A missing field is a compile error, which is // stronger than anything asserted here could be. + + /** + * `table_jobs.payload` also holds a delete job's unbounded `excludeRowIds`, and + * this read runs on essentially every table request — so selecting the whole + * column is a payload leak the type system cannot see (`doomedCount` would still + * be present, just derived in JS). Only the rendered pair list shows it. + */ + it('projects doomedCount out of the payload instead of the payload column', () => { + const pairs = renderPairs() + expect(pairs.sql).toContain("'doomedCount'") + expect(pairs.sql).toContain("->'doomedCount'") + expect(pairs.sql).not.toContain("'payload'") + expect(pairs.sql).not.toContain(', payload') + }) +}) + +/** The `jsonb_build_object` key/value list the lateral builds from JOB_PROJECTION. */ +function renderPairs(): { sql: string; params: unknown[] } { + // double-cast-allowed: the mocked drizzle `sql` tag exposes the raw template parts + const fragment = latestNonExportJobJson(schemaMock.userTableDefinitions.id) as unknown as { + values: Array<{ fragments?: unknown[]; toSQL?: () => { sql: string; params: unknown[] } }> + } + const join = fragment.values.find((value) => Array.isArray(value?.fragments)) + if (!join?.toSQL) throw new Error('lateral no longer builds its pairs with sql.join') + return join.toSQL() +} + +/** + * The list endpoint runs this once per page, so the batch read must narrow too. It + * shares JOB_PROJECTION with the lateral, and this pins that sharing down. + */ +describe('latestJobsForTables', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + it('selects doomedCount rather than the whole payload column', async () => { + await latestJobsForTables(['table-1']) + + const projection = dbChainMockFns.selectDistinctOn.mock.calls[0][1] as Record + expect(Object.keys(projection)).toEqual([ + 'tableId', + 'id', + 'type', + 'status', + 'rowsProcessed', + 'error', + 'doomedCount', + ]) + expect(projection).not.toHaveProperty('payload') + expect(projection.doomedCount).not.toBe(schemaMock.tableJobs.payload) + }) + + it('still derives a running delete job from the narrowed row', async () => { + dbChainMockFns.orderBy.mockResolvedValueOnce([ + { + tableId: 'table-1', + id: 'job-1', + type: 'delete', + status: 'running', + rowsProcessed: 4, + error: null, + doomedCount: 10, + }, + ]) + + const jobs = await latestJobsForTables(['table-1']) + + expect(jobs.get('table-1')).toMatchObject({ jobId: 'job-1', pendingDeleteRemaining: 6 }) + }) }) diff --git a/apps/sim/lib/table/jobs/service.ts b/apps/sim/lib/table/jobs/service.ts index 767345bc357..0f2f4ed27b0 100644 --- a/apps/sim/lib/table/jobs/service.ts +++ b/apps/sim/lib/table/jobs/service.ts @@ -61,15 +61,17 @@ export interface LatestJobRow { status: string rowsProcessed: number error: string | null - payload: unknown + /** + * The one field {@link mapJobRow} needs out of {@link TableDeleteJobPayload}, + * projected on its own so the read never drags the rest of the payload back. + * `null` when the job has no payload, or a payload without the key. + */ + doomedCount: number | null } export function mapJobRow(row: LatestJobRow | null | undefined): DerivedJobFields { if (!row) return EMPTY_JOB_FIELDS - const doomedCount = - row.type === 'delete' && row.status === 'running' - ? ((row.payload as TableDeleteJobPayload | null)?.doomedCount ?? 0) - : 0 + const doomedCount = row.type === 'delete' && row.status === 'running' ? (row.doomedCount ?? 0) : 0 return { jobStatus: row.status as TableDefinition['jobStatus'], jobId: row.id, @@ -81,10 +83,27 @@ export function mapJobRow(row: LatestJobRow | null | undefined): DerivedJobField } /** - * The columns {@link mapJobRow} reads, as one source for both job reads: the - * batch `DISTINCT ON` selects it directly, and {@link latestNonExportJobJson} - * derives its `jsonb_build_object` pairs from it. Adding a field here reaches - * both — the two cannot drift into disagreeing about what a job row is. + * The one number {@link mapJobRow} wants out of `table_jobs.payload`, extracted in + * SQL so the payload itself never crosses the wire. `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 otherwise ship that id list on every read, forever. + * + * `->` (not `->>`) keeps the value jsonb: postgres-js decodes jsonb through its + * built-in `JSON.parse` handler (OID 3802), so this arrives as a JS `number`, or + * `null` for a missing payload, a payload without the key, a payload that is not an + * object, or an explicit JSON `null`. `->>` would hand back text and force a parse. + * All four null cases collapse to the same `?? 0` {@link mapJobRow} already applied + * to `payload?.doomedCount`, so no boundary coercion is needed. + */ +const doomedCountExpr = sql`${tableJobs.payload}->'doomedCount'` + +/** + * What {@link mapJobRow} reads, as one source for both job reads: the batch + * `DISTINCT ON` selects it directly, and {@link latestNonExportJobJson} derives its + * `jsonb_build_object` pairs from it. Adding or renaming a field here reaches both — + * the two cannot drift into disagreeing about what a job row is. Entries may be a + * plain `Column` or a derived `SQL` expression; both render in either position. */ const JOB_PROJECTION = { id: tableJobs.id, @@ -92,8 +111,8 @@ const JOB_PROJECTION = { status: tableJobs.status, rowsProcessed: tableJobs.rowsProcessed, error: tableJobs.error, - payload: tableJobs.payload, -} as const satisfies Record + doomedCount: doomedCountExpr, +} as const satisfies Record /** * The latest non-export job for one table, as a single jsonb value correlated to @@ -106,17 +125,18 @@ const JOB_PROJECTION = { * cannot simply be skipped: a table's reported `rowCount` is the stored count minus * this job's `pendingDeleteRemaining`, so the count and the job row are one read. * - * Semantics match the batch {@link latestJobsForTables} exactly — exports excluded - * (they run concurrently and have their own client surface), newest `started_at` - * first, one row. `NULL` when the table has no such job; feed the result straight to - * {@link mapJobRow}. + * Semantics match the batch {@link latestJobsForTables} exactly — same + * {@link JOB_PROJECTION} fields (which is `doomedCount` extracted from the payload, + * never the payload itself), exports excluded (they run concurrently and have their + * own client surface), newest `started_at` first, one row. `NULL` when the table has + * no such job; feed the result straight to {@link mapJobRow}. */ export function latestNonExportJobJson(outerTableId: Column): SQL { // Keys come from JOB_PROJECTION, never from input, so `sql.raw` here cannot // carry anything a caller controls. - const pairs = Object.entries(JOB_PROJECTION).flatMap(([key, column]) => [ + const pairs = Object.entries(JOB_PROJECTION).flatMap(([key, expression]) => [ sql.raw(`'${key}'`), - column, + expression, ]) return sql`( select jsonb_build_object(${sql.join(pairs, sql`, `)}) diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index 291593471f9..bf96ebf1ce5 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1618,6 +1618,11 @@ export async function updateRow( // probe opens its own transaction and queries once per unique column, so on a // table that has any unique column this was several round trips on every // edit, including edits nowhere near one. + // + // The one case this does not cover is a unique constraint added to a column + // that already held duplicates: such a row is no longer blocked from edits + // elsewhere in it. That is the intended outcome — an unrelated cell edit + // should not fail on data it did not write. const patchedColumnIds = new Set(Object.keys(data.data)) const patchedUniqueColumns = getUniqueColumns(table.schema).filter((column) => patchedColumnIds.has(getColumnId(column)) diff --git a/apps/sim/lib/table/service.test.ts b/apps/sim/lib/table/service.test.ts index f7b276a2ed7..ea6ade3ea84 100644 --- a/apps/sim/lib/table/service.test.ts +++ b/apps/sim/lib/table/service.test.ts @@ -174,7 +174,7 @@ describe('getTableById job derivation', () => { status: 'running', rowsProcessed: 4, error: null, - payload: { doomedCount: 10 }, + doomedCount: 10, }, }), ]) @@ -199,7 +199,7 @@ describe('getTableById job derivation', () => { status: 'running', rowsProcessed: 4, error: null, - payload: { doomedCount: 10 }, + doomedCount: 10, }, }), ]) @@ -216,7 +216,7 @@ describe('getTableById job derivation', () => { status: 'ready', rowsProcessed: 4, error: null, - payload: { doomedCount: 10 }, + doomedCount: 10, }, }), ]) @@ -258,7 +258,7 @@ describe('getTableById job derivation', () => { status: 'running', rowsProcessed: 1, error: null, - payload: { doomedCount: 5 }, + doomedCount: 5, }, }), ]) From 8bfaee465a38d924aae709b7fa2d7b96fe140e99 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 20:13:49 -0700 Subject: [PATCH 11/12] chore(table): drop two comments the code already says One restated the identifier below it (checkUniqueConstraintsDb), and one was a section-divider banner in the factories barrel, which the repo's comment convention rules out. Everything else that survives is a why the code cannot express: round-trip rationale, sql.raw input safety, the narrowing invariant and the one case it does not cover. --- apps/sim/lib/table/rows/service.ts | 2 -- packages/testing/src/factories/index.ts | 1 - 2 files changed, 3 deletions(-) diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index bf96ebf1ce5..5495e7ae38f 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -1610,8 +1610,6 @@ export async function updateRow( ) } - // Check unique constraints using optimized database query. - // // Scoped to the columns this patch actually writes. A merge cannot newly // violate uniqueness on a column it leaves alone: that value is the one // already stored, and it satisfied the constraint when it was written. The diff --git a/packages/testing/src/factories/index.ts b/packages/testing/src/factories/index.ts index e471c3e24b8..a5f1e00ac24 100644 --- a/packages/testing/src/factories/index.ts +++ b/packages/testing/src/factories/index.ts @@ -118,7 +118,6 @@ export { type SerializedConnection, type SerializedWorkflow, } from './serialized-block.factory' -// Table factories export { createTableColumn, createTableDefinition, From 5e0a2cebd5fc93f91547128a34a68cf957daee78 Mon Sep 17 00:00:00 2001 From: Waleed Date: Mon, 17 Aug 2026 22:51:18 -0700 Subject: [PATCH 12/12] refactor(table): move the internal row routes onto the application boundary (#6809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(table): move the single-row route onto the application boundary The hottest table write path authorized in its own handler and queried the database from the adapter — the two things a surface adapter must never do. It now declares itself with defineInternalJsonRoute against the readRow, updateRow and deleteRow use cases: 127 lines instead of 274, with no db import, no drizzle import and no checkAccess. Doing that surfaced why the violation existed. Write-provenance resolution needs the canonical schema to map a caller's column key to the storage column it certifies, and the adapter could only do that because it was already loading the table illegally. The envelope is now split along the real seam: the adapter reads the header and payload field, which is transport, and the use case resolves the selections against the canonical table, which is domain. That split has to preserve a distinction the defaulting logic would erase. An internal caller that sends no envelope stays deliberately untracked; defaulting it to an exact-empty stamp would certify "this write introduced no secrets" on a runtime write that may well have introduced some. Only an interactive caller certifies exact-empty, over the storage columns its write actually persists. Two further changes fell out of it: present() now receives the same { principal, input } pair its sibling hooks responseHeaders and finalizeResponse already got. This route serves a session and a workflow execution on one path and owes them different column keyings, so rendering per caller kind is presentation rather than domain. That was a gap in the builder, not a special case for this route. tableRowWireSchema describes what the single-row routes actually return. The contract claimed a full TableRow, carrying the executions sidecar and Date objects — true of the list and query routes, and never true here. The hand- rolled handler was never checked against its own contract, so the drift was invisible until the builder started validating it. Wire changes, both deliberate and both narrower than before: a cross-tenant table now conceals as 404 where the old blanket handler answered 403, while an in-workspace role denial still answers 403. Nothing in hooks/queries/tables.ts branches on either. Verified to fail: forcing one keying, dropping the actor, pre-resolving the envelope, certifying an untracked internal write, and skipping the bundle completeness check each turn the covering tests red. * refactor(table): move the upsert route onto the application boundary Same shape as the single-row route: declares itself against upsertTableRow, hands the provenance envelope over unresolved, and derives its column keying from the principal rather than assuming one. The keying and presentation helpers the two routes shared are now in row-wire beside the translators they wrap, so a third route does not restate them. Two response details changed on purpose. The row now carries `position`, which the use case always had and this route alone omitted — every other single-row response already returned it, and the contract now describes one shape instead of three. The upsert result also carries read-back provenance, which the route previously assembled for itself. The surface had no route-level tests; it has six now, covering both caller keyings, both operations, and the envelope handover. * refactor(table): move the enrichment-detail route onto the application boundary The last internal adapter that queried the database for itself. It now runs through readTableRowEnrichmentDetail, a new use case that shares tableOperations.readRow — reading a cell's cascade breakdown is a projection of the same row under the same role, not a second semantic operation. Its tests move to the same seam and gain one the old suite could not express: a cross-tenant table now conceals rather than confirming it exists. * fix(table): mirror the storage rule when keying write provenance storageKeyByWireKey mapped an unrecognised id-keyed key to null, but rowDataToStorage persists that key when the caller is not writing strictly. A cell would have been written with no provenance recorded, under a stamp still marked complete — the same failure the bundle completeness check exists to prevent, arriving through the keying map instead of the selection set. The two wires genuinely differ and the code now says so: the name path drops an unrecognised key, so it maps to null; the id path stores what it is given, so every key it sends is a storage key. Unreachable today, since no delegated surface uses id keying and a session bundle is refused earlier. Fixed because the function's stated invariant — that it mirrors how the row data itself is normalized — was not true. Also corrects the delegated principal fixture in these tests, which used a kind that is not in the Principal union, so the subject-id branch was never actually exercised. It is now, and the scope check is asserted to receive the acting principal's own subject id. Verified to fail: restoring the schema-based lookup turns the covering test red. * fix(table): restore executor access to the migrated row routes The migration swapped checkSessionOrInternalAuth for the delegation policy, and that broke every Table block call to these endpoints in two ways at once. The old policy accepted a legacy internal token. The new one requires a delegation token, which the executor only mints when the tool asks for it — and none of the four table row tools did, so get/update/delete/upsert row would each have failed with a 401. The knowledge tools already declare it, because their routes migrated first. Even with a valid token the operations denied the caller: readRow, updateRow, deleteRow and upsertRow ran under a policy whose delegatedServices is ['copilot'], so the executor got a 403. They now use the tool-facing policy that already existed for the group operations. Neither was visible to the route tests, which mock the auth policy wholesale — so the gap is closed at the layer that actually decides: one test pinning that each tool requests delegation and that its operation admits the executor, mutation-verified against both failure modes. Also fixes findings from the review pass: the read surfaces no longer load an executions sidecar none of them put on the wire (two readers rather than a flag, so a caller cannot silently read an empty one); the provenance name index is built once per batch instead of once per row; the uniqueness comment now names the concurrent-insert race as well as the retro-added constraint; and the presenter context gets NoInfer plus a note that the v2 builder passes something different. * fix(table): keep the lock on a 423 and pin the remaining wire changes The rows error policy was built on the concealment base rather than the lock-aware one, so a TableLockedError fell through to the generic handler and the response lost its `lock` field — the only thing that tells a client which lock to clear. A row write is exactly as lockable as a group mutation, so it now shares that base. Also pins the two wire changes the review found undocumented: a mismatched workspace assertion answers 404 rather than 400, which is a superset of the cross-tenant concealment already intended, and an unclassified failure answers the builder's shared "Internal server error" rather than the old per-route text. Both are consistent with the ~80 routes already on this builder; they are asserted so they read as decisions rather than drift. Verified to fail: reverting the policy base turns the lock test red. * refactor(table): apply the quality pass Four parallel reviews (reuse, simplification, efficiency, altitude). The highest-value finding cut against the branch's own purpose: the rows error policy sat in the barrel-exported route-policies module, so its import of the 1,200-line row use-case graph was paid by every one of the ~28 table routes that can never throw a row error — the barrel's import cost went from ~1.1s to ~1.7s. row-route-policies.ts already existed for exactly this and is deliberately not re-exported; the policy now lives there with its v2 sibling. The upsert path still loaded an executions sidecar no surface puts on the wire, and did it inside the write transaction, holding it open for a discarded result. The read path got that fix earlier; the write path next to it did not. rowKeyingForPrincipal fell through to name keying for anything that was not a session. The operation policy admits API-key principals, so the first one to reach these routes would have had every id-keyed cell dropped and the write reported as successful. It is now an exhaustive switch over the two kinds the auth policy yields — which immediately failed three tests using a principal kind that exists nowhere in the repo, so those fixtures are real now too. Also: reuses toWireTimestamp and createUnknownTableRowSecretProvenance instead of re-inlining them; shares one helper for the provenance choice the update and upsert use cases both make; keeps one canonical actorClientId doc with two cross-references; merges two maps keyed by the same string into one; stops round-tripping the principal through the legacy AuthType enum; hoists the presenter function type out of both conditional branches; drops a subsumed test; and freezes the shared locks fixture so a mutating test cannot poison its siblings. --- .../enrichment/[groupId]/route.test.ts | 141 +++---- .../[rowId]/enrichment/[groupId]/route.ts | 67 ++- .../[tableId]/rows/[rowId]/route.test.ts | 391 ++++++++---------- .../api/table/[tableId]/rows/[rowId]/route.ts | 341 ++++----------- .../table/[tableId]/rows/upsert/route.test.ts | 148 +++++++ .../api/table/[tableId]/rows/upsert/route.ts | 157 +++---- .../app/api/table/row-secret-provenance.ts | 58 +++ apps/sim/app/api/table/row-wire.ts | 65 ++- .../sim/app/api/table/table-tool-auth.test.ts | 45 ++ apps/sim/lib/api/contracts/tables.ts | 39 +- .../api/server/routes/internal-json-route.ts | 40 +- .../lib/table/__tests__/update-row.test.ts | 36 ++ apps/sim/lib/table/api/row-route-policies.ts | 28 +- .../lib/table/application/operations.test.ts | 13 +- apps/sim/lib/table/application/operations.ts | 17 +- .../application/row-secret-provenance.test.ts | 216 ++++++++++ .../application/row-secret-provenance.ts | 146 +++++++ apps/sim/lib/table/application/rows.test.ts | 3 + apps/sim/lib/table/application/rows.ts | 165 +++++++- apps/sim/lib/table/events.attribution.test.ts | 57 ++- apps/sim/lib/table/rows/service.ts | 90 ++-- apps/sim/lib/table/trigger.ts | 4 +- apps/sim/lib/table/types.ts | 6 +- apps/sim/tools/table/delete_row.ts | 1 + apps/sim/tools/table/get_row.ts | 1 + apps/sim/tools/table/update_row.ts | 1 + apps/sim/tools/table/upsert_row.ts | 1 + .../testing/src/factories/table.factory.ts | 4 +- 28 files changed, 1501 insertions(+), 780 deletions(-) create mode 100644 apps/sim/app/api/table/[tableId]/rows/upsert/route.test.ts create mode 100644 apps/sim/app/api/table/table-tool-auth.test.ts create mode 100644 apps/sim/lib/table/application/row-secret-provenance.test.ts create mode 100644 apps/sim/lib/table/application/row-secret-provenance.ts diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts index cb59859af26..1ae9263975f 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.test.ts @@ -1,101 +1,102 @@ /** * @vitest-environment node + * + * The enrichment-detail surface after moving onto the shared internal route + * builder. It previously queried the database from the adapter; the assertions + * below are the same wire outcomes, now with the use case as the seam. */ -import { createTableDefinition, hybridAuthMockFns } from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { EnrichmentRunDetail } from '@/lib/table' -const { mockCheckAccess, mockLoadEnrichmentDetail } = vi.hoisted(() => ({ - mockCheckAccess: vi.fn(), - mockLoadEnrichmentDetail: vi.fn(), +const { mocks } = vi.hoisted(() => ({ + mocks: { readDetail: vi.fn(), authenticate: vi.fn() }, })) -vi.mock('@/lib/table/rows/executions', () => ({ - loadEnrichmentDetail: mockLoadEnrichmentDetail, -})) -vi.mock('@/app/api/table/utils', async () => { - const { NextResponse } = await import('next/server') +vi.mock('@/lib/table/application/rows', async (importOriginal) => { + const actual = await importOriginal() return { - checkAccess: mockCheckAccess, - accessError: (result: { status: number }) => - NextResponse.json({ error: 'denied' }, { status: result.status }), + ...actual, + readTableRowEnrichmentDetail: { + operation: { id: 'tables.rows.read' }, + execute: mocks.readDetail, + }, } }) +vi.mock('@/lib/table/api', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, internalTableSessionOrExecutorAuth: { authenticate: mocks.authenticate } } +}) + +import { InternalUnauthenticatedError } from '@/lib/api/server/routes' +import { NoWorkspaceAccessError } from '@/lib/core/application' import { GET } from '@/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route' -function makeRequest(tableId = 'tbl_1', rowId = 'row_1', groupId = 'grp_1') { - const req = new NextRequest( - `http://localhost:3000/api/table/${tableId}/rows/${rowId}/enrichment/${groupId}` - ) - return GET(req, { params: Promise.resolve({ tableId, rowId, groupId }) }) +const TABLE = { id: 'tbl_1', workspaceId: 'workspace-1', schema: { columns: [] } } +const DETAIL = { providers: [{ id: 'clearbit', status: 'hit' }], costUsd: 0.01 } + +function routeContext() { + return { + params: Promise.resolve({ tableId: 'tbl_1', rowId: 'row_1', groupId: 'grp_1' }), + } } -const detail: EnrichmentRunDetail = { - startedAt: '2026-06-18T00:00:00.000Z', - completedAt: '2026-06-18T00:00:01.000Z', - durationMs: 1000, - totalCost: 0.05, - matchedProvider: 'hunter', - aborted: false, - providers: [ - { - id: 'hunter', - label: 'Hunter', - toolId: 'hunter_find_email', - status: 'matched', - cost: 0.05, - durationMs: 1000, - error: null, - }, - ], +function request() { + return new NextRequest('http://localhost/api/table/tbl_1/rows/row_1/enrichment/grp_1', { + method: 'GET', + }) } +beforeEach(() => { + vi.clearAllMocks() + mocks.authenticate.mockResolvedValue({ + kind: 'session', + userId: 'user-1', + sessionId: 'session-1', + }) + mocks.readDetail.mockResolvedValue({ table: TABLE, detail: DETAIL }) +}) + describe('GET /api/table/[tableId]/rows/[rowId]/enrichment/[groupId]', () => { - beforeEach(() => { - vi.clearAllMocks() - hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ - success: true, - userId: 'user-1', - authType: 'session', - }) - mockCheckAccess.mockResolvedValue({ ok: true, table: createTableDefinition({ rowCount: 1 }) }) + it('returns 401 when the caller is not authenticated', async () => { + mocks.authenticate.mockRejectedValue(new InternalUnauthenticatedError()) + + const response = await GET(request(), routeContext()) + + expect(response.status).toBe(401) + expect(mocks.readDetail).not.toHaveBeenCalled() }) it('returns the enrichment detail', async () => { - mockLoadEnrichmentDetail.mockResolvedValue(detail) - const res = await makeRequest() - expect(res.status).toBe(200) - const json = await res.json() - expect(json).toEqual({ success: true, data: { detail } }) - expect(mockLoadEnrichmentDetail).toHaveBeenCalledWith( - expect.anything(), - 'tbl_1', - 'row_1', - 'grp_1' - ) + const response = await GET(request(), routeContext()) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ success: true, data: { detail: DETAIL } }) }) it('returns null when there is no recorded run', async () => { - mockLoadEnrichmentDetail.mockResolvedValue(null) - const res = await makeRequest() - expect(res.status).toBe(200) - const json = await res.json() - expect(json).toEqual({ success: true, data: { detail: null } }) + mocks.readDetail.mockResolvedValue({ table: TABLE, detail: null }) + + const body = await (await GET(request(), routeContext())).json() + + expect(body).toEqual({ success: true, data: { detail: null } }) }) - it('401s when unauthenticated', async () => { - hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: false }) - const res = await makeRequest() - expect(res.status).toBe(401) - expect(mockLoadEnrichmentDetail).not.toHaveBeenCalled() + it('passes the row and group through to the use case', async () => { + await GET(request(), routeContext()) + + expect(mocks.readDetail.mock.calls[0][0].input).toMatchObject({ + tableId: 'tbl_1', + rowId: 'row_1', + groupId: 'grp_1', + }) }) - it('denies when access check fails', async () => { - mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) - const res = await makeRequest() - expect(res.status).toBe(403) - expect(mockLoadEnrichmentDetail).not.toHaveBeenCalled() + it('conceals a cross-tenant table rather than confirming it exists', async () => { + mocks.readDetail.mockRejectedValue(new NoWorkspaceAccessError()) + + const response = await GET(request(), routeContext()) + + expect(response.status).toBe(404) }) }) diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.ts index 34a045f7677..3caf42247f5 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/enrichment/[groupId]/route.ts @@ -1,51 +1,32 @@ -import { db } from '@sim/db' -import { createLogger } from '@sim/logger' -import { type NextRequest, NextResponse } from 'next/server' import { getEnrichmentDetailContract } from '@/lib/api/contracts/tables' -import { parseRequest } from '@/lib/api/server' -import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' -import { generateRequestId } from '@/lib/core/utils/request' -import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { loadEnrichmentDetail } from '@/lib/table/rows/executions' -import { accessError, checkAccess } from '@/app/api/table/utils' +import { defineInternalJsonRoute, internalRateLimits } from '@/lib/api/server/routes' +import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' +import { internalTableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' +import { tableOperations } from '@/lib/table/application/operations' +import { readTableRowEnrichmentDetail } from '@/lib/table/application/rows' -const logger = createLogger('EnrichmentDetailAPI') - -interface RouteParams { - params: Promise<{ tableId: string; rowId: string; groupId: string }> -} +export const dynamic = 'force-dynamic' /** * GET /api/table/[tableId]/rows/[rowId]/enrichment/[groupId] * - * Returns the enrichment cascade breakdown (provider outcomes, cost, timing) - * for one enrichment cell. Read on demand by the enrichment details panel — - * this data is deliberately kept off the hot grid read. Returns `null` for - * cells with no recorded run or runs that predate the feature. + * The enrichment cascade breakdown — provider outcomes, cost, timing — for one + * enrichment cell. Read on demand by the details panel; this data is + * deliberately kept off the hot grid read. */ -export const GET = withRouteHandler(async (request: NextRequest, { params }: RouteParams) => { - const requestId = generateRequestId() - - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) - } - - const parsed = await parseRequest(getEnrichmentDetailContract, request, { params }) - if (!parsed.success) return parsed.response - const { tableId, rowId, groupId } = parsed.data.params - - const result = await checkAccess(tableId, authResult.userId, 'read') - if (!result.ok) return accessError(result, requestId, tableId) - - const detail = await loadEnrichmentDetail(db, tableId, rowId, groupId) - - logger.info(`[${requestId}] Loaded enrichment detail`, { - tableId, - rowId, - groupId, - hasDetail: detail !== null, - }) - - return NextResponse.json({ success: true, data: { detail } }) +export const GET = defineInternalJsonRoute({ + contract: getEnrichmentDetailContract, + operation: tableOperations.readRow, + auth: internalTableSessionOrExecutorAuth, + rateLimit: internalRateLimits.none({ + reason: 'Preserve existing internal enrichment-detail behavior', + }), + errorPolicy: internalTableRowsErrorPolicy, + mapInput: ({ params }) => ({ + tableId: params.tableId, + rowId: params.rowId, + groupId: params.groupId, + }), + useCase: readTableRowEnrichmentDetail, + present: ({ detail }) => ({ success: true as const, data: { detail } }), }) diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts index e57151a35e7..cf4935b3b1e 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.test.ts @@ -1,68 +1,49 @@ /** * @vitest-environment node * - * Characterization tests for the single-row surface. + * Characterization tests for the single-row surface, carried across its + * migration onto the shared internal route builder. * - * These pin the wire behavior this route emits TODAY — status codes, body - * shapes, date serialization, and which collaborators are invoked — so the - * route can be migrated onto the shared internal route builder without - * silently changing what clients observe. They intentionally assert the - * existing contract rather than an idealized one. + * The assertions are the ones written against the hand-rolled handler — status + * codes, body shapes, ISO-8601 timestamps, and the dual-caller wire keying, + * where a session speaks stable column ids and a workflow execution speaks + * column names. What moved is the seam they mock: the route no longer loads the + * table or calls the row primitives itself, so the use cases are stubbed and the + * real builder runs. + * + * Two wire changes are deliberate; see the `deliberate wire changes` block. */ -import { - createTableDefinition, - hybridAuthMockFns, - queueTableRows, - resetDbChainMock, - schemaMock, -} from '@sim/testing' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { - mockCheckAccess, - mockUpdateRow, - mockPerformDeleteTableRow, - mockSignalTableRowsChangedByActor, -} = vi.hoisted(() => ({ - mockCheckAccess: vi.fn(), - mockUpdateRow: vi.fn(), - mockPerformDeleteTableRow: vi.fn(), - mockSignalTableRowsChangedByActor: vi.fn(), +const { mocks } = vi.hoisted(() => ({ + mocks: { + readRow: vi.fn(), + updateRow: vi.fn(), + deleteRow: vi.fn(), + authenticate: vi.fn(), + }, })) -vi.mock('@/app/api/table/utils', async () => { - const { NextResponse } = await import('next/server') +vi.mock('@/lib/table/application/rows', async (importOriginal) => { + const actual = await importOriginal() return { - checkAccess: mockCheckAccess, - accessError: (result: { status: number }) => - NextResponse.json({ error: 'Access denied' }, { status: result.status }), - orchestrationErrorResponse: (error: unknown) => - (error as { __orchestrated?: boolean })?.__orchestrated - ? NextResponse.json({ error: 'Orchestration failed' }, { status: 409 }) - : null, - orchestrationOutcomeErrorResponse: (_outcome: unknown, message: string) => - NextResponse.json({ error: message }, { status: 400 }), - tableLockErrorResponse: (error: unknown) => - (error as { __locked?: boolean })?.__locked - ? NextResponse.json({ error: 'Table is locked' }, { status: 423 }) - : null, + ...actual, + readTableRow: { operation: { id: 'tables.rows.read' }, execute: mocks.readRow }, + updateTableRow: { operation: { id: 'tables.rows.update' }, execute: mocks.updateRow }, + deleteTableRow: { operation: { id: 'tables.rows.delete' }, execute: mocks.deleteRow }, } }) -vi.mock('@/lib/table', async () => { - const columnKeys = await import('@/lib/table/column-keys') - return { ...columnKeys, updateRow: mockUpdateRow } +vi.mock('@/lib/table/api', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, internalTableSessionOrExecutorAuth: { authenticate: mocks.authenticate } } }) -vi.mock('@/lib/table/orchestration', () => ({ - performDeleteTableRow: mockPerformDeleteTableRow, -})) - -vi.mock('@/lib/table/events', () => ({ - signalTableRowsChangedByActor: mockSignalTableRowsChangedByActor, -})) - +import { InternalUnauthenticatedError } from '@/lib/api/server/routes' +import { NoWorkspaceAccessError } from '@/lib/core/application' +import { OrchestrationError } from '@/lib/core/orchestration/types' +import { TableLockedError } from '@/lib/table/mutation-locks' import { DELETE, GET, PATCH } from '@/app/api/table/[tableId]/rows/[rowId]/route' const TABLE_ID = 'tbl_1' @@ -71,26 +52,49 @@ const WORKSPACE_ID = 'workspace-1' const CREATED_AT = new Date('2024-01-01T00:00:00.000Z') const UPDATED_AT = new Date('2024-02-02T00:00:00.000Z') -function buildStoredRow() { - return { - id: ROW_ID, - data: { col_aaa: 'Ada', col_bbb: 36 }, - position: 0, - createdAt: CREATED_AT, - updatedAt: UPDATED_AT, - } +const TABLE = { + id: TABLE_ID, + workspaceId: WORKSPACE_ID, + schema: { + columns: [ + { id: 'col_aaa', name: 'Name', type: 'string' as const }, + { id: 'col_bbb', name: 'Age', type: 'number' as const }, + ], + }, +} + +const ROW = { + id: ROW_ID, + data: { col_aaa: 'Ada', col_bbb: 36 }, + executions: {}, + position: 0, + createdAt: CREATED_AT, + updatedAt: UPDATED_AT, } -function authAs(authType: 'session' | 'internal_jwt' = 'session') { - hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ - success: true, +function sessionPrincipal() { + mocks.authenticate.mockResolvedValue({ + kind: 'session', userId: 'user-1', - authType, + sessionId: 'session-1', + }) +} + +function executorPrincipal() { + mocks.authenticate.mockResolvedValue({ + kind: 'delegated', + serviceId: 'executor', + subjectUserId: 'user-1', + workspaceId: WORKSPACE_ID, + delegationId: 'delegation-1', + audience: 'table', + issuedAt: new Date('2026-01-01'), + expiresAt: new Date('2026-01-02'), }) } function unauthenticated() { - hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: false }) + mocks.authenticate.mockRejectedValue(new InternalUnauthenticatedError()) } function routeContext() { @@ -103,32 +107,20 @@ function getRequest(workspaceId: string | null = WORKSPACE_ID) { return new NextRequest(url, { method: 'GET' }) } -function bodyRequest(method: 'PATCH' | 'DELETE', body: unknown) { +function bodyRequest(method: 'PATCH' | 'DELETE', body: unknown, headers: HeadersInit = {}) { return new NextRequest(`http://localhost/api/table/${TABLE_ID}/rows/${ROW_ID}`, { method, - headers: { 'content-type': 'application/json' }, + headers: { 'content-type': 'application/json', ...headers }, body: JSON.stringify(body), }) } beforeEach(() => { vi.clearAllMocks() - resetDbChainMock() - authAs() - mockCheckAccess.mockResolvedValue({ - ok: true, - table: createTableDefinition({ - id: TABLE_ID, - columns: [ - { id: 'col_aaa', name: 'Name', type: 'string' }, - { id: 'col_bbb', name: 'Age', type: 'number' }, - ], - maxRows: 100, - workspaceId: WORKSPACE_ID, - createdAt: CREATED_AT, - updatedAt: UPDATED_AT, - }), - }) + sessionPrincipal() + mocks.readRow.mockResolvedValue({ table: TABLE, row: ROW }) + mocks.updateRow.mockResolvedValue({ table: TABLE, row: ROW, changed: true }) + mocks.deleteRow.mockResolvedValue({ table: TABLE, deletedRowId: ROW_ID }) }) describe('GET /api/table/[tableId]/rows/[rowId]', () => { @@ -138,45 +130,35 @@ describe('GET /api/table/[tableId]/rows/[rowId]', () => { const response = await GET(getRequest(), routeContext()) expect(response.status).toBe(401) - await expect(response.json()).resolves.toEqual({ error: 'Authentication required' }) - expect(mockCheckAccess).not.toHaveBeenCalled() + expect(mocks.readRow).not.toHaveBeenCalled() }) it('returns 400 when workspaceId is absent from the query string', async () => { const response = await GET(getRequest(null), routeContext()) expect(response.status).toBe(400) - expect(mockCheckAccess).not.toHaveBeenCalled() + expect(mocks.readRow).not.toHaveBeenCalled() }) - it('propagates the access decision when the caller lacks read access', async () => { - mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) - - const response = await GET(getRequest(), routeContext()) + it('asserts the caller-supplied workspace on the use case rather than checking it here', async () => { + await GET(getRequest(), routeContext()) - expect(response.status).toBe(403) - expect(mockCheckAccess).toHaveBeenCalledWith(TABLE_ID, 'user-1', 'read') - }) - - it('returns 400 when the asserted workspace does not own the table', async () => { - const response = await GET(getRequest('workspace-other'), routeContext()) - - expect(response.status).toBe(400) - await expect(response.json()).resolves.toEqual({ error: 'Invalid workspace ID' }) + expect(mocks.readRow.mock.calls[0][0].input).toMatchObject({ + tableId: TABLE_ID, + rowId: ROW_ID, + assertedWorkspaceId: WORKSPACE_ID, + }) }) it('returns 404 when the row does not exist', async () => { - queueTableRows(schemaMock.userTableRows, []) + mocks.readRow.mockRejectedValue(new OrchestrationError('not_found', 'Row not found')) const response = await GET(getRequest(), routeContext()) expect(response.status).toBe(404) - await expect(response.json()).resolves.toEqual({ error: 'Row not found' }) }) it('returns the row with ISO-8601 timestamps under data.row', async () => { - queueTableRows(schemaMock.userTableRows, [buildStoredRow()]) - const response = await GET(getRequest(), routeContext()) expect(response.status).toBe(200) @@ -193,25 +175,27 @@ describe('GET /api/table/[tableId]/rows/[rowId]', () => { }, }) }) + + it('returns column names to a workflow execution', async () => { + executorPrincipal() + + const response = await GET(getRequest(), routeContext()) + + const body = await response.json() + expect(body.data.row.data).toEqual({ Name: 'Ada', Age: 36 }) + }) }) describe('PATCH /api/table/[tableId]/rows/[rowId]', () => { const patchBody = { workspaceId: WORKSPACE_ID, data: { col_aaa: 'Grace' } } - beforeEach(() => { - mockUpdateRow.mockResolvedValue({ - ...buildStoredRow(), - data: { col_aaa: 'Grace', col_bbb: 36 }, - }) - }) - it('returns 401 when the caller is not authenticated', async () => { unauthenticated() const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) expect(response.status).toBe(401) - expect(mockUpdateRow).not.toHaveBeenCalled() + expect(mocks.updateRow).not.toHaveBeenCalled() }) it('returns 400 when the body fails contract validation', async () => { @@ -221,27 +205,7 @@ describe('PATCH /api/table/[tableId]/rows/[rowId]', () => { ) expect(response.status).toBe(400) - expect(mockUpdateRow).not.toHaveBeenCalled() - }) - - it('requires write access, not read access', async () => { - mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) - - const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) - - expect(response.status).toBe(403) - expect(mockCheckAccess).toHaveBeenCalledWith(TABLE_ID, 'user-1', 'write') - }) - - it('returns 400 when the asserted workspace does not own the table', async () => { - const response = await PATCH( - bodyRequest('PATCH', { ...patchBody, workspaceId: 'workspace-other' }), - routeContext() - ) - - expect(response.status).toBe(400) - await expect(response.json()).resolves.toEqual({ error: 'Invalid workspace ID' }) - expect(mockUpdateRow).not.toHaveBeenCalled() + expect(mocks.updateRow).not.toHaveBeenCalled() }) it('returns the updated row and the success message', async () => { @@ -253,7 +217,7 @@ describe('PATCH /api/table/[tableId]/rows/[rowId]', () => { data: { row: { id: ROW_ID, - data: { col_aaa: 'Grace', col_bbb: 36 }, + data: { col_aaa: 'Ada', col_bbb: 36 }, position: 0, createdAt: CREATED_AT.toISOString(), updatedAt: UPDATED_AT.toISOString(), @@ -263,114 +227,63 @@ describe('PATCH /api/table/[tableId]/rows/[rowId]', () => { }) }) - it('passes the acting user and the column-keyed patch to updateRow', async () => { + it('tells the use case a session speaks column ids', async () => { await PATCH(bodyRequest('PATCH', patchBody), routeContext()) - expect(mockUpdateRow).toHaveBeenCalledTimes(1) - const [input, table] = mockUpdateRow.mock.calls[0] - expect(input).toMatchObject({ - tableId: TABLE_ID, - rowId: ROW_ID, - workspaceId: WORKSPACE_ID, - actorUserId: 'user-1', + expect(mocks.updateRow.mock.calls[0][0].input).toMatchObject({ data: { col_aaa: 'Grace' }, + dataKeying: 'ids', + strictWrite: false, }) - expect(table.id).toBe(TABLE_ID) }) - it('translates column names to ids for an internal JWT caller', async () => { - authAs('internal_jwt') + it('tells the use case a workflow execution speaks column names', async () => { + executorPrincipal() await PATCH( bodyRequest('PATCH', { workspaceId: WORKSPACE_ID, data: { Name: 'Grace' } }), routeContext() ) - expect(mockUpdateRow.mock.calls[0][0]).toMatchObject({ data: { col_aaa: 'Grace' } }) - }) - - it('returns column names to an internal JWT caller', async () => { - authAs('internal_jwt') - - const response = await PATCH( - bodyRequest('PATCH', { workspaceId: WORKSPACE_ID, data: { Name: 'Grace' } }), - routeContext() - ) - - const body = await response.json() - expect(body.data.row.data).toEqual({ Name: 'Grace', Age: 36 }) + expect(mocks.updateRow.mock.calls[0][0].input).toMatchObject({ + data: { Name: 'Grace' }, + dataKeying: 'names', + }) }) - it('signals open collaborators that the row changed', async () => { + it('hands the provenance envelope over unresolved rather than interpreting it', async () => { await PATCH(bodyRequest('PATCH', patchBody), routeContext()) - expect(mockSignalTableRowsChangedByActor).toHaveBeenCalledWith(TABLE_ID, undefined) - }) - - it('forwards the originating tab id so that tab ignores its own broadcast', async () => { - const request = new NextRequest(`http://localhost/api/table/${TABLE_ID}/rows/${ROW_ID}`, { - method: 'PATCH', - headers: { 'content-type': 'application/json', 'x-sim-client-id': 'tab-42' }, - body: JSON.stringify(patchBody), + expect(mocks.updateRow.mock.calls[0][0].input.secretProvenanceEnvelope).toEqual({ + kind: 'none', }) + }) - await PATCH(request, routeContext()) + it('forwards the originating tab so that tab can skip its own refetch', async () => { + await PATCH(bodyRequest('PATCH', patchBody, { 'x-sim-client-id': 'tab-42' }), routeContext()) - expect(mockSignalTableRowsChangedByActor).toHaveBeenCalledWith(TABLE_ID, 'tab-42') + expect(mocks.updateRow.mock.calls[0][0].input.actorClientId).toBe('tab-42') }) it('projects a classified orchestration failure instead of a generic 500', async () => { - mockUpdateRow.mockRejectedValue(Object.assign(new Error('conflict'), { __orchestrated: true })) + mocks.updateRow.mockRejectedValue(new OrchestrationError('conflict', 'Row changed')) const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) expect(response.status).toBe(409) }) - - it('falls back to 500 for an unclassified failure', async () => { - mockUpdateRow.mockRejectedValue(new Error('boom')) - - const response = await PATCH(bodyRequest('PATCH', patchBody), routeContext()) - - expect(response.status).toBe(500) - await expect(response.json()).resolves.toEqual({ error: 'Failed to update row' }) - }) }) describe('DELETE /api/table/[tableId]/rows/[rowId]', () => { const deleteBody = { workspaceId: WORKSPACE_ID } - beforeEach(() => { - mockPerformDeleteTableRow.mockResolvedValue({ success: true }) - }) - it('returns 401 when the caller is not authenticated', async () => { unauthenticated() const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) expect(response.status).toBe(401) - expect(mockPerformDeleteTableRow).not.toHaveBeenCalled() - }) - - it('requires write access', async () => { - mockCheckAccess.mockResolvedValue({ ok: false, status: 403 }) - - const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) - - expect(response.status).toBe(403) - expect(mockCheckAccess).toHaveBeenCalledWith(TABLE_ID, 'user-1', 'write') - }) - - it('returns 400 when the asserted workspace does not own the table', async () => { - const response = await DELETE( - bodyRequest('DELETE', { workspaceId: 'workspace-other' }), - routeContext() - ) - - expect(response.status).toBe(400) - await expect(response.json()).resolves.toEqual({ error: 'Invalid workspace ID' }) - expect(mockPerformDeleteTableRow).not.toHaveBeenCalled() + expect(mocks.deleteRow).not.toHaveBeenCalled() }) it('reports a deleted count of one on success', async () => { @@ -381,26 +294,80 @@ describe('DELETE /api/table/[tableId]/rows/[rowId]', () => { success: true, data: { message: 'Row deleted successfully', deletedCount: 1 }, }) - expect(mockSignalTableRowsChangedByActor).toHaveBeenCalledWith(TABLE_ID, undefined) }) - it('projects an unsuccessful delete outcome as a client error', async () => { - mockPerformDeleteTableRow.mockResolvedValue({ success: false }) + it('forwards the originating tab so that tab can skip its own refetch', async () => { + await DELETE(bodyRequest('DELETE', deleteBody, { 'x-sim-client-id': 'tab-42' }), routeContext()) - const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + expect(mocks.deleteRow.mock.calls[0][0].input.actorClientId).toBe('tab-42') + }) +}) - expect(response.status).toBe(400) - await expect(response.json()).resolves.toEqual({ error: 'Failed to delete row' }) - expect(mockSignalTableRowsChangedByActor).not.toHaveBeenCalled() +/** + * The wire changes the migration makes on purpose. + * + * Both follow from adopting the shared concealment policy — what the v2 table + * surface already does, and what stops a caller learning whether a table it + * cannot reach exists. Nothing in `hooks/queries/tables.ts` branches on either + * status, which is why they are safe to change. + * + * Note the concealment is narrower than the handler it replaces: the old route + * answered a blanket 403 for every access failure, while this one conceals only + * *cross-tenant* denials and still answers 403 for an in-workspace role denial. + */ +describe('deliberate wire changes', () => { + it('conceals a cross-tenant table as 404, where it used to answer 403', async () => { + mocks.readRow.mockRejectedValue(new NoWorkspaceAccessError()) + + const response = await GET(getRequest(), routeContext()) + + expect(response.status).toBe(404) + await expect(response.json()).resolves.toMatchObject({ error: 'Table not found' }) }) - it('projects a table lock failure ahead of the generic handler', async () => { - mockPerformDeleteTableRow.mockRejectedValue( - Object.assign(new Error('locked'), { __locked: true }) - ) + it('still answers 403 for an in-workspace denial, which is not concealed', async () => { + mocks.readRow.mockRejectedValue(new OrchestrationError('forbidden', 'Insufficient role')) - const response = await DELETE(bodyRequest('DELETE', deleteBody), routeContext()) + const response = await GET(getRequest(), routeContext()) + + expect(response.status).toBe(403) + }) + + it('keeps the lock on a 423 so the client knows which one to clear', async () => { + mocks.updateRow.mockRejectedValue(new TableLockedError('update')) + + const response = await PATCH( + bodyRequest('PATCH', { workspaceId: WORKSPACE_ID, data: { col_aaa: 'x' } }), + routeContext() + ) expect(response.status).toBe(423) + await expect(response.json()).resolves.toMatchObject({ lock: 'update' }) + }) + + it('answers a generic 500 message where the handler named the operation', async () => { + // The builder has one internal error envelope, shared with ~80 other + // migrated routes. The old per-route text ("Failed to update row") was more + // specific; consistency won, and the client only ever toasts the message. + mocks.updateRow.mockRejectedValue(new Error('boom')) + + const response = await PATCH( + bodyRequest('PATCH', { workspaceId: WORKSPACE_ID, data: { col_aaa: 'x' } }), + routeContext() + ) + + expect(response.status).toBe(500) + await expect(response.json()).resolves.toMatchObject({ error: 'Internal server error' }) + }) + + it('conceals a mismatched workspace assertion as 404, where it used to answer 400', async () => { + mocks.updateRow.mockRejectedValue(new OrchestrationError('not_found', 'Table not found')) + + const response = await PATCH( + bodyRequest('PATCH', { workspaceId: 'workspace-other', data: { col_aaa: 'x' } }), + routeContext() + ) + + expect(response.status).toBe(404) }) }) diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts index 66ec90870f8..02a5a45cfa4 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts @@ -1,274 +1,101 @@ -import { db } from '@sim/db' -import { userTableRows } from '@sim/db/schema' -import { createLogger } from '@sim/logger' -import { and, eq } from 'drizzle-orm' -import { type NextRequest, NextResponse } from 'next/server' import { readClientId } from '@/lib/api/client-id' import { deleteTableRowContract, - getTableQuerySchema, + getTableRowContract, updateTableRowContract, } from '@/lib/api/contracts/tables' -import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation' -import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' -import { generateRequestId } from '@/lib/core/utils/request' -import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import type { RowData, TableSchema } from '@/lib/table' -import { updateRow } from '@/lib/table' -import { signalTableRowsChangedByActor } from '@/lib/table/events' -import { performDeleteTableRow } from '@/lib/table/orchestration' +import { defineInternalJsonRoute, internalRateLimits } from '@/lib/api/server/routes' +import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' +import { internalTableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' +import { tableOperations } from '@/lib/table/application/operations' +import { deleteTableRow, readTableRow, updateTableRow } from '@/lib/table/application/rows' +import type { RowData } from '@/lib/table/types' import { - createTableRowsResponse, - createTableWriteProvenanceTargets, - resolveTableWriteSecretProvenance, + finalizeTableRowsProvenance, + negotiateTableRowsProvenance, + readTableRowProvenanceEnvelope, } from '@/app/api/table/row-secret-provenance' -import { rowWireTranslators } from '@/app/api/table/row-wire' -import { - accessError, - checkAccess, - orchestrationErrorResponse, - orchestrationOutcomeErrorResponse, - tableLockErrorResponse, -} from '@/app/api/table/utils' - -const logger = createLogger('TableRowAPI') - -interface RowRouteParams { - params: Promise<{ tableId: string; rowId: string }> -} - -/** GET /api/table/[tableId]/rows/[rowId] - Retrieves a single row. */ -export const GET = withRouteHandler(async (request: NextRequest, { params }: RowRouteParams) => { - const requestId = generateRequestId() - const { tableId, rowId } = await params - - try { - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) - } +import { presentRowForPrincipal, rowKeyingForPrincipal } from '@/app/api/table/row-wire' - const { searchParams } = new URL(request.url) - const validated = getTableQuerySchema.parse({ - workspaceId: searchParams.get('workspaceId'), - }) +export const dynamic = 'force-dynamic' - const result = await checkAccess(tableId, authResult.userId, 'read') - if (!result.ok) return accessError(result, requestId, tableId) - - const { table } = result - - if (table.workspaceId !== validated.workspaceId) { - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } - - const [row] = await db - .select({ - id: userTableRows.id, - data: userTableRows.data, - position: userTableRows.position, - createdAt: userTableRows.createdAt, - updatedAt: userTableRows.updatedAt, - }) - .from(userTableRows) - .where( - and( - eq(userTableRows.id, rowId), - eq(userTableRows.tableId, tableId), - eq(userTableRows.workspaceId, validated.workspaceId) - ) - ) - .limit(1) - - if (!row) { - return NextResponse.json({ error: 'Row not found' }, { status: 404 }) - } - - logger.info(`[${requestId}] Retrieved row ${rowId} from table ${tableId}`) - - const wire = rowWireTranslators(authResult.authType, table.schema as TableSchema) - - const responseBody = { - success: true, - data: { - row: { - id: row.id, - data: wire.dataOut(row.data as RowData), - position: row.position, - createdAt: - row.createdAt instanceof Date ? row.createdAt.toISOString() : String(row.createdAt), - updatedAt: - row.updatedAt instanceof Date ? row.updatedAt.toISOString() : String(row.updatedAt), - }, - }, - } - return createTableRowsResponse({ - request, - authType: authResult.authType, - userId: authResult.userId, - workspaceId: table.workspaceId, - body: responseBody, - rows: [{ ...row, data: row.data as RowData }], - }) - } catch (error) { - if (isZodError(error)) { - return validationErrorResponse(error) - } - - logger.error(`[${requestId}] Error getting row:`, error) - return NextResponse.json({ error: 'Failed to get row' }, { status: 500 }) - } +const rateLimit = internalRateLimits.none({ + reason: 'Preserve existing internal single-row table behavior', }) -/** PATCH /api/table/[tableId]/rows/[rowId] - Updates a single row (supports partial updates). */ -export const PATCH = withRouteHandler(async (request: NextRequest, context: RowRouteParams) => { - const requestId = generateRequestId() - - try { - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) - } - - const parsed = await parseRequest(updateTableRowContract, request, context, { - validationErrorResponse: (error) => validationErrorResponse(error), - }) - if (!parsed.success) return parsed.response - - const { tableId, rowId } = parsed.data.params - const validated = parsed.data.body - - const result = await checkAccess(tableId, authResult.userId, 'write') - if (!result.ok) return accessError(result, requestId, tableId) - - const { table } = result - - if (table.workspaceId !== validated.workspaceId) { - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } - - const wire = rowWireTranslators(authResult.authType, table.schema as TableSchema) - const rowData = validated.data as RowData - const provenance = resolveTableWriteSecretProvenance({ - request, - payload: validated, - authType: authResult.authType, - userId: authResult.userId, - workspaceId: table.workspaceId, - targets: createTableWriteProvenanceTargets([rowData], wire.dataIn), - rowKeys: ['0'], - }) - if (!provenance.success) return provenance.response - const updatedRow = await updateRow( - { - tableId, - rowId, - data: wire.dataIn(rowData), - workspaceId: validated.workspaceId, - actorUserId: authResult.userId, - secretProvenance: provenance.provenanceByRowKey?.['0'], - }, - table, - requestId - ) - - // Live-collab: tell open viewers the change landed so they refetch. - signalTableRowsChangedByActor(tableId, readClientId(request)) - // Only `null` when a `cancellationGuard` is supplied and the SQL guard - // rejects the write — this route doesn't pass one, so reaching null is a bug. - if (!updatedRow) throw new Error('updateRow returned null without a cancellationGuard') - // Auto-dispatch for user edits is handled inside `updateRow` (mode: 'new'). - // Firing a second mode: 'incomplete' dispatch here would race with the - // `mode: 'new'` one AND bulk-clear sibling-group outputs (the incomplete - // bulk-clear wipes ALL targeted columns when any one column on the row - // is empty). - - const responseBody = { - success: true, - data: { - row: { - id: updatedRow.id, - data: wire.dataOut(updatedRow.data), - position: updatedRow.position, - createdAt: - updatedRow.createdAt instanceof Date - ? updatedRow.createdAt.toISOString() - : updatedRow.createdAt, - updatedAt: - updatedRow.updatedAt instanceof Date - ? updatedRow.updatedAt.toISOString() - : updatedRow.updatedAt, - }, - message: 'Row updated successfully', - }, - } - return createTableRowsResponse({ +export const GET = defineInternalJsonRoute({ + contract: getTableRowContract, + operation: tableOperations.readRow, + auth: internalTableSessionOrExecutorAuth, + rateLimit, + errorPolicy: internalTableRowsErrorPolicy, + mapInput: ({ params, query }, { principal, request }) => ({ + tableId: params.tableId, + rowId: params.rowId, + assertedWorkspaceId: query.workspaceId, + includePersistedSecretProvenance: negotiateTableRowsProvenance( request, - authType: authResult.authType, - userId: authResult.userId, - workspaceId: table.workspaceId, - body: responseBody, - rows: [updatedRow], - }) - } catch (error) { - const response = orchestrationErrorResponse(error) - if (response) return response - - logger.error(`[${requestId}] Error updating row:`, error) - return NextResponse.json({ error: 'Failed to update row' }, { status: 500 }) - } + principal.kind !== 'session' + ), + }), + useCase: readTableRow, + present: ({ table, row }, { principal }) => ({ + success: true as const, + data: { row: presentRowForPrincipal(row, table.schema, principal) }, + }), + finalizeResponse: ({ result }) => finalizeTableRowsProvenance(result.secretProvenance), }) -/** DELETE /api/table/[tableId]/rows/[rowId] - Deletes a single row. */ -export const DELETE = withRouteHandler(async (request: NextRequest, context: RowRouteParams) => { - const requestId = generateRequestId() - - try { - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) +export const PATCH = defineInternalJsonRoute({ + contract: updateTableRowContract, + operation: tableOperations.updateRow, + auth: internalTableSessionOrExecutorAuth, + rateLimit, + errorPolicy: internalTableRowsErrorPolicy, + mapInput: ({ params, body }, { principal, request }) => { + return { + tableId: params.tableId, + rowId: params.rowId, + assertedWorkspaceId: body.workspaceId, + data: body.data as RowData, + dataKeying: rowKeyingForPrincipal(principal), + strictWrite: false, + // Handed over unresolved: interpreting the selections needs the canonical + // schema, which this adapter must not load. + secretProvenanceEnvelope: readTableRowProvenanceEnvelope(request, body), + includePersistedSecretProvenance: negotiateTableRowsProvenance( + request, + principal.kind !== 'session' + ), + actorClientId: readClientId(request), } + }, + useCase: updateTableRow, + present: ({ table, row }, { principal }) => ({ + success: true as const, + data: { + row: presentRowForPrincipal(row, table.schema, principal), + message: 'Row updated successfully', + }, + }), + finalizeResponse: ({ result }) => finalizeTableRowsProvenance(result.secretProvenance), +}) - const parsed = await parseRequest(deleteTableRowContract, request, context, { - validationErrorResponse: (error) => validationErrorResponse(error), - }) - if (!parsed.success) return parsed.response - - const { tableId, rowId } = parsed.data.params - const validated = parsed.data.body - - const result = await checkAccess(tableId, authResult.userId, 'write') - if (!result.ok) return accessError(result, requestId, tableId) - - const { table } = result - - if (table.workspaceId !== validated.workspaceId) { - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } - - const outcome = await performDeleteTableRow({ table, rowId, requestId }) - if (!outcome.success) { - return orchestrationOutcomeErrorResponse(outcome, 'Failed to delete row') - } - - // Live-collab: tell open viewers the change landed so they refetch. - signalTableRowsChangedByActor(tableId, readClientId(request)) - - return NextResponse.json({ - success: true, - data: { - message: 'Row deleted successfully', - deletedCount: 1, - }, - }) - } catch (error) { - const lockError = tableLockErrorResponse(error) - if (lockError) return lockError - - const classified = orchestrationErrorResponse(error) - if (classified) return classified - - logger.error(`[${requestId}] Error deleting row:`, error) - return NextResponse.json({ error: 'Failed to delete row' }, { status: 500 }) - } +export const DELETE = defineInternalJsonRoute({ + contract: deleteTableRowContract, + operation: tableOperations.deleteRow, + auth: internalTableSessionOrExecutorAuth, + rateLimit, + errorPolicy: internalTableRowsErrorPolicy, + mapInput: ({ params, body }, { request }) => ({ + tableId: params.tableId, + rowId: params.rowId, + assertedWorkspaceId: body.workspaceId, + actorClientId: readClientId(request), + }), + useCase: deleteTableRow, + present: () => ({ + success: true as const, + data: { message: 'Row deleted successfully', deletedCount: 1 }, + }), }) diff --git a/apps/sim/app/api/table/[tableId]/rows/upsert/route.test.ts b/apps/sim/app/api/table/[tableId]/rows/upsert/route.test.ts new file mode 100644 index 00000000000..2d07bd5e0d0 --- /dev/null +++ b/apps/sim/app/api/table/[tableId]/rows/upsert/route.test.ts @@ -0,0 +1,148 @@ +/** + * @vitest-environment node + * + * The upsert surface had no route-level tests. These pin what it emits now that + * it runs on the shared internal route builder, including the dual-caller wire + * keying and the provenance envelope being handed over unresolved. + */ +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mocks } = vi.hoisted(() => ({ + mocks: { upsertRow: vi.fn(), authenticate: vi.fn() }, +})) + +vi.mock('@/lib/table/application/rows', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + upsertTableRow: { operation: { id: 'tables.rows.upsert' }, execute: mocks.upsertRow }, + } +}) + +vi.mock('@/lib/table/api', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, internalTableSessionOrExecutorAuth: { authenticate: mocks.authenticate } } +}) + +import { InternalUnauthenticatedError } from '@/lib/api/server/routes' +import { POST } from '@/app/api/table/[tableId]/rows/upsert/route' + +const TABLE_ID = 'tbl_1' +const WORKSPACE_ID = 'workspace-1' +const CREATED_AT = new Date('2024-01-01T00:00:00.000Z') +const UPDATED_AT = new Date('2024-02-02T00:00:00.000Z') + +const TABLE = { + id: TABLE_ID, + workspaceId: WORKSPACE_ID, + schema: { columns: [{ id: 'col_aaa', name: 'Name', type: 'string' as const }] }, +} +const ROW = { + id: 'row_1', + data: { col_aaa: 'Ada' }, + executions: {}, + position: 0, + createdAt: CREATED_AT, + updatedAt: UPDATED_AT, +} + +function routeContext() { + return { params: Promise.resolve({ tableId: TABLE_ID }) } +} + +function request(body: unknown) { + return new NextRequest(`http://localhost/api/table/${TABLE_ID}/rows/upsert`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }) +} + +const BODY = { workspaceId: WORKSPACE_ID, data: { col_aaa: 'Ada' }, conflictTarget: 'col_aaa' } + +beforeEach(() => { + vi.clearAllMocks() + mocks.authenticate.mockResolvedValue({ + kind: 'session', + userId: 'user-1', + sessionId: 'session-1', + }) + mocks.upsertRow.mockResolvedValue({ table: TABLE, row: ROW, operation: 'insert' }) +}) + +describe('POST /api/table/[tableId]/rows/upsert', () => { + it('returns 401 when the caller is not authenticated', async () => { + mocks.authenticate.mockRejectedValue(new InternalUnauthenticatedError()) + + const response = await POST(request(BODY), routeContext()) + + expect(response.status).toBe(401) + expect(mocks.upsertRow).not.toHaveBeenCalled() + }) + + it('names the operation it performed in the body and the message', async () => { + const response = await POST(request(BODY), routeContext()) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ + success: true, + data: { + row: { + id: 'row_1', + data: { col_aaa: 'Ada' }, + position: 0, + createdAt: CREATED_AT.toISOString(), + updatedAt: UPDATED_AT.toISOString(), + }, + operation: 'insert', + message: 'Row inserted successfully', + }, + }) + }) + + it('says updated when the row already existed', async () => { + mocks.upsertRow.mockResolvedValue({ table: TABLE, row: ROW, operation: 'update' }) + + const body = await (await POST(request(BODY), routeContext())).json() + + expect(body.data.message).toBe('Row updated successfully') + }) + + it('tells the use case a session speaks column ids and forwards the conflict target', async () => { + await POST(request(BODY), routeContext()) + + expect(mocks.upsertRow.mock.calls[0][0].input).toMatchObject({ + tableId: TABLE_ID, + assertedWorkspaceId: WORKSPACE_ID, + dataKeying: 'ids', + strictWrite: false, + conflictTarget: 'col_aaa', + }) + }) + + it('tells the use case a workflow execution speaks column names', async () => { + mocks.authenticate.mockResolvedValue({ + kind: 'delegated', + serviceId: 'executor', + subjectUserId: 'user-1', + workspaceId: WORKSPACE_ID, + delegationId: 'delegation-1', + audience: 'table', + issuedAt: new Date('2026-01-01'), + expiresAt: new Date('2026-01-02'), + }) + + await POST(request({ ...BODY, data: { Name: 'Ada' }, conflictTarget: 'Name' }), routeContext()) + + expect(mocks.upsertRow.mock.calls[0][0].input).toMatchObject({ dataKeying: 'names' }) + }) + + it('hands the provenance envelope over unresolved rather than interpreting it', async () => { + await POST(request(BODY), routeContext()) + + expect(mocks.upsertRow.mock.calls[0][0].input.secretProvenanceEnvelope).toEqual({ + kind: 'none', + }) + }) +}) diff --git a/apps/sim/app/api/table/[tableId]/rows/upsert/route.ts b/apps/sim/app/api/table/[tableId]/rows/upsert/route.ts index abfbc7e0384..d34559cb66f 100644 --- a/apps/sim/app/api/table/[tableId]/rows/upsert/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/upsert/route.ts @@ -1,114 +1,53 @@ -import { createLogger } from '@sim/logger' -import { type NextRequest, NextResponse } from 'next/server' import { upsertTableRowContract } from '@/lib/api/contracts/tables' -import { parseRequest } from '@/lib/api/server' -import { isZodError, validationErrorResponse } from '@/lib/api/server/validation' -import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' -import { generateRequestId } from '@/lib/core/utils/request' -import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import type { RowData, TableSchema } from '@/lib/table' -import { upsertRow } from '@/lib/table' -import { signalTableRowsChanged } from '@/lib/table/events' +import { defineInternalJsonRoute, internalRateLimits } from '@/lib/api/server/routes' +import { internalTableSessionOrExecutorAuth } from '@/lib/table/api' +import { internalTableRowsErrorPolicy } from '@/lib/table/api/row-route-policies' +import { tableOperations } from '@/lib/table/application/operations' +import { upsertTableRow } from '@/lib/table/application/rows' +import type { RowData } from '@/lib/table/types' import { - createTableRowsResponse, - createTableWriteProvenanceTargets, - resolveTableWriteSecretProvenance, + finalizeTableRowsProvenance, + negotiateTableRowsProvenance, + readTableRowProvenanceEnvelope, } from '@/app/api/table/row-secret-provenance' -import { rowWireTranslators } from '@/app/api/table/row-wire' -import { accessError, checkAccess, orchestrationErrorResponse } from '@/app/api/table/utils' - -const logger = createLogger('TableUpsertAPI') - -interface UpsertRouteParams { - params: Promise<{ tableId: string }> -} - -/** POST /api/table/[tableId]/rows/upsert - Inserts or updates based on unique columns. */ -export const POST = withRouteHandler(async (request: NextRequest, context: UpsertRouteParams) => { - const requestId = generateRequestId() - const { tableId } = await context.params - - try { - const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) - if (!authResult.success || !authResult.userId) { - return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) - } - - const validation = await parseRequest(upsertTableRowContract, request, context) - if (!validation.success) return validation.response - const validated = validation.data.body - - const result = await checkAccess(tableId, authResult.userId, 'write') - if (!result.ok) return accessError(result, requestId, tableId) - - const { table } = result - - if (table.workspaceId !== validated.workspaceId) { - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } - - const wire = rowWireTranslators(authResult.authType, table.schema as TableSchema) - const provenance = resolveTableWriteSecretProvenance({ - request, - payload: validated, - authType: authResult.authType, - userId: authResult.userId, - workspaceId: table.workspaceId, - targets: createTableWriteProvenanceTargets([validated.data as RowData], wire.dataIn), - rowKeys: ['0'], - }) - if (!provenance.success) return provenance.response - // conflictTarget passes through untranslated — upsertRow resolves it id-or-name. - const upsertResult = await upsertRow( - { - tableId, - workspaceId: validated.workspaceId, - data: wire.dataIn(validated.data as RowData), - userId: authResult.userId, - conflictTarget: validated.conflictTarget, - secretProvenance: provenance.provenanceByRowKey?.['0'], - }, - table, - requestId - ) - signalTableRowsChanged(tableId) - - const responseBody = { - success: true, - data: { - row: { - id: upsertResult.row.id, - data: wire.dataOut(upsertResult.row.data), - createdAt: - upsertResult.row.createdAt instanceof Date - ? upsertResult.row.createdAt.toISOString() - : upsertResult.row.createdAt, - updatedAt: - upsertResult.row.updatedAt instanceof Date - ? upsertResult.row.updatedAt.toISOString() - : upsertResult.row.updatedAt, - }, - operation: upsertResult.operation, - message: `Row ${upsertResult.operation === 'update' ? 'updated' : 'inserted'} successfully`, - }, - } - return createTableRowsResponse({ +import { presentRowForPrincipal, rowKeyingForPrincipal } from '@/app/api/table/row-wire' + +export const dynamic = 'force-dynamic' + +/** POST /api/table/[tableId]/rows/upsert — inserts or updates based on unique columns. */ +export const POST = defineInternalJsonRoute({ + contract: upsertTableRowContract, + operation: tableOperations.upsertRow, + auth: internalTableSessionOrExecutorAuth, + rateLimit: internalRateLimits.none({ + reason: 'Preserve existing internal table upsert behavior', + }), + errorPolicy: internalTableRowsErrorPolicy, + mapInput: ({ params, body }, { principal, request }) => ({ + tableId: params.tableId, + assertedWorkspaceId: body.workspaceId, + data: body.data as RowData, + dataKeying: rowKeyingForPrincipal(principal), + strictWrite: false, + // The conflict target follows the same keying as the data; the use case + // resolves it id-or-name against the canonical schema. + conflictTarget: body.conflictTarget, + // Handed over unresolved: interpreting the selections needs the canonical + // schema, which this adapter must not load. + secretProvenanceEnvelope: readTableRowProvenanceEnvelope(request, body), + includePersistedSecretProvenance: negotiateTableRowsProvenance( request, - authType: authResult.authType, - userId: authResult.userId, - workspaceId: table.workspaceId, - body: responseBody, - rows: [upsertResult.row], - }) - } catch (error) { - if (isZodError(error)) { - return validationErrorResponse(error) - } - - const response = orchestrationErrorResponse(error) - if (response) return response - - logger.error(`[${requestId}] Error upserting row:`, error) - return NextResponse.json({ error: 'Failed to upsert row' }, { status: 500 }) - } + principal.kind !== 'session' + ), + }), + useCase: upsertTableRow, + present: ({ table, row, operation }, { principal }) => ({ + success: true as const, + data: { + row: presentRowForPrincipal(row, table.schema, principal), + operation, + message: `Row ${operation === 'update' ? 'updated' : 'inserted'} successfully`, + }, + }), + finalizeResponse: ({ result }) => finalizeTableRowsProvenance(result.secretProvenance), }) diff --git a/apps/sim/app/api/table/row-secret-provenance.ts b/apps/sim/app/api/table/row-secret-provenance.ts index 9d72ce58990..d455a6617d8 100644 --- a/apps/sim/app/api/table/row-secret-provenance.ts +++ b/apps/sim/app/api/table/row-secret-provenance.ts @@ -10,6 +10,10 @@ import { RESOLVED_SECRET_PROVENANCE_METADATA_V1, serializePrivateToolMetadataResponseEnvelope, } from '@/lib/execution/private-tool-metadata' +import { + type TableRowProvenanceEnvelope, + TableRowProvenanceError, +} from '@/lib/table/application/row-secret-provenance' import { loadTableRowSecretProvenance } from '@/lib/table/rows/secret-provenance' import { tableRowSecretProvenanceSelectionKey } from '@/lib/table/secret-provenance-selection' import type { RowData, TableRowSecretProvenanceWrite } from '@/lib/table/types' @@ -195,3 +199,57 @@ export async function createTableRowsResponse(options: { ) return NextResponse.json(envelope.body, { headers: envelope.headers }) } + +/** + * Reads the private provenance envelope off the request. Transport only — the + * selections are interpreted against the canonical schema inside the use case, + * by {@link resolveRowWriteProvenance}. + */ +export function readTableRowProvenanceEnvelope( + request: NextRequest, + payload: unknown +): TableRowProvenanceEnvelope { + const inspection = inspectPrivateSecretProvenanceRequest(request.headers, payload) + if (inspection.status === 'unsupported') return { kind: 'none' } + if (inspection.status !== 'verified') throw new TableRowProvenanceError() + return { kind: 'bundle', value: inspection.value } +} + +/** + * Whether this caller asked for persisted row provenance on the response. + * + * Only an authenticated internal caller that explicitly requested the capability + * gets it; every ordinary UI and API response keeps its existing wire shape. The + * answer feeds the use case's `includePersistedSecretProvenance`, so the load + * itself happens inside the authorized operation rather than in the adapter. + */ +export function negotiateTableRowsProvenance( + request: NextRequest, + isInternalCaller: boolean +): boolean { + const negotiation = negotiatePrivateToolMetadataResponse( + request.headers, + RESOLVED_SECRET_PROVENANCE_METADATA_V1, + isInternalCaller + ) + if (negotiation.status === 'rejected') throw new TableRowProvenanceError() + return negotiation.status !== 'not-requested' +} + +/** + * The response half of the envelope, shaped for a declarative route's + * `finalizeResponse`: one sibling body field and one capability header, added + * only when the use case actually loaded provenance. + */ +export function finalizeTableRowsProvenance(provenance: unknown): { + bodyFields?: Record + headers?: HeadersInit +} { + if (provenance === undefined) return {} + const envelope = serializePrivateToolMetadataResponseEnvelope( + {}, + RESOLVED_SECRET_PROVENANCE_METADATA_V1, + provenance + ) + return { bodyFields: envelope.body, headers: envelope.headers } +} diff --git a/apps/sim/app/api/table/row-wire.ts b/apps/sim/app/api/table/row-wire.ts index f4c0a85d988..b20121a621e 100644 --- a/apps/sim/app/api/table/row-wire.ts +++ b/apps/sim/app/api/table/row-wire.ts @@ -1,5 +1,15 @@ +import type { SessionPrincipal, WorkflowExecutionDelegatedPrincipal } from '@sim/auth/principal' import { AuthType, type AuthTypeValue } from '@/lib/auth/hybrid' -import type { Filter, RowData, Sort, SortSpec, TablePredicate, TableSchema } from '@/lib/table' +import type { + Filter, + RowData, + Sort, + SortSpec, + TablePredicate, + TableRow, + TableSchema, +} from '@/lib/table' +import type { TableRowDataKeying } from '@/lib/table/application/rows' import { namedRowMapper } from '@/lib/table/cell-format' import { buildIdByName, @@ -9,6 +19,7 @@ import { sortSpecNamesToIds, } from '@/lib/table/column-keys' import { predicateToStorage, resolveFilterSelectValues } from '@/lib/table/select-values' +import { toWireTimestamp } from '@/lib/table/wire' export interface RowWireTranslators { /** Inbound row data: wire keys → storage column ids. */ @@ -61,3 +72,55 @@ export function rowWireTranslators( sortSpecIn: (sort) => sortSpecNamesToIds(sort, idByName), } } + +/** + * The principal kinds the internal table row routes admit — the auth policy + * yields exactly these two. Typed as the union rather than `Principal` so a + * third kind becomes an exhaustiveness error here instead of silently taking + * the name-keyed branch, which would drop every id-keyed cell of a write and + * report success. + */ +type TableRowRoutePrincipal = SessionPrincipal | WorkflowExecutionDelegatedPrincipal + +/** + * The internal table routes serve two caller kinds on the same paths, and they + * speak different column keyings: the first-party grid holds the schema it + * rendered and addresses cells by stable id, while a workflow tool execution + * speaks column names, because names are what tool enrichment surfaces to the + * model. Keying is therefore a property of the caller, not of the endpoint. + */ +export function rowKeyingForPrincipal(principal: TableRowRoutePrincipal): TableRowDataKeying { + switch (principal.kind) { + case 'session': + return 'ids' + case 'delegated': + return 'names' + } +} + +/** + * One row in the narrower projection the single-row and upsert routes return: + * the stored cells in the caller's keying, plus position, with timestamps + * already serialized. See `tableRowWireSchema`, which is its contract. + */ +export function presentRowForPrincipal( + row: Pick, + schema: TableSchema, + principal: TableRowRoutePrincipal +) { + // Only the outbound mapper is needed here; building the full translator set + // would also index the schema name→id for inbound paths a presenter cannot reach. + const dataOut = + rowKeyingForPrincipal(principal) === 'names' ? namedRowMapper(schema.columns) : identity + return { + id: row.id, + data: dataOut(row.data), + position: row.position, + createdAt: toWireTimestamp(row.createdAt), + updatedAt: toWireTimestamp(row.updatedAt), + } +} + +function identity(value: T): T { + return value +} diff --git a/apps/sim/app/api/table/table-tool-auth.test.ts b/apps/sim/app/api/table/table-tool-auth.test.ts new file mode 100644 index 00000000000..9f2a6a3bede --- /dev/null +++ b/apps/sim/app/api/table/table-tool-auth.test.ts @@ -0,0 +1,45 @@ +/** + * @vitest-environment node + * + * The executor reaches the internal table row routes through the Table block's + * tools, and those routes now authenticate with the delegation policy rather + * than the legacy internal token. Two things have to line up for that to work, + * and neither is visible to a route test that mocks the auth policy: + * + * 1. the tool must ask the executor to mint a delegation token, and + * 2. the operation's policy must admit the `executor` delegated service. + * + * Both are pinned here because getting either wrong fails every workflow call + * to these endpoints — the first as a 401, the second as a 403 — while every + * route-level test keeps passing. + */ +import { describe, expect, it } from 'vitest' +import { tableOperations } from '@/lib/table/application/operations' +import { tableDeleteRowTool } from '@/tools/table/delete_row' +import { tableGetRowTool } from '@/tools/table/get_row' +import { tableUpdateRowTool } from '@/tools/table/update_row' +import { tableUpsertRowTool } from '@/tools/table/upsert_row' + +/** Tool → the operation its route runs under. */ +const EXECUTOR_ROW_TOOLS = [ + ['table_get_row', tableGetRowTool, tableOperations.readRow], + ['table_update_row', tableUpdateRowTool, tableOperations.updateRow], + ['table_delete_row', tableDeleteRowTool, tableOperations.deleteRow], + ['table_upsert_row', tableUpsertRowTool, tableOperations.upsertRow], +] as const + +describe('executor access to the migrated table row routes', () => { + it.each(EXECUTOR_ROW_TOOLS)('%s asks the executor for a delegation token', (_name, tool) => { + // Without this the executor mints a legacy internal token, which the + // delegation policy rejects outright. + expect(tool.request.internalAuth).toBe('executor_delegation') + }) + + it.each(EXECUTOR_ROW_TOOLS)( + '%s runs under an operation that admits the executor', + (_name, _tool, operation) => { + expect(operation.delegatedServices).toContain('executor') + expect(operation.principalKinds).toContain('delegated') + } + ) +}) diff --git a/apps/sim/lib/api/contracts/tables.ts b/apps/sim/lib/api/contracts/tables.ts index 0ed44ed4543..e5eeefac94e 100644 --- a/apps/sim/lib/api/contracts/tables.ts +++ b/apps/sim/lib/api/contracts/tables.ts @@ -350,6 +350,25 @@ export const rowDataSchema = domainObjectSchema() export const tableDefinitionSchema = domainObjectSchema() export const tableRowSchema = domainObjectSchema() +/** + * One row as the single-row routes actually emit it: the stored cells plus + * position, with timestamps already serialized. + * + * Deliberately not {@link tableRowSchema}. That one describes a `TableRow`, + * which carries the per-cell `executions` sidecar and `Date` objects — accurate + * for the list and query routes, which return exactly that, and wrong for the + * single-row routes, which have always projected a narrower object with ISO + * strings. Two shapes on the wire need two schemas; collapsing them would make + * one of the two lie to its clients. + */ +export const tableRowWireSchema = z.object({ + id: z.string(), + data: rowDataSchema, + position: z.number(), + createdAt: z.string(), + updatedAt: z.string(), +}) + /** * Plain-object base for the single-row insert body. Kept un-refined so callers * (e.g. the v1 public contract) can `.omit()` fields before applying @@ -1425,7 +1444,7 @@ export const upsertTableRowContract = defineRouteContract({ mode: 'json', schema: successResponseSchema( z.object({ - row: tableRowSchema, + row: tableRowWireSchema, operation: z.enum(['insert', 'update']), message: z.string(), }) @@ -1433,6 +1452,22 @@ export const upsertTableRowContract = defineRouteContract({ }, }) +/** + * Reads one row. The sibling of {@link updateTableRowContract} and + * {@link deleteTableRowContract}, which take their workspace scope from a body; + * a GET has none, so it is asserted on the query string instead. + */ +export const getTableRowContract = defineRouteContract({ + method: 'GET', + path: '/api/table/[tableId]/rows/[rowId]', + params: tableRowParamsSchema, + query: getTableQuerySchema, + response: { + mode: 'json', + schema: successResponseSchema(z.object({ row: tableRowWireSchema })), + }, +}) + export const updateTableRowContract = defineRouteContract({ method: 'PATCH', path: '/api/table/[tableId]/rows/[rowId]', @@ -1442,7 +1477,7 @@ export const updateTableRowContract = defineRouteContract({ mode: 'json', schema: successResponseSchema( z.object({ - row: tableRowSchema, + row: tableRowWireSchema, message: z.string(), }) ), diff --git a/apps/sim/lib/api/server/routes/internal-json-route.ts b/apps/sim/lib/api/server/routes/internal-json-route.ts index 8e362c35f66..ae198a2684d 100644 --- a/apps/sim/lib/api/server/routes/internal-json-route.ts +++ b/apps/sim/lib/api/server/routes/internal-json-route.ts @@ -199,15 +199,33 @@ type InternalJsonParseOptions = Pick< 'maxBodyBytes' | 'validationErrorResponse' > -type InternalJsonPresenter = [R] extends [ - ContractJsonResponse, -] - ? { - present?(result: NoInfer): ContractJsonResponse | Promise> - } - : { - present(result: NoInfer): ContractJsonResponse | Promise> - } +/** + * What a presenter may render from, beyond the use case's result. + * + * A surface that serves more than one caller kind can owe them different wire + * shapes for the same domain result — the internal table row routes answer a + * session in stable column ids and a workflow execution in column names. That is + * presentation, not domain, so it belongs in the adapter rather than the use + * case. {@link InternalJsonRouteOptions.responseHeaders} and + * {@link InternalJsonRouteOptions.finalizeResponse} already receive this pair; + * this closes the same gap for `present`. + */ +export interface InternalJsonPresenterContext { + principal: P + input: I +} + +type InternalJsonPresentFn = ( + result: NoInfer, + context: InternalJsonPresenterContext, NoInfer

> +) => ContractJsonResponse | Promise> + +/** The presenter is optional exactly when the result already is the response body. */ +type InternalJsonPresenter = [ + R, +] extends [ContractJsonResponse] + ? { present?: InternalJsonPresentFn } + : { present: InternalJsonPresentFn } type InternalJsonRouteOptions< C extends JsonApiRouteContract, @@ -239,7 +257,7 @@ type InternalJsonRouteOptions< result: NoInfer body: ContractJsonResponse }): InternalJsonResponseFinalization | Promise -} & InternalJsonPresenter +} & InternalJsonPresenter function createJsonErrorResponse(descriptor: JsonErrorResponseDescriptor): NextResponse { return NextResponse.json(withRequestId(descriptor.body), { @@ -333,7 +351,7 @@ export function defineInternalJsonRoute< request, }) await options.onSuccess?.({ principal, input, result }) - const body = options.present ? await options.present(result) : result + const body = options.present ? await options.present(result, { principal, input }) : result const responseSchema = options.contract.response if (responseSchema.mode !== 'json') { throw new Error('Internal JSON route response mode changed after initialization') diff --git a/apps/sim/lib/table/__tests__/update-row.test.ts b/apps/sim/lib/table/__tests__/update-row.test.ts index 46777d4b185..f2027de5445 100644 --- a/apps/sim/lib/table/__tests__/update-row.test.ts +++ b/apps/sim/lib/table/__tests__/update-row.test.ts @@ -8,6 +8,8 @@ import { deleteColumn, renameColumn } from '@/lib/table/columns/service' import { batchInsertRows, batchUpdateRows, + getRowById, + getRowSummaryById, insertRow, replaceTableRows, updateRow, @@ -658,3 +660,37 @@ describe('updateRow — uniqueness probe scoping', () => { expect(checkUniqueConstraintsDb).not.toHaveBeenCalled() }) }) + +/** + * The read surfaces never put the executions sidecar on the wire, so loading it + * for them is a query whose result is discarded. Two readers rather than a flag: + * a caller that forgets a flag reads an empty sidecar and cannot tell that from + * a row that has none, whereas here the field is not on the type. + */ +describe('row readers', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + dbChainMockFns.limit.mockResolvedValue([EXISTING_ROW]) + }) + + it('getRowSummaryById issues one select and returns no sidecar', async () => { + const row = await getRowSummaryById('tbl-1', 'row-1', 'ws-1') + + expect(row).not.toHaveProperty('executions') + expect(dbChainMockFns.select).toHaveBeenCalledTimes(1) + }) + + it('getRowById issues the extra select the sidecar needs', async () => { + const row = await getRowById('tbl-1', 'row-1', 'ws-1') + + expect(row).toHaveProperty('executions') + expect(dbChainMockFns.select).toHaveBeenCalledTimes(2) + }) + + it('getRowSummaryById returns null for a missing row', async () => { + dbChainMockFns.limit.mockResolvedValue([]) + + await expect(getRowSummaryById('tbl-1', 'nope', 'ws-1')).resolves.toBeNull() + }) +}) diff --git a/apps/sim/lib/table/api/row-route-policies.ts b/apps/sim/lib/table/api/row-route-policies.ts index 08dba0ffdda..4a93c4e74c6 100644 --- a/apps/sim/lib/table/api/row-route-policies.ts +++ b/apps/sim/lib/table/api/row-route-policies.ts @@ -1,5 +1,10 @@ -import type { V2ErrorPolicy } from '@/lib/api/server/routes' -import { v2TableErrorPolicies } from '@/lib/table/api/route-policies' +import { + extendInternalErrorPolicy, + internalErrorResponse, + type V2ErrorPolicy, +} from '@/lib/api/server/routes' +import { internalTableErrorPolicies, v2TableErrorPolicies } from '@/lib/table/api/route-policies' +import { TableRowProvenanceError } from '@/lib/table/application/row-secret-provenance' import { TableRowsValidationError } from '@/lib/table/application/rows' import { v2Error } from '@/app/api/v2/lib/response' @@ -11,3 +16,22 @@ export const v2TableRowsErrorPolicy = { return v2TableErrorPolicies.concealTableAuthorization.render(error) }, } satisfies V2ErrorPolicy + +/** + * Row routes on the internal surface. The internal counterpart of + * {@link v2TableRowsErrorPolicy}: a row-shape complaint and a provenance + * envelope that does not authenticate are both the caller's to fix and answer + * 400; everything else conceals a cross-tenant table behind the same not-found + * wording the rest of the table surface uses. + * + * Built on the lock-aware base so a 423 keeps carrying `lock` — the only field + * that tells a client which lock to clear. A row write is exactly as lockable + * as a group mutation. + */ +export const internalTableRowsErrorPolicy = extendInternalErrorPolicy( + internalTableErrorPolicies.concealTableGroupAuthorization, + (error) => + error instanceof TableRowsValidationError || error instanceof TableRowProvenanceError + ? internalErrorResponse(400, { error: error.message }) + : null +) diff --git a/apps/sim/lib/table/application/operations.test.ts b/apps/sim/lib/table/application/operations.test.ts index 3b9dc95cc27..269719d8655 100644 --- a/apps/sim/lib/table/application/operations.test.ts +++ b/apps/sim/lib/table/application/operations.test.ts @@ -65,17 +65,26 @@ describe('table operation registry', () => { tableOperations.cancelExport.id, tableOperations.downloadExport.id, ]) - const sharedGroupOperations = new Set([ + // Reachable from the executor's Table block as well as from Copilot. The + // single-row operations joined this set when their routes moved onto the + // delegation auth policy: the Table block's get/update/delete/upsert row + // tools run under them, and a policy without `executor` fails every one of + // those calls with a 403 while every route test still passes. + const sharedToolOperations = new Set([ tableOperations.createGroup.id, tableOperations.updateGroup.id, tableOperations.deleteGroup.id, + tableOperations.readRow.id, + tableOperations.updateRow.id, + tableOperations.deleteRow.id, + tableOperations.upsertRow.id, ]) for (const operation of Object.values(tableOperations)) { expect(operation.delegatedServices).toEqual( executorOnlyOperations.has(operation.id) ? ['executor'] - : sharedGroupOperations.has(operation.id) + : sharedToolOperations.has(operation.id) ? ['copilot', 'executor'] : ['copilot'] ) diff --git a/apps/sim/lib/table/application/operations.ts b/apps/sim/lib/table/application/operations.ts index 42476b3783a..dfc7fc8e698 100644 --- a/apps/sim/lib/table/application/operations.ts +++ b/apps/sim/lib/table/application/operations.ts @@ -46,6 +46,15 @@ function toolWriteOperation(id: Id) { }) } +function toolReadOperation(id: Id) { + return defineWorkspaceOperation({ + id, + minimumRole: 'read', + workspaceApiKey: 'allow', + ...ALL_TABLE_TOOL_PRINCIPAL_POLICY, + }) +} + function internalExecutorReadOperation(id: Id) { return defineWorkspaceOperation({ id, @@ -104,14 +113,14 @@ export const tableOperations = { listRows: readOperation('tables.rows.list'), queryRows: readOperation('tables.rows.query'), findRows: readOperation('tables.rows.find'), - readRow: readOperation('tables.rows.read'), + readRow: toolReadOperation('tables.rows.read'), createRows: writeOperation('tables.rows.create'), replaceRows: writeOperation('tables.rows.replace'), - updateRow: writeOperation('tables.rows.update'), + updateRow: toolWriteOperation('tables.rows.update'), updateRows: writeOperation('tables.rows.update_many'), - deleteRow: writeOperation('tables.rows.delete'), + deleteRow: toolWriteOperation('tables.rows.delete'), deleteRows: writeOperation('tables.rows.delete_many'), - upsertRow: writeOperation('tables.rows.upsert'), + upsertRow: toolWriteOperation('tables.rows.upsert'), listViews: readOperation('tables.views.list'), readView: readOperation('tables.views.read'), createView: writeOperation('tables.views.create'), diff --git a/apps/sim/lib/table/application/row-secret-provenance.test.ts b/apps/sim/lib/table/application/row-secret-provenance.test.ts new file mode 100644 index 00000000000..c43d7546dfe --- /dev/null +++ b/apps/sim/lib/table/application/row-secret-provenance.test.ts @@ -0,0 +1,216 @@ +/** + * @vitest-environment node + * + * The provenance envelope moved out of the route adapter and into the domain, + * because interpreting a caller's selections requires the canonical schema and + * the adapter must not load it. These pin the semantics that move with it. + */ +import { describe, expect, it, vi } from 'vitest' + +const { mocks } = vi.hoisted(() => ({ + mocks: { scopeCompatible: vi.fn(() => true), isBundle: vi.fn(() => true) }, +})) + +vi.mock('@/lib/execution/durable-secret-provenance', () => ({ + isPrivateSecretProvenanceScopeCompatible: mocks.scopeCompatible, +})) + +vi.mock('@/lib/execution/model-input-provenance', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, isPrivateSecretProvenanceBundleV1: mocks.isBundle } +}) + +import { + resolveRowWriteProvenance, + TableRowProvenanceError, +} from '@/lib/table/application/row-secret-provenance' +import type { TableDefinition } from '@/lib/table/types' + +const TABLE = { + id: 'tbl_1', + workspaceId: 'workspace-1', + schema: { + columns: [ + { id: 'col_aaa', name: 'Name', type: 'string' }, + { id: 'col_bbb', name: 'Age', type: 'number' }, + ], + }, +} as unknown as TableDefinition + +const SESSION = { kind: 'session' as const, userId: 'user-1', sessionId: 'session-1' } +const EXECUTOR = { + kind: 'delegated' as const, + serviceId: 'executor' as const, + subjectUserId: 'user-1', + workspaceId: 'workspace-1', + delegationId: 'delegation-1', + audience: 'table', + issuedAt: new Date('2026-01-01'), + expiresAt: new Date('2026-01-02'), +} + +function resolve(overrides: Partial[0]>) { + return resolveRowWriteProvenance({ + envelope: { kind: 'none' }, + principal: SESSION, + workspaceId: 'workspace-1', + table: TABLE, + keying: 'ids', + wireRows: [{ col_aaa: 'Ada' }], + storageRows: [{ col_aaa: 'Ada' }], + ...overrides, + }) +} + +describe('row write provenance', () => { + it('certifies an interactive write exact-empty over the columns it persists', () => { + const { stamps } = resolve({}) + + expect(stamps).toEqual([ + { complete: true, columns: { col_aaa: { version: 1, complete: true, entries: [] } } }, + ]) + }) + + it('leaves an internal caller that sent no envelope untracked', () => { + // Not exact-empty: stamping "this write introduced no secrets" on a runtime + // write that sent no envelope would be a false certification. + const { stamps } = resolve({ principal: EXECUTOR }) + + expect(stamps).toEqual([undefined]) + }) + + it('refuses a bundle from a session caller', () => { + expect(() => + resolve({ envelope: { kind: 'bundle', value: { complete: true, selections: [] } } }) + ).toThrow(TableRowProvenanceError) + }) + + it('refuses a bundle that is not a recognised envelope', () => { + mocks.isBundle.mockReturnValueOnce(false) + + expect(() => + resolve({ principal: EXECUTOR, envelope: { kind: 'bundle', value: { nope: true } } }) + ).toThrow(TableRowProvenanceError) + }) + + it('refuses a complete bundle that does not account for every written cell', () => { + expect(() => + resolve({ + principal: EXECUTOR, + envelope: { kind: 'bundle', value: { complete: true, selections: [] } }, + wireRows: [{ col_aaa: 'Ada', col_bbb: 36 }], + storageRows: [{ col_aaa: 'Ada', col_bbb: 36 }], + }) + ).toThrow(TableRowProvenanceError) + }) + + it('refuses a selection whose scope this principal may not read', () => { + mocks.scopeCompatible.mockReturnValueOnce(false) + + expect(() => + resolve({ + principal: EXECUTOR, + envelope: { + kind: 'bundle', + value: { + complete: true, + selections: [ + { key: JSON.stringify([0, 'col_aaa']), provenance: { scope: { kind: 'workspace' } } }, + ], + }, + }, + }) + ).toThrow(TableRowProvenanceError) + }) + + it('marks an incomplete bundle unknown rather than certifying it', () => { + const { stamps } = resolve({ + principal: EXECUTOR, + envelope: { kind: 'bundle', value: { complete: false, selections: [] } }, + }) + + expect(stamps).toEqual([{ complete: false, columns: {} }]) + }) + + it('keys a name-wire selection to the storage column it certifies', () => { + const { stamps } = resolve({ + principal: EXECUTOR, + keying: 'names', + wireRows: [{ Name: 'Ada' }], + storageRows: [{ col_aaa: 'Ada' }], + envelope: { + kind: 'bundle', + value: { + complete: true, + selections: [ + { key: JSON.stringify([0, 'Name']), provenance: { scope: { kind: 'workspace' } } }, + ], + }, + }, + }) + + expect(stamps[0]).toEqual({ + complete: true, + columns: { col_aaa: { scope: { kind: 'workspace' } } }, + }) + }) + + it('checks the scope against the acting principal, not a billing owner', () => { + resolve({ + principal: EXECUTOR, + envelope: { + kind: 'bundle', + value: { + complete: true, + selections: [{ key: JSON.stringify([0, 'col_aaa']), provenance: { scope: {} } }], + }, + }, + }) + + expect(mocks.scopeCompatible).toHaveBeenCalledWith( + {}, + { userId: 'user-1', workspaceId: 'workspace-1' } + ) + }) + + it('records provenance for an id-keyed key the write persists, recognised or not', () => { + // The id wire stores what it is given, so every key it sends is a storage + // key. Mapping an unrecognised one to null would leave a written cell + // uncertified under a complete stamp. + const { stamps } = resolve({ + principal: EXECUTOR, + keying: 'ids', + wireRows: [{ 'col-unknown': 'x' }], + storageRows: [{ 'col-unknown': 'x' }], + envelope: { + kind: 'bundle', + value: { + complete: true, + selections: [{ key: JSON.stringify([0, 'col-unknown']), provenance: { scope: {} } }], + }, + }, + }) + + expect(stamps[0]).toEqual({ complete: true, columns: { 'col-unknown': { scope: {} } } }) + }) + + it('records nothing for a key that names no column, since it is never stored', () => { + const { stamps } = resolve({ + principal: EXECUTOR, + keying: 'names', + wireRows: [{ Nope: 'x' }], + storageRows: [{}], + envelope: { + kind: 'bundle', + value: { + complete: true, + selections: [ + { key: JSON.stringify([0, 'Nope']), provenance: { scope: { kind: 'workspace' } } }, + ], + }, + }, + }) + + expect(stamps[0]).toEqual({ complete: true, columns: {} }) + }) +}) diff --git a/apps/sim/lib/table/application/row-secret-provenance.ts b/apps/sim/lib/table/application/row-secret-provenance.ts new file mode 100644 index 00000000000..3911703ac42 --- /dev/null +++ b/apps/sim/lib/table/application/row-secret-provenance.ts @@ -0,0 +1,146 @@ +import { type Principal, requirePrincipalSubjectUserId } from '@sim/auth/principal' +import { isPrivateSecretProvenanceScopeCompatible } from '@/lib/execution/durable-secret-provenance' +import { isPrivateSecretProvenanceBundleV1 } from '@/lib/execution/model-input-provenance' +import { buildIdByName } from '@/lib/table/column-keys' +import { + createExactEmptyTableRowSecretProvenance, + createUnknownTableRowSecretProvenance, +} from '@/lib/table/rows/secret-provenance' +import { tableRowSecretProvenanceSelectionKey } from '@/lib/table/secret-provenance-selection' +import type { RowData, TableDefinition, TableRowSecretProvenanceWrite } from '@/lib/table/types' + +/** + * The private provenance envelope exactly as it arrived on the wire. + * + * A surface adapter can read the header and the payload field — that is + * transport — but it cannot decide what the selections mean, because mapping a + * caller's column key to the storage column id it certifies requires the + * canonical schema. That resolution lives here, behind authorization, which is + * what lets the row routes stop loading the table for themselves. + */ +export type TableRowProvenanceEnvelope = { kind: 'none' } | { kind: 'bundle'; value: unknown } + +/** Raised when an envelope does not authenticate against the canonical table. */ +export class TableRowProvenanceError extends Error { + constructor(message = 'Invalid table row secret provenance') { + super(message) + this.name = 'TableRowProvenanceError' + } +} + +/** + * Storage column id for each key the caller wrote, or `null` where the key names + * no column and is therefore never persisted. + * + * This must mirror {@link rowDataToStorage} exactly, or a cell could be written + * with no provenance recorded under a `complete` stamp. The two wires differ in + * what they do with an unrecognised key: the name path drops it, so it gets + * `null`; the id path stores what it is given, so every key it sends is a + * storage key and none of them is `null`. + */ +function storageKeyResolver( + table: TableDefinition, + keying: 'names' | 'ids' +): (wireKey: string) => string | null { + // Built once for the whole batch rather than once per row, matching how + // `rowsToStorage` hoists the same index. + if (keying === 'ids') return (wireKey) => wireKey + const idByName = buildIdByName(table.schema) + return (wireKey) => idByName.get(wireKey) ?? null +} + +/** + * What a write should stamp on its provenance sidecar. + * + * `undefined` is not "nothing to record" — it means *deliberately untracked*, the + * legacy protocol for an internal caller that sent no envelope. Defaulting it to + * an exact-empty stamp would certify "this write introduced no secrets" on a + * runtime write that may well have introduced some, so the two must stay + * distinguishable all the way to the sidecar. + */ +export interface ResolvedRowWriteProvenance { + stamps: Array +} + +/** + * Resolves a wire envelope into per-row provenance stamps against the canonical + * table. + * + * An interactive caller (session) certifies exact-empty over the storage columns + * its write actually persists. An internal execution may submit an encrypted + * bundle, which must name exactly the columns the write touched and must carry a + * scope this principal is allowed to read. Anything else fails closed. + */ +export function resolveRowWriteProvenance(options: { + envelope: TableRowProvenanceEnvelope + principal: Principal + workspaceId: string + table: TableDefinition + keying: 'names' | 'ids' + wireRows: readonly RowData[] + storageRows: readonly RowData[] +}): ResolvedRowWriteProvenance { + const { envelope, principal, table, keying, wireRows, storageRows } = options + const isDelegated = principal.kind !== 'session' + + if (envelope.kind === 'none') { + // An internal caller that sent nothing stays untracked, as it always has. + if (isDelegated) return { stamps: wireRows.map(() => undefined) } + return { stamps: storageRows.map((row) => createExactEmptyTableRowSecretProvenance(row)) } + } + + if (!isDelegated || !isPrivateSecretProvenanceBundleV1(envelope.value)) { + throw new TableRowProvenanceError() + } + const bundle = envelope.value + + const storageKeyFor = storageKeyResolver(table, keying) + /** Every cell this write touches, by the selection key a bundle must name. */ + const touchedBySelectionKey = new Map() + wireRows.forEach((row, rowIndex) => { + for (const wireKey of Object.keys(row)) { + touchedBySelectionKey.set(tableRowSecretProvenanceSelectionKey(rowIndex, wireKey), { + rowIndex, + columnId: storageKeyFor(wireKey), + }) + } + }) + + // A complete bundle must account for every cell the write touched, and only + // those — otherwise a caller could certify a column it never wrote. + if ( + bundle.complete && + (bundle.selections.length !== touchedBySelectionKey.size || + bundle.selections.some((selection) => !touchedBySelectionKey.has(selection.key))) + ) { + throw new TableRowProvenanceError() + } + + if (!bundle.complete) { + return { stamps: wireRows.map(() => createUnknownTableRowSecretProvenance()) } + } + + const stamps: TableRowSecretProvenanceWrite[] = wireRows.map(() => ({ + complete: true, + columns: {}, + })) + const subjectUserId = requirePrincipalSubjectUserId(principal) + for (const selection of bundle.selections) { + const touched = touchedBySelectionKey.get(selection.key) + if ( + !touched || + !isPrivateSecretProvenanceScopeCompatible(selection.provenance.scope, { + userId: subjectUserId, + workspaceId: options.workspaceId, + }) + ) { + throw new TableRowProvenanceError() + } + if (touched.columnId === null) continue + if (Object.hasOwn(stamps[touched.rowIndex].columns, touched.columnId)) { + throw new TableRowProvenanceError() + } + stamps[touched.rowIndex].columns[touched.columnId] = selection.provenance + } + return { stamps } +} diff --git a/apps/sim/lib/table/application/rows.test.ts b/apps/sim/lib/table/application/rows.test.ts index 23d52dcb87c..2bfbeb337b9 100644 --- a/apps/sim/lib/table/application/rows.test.ts +++ b/apps/sim/lib/table/application/rows.test.ts @@ -19,6 +19,7 @@ const { mockResolveContext, mockResolvePermission, mockSignalRowsChanged, + mockSignalRowsChangedByActor, mockUpsertRow, mockWithLockedTable, mockInsertRow, @@ -41,6 +42,7 @@ const { mockResolveContext: vi.fn(), mockResolvePermission: vi.fn(), mockSignalRowsChanged: vi.fn(), + mockSignalRowsChangedByActor: vi.fn(), mockUpsertRow: vi.fn(), mockWithLockedTable: vi.fn(), mockInsertRow: vi.fn(), @@ -137,6 +139,7 @@ vi.mock('@/lib/table/application/context', () => ({ vi.mock('@/lib/table/events', () => ({ signalTableRowsChanged: mockSignalRowsChanged, + signalTableRowsChangedByActor: mockSignalRowsChangedByActor, })) import { diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index 2f00f7b9696..3e0a06710fa 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -1,6 +1,11 @@ import { isDeepStrictEqual } from 'node:util' import { AuditAction, AuditResourceType } from '@sim/audit' -import { requirePrincipalSubjectUserId, resolvePrincipalAttribution } from '@sim/auth/principal' +import { + type Principal, + requirePrincipalSubjectUserId, + resolvePrincipalAttribution, +} from '@sim/auth/principal' +import { db } from '@sim/db' import { getRequestContext } from '@sim/logger' import { generateId } from '@sim/utils/id' import { isPlainRecord } from '@sim/utils/object' @@ -25,13 +30,14 @@ import { deleteRowsByFilter, deleteRowsByIds, findRowMatches, - getRowById, + getRowSummaryById, insertRow, queryRows, replaceTableRows as replaceTableRowsPrimitive, rowDataNameToId, sortSpecNamesToIds, TABLE_LIMITS, + type TableRowSummary, updateRow, updateRowsByFilter, upsertRow, @@ -42,11 +48,15 @@ import { import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized-table-use-case' import { resolveActiveTableContext } from '@/lib/table/application/context' import { tableOperations } from '@/lib/table/application/operations' +import { + resolveRowWriteProvenance, + type TableRowProvenanceEnvelope, +} from '@/lib/table/application/row-secret-provenance' import { assertRowCapacity, notifyTableRowUsage } from '@/lib/table/billing' import { buildColumnNameById, buildIdByName, unknownColumnNames } from '@/lib/table/column-keys' import { columnTypeOf } from '@/lib/table/column-types' import { TableQueryValidationError } from '@/lib/table/errors' -import { signalTableRowsChanged } from '@/lib/table/events' +import { signalTableRowsChanged, signalTableRowsChangedByActor } from '@/lib/table/events' import { predicateToFilter } from '@/lib/table/query-builder/converters' import { validatePredicate, @@ -55,6 +65,7 @@ import { validateStoragePredicate, } from '@/lib/table/query-builder/validate' import { assertCursorQueryBinding, decodeCursor } from '@/lib/table/rows/cursor' +import { loadEnrichmentDetail } from '@/lib/table/rows/executions' import { createExactEmptyTableRowSecretProvenance, createTableRowSecretProvenanceFromRegistry, @@ -109,7 +120,9 @@ type TableRowsProvenance = Awaited[0], workspaceId: string, - rows: TableRow[], + // The loader reads only id, updatedAt and data, so a row without its + // executions sidecar is enough — see `TABLE_ROW_SIDECAR_SELECTION`. + rows: TableRowSummary[], include: boolean | undefined ): Promise { if (!include) return undefined @@ -264,6 +277,38 @@ function defaultedRowSecretProvenance( return provided ?? createExactEmptyTableRowSecretProvenance(storageData) } +/** + * The stamp a single-row write should carry: resolved from the caller's envelope + * when it handed one over, otherwise defaulted. Shared by the update and upsert + * use cases so the envelope contract has one implementation, not two. + */ +function singleRowWriteProvenance(options: { + principal: Principal + workspaceId: string + table: TableDefinition + input: { + dataKeying: TableRowDataKeying + data: RowData + secretProvenance?: TableRowSecretProvenanceWrite + secretProvenanceEnvelope?: TableRowProvenanceEnvelope + } + storageData: RowData +}): TableRowSecretProvenanceWrite | undefined { + const { principal, workspaceId, table, input, storageData } = options + if (!input.secretProvenanceEnvelope) { + return defaultedRowSecretProvenance(storageData, input.secretProvenance) + } + return resolveRowWriteProvenance({ + envelope: input.secretProvenanceEnvelope, + principal, + workspaceId, + table, + keying: input.dataKeying, + wireRows: [input.data], + storageRows: [storageData], + }).stamps[0] +} + function defaultedRowsSecretProvenance( storageRows: RowData[], provided: Array | undefined @@ -466,7 +511,8 @@ export interface ReadTableRowInput extends TableScopedInput { } export interface ReadTableRowResult extends TableResult { - row: TableRow + /** Without the executions sidecar — no read surface puts it on the wire. */ + row: TableRowSummary secretProvenance?: TableRowsProvenance } @@ -474,7 +520,7 @@ export const readTableRow = defineAuthorizedTableUseCase({ operation: tableOperations.readRow, resolveContext: ({ input }: { input: ReadTableRowInput }) => resolveActiveTableContext(input), async execute({ principal, input, context }): Promise { - const row = await getRowById(context.tableId, input.rowId, context.workspaceId) + const row = await getRowSummaryById(context.tableId, input.rowId, context.workspaceId) if (!row) throw new OrchestrationError('not_found', 'Row not found') return { table: context.table, @@ -489,12 +535,44 @@ export const readTableRow = defineAuthorizedTableUseCase({ }, }) +export interface ReadTableRowEnrichmentInput extends TableScopedInput { + rowId: string + groupId: string +} + +export interface ReadTableRowEnrichmentResult extends TableResult { + detail: Awaited> +} + +/** + * The enrichment cascade breakdown — provider outcomes, cost, timing — for one + * cell. Deliberately kept off the hot grid read and fetched on demand by the + * details panel; `null` for a cell with no recorded run, or a run predating the + * feature. + * + * Shares {@link tableOperations.readRow}: this is a projection of the same row, + * under the same role, so it is not a second semantic operation. + */ +export const readTableRowEnrichmentDetail = defineAuthorizedTableUseCase({ + operation: tableOperations.readRow, + resolveContext: ({ input }: { input: ReadTableRowEnrichmentInput }) => + resolveActiveTableContext(input), + async execute({ input, context }): Promise { + return { + table: context.table, + detail: await loadEnrichmentDetail(db, context.tableId, input.rowId, input.groupId), + } + }, +}) + interface CreateSingleTableRowInput extends TableScopedInput { /** See {@link rowWriteOptions}. Required so a new write surface must choose. */ strictWrite: boolean /** See {@link TableRowDataKeying}. Required so a new write surface must choose. */ dataKeying: TableRowDataKeying kind: 'single' + /** See {@link UpdateTableRowInput.actorClientId}. */ + actorClientId?: string data: RowData position?: number afterRowId?: string @@ -597,9 +675,16 @@ export const createTableRows = defineAuthorizedTableUseCase({ ) return { kind: 'batch', table: context.table, rows: created } }, - afterSuccess: ({ context, result }) => { - const affected = result.kind === 'single' ? 1 : result.rows.length - if (affected > 0) signalTableRowsChanged(context.tableId) + afterSuccess: ({ context, input, result }) => { + // Narrowed on the input, not the result: only the single-row variant carries + // an actor, and the two discriminants always agree. + if (input.kind === 'single') { + signalTableRowsChangedByActor(context.tableId, input.actorClientId) + return + } + // A batch insert is not reconciled locally by the acting tab, so it must + // refetch like every other subscriber. + if (result.kind === 'batch' && result.rows.length > 0) signalTableRowsChanged(context.tableId) }, }) @@ -834,7 +919,23 @@ export interface UpdateTableRowInput extends TableScopedInput { rowId: string data: RowData secretProvenance?: TableRowSecretProvenanceWrite + /** + * Private provenance envelope as it arrived on the wire, resolved here against + * the canonical schema. Mutually exclusive with {@link secretProvenance}: a + * surface either resolves its own stamp or hands over the envelope for this + * use case to resolve, never both. + */ + secretProvenanceEnvelope?: TableRowProvenanceEnvelope includePersistedSecretProvenance?: boolean + /** + * Tab that caused this write, when the calling surface knows it. Lets that tab + * skip refetching its own write — see {@link signalTableRowsChangedByActor}, + * whose soundness condition is that the caller's hook reconciles the write + * locally across every cached rows query. Only the single-row paths accept + * one: a batch or filter-scoped write genuinely needs the acting tab to + * refetch. Absent by default, which broadcasts to every subscriber as before. + */ + actorClientId?: string } export interface UpdateTableRowResult extends TableResult { @@ -848,6 +949,13 @@ export const updateTableRow = defineAuthorizedTableUseCase({ resolveContext: ({ input }: { input: UpdateTableRowInput }) => resolveActiveTableContext(input), async execute({ principal, input, context }): Promise { const data = rowDataToStorage(input.data, context.table, input.dataKeying, input.strictWrite) + const secretProvenance = singleRowWriteProvenance({ + principal, + workspaceId: context.workspaceId, + table: context.table, + input, + storageData: data, + }) const row = await updateRow( { tableId: context.tableId, @@ -855,7 +963,7 @@ export const updateTableRow = defineAuthorizedTableUseCase({ rowId: input.rowId, data, actorUserId: actorUserId(principal, context.billedAccountUserId), - secretProvenance: defaultedRowSecretProvenance(data, input.secretProvenance), + secretProvenance, }, context.table, requestId(input), @@ -874,8 +982,8 @@ export const updateTableRow = defineAuthorizedTableUseCase({ ), } }, - afterSuccess: ({ context, result }) => { - if (result.changed) signalTableRowsChanged(context.tableId) + afterSuccess: ({ context, input, result }) => { + if (result.changed) signalTableRowsChangedByActor(context.tableId, input.actorClientId) }, }) @@ -925,6 +1033,8 @@ export const updateTableRows = defineAuthorizedTableUseCase({ export interface DeleteTableRowInput extends TableScopedInput { rowId: string + /** See {@link UpdateTableRowInput.actorClientId}. */ + actorClientId?: string } export interface DeleteTableRowResult extends TableResult { @@ -938,7 +1048,8 @@ export const deleteTableRow = defineAuthorizedTableUseCase({ await deleteRow(context.table, input.rowId, requestId(input)) return { table: context.table, deletedRowId: input.rowId } }, - afterSuccess: ({ context }) => signalTableRowsChanged(context.tableId), + afterSuccess: ({ context, input }) => + signalTableRowsChangedByActor(context.tableId, input.actorClientId), }) export type DeleteTableRowsInput = TableScopedInput & @@ -1014,11 +1125,16 @@ export interface UpsertTableRowInput extends TableScopedInput { data: RowData conflictTarget?: string secretProvenance?: TableRowSecretProvenanceWrite + /** See {@link UpdateTableRowInput.secretProvenanceEnvelope}. */ + secretProvenanceEnvelope?: TableRowProvenanceEnvelope + includePersistedSecretProvenance?: boolean } export interface UpsertTableRowResult extends TableResult { - row: TableRow + /** Without the executions sidecar — see {@link UpsertResult.row}. */ + row: TableRowSummary operation: 'insert' | 'update' + secretProvenance?: TableRowsProvenance } export const upsertTableRow = defineAuthorizedTableUseCase({ @@ -1032,6 +1148,13 @@ export const upsertTableRow = defineAuthorizedTableUseCase({ ? (buildIdByName(context.table.schema).get(input.conflictTarget) ?? input.conflictTarget) : input.conflictTarget const data = rowDataToStorage(input.data, context.table, input.dataKeying, input.strictWrite) + const secretProvenance = singleRowWriteProvenance({ + principal, + workspaceId: context.workspaceId, + table: context.table, + input, + storageData: data, + }) const result = await upsertRow( { tableId: context.tableId, @@ -1039,13 +1162,23 @@ export const upsertTableRow = defineAuthorizedTableUseCase({ data, conflictTarget, userId: actorUserId(principal, context.billedAccountUserId), - secretProvenance: defaultedRowSecretProvenance(data, input.secretProvenance), + secretProvenance, }, context.table, requestId(input), rowWriteOptions(input) ) - return { table: context.table, row: result.row, operation: result.operation } + return { + table: context.table, + row: result.row, + operation: result.operation, + secretProvenance: await loadAuthorizedRowsProvenance( + principal, + context.workspaceId, + [result.row], + input.includePersistedSecretProvenance + ), + } }, afterSuccess: ({ context }) => signalTableRowsChanged(context.tableId), }) diff --git a/apps/sim/lib/table/events.attribution.test.ts b/apps/sim/lib/table/events.attribution.test.ts index e0f8c9ee448..351aa715f5e 100644 --- a/apps/sim/lib/table/events.attribution.test.ts +++ b/apps/sim/lib/table/events.attribution.test.ts @@ -9,16 +9,25 @@ import { describe, expect, it } from 'vitest' * `signalTableRowsChangedByActor` lets the acting tab skip its own refetch, which is only sound * where that tab's mutation hook already applies the server's answer to every cached rows query. * That invariant lives in `hooks/queries/tables.ts` — nothing in the type system ties it to the - * call site, so a well-meaning fourth call would silently strand that client on stale rows. + * call site, so a well-meaning extra call would silently strand that client on stale rows. * - * This pins the allowlist. If you are here because it failed: adding a call means proving the - * calling route's client hook reconciles locally, then adding it below. Removing one is always safe. + * Two lists are pinned, because a migrated single-row route now signals from inside its + * application use case rather than from the route. The call itself is no longer the decision: + * that use case is shared with `/api/v2` and Copilot, and it degrades to a broadcast whenever no + * actor is named. What actually selects the behavior is which surface supplies `actorClientId`, + * so that is pinned too and is the list to scrutinise. + * + * If you are here because it failed: adding a supplier means proving that surface's client hook + * reconciles the write locally across every cached rows query. Removing one is always safe. */ const ATTRIBUTED_CALL_SITES = [ 'app/api/table/[tableId]/rows/route.ts', - 'app/api/table/[tableId]/rows/[rowId]/route.ts', + 'lib/table/application/rows.ts', ] as const +/** Surfaces that name the acting tab. See the note above — this is the real allowlist. */ +const ACTOR_SUPPLYING_SURFACES = ['app/api/table/[tableId]/rows/[rowId]/route.ts'] as const + const APP_ROOT = join(import.meta.dirname, '../..') /** Declares the function; matching its own definition would say nothing about call sites. */ const DECLARING_MODULE = 'lib/table/events.ts' @@ -32,17 +41,37 @@ async function* walk(dir: string): AsyncGenerator { } } +async function filesContaining(needle: string, skip: (relative: string) => boolean = () => false) { + const found: string[] = [] + for await (const file of walk(APP_ROOT)) { + const source = await readFile(file, 'utf8') + if (!source.includes(needle)) continue + const relative = file.slice(APP_ROOT.length + 1) + if (skip(relative)) continue + found.push(relative) + } + return found.sort() +} + describe('signalTableRowsChangedByActor call sites', () => { it('is called only where the acting tab reconciles the write locally', async () => { - const callers: string[] = [] - for await (const file of walk(APP_ROOT)) { - const source = await readFile(file, 'utf8') - if (!source.includes('signalTableRowsChangedByActor(')) continue - const relative = file.slice(APP_ROOT.length + 1) - if (relative === DECLARING_MODULE) continue - callers.push(relative) - } - - expect(callers.sort()).toEqual([...ATTRIBUTED_CALL_SITES].sort()) + const callers = await filesContaining( + 'signalTableRowsChangedByActor(', + (relative) => relative === DECLARING_MODULE + ) + + expect(callers).toEqual([...ATTRIBUTED_CALL_SITES].sort()) + }) + + it('is given an actor only by surfaces whose client hook reconciles locally', async () => { + const suppliers = await filesContaining( + 'actorClientId:', + // These declare or forward the field rather than naming a tab. + (relative) => + relative === 'lib/table/application/rows.ts' || + relative === 'lib/table/application/row-secret-provenance.ts' + ) + + expect(suppliers).toEqual([...ACTOR_SUPPLYING_SURFACES].sort()) }) }) diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index 5495e7ae38f..4857338e68a 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -791,12 +791,13 @@ export async function upsertRow( }) if (!updatedRow) throw new Error('Matched table row no longer exists') - const executions = await loadExecutionsForRow(trx, updatedRow.id) + // No executions sidecar: no upsert surface puts one on the wire, and + // loading it here would hold the write transaction open for a result that + // is discarded. See `getRowSummaryById` for the same reasoning on reads. return { row: { id: updatedRow.id, data: updatedRow.data as RowData, - executions, position: updatedRow.position, orderKey: updatedRow.orderKey ?? undefined, createdAt: updatedRow.createdAt, @@ -842,7 +843,6 @@ export async function upsertRow( row: { id: insertedRow.id, data: insertedRow.data as RowData, - executions: {}, position: insertedRow.position, orderKey: insertedRow.orderKey ?? undefined, createdAt: insertedRow.createdAt, @@ -1409,14 +1409,54 @@ async function fetchRowsBounded(params: BoundedFetchParams): Promise + +function selectRowRecord(tableId: string, rowId: string, workspaceId: string) { + return db + .select() + .from(userTableRows) + .where( + and( + eq(userTableRows.id, rowId), + eq(userTableRows.tableId, tableId), + eq(userTableRows.workspaceId, workspaceId) + ) + ) + .limit(1) +} + +function toRowSummary(row: Awaited>[number]): TableRowSummary { + return { + id: row.id, + data: row.data as RowData, + position: row.position, + orderKey: row.orderKey ?? undefined, + createdAt: row.createdAt, + updatedAt: row.updatedAt, + } +} + /** - * Gets a single row by ID. + * One row without its executions sidecar, for the surfaces that never put + * executions on the wire — the single-row read routes and the Copilot row tool. + * Loading the sidecar for them is a query whose result is discarded. * - * @param tableId - Table ID - * @param rowId - Row ID to fetch - * @param workspaceId - Workspace ID for access control - * @returns Row or null if not found + * Deliberately a separate function rather than a flag on {@link getRowById}: a + * caller that forgets to pass the flag reads an empty sidecar and cannot tell + * that from a row with no executions, whereas here the field simply is not on + * the type. */ +export async function getRowSummaryById( + tableId: string, + rowId: string, + workspaceId: string +): Promise { + const [row] = await selectRowRecord(tableId, rowId, workspaceId) + return row ? toRowSummary(row) : null +} + +/** One row with its executions sidecar, for the write and background paths. */ export async function getRowById( tableId: string, rowId: string, @@ -1427,32 +1467,13 @@ export async function getRowById( // round trip instead of two. A miss pays one redundant sidecar read, which is // the rare path and costs no extra wall time. const [results, executions] = await Promise.all([ - db - .select() - .from(userTableRows) - .where( - and( - eq(userTableRows.id, rowId), - eq(userTableRows.tableId, tableId), - eq(userTableRows.workspaceId, workspaceId) - ) - ) - .limit(1), + selectRowRecord(tableId, rowId, workspaceId), loadExecutionsForRow(db, rowId), ]) if (results.length === 0) return null - const row = results[0] - return { - id: row.id, - data: row.data as RowData, - executions, - position: row.position, - orderKey: row.orderKey ?? undefined, - createdAt: row.createdAt, - updatedAt: row.updatedAt, - } + return { ...toRowSummary(results[0]), executions } } /** @@ -1617,10 +1638,13 @@ export async function updateRow( // table that has any unique column this was several round trips on every // edit, including edits nowhere near one. // - // The one case this does not cover is a unique constraint added to a column - // that already held duplicates: such a row is no longer blocked from edits - // elsewhere in it. That is the intended outcome — an unrelated cell edit - // should not fail on data it did not write. + // What this does not cover is a duplicate that already exists — either from a + // constraint added to a column that already held one, or from two concurrent + // inserts both passing this probe, since uniqueness here is advisory (a + // SELECT, not a DB constraint). Such a row is no longer blocked from edits + // elsewhere in it. That is the intended outcome: an unrelated cell edit + // should not fail on data it did not write, and blocking it was never a + // repair mechanism. const patchedColumnIds = new Set(Object.keys(data.data)) const patchedUniqueColumns = getUniqueColumns(table.schema).filter((column) => patchedColumnIds.has(getColumnId(column)) diff --git a/apps/sim/lib/table/trigger.ts b/apps/sim/lib/table/trigger.ts index cdb2b2a3169..a08ebc093a0 100644 --- a/apps/sim/lib/table/trigger.ts +++ b/apps/sim/lib/table/trigger.ts @@ -51,7 +51,9 @@ export async function fireTableTrigger( tableId: string, tableName: string, eventType: EventType, - rows: TableRow[], + // Accepts a row without its executions sidecar: the payload projects id and + // data only, and the upsert path deliberately does not load one. + rows: Array>, oldRows: Map | null, schema: TableSchema, requestId: string diff --git a/apps/sim/lib/table/types.ts b/apps/sim/lib/table/types.ts index 1e11fc1f857..b0950443756 100644 --- a/apps/sim/lib/table/types.ts +++ b/apps/sim/lib/table/types.ts @@ -718,7 +718,11 @@ export interface UpsertRowData { } export interface UpsertResult { - row: TableRow + /** + * Without the executions sidecar: no upsert surface puts one on the wire, and + * loading it would hold the write transaction open for a discarded result. + */ + row: Omit operation: 'insert' | 'update' previousData?: RowData } diff --git a/apps/sim/tools/table/delete_row.ts b/apps/sim/tools/table/delete_row.ts index 47d46d699aa..fa339c07c49 100644 --- a/apps/sim/tools/table/delete_row.ts +++ b/apps/sim/tools/table/delete_row.ts @@ -23,6 +23,7 @@ export const tableDeleteRowTool: ToolConfig `/api/table/${params.tableId}/rows/${params.rowId}`, method: 'DELETE', headers: () => ({ diff --git a/apps/sim/tools/table/get_row.ts b/apps/sim/tools/table/get_row.ts index 7b76e605fda..010dcc53af5 100644 --- a/apps/sim/tools/table/get_row.ts +++ b/apps/sim/tools/table/get_row.ts @@ -23,6 +23,7 @@ export const tableGetRowTool: ToolConfig = }, request: { + internalAuth: 'executor_delegation', secretProvenance: { response: { incomplete: 'propagate' } }, url: (params: TableRowGetParams) => { const workspaceId = params._context?.workspaceId diff --git a/apps/sim/tools/table/update_row.ts b/apps/sim/tools/table/update_row.ts index c9792f95680..24f781cf042 100644 --- a/apps/sim/tools/table/update_row.ts +++ b/apps/sim/tools/table/update_row.ts @@ -38,6 +38,7 @@ export const tableUpdateRowTool: ToolConfig selectTableRowSecretProvenance([params.data]), response: { incomplete: 'propagate' }, diff --git a/apps/sim/tools/table/upsert_row.ts b/apps/sim/tools/table/upsert_row.ts index 70afc179872..7a62a79ef82 100644 --- a/apps/sim/tools/table/upsert_row.ts +++ b/apps/sim/tools/table/upsert_row.ts @@ -39,6 +39,7 @@ export const tableUpsertRowTool: ToolConfig selectTableRowSecretProvenance([params.data]), response: { incomplete: 'propagate' }, diff --git a/packages/testing/src/factories/table.factory.ts b/packages/testing/src/factories/table.factory.ts index 70e418618d5..65ccf7dbfde 100644 --- a/packages/testing/src/factories/table.factory.ts +++ b/packages/testing/src/factories/table.factory.ts @@ -120,12 +120,12 @@ export interface TableDefinitionFactoryOptions { updatedAt?: Date | string } -const UNLOCKED_TABLE_LOCKS: TableLocksFixture = { +const UNLOCKED_TABLE_LOCKS: TableLocksFixture = Object.freeze({ schemaLocked: false, insertLocked: false, updateLocked: false, deleteLocked: false, -} +}) /** * Creates a table definition fixture with sensible defaults — the shape route