-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: add pagination to GET /api/workflows to prevent memory exhaustion #3479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||||||||||||||||||||||||||
| import { db } from '@sim/db' | ||||||||||||||||||||||||||||
| import { permissions, workflow, workflowFolder } from '@sim/db/schema' | ||||||||||||||||||||||||||||
| import { createLogger } from '@sim/logger' | ||||||||||||||||||||||||||||
| import { and, asc, eq, inArray, isNull, min } from 'drizzle-orm' | ||||||||||||||||||||||||||||
| import { and, asc, count, eq, inArray, isNull, min } from 'drizzle-orm' | ||||||||||||||||||||||||||||
| import { type NextRequest, NextResponse } from 'next/server' | ||||||||||||||||||||||||||||
| import { z } from 'zod' | ||||||||||||||||||||||||||||
| import { AuditAction, AuditResourceType, recordAudit } from '@/lib/audit/log' | ||||||||||||||||||||||||||||
|
|
@@ -12,6 +12,9 @@ import { verifyWorkspaceMembership } from '@/app/api/workflows/utils' | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const logger = createLogger('WorkflowAPI') | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const DEFAULT_PAGE_LIMIT = 200 | ||||||||||||||||||||||||||||
| const MAX_PAGE_LIMIT = 500 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const CreateWorkflowSchema = z.object({ | ||||||||||||||||||||||||||||
| name: z.string().min(1, 'Name is required'), | ||||||||||||||||||||||||||||
| description: z.string().optional().default(''), | ||||||||||||||||||||||||||||
|
|
@@ -28,6 +31,14 @@ export async function GET(request: NextRequest) { | |||||||||||||||||||||||||||
| const url = new URL(request.url) | ||||||||||||||||||||||||||||
| const workspaceId = url.searchParams.get('workspaceId') | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const rawLimit = url.searchParams.get('limit') | ||||||||||||||||||||||||||||
| const rawOffset = url.searchParams.get('offset') | ||||||||||||||||||||||||||||
| const limit = Math.min( | ||||||||||||||||||||||||||||
| Math.max(1, rawLimit ? Number(rawLimit) : DEFAULT_PAGE_LIMIT), | ||||||||||||||||||||||||||||
| MAX_PAGE_LIMIT | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| const offset = Math.max(0, rawOffset ? Number(rawOffset) : 0) | ||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-numeric When a client passes Add validation to ensure both params are finite numbers before clamping:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NaN propagates to SQL when limit/offset are non-numericMedium Severity When There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicates existing
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const auth = await checkSessionOrInternalAuth(request, { requireWorkflowId: false }) | ||||||||||||||||||||||||||||
| if (!auth.success || !auth.userId) { | ||||||||||||||||||||||||||||
|
|
@@ -63,32 +74,68 @@ export async function GET(request: NextRequest) { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| let workflows | ||||||||||||||||||||||||||||
| let total: number | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const orderByClause = [asc(workflow.sortOrder), asc(workflow.createdAt), asc(workflow.id)] | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (workspaceId) { | ||||||||||||||||||||||||||||
| workflows = await db | ||||||||||||||||||||||||||||
| .select() | ||||||||||||||||||||||||||||
| .from(workflow) | ||||||||||||||||||||||||||||
| .where(eq(workflow.workspaceId, workspaceId)) | ||||||||||||||||||||||||||||
| .orderBy(...orderByClause) | ||||||||||||||||||||||||||||
| const whereCondition = eq(workflow.workspaceId, workspaceId) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const [countResult, workflowRows] = await Promise.all([ | ||||||||||||||||||||||||||||
| db.select({ count: count() }).from(workflow).where(whereCondition), | ||||||||||||||||||||||||||||
| db | ||||||||||||||||||||||||||||
| .select() | ||||||||||||||||||||||||||||
| .from(workflow) | ||||||||||||||||||||||||||||
| .where(whereCondition) | ||||||||||||||||||||||||||||
| .orderBy(...orderByClause) | ||||||||||||||||||||||||||||
| .limit(limit) | ||||||||||||||||||||||||||||
| .offset(offset), | ||||||||||||||||||||||||||||
| ]) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| total = countResult[0]?.count ?? 0 | ||||||||||||||||||||||||||||
| workflows = workflowRows | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| const workspacePermissionRows = await db | ||||||||||||||||||||||||||||
| .select({ workspaceId: permissions.entityId }) | ||||||||||||||||||||||||||||
| .from(permissions) | ||||||||||||||||||||||||||||
| .where(and(eq(permissions.userId, userId), eq(permissions.entityType, 'workspace'))) | ||||||||||||||||||||||||||||
| const workspaceIds = workspacePermissionRows.map((row) => row.workspaceId) | ||||||||||||||||||||||||||||
| if (workspaceIds.length === 0) { | ||||||||||||||||||||||||||||
| return NextResponse.json({ data: [] }, { status: 200 }) | ||||||||||||||||||||||||||||
| return NextResponse.json( | ||||||||||||||||||||||||||||
| { data: [], pagination: { total: 0, limit, offset, hasMore: false } }, | ||||||||||||||||||||||||||||
| { status: 200 } | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| workflows = await db | ||||||||||||||||||||||||||||
| .select() | ||||||||||||||||||||||||||||
| .from(workflow) | ||||||||||||||||||||||||||||
| .where(inArray(workflow.workspaceId, workspaceIds)) | ||||||||||||||||||||||||||||
| .orderBy(...orderByClause) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const whereCondition = inArray(workflow.workspaceId, workspaceIds) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const [countResult, workflowRows] = await Promise.all([ | ||||||||||||||||||||||||||||
| db.select({ count: count() }).from(workflow).where(whereCondition), | ||||||||||||||||||||||||||||
| db | ||||||||||||||||||||||||||||
| .select() | ||||||||||||||||||||||||||||
| .from(workflow) | ||||||||||||||||||||||||||||
| .where(whereCondition) | ||||||||||||||||||||||||||||
| .orderBy(...orderByClause) | ||||||||||||||||||||||||||||
| .limit(limit) | ||||||||||||||||||||||||||||
| .offset(offset), | ||||||||||||||||||||||||||||
| ]) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| total = countResult[0]?.count ?? 0 | ||||||||||||||||||||||||||||
| workflows = workflowRows | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return NextResponse.json({ data: workflows }, { status: 200 }) | ||||||||||||||||||||||||||||
| return NextResponse.json( | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| data: workflows, | ||||||||||||||||||||||||||||
| pagination: { | ||||||||||||||||||||||||||||
| total, | ||||||||||||||||||||||||||||
| limit, | ||||||||||||||||||||||||||||
| offset, | ||||||||||||||||||||||||||||
| hasMore: offset + workflows.length < total, | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| { status: 200 } | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| } catch (error: any) { | ||||||||||||||||||||||||||||
| const elapsed = Date.now() - startTime | ||||||||||||||||||||||||||||
| logger.error(`[${requestId}] Workflow fetch error after ${elapsed}ms`, error) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /** | ||
| * Fetches all pages from a paginated API endpoint. | ||
| * | ||
| * The endpoint is expected to return `{ data: T[], pagination: { hasMore: boolean } }`. | ||
| * Pages are fetched sequentially until `hasMore` is `false`. | ||
| * | ||
| * @param baseUrl - Base URL including any existing query params (e.g. `/api/workflows?workspaceId=ws-1`) | ||
| * @param pageSize - Number of items per page (default 200) | ||
| * @returns All items concatenated across pages | ||
| */ | ||
| const MAX_PAGES = 100 | ||
|
|
||
| export async function fetchAllPages<T>(baseUrl: string, pageSize = 200): Promise<T[]> { | ||
| const allItems: T[] = [] | ||
| let offset = 0 | ||
| let pages = 0 | ||
| const separator = baseUrl.includes('?') ? '&' : '?' | ||
|
|
||
| while (pages < MAX_PAGES) { | ||
| const response = await fetch(`${baseUrl}${separator}limit=${pageSize}&offset=${offset}`) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch from ${baseUrl}: ${response.statusText}`) | ||
| } | ||
|
|
||
| const json = await response.json() | ||
| const data: T[] = Array.isArray(json.data) ? json.data : [] | ||
| allItems.push(...data) | ||
|
|
||
| if (!json.pagination?.hasMore || data.length === 0) { | ||
| break | ||
| } | ||
|
|
||
| offset += pageSize | ||
| pages++ | ||
| } | ||
|
|
||
| return allItems | ||
| } |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Proxy-based mock doesn't capture arguments passed to
.limit()and.offset(). This means the tests confirm the response shape is correct but don't verify that the actual pagination parameters were forwarded to the database queries.For example, if the route silently ignored the
limitparam and always used the default, the test would still pass (if the mock data happens to be the right size). Consider capturing these arguments to add stronger assertions: