Skip to content

Commit 262ce32

Browse files
authored
Merge pull request #6569 from simstudioai/fix/v1-response-parity
fix(api): stop leaking failed SQL in v1 table 500s and restore the 423 lock field
2 parents 3cab8ef + 5519d45 commit 262ce32

15 files changed

Lines changed: 434 additions & 77 deletions

File tree

apps/sim/app/api/table/[tableId]/columns/route.test.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*/
1111
import { hybridAuthMockFns } from '@sim/testing'
1212
import { getErrorMessage } from '@sim/utils/errors'
13-
import { NextRequest } from 'next/server'
13+
import { NextRequest, NextResponse } from 'next/server'
1414
import { beforeEach, describe, expect, it, vi } from 'vitest'
1515

1616
const {
@@ -53,11 +53,24 @@ vi.mock('@/app/api/table/utils', () => ({
5353
accessError: () => new Response('denied', { status: 403 }),
5454
checkAccess: mockCheckAccess,
5555
normalizeColumn: (c: unknown) => c,
56+
orchestrationOutcomeErrorResponse: (
57+
outcome: { error?: string; errorCode?: OrchestrationErrorCode },
58+
fallback: string
59+
) =>
60+
NextResponse.json(
61+
{ error: messageForOrchestrationError(outcome, fallback) },
62+
{ status: statusForOrchestrationError(outcome.errorCode) }
63+
),
5664
rootErrorMessage: (e: unknown) => getErrorMessage(e),
5765
tableLockErrorResponse: () => null,
5866
}))
5967

60-
import { OrchestrationError } from '@/lib/core/orchestration/types'
68+
import {
69+
messageForOrchestrationError,
70+
OrchestrationError,
71+
type OrchestrationErrorCode,
72+
statusForOrchestrationError,
73+
} from '@/lib/core/orchestration/types'
6174
import { PATCH } from '@/app/api/table/[tableId]/columns/route'
6275

6376
const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'

apps/sim/app/api/table/[tableId]/columns/route.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import {
88
import { parseRequest } from '@/lib/api/server'
99
import { isZodError, validationErrorResponse } from '@/lib/api/server/validation'
1010
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
11-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1211
import { generateRequestId } from '@/lib/core/utils/request'
1312
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1413
import { addTableColumn, deleteColumn } from '@/lib/table'
@@ -18,6 +17,7 @@ import {
1817
accessError,
1918
checkAccess,
2019
normalizeColumn,
20+
orchestrationOutcomeErrorResponse,
2121
rootErrorMessage,
2222
tableLockErrorResponse,
2323
} from '@/app/api/table/utils'
@@ -122,10 +122,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
122122
request,
123123
})
124124
if (!outcome.success || !outcome.table) {
125-
return NextResponse.json(
126-
{ error: outcome.error ?? 'Failed to update column' },
127-
{ status: statusForOrchestrationError(outcome.errorCode) }
128-
)
125+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to update column')
129126
}
130127

131128
// Live-collab: tell open viewers the change landed so they refetch.

apps/sim/app/api/table/[tableId]/route.ts

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { getTableQuerySchema, updateTableContract } from '@/lib/api/contracts/ta
55
import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation'
66
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
77
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
8-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
98
import { generateRequestId } from '@/lib/core/utils/request'
109
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1110
import { findActiveFolder } from '@/lib/folders/queries'
@@ -24,6 +23,7 @@ import {
2423
accessError,
2524
checkAccess,
2625
normalizeColumn,
26+
orchestrationOutcomeErrorResponse,
2727
tableLockErrorResponse,
2828
} from '@/app/api/table/utils'
2929

@@ -187,10 +187,7 @@ export const PATCH = withRouteHandler(
187187
request,
188188
})
189189
if (!lockOutcome.success) {
190-
return NextResponse.json(
191-
{ error: lockOutcome.error ?? 'Failed to update table locks' },
192-
{ status: statusForOrchestrationError(lockOutcome.errorCode) }
193-
)
190+
return orchestrationOutcomeErrorResponse(lockOutcome, 'Failed to update table locks')
194191
}
195192
}
196193

