Skip to content

Commit cdd5045

Browse files
committed
fix(folders): map reorder internal errors to 500 instead of 400
performReorderFolders caught every transaction failure -- including genuine unexpected DB/transaction errors, not just client-caused validation issues -- and returned success:false with no way to distinguish them. The route then unconditionally mapped any failure to 400, mislabeling real server errors as bad requests. Adds errorCode: OrchestrationErrorCode to the result (not_found for concurrent-deletion races and missing parents, internal for anything else, following the same pattern already used elsewhere in this file), and the route now maps status via the shared statusForOrchestrationError helper. Unexpected errors are logged with their real cause but returned to the client as a generic message instead of leaking internals.
1 parent 374f2ba commit cdd5045

3 files changed

Lines changed: 67 additions & 10 deletions

File tree

apps/sim/app/api/folders/reorder/route.test.ts

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,11 +262,40 @@ describe('PUT /api/folders/reorder', () => {
262262

263263
const response = await PUT(req)
264264

265-
expect(response.status).toBe(400)
265+
expect(response.status).toBe(404)
266266
const data = await response.json()
267267
expect(data.error).toBe('One or more folders were not found')
268268
})
269269

270+
it('returns a 500 when the transaction fails for an unexpected reason', async () => {
271+
// Regression test: performReorderFolders previously mapped every non-lock
272+
// transaction failure -- including genuine unexpected DB errors, not just
273+
// client-caused validation issues -- to a 400 in the route. An internal
274+
// failure should surface as a 500 with a generic message, not leak the raw
275+
// error or masquerade as a client error.
276+
mockWhere
277+
.mockReturnValueOnce([
278+
{ id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
279+
])
280+
.mockReturnValueOnce([{ id: 'folder-1', parentId: null }])
281+
.mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
282+
283+
mockDb.transaction.mockImplementation(async () => {
284+
throw new Error('connection terminated unexpectedly')
285+
})
286+
287+
const req = createMockRequest('PUT', {
288+
workspaceId: 'workspace-123',
289+
updates: [{ id: 'folder-1', sortOrder: 2, parentId: null }],
290+
})
291+
292+
const response = await PUT(req)
293+
294+
expect(response.status).toBe(500)
295+
const data = await response.json()
296+
expect(data.error).toBe('Failed to reorder folders')
297+
})
298+
270299
it('returns a 423 when a folder is concurrently locked before the write', async () => {
271300
// Regression test: the route checks lock state before calling
272301
// performReorderFolders (both of the route's own checks -- for update.id and

apps/sim/app/api/folders/reorder/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { generateRequestId } from '@/lib/core/utils/request'
1111
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1212
import { performReorderFolders } from '@/lib/folders/orchestration'
1313
import { FOLDER_RESOURCE_POLICIES } from '@/lib/folders/policy'
14+
import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types'
1415
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1516

1617
const logger = createLogger('FolderReorderAPI')
@@ -139,7 +140,7 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
139140
if (!result.success) {
140141
return NextResponse.json(
141142
{ error: result.error ?? 'No valid folders to update' },
142-
{ status: 400 }
143+
{ status: statusForOrchestrationError(result.errorCode) }
143144
)
144145
}
145146

apps/sim/lib/folders/orchestration.ts

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -777,9 +777,15 @@ export async function performRestoreFolder(
777777
return performRestoreResourceFolder(params, TABLE_FOLDER_CASCADE)
778778
}
779779

780-
export async function performReorderFolders(
781-
params: PerformReorderFoldersParams
782-
): Promise<{ success: boolean; updated: number; error?: string }> {
780+
/** Marks a concurrent-deletion race caught inside the reorder transaction as a 404, not a 500. */
781+
class FolderReorderNotFoundError extends Error {}
782+
783+
export async function performReorderFolders(params: PerformReorderFoldersParams): Promise<{
784+
success: boolean
785+
updated: number
786+
error?: string
787+
errorCode?: OrchestrationErrorCode
788+
}> {
783789
const { resourceType, workspaceId, updates } = params
784790

785791
const folderIds = updates.map((u) => u.id)
@@ -803,7 +809,12 @@ export async function performReorderFolders(
803809
// reporting success with a smaller `updated` count.
804810
const invalidId = updates.find((u) => !validIds.has(u.id))
805811
if (invalidId) {
806-
return { success: false, updated: 0, error: 'One or more folders were not found' }
812+
return {
813+
success: false,
814+
updated: 0,
815+
error: 'One or more folders were not found',
816+
errorCode: 'not_found',
817+
}
807818
}
808819
const validUpdates = updates
809820

@@ -822,7 +833,12 @@ export async function performReorderFolders(
822833
)
823834
const firstParentError = parentErrors.find((error) => error !== null)
824835
if (firstParentError) {
825-
return { success: false, updated: 0, error: firstParentError.error }
836+
return {
837+
success: false,
838+
updated: 0,
839+
error: firstParentError.error,
840+
errorCode: firstParentError.errorCode,
841+
}
826842
}
827843

828844
try {
@@ -837,7 +853,7 @@ export async function performReorderFolders(
837853
.from(folderTable)
838854
.where(and(inArray(folderTable.id, targetParentIds), isNull(folderTable.deletedAt)))
839855
if (activeParents.length !== targetParentIds.length) {
840-
throw new Error('Parent folder not found')
856+
throw new FolderReorderNotFoundError('Parent folder not found')
841857
}
842858
}
843859

@@ -871,7 +887,7 @@ export async function performReorderFolders(
871887
.where(and(eq(folderTable.id, update.id), isNull(folderTable.deletedAt)))
872888
.returning({ id: folderTable.id })
873889
if (!updated) {
874-
throw new Error('One or more folders were not found')
890+
throw new FolderReorderNotFoundError('One or more folders were not found')
875891
}
876892
}
877893
})
@@ -880,7 +896,18 @@ export async function performReorderFolders(
880896
if (error instanceof ResourceLockedError) {
881897
throw error
882898
}
883-
return { success: false, updated: 0, error: toError(error).message }
899+
if (error instanceof FolderReorderNotFoundError) {
900+
return { success: false, updated: 0, error: error.message, errorCode: 'not_found' }
901+
}
902+
// An unexpected DB/transaction failure, not a client-caused validation error --
903+
// log the real cause but don't leak internal error details to the response.
904+
logger.error('Unexpected error reordering folders', { error })
905+
return {
906+
success: false,
907+
updated: 0,
908+
error: 'Failed to reorder folders',
909+
errorCode: 'internal',
910+
}
884911
}
885912

886913
return { success: true, updated: validUpdates.length }

0 commit comments

Comments
 (0)