Skip to content

Commit 3577b08

Browse files
fix(api): make resource cleanup and metadata consistent
1 parent ab11711 commit 3577b08

10 files changed

Lines changed: 266 additions & 72 deletions

File tree

apps/docs/openapi-v2-tables.json

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@
270270
"delete": {
271271
"operationId": "deleteTable",
272272
"summary": "Delete Table",
273-
"description": "Delete a table. Returns the id of the deleted table.",
273+
"description": "Delete a table. Returns the id and an explicit deletion confirmation.",
274274
"tags": ["Tables"],
275275
"x-codeSamples": [
276276
{
@@ -333,7 +333,7 @@
333333
"patch": {
334334
"operationId": "updateTable",
335335
"summary": "Update Table",
336-
"description": "Rename a table and/or move it between folders. Provide at least one of `name` or `folderPath`. Each field is applied independently, so one request can rename and move at once, and the response reflects every applied change.\n\nBoth fields need workspace write.\n\n**Lock flags are read-only here.** A table's `locks` are returned on the table resource and enforced on every write (a locked verb returns 423), but they cannot be changed through the API — a write-level key must not be able to clear the guard placed there to stop it. Changing a lock is a first-party workspace-admin action. A request carrying `locks` is rejected with 400 rather than silently ignored.\n\n**Partial-success semantics.** The two operations commit independently, so this endpoint is not atomic. Everything that can be *rejected* — the body shape, folder existence — is validated before the first write, so a rejected request changes nothing. If a genuine fault (a lost race, the table deleted mid-request, a database error) fails the later operation after the earlier one has committed, the response is an error whose `error.details.applied` lists the operations that are nevertheless live (`\"name\"`, `\"folderPath\"`). The field is absent when nothing was applied, so its presence always means \"these changes took effect despite the error\" — re-read the table to confirm before retrying.",
336+
"description": "Rename a table, edit its description, and/or move it between folders. Provide at least one of `name`, `description`, or `folderPath`. Each field is applied independently, so one request can combine changes and the response reflects every applied change.\n\nAll three fields need workspace write.\n\n**Lock flags are read-only here.** A table's `locks` are returned on the table resource and enforced on every write (a locked verb returns 423), but they cannot be changed through the API — a write-level key must not be able to clear the guard placed there to stop it. Changing a lock is a first-party workspace-admin action. A request carrying `locks` is rejected with 400 rather than silently ignored.\n\n**Partial-success semantics.** The operations commit independently, so this endpoint is not atomic. Everything that can be *rejected* — the body shape, folder existence — is validated before the first write, so a rejected request changes nothing. If a genuine fault (a lost race, the table deleted mid-request, a database error) fails a later operation after an earlier one has committed, the response is an error whose `error.details.applied` lists the operations that are nevertheless live (`\"name\"`, `\"description\"`, `\"folderPath\"`). The field is absent when nothing was applied, so its presence always means \"these changes took effect despite the error\" — re-read the table to confirm before retrying.",
337337
"tags": ["Tables"],
338338
"x-codeSamples": [
339339
{
@@ -4976,11 +4976,16 @@
49764976
"properties": {
49774977
"data": {
49784978
"type": "object",
4979-
"required": ["id"],
4979+
"required": ["id", "deleted"],
49804980
"properties": {
49814981
"id": {
49824982
"type": "string",
49834983
"description": "The id of the deleted table."
4984+
},
4985+
"deleted": {
4986+
"type": "boolean",
4987+
"const": true,
4988+
"description": "Confirms the table was deleted."
49844989
}
49854990
}
49864991
}
@@ -5301,7 +5306,7 @@
53015306
},
53025307
"UpdateTableBody": {
53035308
"type": "object",
5304-
"description": "Rename and/or move a table. Every field beyond `workspaceId` is optional, but at least one must be present. Lock flags are read-only on this API and are not accepted here.",
5309+
"description": "Rename, edit the description, and/or move a table. Every field beyond `workspaceId` is optional, but at least one must be present. Lock flags are read-only on this API and are not accepted here.",
53055310
"required": ["workspaceId"],
53065311
"properties": {
53075312
"workspaceId": {
@@ -5314,6 +5319,11 @@
53145319
"minLength": 1,
53155320
"description": "New table name."
53165321
},
5322+
"description": {
5323+
"type": ["string", "null"],
5324+
"maxLength": 500,
5325+
"description": "New table description, or null to clear it."
5326+
},
53175327
"folderPath": {
53185328
"type": "string",
53195329
"description": "Canonical containing-folder path. `/` is the workspace root."

apps/sim/app/api/v2/tables/[tableId]/route.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ const {
1515
mockCheckAccess,
1616
mockPerformDeleteTable,
1717
mockPerformRenameTable,
18+
mockPerformUpdateTableDescription,
1819
mockPerformMoveTableToFolder,
1920
mockPerformUpdateTableLocks,
2021
mockRecordAudit,
@@ -28,6 +29,7 @@ const {
2829
mockCheckAccess: vi.fn(),
2930
mockPerformDeleteTable: vi.fn(),
3031
mockPerformRenameTable: vi.fn(),
32+
mockPerformUpdateTableDescription: vi.fn(),
3133
mockPerformMoveTableToFolder: vi.fn(),
3234
mockPerformUpdateTableLocks: vi.fn(),
3335
mockRecordAudit: vi.fn(),
@@ -76,6 +78,7 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
7678
vi.mock('@/lib/table/orchestration', () => ({
7779
performDeleteTable: mockPerformDeleteTable,
7880
performRenameTable: mockPerformRenameTable,
81+
performUpdateTableDescription: mockPerformUpdateTableDescription,
7982
performMoveTableToFolder: mockPerformMoveTableToFolder,
8083
performUpdateTableLocks: mockPerformUpdateTableLocks,
8184
}))
@@ -158,6 +161,7 @@ describe('DELETE /api/v2/tables/[tableId]', () => {
158161
expect(mockPerformDeleteTable).toHaveBeenCalledWith(
159162
expect.objectContaining({ table: TABLE, userId: 'user-1' })
160163
)
164+
expect((await res.json()).data).toEqual({ id: 'table-1', deleted: true })
161165
// The route no longer audits: doing so out here fired TABLE_DELETED even
162166
// when the delete was a no-op on an already-archived table.
163167
expect(mockRecordAudit).not.toHaveBeenCalled()
@@ -206,6 +210,27 @@ describe('PATCH /api/v2/tables/[tableId]', () => {
206210
expect(mockPerformUpdateTableLocks).not.toHaveBeenCalled()
207211
})
208212

213+
it('updates and clears the table description through orchestration', async () => {
214+
mockPerformUpdateTableDescription.mockResolvedValue({ success: true })
215+
216+
const updateResponse = await callPatch({ workspaceId: 'ws-1', description: 'Finance data' })
217+
218+
expect(updateResponse.status).toBe(200)
219+
expect(mockPerformUpdateTableDescription).toHaveBeenCalledWith(
220+
expect.objectContaining({
221+
table: TABLE,
222+
description: 'Finance data',
223+
userId: 'user-1',
224+
})
225+
)
226+
227+
const clearResponse = await callPatch({ workspaceId: 'ws-1', description: null })
228+
expect(clearResponse.status).toBe(200)
229+
expect(mockPerformUpdateTableDescription).toHaveBeenLastCalledWith(
230+
expect.objectContaining({ description: null })
231+
)
232+
})
233+
209234
it('surfaces a running import so an async job is observable, not just startable', async () => {
210235
// `POST /import-async` and `POST /job/cancel` let a caller start and stop an
211236
// import; without this the table never reports that it is running, so there

apps/sim/app/api/v2/tables/[tableId]/route.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
performDeleteTable,
1818
performMoveTableToFolder,
1919
performRenameTable,
20+
performUpdateTableDescription,
2021
} from '@/lib/table/orchestration'
2122
import { checkAccess } from '@/app/api/table/utils'
2223
import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware'
@@ -45,7 +46,7 @@ const logger = createLogger('V2TableDetailAPI')
4546
* means "these changes are live despite the error".
4647
*/
4748
function appliedDetails(
48-
applied: readonly ('name' | 'folderPath')[]
49+
applied: readonly ('name' | 'description' | 'folderPath')[]
4950
): { applied: readonly string[] } | undefined {
5051
return applied.length > 0 ? { applied } : undefined
5152
}
@@ -124,7 +125,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Tabl
124125
* archived. Reporting a bare 500 there tells the caller nothing landed, and
125126
* it retries into a duplicate-name conflict or a repeated move.
126127
*/
127-
const applied: ('name' | 'folderPath')[] = []
128+
const applied: ('name' | 'description' | 'folderPath')[] = []
128129

129130
try {
130131
const rateLimit = await checkRateLimit(request, 'table-detail')
@@ -180,6 +181,18 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Tabl
180181
else failure = { outcome, fallback: 'Failed to rename table' }
181182
}
182183

184+
if (!failure && validated.description !== undefined) {
185+
const outcome = await performUpdateTableDescription({
186+
table,
187+
description: validated.description,
188+
userId,
189+
requestId,
190+
request,
191+
})
192+
if (outcome.success) applied.push('description')
193+
else failure = { outcome, fallback: 'Failed to update table description' }
194+
}
195+
183196
if (!failure && validated.folderPath !== undefined) {
184197
const outcome = await performMoveTableToFolder({
185198
table,
@@ -272,7 +285,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
272285
return v2TableOrchestrationError(outcome, 'Failed to delete table')
273286
}
274287

275-
return v2Data({ id: tableId }, { rateLimit })
288+
return v2Data({ id: tableId, deleted: true }, { rateLimit })
276289
} catch (error) {
277290
const lockError = v2TableLockError(error)
278291
if (lockError) return lockError

apps/sim/lib/api/contracts/v2/tables.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ export type V2ApiRow = z.output<typeof v2ApiRowSchema>
150150
export const v2TableDataSchema = z.object({ table: v2ApiTableSchema })
151151
export type V2TableData = z.output<typeof v2TableDataSchema>
152152

153-
export const v2DeleteTableDataSchema = z.object({ id: z.string() })
153+
export const v2DeleteTableDataSchema = z.object({ id: z.string(), deleted: z.literal(true) })
154154
export type V2DeleteTableData = z.output<typeof v2DeleteTableDataSchema>
155155

156156
/** The table's full column list after a column mutation. */
@@ -279,8 +279,9 @@ export const v2GetTableContract = defineRouteContract({
279279

280280
/**
281281
* Table update. Every field is optional but at least one must be present:
282-
* `name` renames and `folderPath` moves the table. Omission leaves placement
283-
* untouched; `/` moves it to the workspace root.
282+
* `name` renames, `description` edits metadata, and `folderPath` moves the
283+
* table. Omission leaves placement untouched; `/` moves it to the workspace
284+
* root.
284285
*
285286
* `locks` is deliberately **not** accepted here, which is why this body is
286287
* declared rather than reusing the first-party `updateTableBodySchema`. The
@@ -294,14 +295,19 @@ export const v2UpdateTableBodySchema = z
294295
.object({
295296
workspaceId: workspaceIdSchema,
296297
name: tableNameSchema.optional(),
298+
description: v1CreateTableBodySchema.shape.description.nullable(),
297299
folderPath: v2FolderPathInputSchema.optional(),
298300
})
299301
.strict()
300302
.superRefine((body, ctx) => {
301-
if (body.name === undefined && body.folderPath === undefined) {
303+
if (
304+
body.name === undefined &&
305+
body.description === undefined &&
306+
body.folderPath === undefined
307+
) {
302308
ctx.addIssue({
303309
code: 'custom',
304-
message: 'Provide a new name or folder',
310+
message: 'Provide a new name, description, or folder',
305311
path: ['name'],
306312
})
307313
}

apps/sim/lib/folders/orchestration.test.ts

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,8 @@ describe('createFolder', () => {
172172
name: 'Reports',
173173
workspaceId: 'ws-1',
174174
parentId: null,
175+
createdAt: expect.any(Date),
176+
updatedAt: expect.any(Date),
175177
})
176178
)
177179
})
@@ -352,8 +354,49 @@ describe('path-owned folder mutations', () => {
352354

353355
expect(result).toMatchObject({ success: true, path: '/Reports/Q1' })
354356
expect(dbChainMockFns.values).toHaveBeenCalledWith(
355-
expect.objectContaining({ name: 'Q1', parentId: 'parent-1' })
357+
expect.objectContaining({
358+
name: 'Q1',
359+
parentId: 'parent-1',
360+
createdAt: expect.any(Date),
361+
updatedAt: expect.any(Date),
362+
})
363+
)
364+
})
365+
366+
it('releases the folder transaction before running the domain delete cascade', async () => {
367+
const source = folderRow({ id: 'folder-1', name: 'Reports' })
368+
mockLoadActiveFolderPathIndex.mockResolvedValue({
369+
rowById: new Map([['folder-1', source]]),
370+
pathById: new Map([['folder-1', '/Reports']]),
371+
idByPath: new Map([['/Reports', 'folder-1']]),
372+
})
373+
queueTableRows(schemaMock.folder, [{ deletedAt: null }])
374+
375+
let inFolderTransaction = false
376+
dbChainMockFns.transaction.mockImplementationOnce(
377+
async (operation: (tx: unknown) => Promise<unknown>) => {
378+
inFolderTransaction = true
379+
try {
380+
return await operation(dbChainMock.db)
381+
} finally {
382+
inFolderTransaction = false
383+
}
384+
}
356385
)
386+
mockArchiveFolderCascade.mockImplementationOnce(async () => {
387+
expect(inFolderTransaction).toBe(false)
388+
return { folders: 1, children: 0 }
389+
})
390+
391+
const result = await deleteFolderByPath({
392+
resourceType: 'table',
393+
workspaceId: 'ws-1',
394+
userId: 'user-1',
395+
path: '/Reports',
396+
recursive: true,
397+
})
398+
399+
expect(result).toMatchObject({ success: true, path: '/Reports' })
357400
})
358401

359402
it('rejects relocating a folder beneath its own descendant before writing', async () => {

0 commit comments

Comments
 (0)