@@ -203,10 +200,7 @@ export const PATCH = withRouteHandler(
203200
request,
204201
})
205202
if (!renameOutcome.success) {
206-
return NextResponse.json(
207-
{ error: renameOutcome.error ?? 'Failed to rename table' },
208-
{ status: statusForOrchestrationError(renameOutcome.errorCode) }
209-
)
203+
return orchestrationOutcomeErrorResponse(renameOutcome, 'Failed to rename table')
210204
}
211205
}
212206

@@ -229,11 +223,11 @@ export const PATCH = withRouteHandler(
229223
request,
230224
})
231225
if (!moveOutcome.success) {
232-
return NextResponse.json(
233-
{
234-
error: moveOutcome.errorCode === 'not_found' ? 'Table not found' : moveOutcome.error,
235-
},
236-
{ status: statusForOrchestrationError(moveOutcome.errorCode) }
226+
return orchestrationOutcomeErrorResponse(
227+
moveOutcome.errorCode === 'not_found'
228+
? { ...moveOutcome, error: 'Table not found' }
229+
: moveOutcome,
230+
'Failed to move table'
237231
)
238232
}
239233
}
@@ -302,10 +296,7 @@ export const DELETE = withRouteHandler(
302296
request,
303297
})
304298
if (!outcome.success) {
305-
return NextResponse.json(
306-
{ error: outcome.error ?? 'Failed to delete table' },
307-
{ status: statusForOrchestrationError(outcome.errorCode) }
308-
)
299+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to delete table')
309300
}
310301

311302
return NextResponse.json({

apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
} from '@/lib/api/contracts/tables'
1111
import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation'
1212
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
13-
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1413
import { generateRequestId } from '@/lib/core/utils/request'
1514
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1615
import type { RowData, TableSchema } from '@/lib/table'
@@ -27,7 +26,7 @@ import {
2726
accessError,
2827
checkAccess,
2928
orchestrationErrorResponse,
30-
rowWriteErrorResponse,
29+
orchestrationOutcomeErrorResponse,
3130
tableLockErrorResponse,
3231
} from '@/app/api/table/utils'
3332

@@ -211,7 +210,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: RowR
211210
rows: [updatedRow],
212211
})
213212
} catch (error) {
214-
const response = rowWriteErrorResponse(error)
213+
const response = orchestrationErrorResponse(error)
215214
if (response) return response
216215

217216
logger.error(`[${requestId}] Error updating row:`, error)
@@ -248,10 +247,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row
248247

249248
const outcome = await performDeleteTableRow({ table, rowId, requestId })
250249
if (!outcome.success) {
251-
return NextResponse.json(
252-
{ error: outcome.error ?? 'Failed to delete row' },
253-
{ status: statusForOrchestrationError(outcome.errorCode) }
254-
)
250+
return orchestrationOutcomeErrorResponse(outcome, 'Failed to delete row')
255251
}
256252

257253
// Live-collab: tell open viewers the change landed so they refetch.

apps/sim/app/api/table/[tableId]/rows/route.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ import {
4040
resolveTableWriteSecretProvenance,
4141
} from '@/app/api/table/row-secret-provenance'
4242
import { type RowWireTranslators, rowWireTranslators } from '@/app/api/table/row-wire'
43-
import { accessError, checkAccess, rowWriteErrorResponse } from '@/app/api/table/utils'
43+
import { accessError, checkAccess, orchestrationErrorResponse } from '@/app/api/table/utils'
4444

4545
const logger = createLogger('TableRowsAPI')
4646

@@ -168,7 +168,7 @@ async function handleBatchInsert(
168168
rows: insertedRows,
169169
})
170170
} catch (error) {
171-
const response = rowWriteErrorResponse(error)
171+
const response = orchestrationErrorResponse(error)
172172
if (response) return response
173173

174174
logger.error(`[${requestId}] Error batch inserting rows:`, error)
@@ -284,7 +284,7 @@ export const POST = withRouteHandler(
284284
return validationErrorResponse(error)
285285
}
286286

287-
const response = rowWriteErrorResponse(error)
287+
const response = orchestrationErrorResponse(error)
288288
if (response) return response
289289

290290
logger.error(`[${requestId}] Error inserting row:`, error)
@@ -527,7 +527,7 @@ export const PUT = withRouteHandler(
527527
return NextResponse.json({ error: error.message }, { status: 400 })
528528
}
529529

530-
const response = rowWriteErrorResponse(error)
530+
const response = orchestrationErrorResponse(error)
531531
if (response) return response
532532

533533
logger.error(`[${requestId}] Error updating rows by filter:`, error)
@@ -627,7 +627,7 @@ export const DELETE = withRouteHandler(
627627
return NextResponse.json({ error: error.message }, { status: 400 })
628628
}
629629

630-
const response = rowWriteErrorResponse(error)
630+
const response = orchestrationErrorResponse(error)
631631
if (response) return response
632632

633633
logger.error(`[${requestId}] Error deleting rows:`, error)
@@ -716,7 +716,7 @@ export const PATCH = withRouteHandler(
716716
return validationErrorResponse(error)
717717
}
718718

719-
const response = rowWriteErrorResponse(error)
719+
const response = orchestrationErrorResponse(error)
720720
if (response) return response
721721

722722
logger.error(`[${requestId}] Error batch updating rows:`, error)

apps/sim/app/api/table/[tableId]/rows/upsert/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
resolveTableWriteSecretProvenance,
1616
} from '@/app/api/table/row-secret-provenance'
1717
import { rowWireTranslators } from '@/app/api/table/row-wire'
18-
import { accessError, checkAccess, rowWriteErrorResponse } from '@/app/api/table/utils'
18+
import { accessError, checkAccess, orchestrationErrorResponse } from '@/app/api/table/utils'
1919

2020
const logger = createLogger('TableUpsertAPI')
2121

@@ -105,7 +105,7 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Upser
105105
return validationErrorResponse(error)
106106
}
107107

108-
const response = rowWriteErrorResponse(error)
108+
const response = orchestrationErrorResponse(error)
109109
if (response) return response
110110

111111
logger.error(`[${requestId}] Error upserting row:`, error)

apps/sim/app/api/table/utils.test.ts

Lines changed: 74 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@ import { describe, expect, it } from 'vitest'
55
import { OrchestrationError } from '@/lib/core/orchestration/types'
66
import { TableRowLimitError } from '@/lib/table/billing'
77
import type { ColumnDefinition } from '@/lib/table/types'
8-
import { rootErrorMessage, rowWriteErrorResponse, tableFilterError } from '@/app/api/table/utils'
8+
import {
9+
orchestrationErrorResponse,
10+
orchestrationOutcomeErrorResponse,
11+
rootErrorMessage,
12+
tableFilterError,
13+
} from '@/app/api/table/utils'
914

1015
/** Mimics drizzle's DrizzleQueryError: message is the failed SQL, real error on `cause`. */
1116
function wrapLikeDrizzle(cause: Error): Error {
@@ -29,9 +34,9 @@ describe('rootErrorMessage', () => {
2934
})
3035
})
3136

32-
describe('rowWriteErrorResponse', () => {
37+
describe('orchestrationErrorResponse', () => {
3338
it('passes the plan row-limit error through as a 400', async () => {
34-
const response = rowWriteErrorResponse(new TableRowLimitError(10000))
39+
const response = orchestrationErrorResponse(new TableRowLimitError(10000))
3540
expect(response?.status).toBe(400)
3641
const body = await response?.json()
3742
expect(body.error).toBe(
@@ -40,7 +45,7 @@ describe('rowWriteErrorResponse', () => {
4045
})
4146

4247
it('passes a classified validation failure through as 400', async () => {
43-
const response = rowWriteErrorResponse(
48+
const response = orchestrationErrorResponse(
4449
new OrchestrationError('validation', 'Value for column "email" must be unique')
4550
)
4651
expect(response?.status).toBe(400)
@@ -50,24 +55,26 @@ describe('rowWriteErrorResponse', () => {
5055

5156
it('answers the code the failure carries, not one derived from its wording', () => {
5257
expect(
53-
rowWriteErrorResponse(new OrchestrationError('not_found', 'Row not found'))?.status
58+
orchestrationErrorResponse(new OrchestrationError('not_found', 'Row not found'))?.status
5459
).toBe(404)
5560
// The phrase that used to force a 400 no longer decides anything.
5661
expect(
57-
rowWriteErrorResponse(new OrchestrationError('conflict', 'Row 3: must be unique'))?.status
62+
orchestrationErrorResponse(new OrchestrationError('conflict', 'Row 3: must be unique'))
63+
?.status
5864
).toBe(409)
5965
})
6066

6167
it('unwraps a classified failure drizzle wrapped in a query error', () => {
6268
expect(
63-
rowWriteErrorResponse(wrapLikeDrizzle(new OrchestrationError('validation', 'Row 3: bad')))
64-
?.status
69+
orchestrationErrorResponse(
70+
wrapLikeDrizzle(new OrchestrationError('validation', 'Row 3: bad'))
71+
)?.status
6572
).toBe(400)
6673
})
6774

6875
it('returns null for unknown errors so callers keep their generic 500', () => {
69-
expect(rowWriteErrorResponse(new Error('connection refused'))).toBeNull()
70-
expect(rowWriteErrorResponse(wrapLikeDrizzle(new Error('deadlock detected')))).toBeNull()
76+
expect(orchestrationErrorResponse(new Error('connection refused'))).toBeNull()
77+
expect(orchestrationErrorResponse(wrapLikeDrizzle(new Error('deadlock detected')))).toBeNull()
7178
})
7279
})
7380

@@ -117,3 +124,60 @@ describe('tableFilterError', () => {
117124
expect(tableFilterError({ col_status: { $regex: 'x' } } as never, columns)?.status).toBe(400)
118125
})
119126
})
127+
128+
describe('orchestrationOutcomeErrorResponse', () => {
129+
/**
130+
* Shaped like a driver fault surfacing verbatim — a statement plus its bound
131+
* parameters — so the assertion proves none of it reaches the response body.
132+
*/
133+
const leakyMessage =
134+
'Failed query: delete from "user_table" where "user_table"."id" = $1 params: tbl-1'
135+
136+
it('replaces an unclassified failure message with the fallback', async () => {
137+
const response = orchestrationOutcomeErrorResponse(
138+
{ success: false, error: leakyMessage, errorCode: 'internal' },
139+
'Failed to delete table'
140+
)
141+
142+
expect(response.status).toBe(500)
143+
const body = await response.json()
144+
expect(body).toEqual({ error: 'Failed to delete table' })
145+
expect(JSON.stringify(body)).not.toContain('Failed query')
146+
expect(JSON.stringify(body)).not.toContain('params:')
147+
})
148+
149+
it('replaces an unclassified failure with no error code too', async () => {
150+
const response = orchestrationOutcomeErrorResponse(
151+
{ error: leakyMessage },
152+
'Failed to delete table'
153+
)
154+
155+
expect(response.status).toBe(500)
156+
expect(await response.json()).toEqual({ error: 'Failed to delete table' })
157+
})
158+
159+
it('keeps the message of a classified failure', async () => {
160+
const response = orchestrationOutcomeErrorResponse(
161+
{ error: 'A table named "Orders" already exists in this workspace', errorCode: 'conflict' },
162+
'Failed to rename table'
163+
)
164+
165+
expect(response.status).toBe(409)
166+
expect(await response.json()).toEqual({
167+
error: 'A table named "Orders" already exists in this workspace',
168+
})
169+
})
170+
171+
it('carries the rejecting lock kind on a 423', async () => {
172+
const response = orchestrationOutcomeErrorResponse(
173+
{ error: 'Table is locked against deletion', errorCode: 'locked', lock: 'delete' },
174+
'Failed to delete table'
175+
)
176+
177+
expect(response.status).toBe(423)
178+
expect(await response.json()).toEqual({
179+
error: 'Table is locked against deletion',
180+
lock: 'delete',
181+
})
182+
})
183+
})

0 commit comments

Comments
 (0)