From 80270ce7b2cc47dac9363ac8cb23eec063ce6331 Mon Sep 17 00:00:00 2001 From: Lakee Sivaraya Date: Thu, 15 Jan 2026 12:49:08 -0800 Subject: [PATCH] fix comments --- CODE_REVIEW.md | 277 ------------------ apps/sim/app/api/table/[tableId]/route.ts | 32 +- .../api/table/[tableId]/rows/[rowId]/route.ts | 57 ++-- .../sim/app/api/table/[tableId]/rows/route.ts | 64 ++-- apps/sim/lib/table/service.ts | 15 +- apps/sim/lib/table/types.ts | 2 +- 6 files changed, 79 insertions(+), 368 deletions(-) delete mode 100644 CODE_REVIEW.md diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md deleted file mode 100644 index be8fdcf21..000000000 --- a/CODE_REVIEW.md +++ /dev/null @@ -1,277 +0,0 @@ -# Code Review: User Tables Feature - -**Branch:** `lakees/db` -**Reviewer:** Code Review Agent -**Date:** 2026-01-14 - ---- - -## Executive Summary - -This feature implements user-defined tables functionality, allowing users to store and query structured data within workflows. Overall, this is a **well-architected feature** that follows the codebase patterns and demonstrates good separation of concerns. There are some areas that could benefit from refinement. - ---- - -## Strengths - -### 1. Clean Architecture & Separation of Concerns - -The code is well-organized with clear boundaries: - -- **Service Layer** (`lib/table/service.ts`) - Business logic extracted from route handlers -- **Validation Layer** (`lib/table/validation/`) - Schema and row validation utilities -- **Query Builder** (`lib/table/query-builder.ts`) - SQL generation with injection protection -- **API Routes** - Thin handlers delegating to service layer -- **Tools** - Clean tool definitions following existing patterns - -### 2. Type Safety - -Excellent TypeScript usage throughout: -- Well-defined interfaces in `lib/table/types.ts` -- Proper use of `import type` for type-only imports -- Generic tool configurations with proper response types -- Discriminated unions for access result types (`TableAccessResult | TableAccessDenied`) - -### 3. Security Considerations - -Good security practices implemented: -- SQL injection prevention via `validateFieldName()` with `NAME_PATTERN` regex -- Operator whitelist in query builder -- Workspace ID verification to prevent spoofing -- Permission checks via `getUserEntityPermissions()` -- Soft delete implementation for tables - -### 4. Testing - -The `query-builder.test.ts` includes: -- SQL injection attack prevention tests -- Invalid field name rejection -- Operator validation -- Good coverage of filter operations - -### 5. Documentation - -TSDoc comments are thorough and follow project standards: -- Module-level documentation -- Function JSDoc with `@param`, `@returns`, `@example` -- Clear remarks on behavior specifics - ---- - -## Areas for Improvement - -### Critical Issues - -#### 1. Unique Constraint Check Performance - -**Location:** `lib/table/service.ts:246-260` and similar in `batchInsertRows` - -```typescript -// Current implementation fetches ALL rows to check unique constraints -const existingRows = await db - .select({ id: userTableRows.id, data: userTableRows.data }) - .from(userTableRows) - .where(eq(userTableRows.tableId, data.tableId)) -``` - -**Problem:** This fetches all rows in the table to check unique constraints, which will not scale. For a table with 10,000 rows (the limit), this loads all data into memory. - -**Recommendation:** Use a database-level unique constraint check: -1. Create a partial index on JSONB fields for unique columns, or -2. Use a targeted query that only checks for the specific unique values being inserted - -#### 2. Row Count Race Condition - -**Location:** Multiple route handlers and service functions - -The row count update uses SQL increment: -```typescript -rowCount: sql`${userTableDefinitions.rowCount} + 1` -``` - -While this is atomic within a single statement, concurrent inserts could still result in exceeding `maxRows` because the capacity check and insert are not atomic: - -```typescript -// Gap between check and insert allows race condition -if (table.rowCount >= table.maxRows) { ... } // Check -await db.transaction(async (trx) => { // Insert (later) -``` - -**Recommendation:** Either: -1. Use `SELECT FOR UPDATE` on the table definition row before checking capacity -2. Add a database constraint, or -3. Use optimistic locking with retry logic - -### Moderate Issues - -#### 3. Duplicated Access Control Logic - -**Location:** `app/api/table/utils.ts` and `app/api/table/route.ts` - -There are two implementations of workspace access checking: -- `checkWorkspaceAccess()` in `route.ts` -- `checkTableAccessInternal()` and related functions in `utils.ts` - -**Recommendation:** Consolidate into the `utils.ts` implementation and remove the duplication from `route.ts`. - -#### 4. Missing Error Handling in Tools - -**Location:** `tools/table/query-rows.ts:76-90` - -```typescript -transformResponse: async (response): Promise => { - const result = await response.json() - const data = result.data || result - // No check for error responses - return { - success: true, - // ... - } -} -``` - -**Problem:** If the API returns an error, this will still return `success: true` with potentially undefined fields. - -**Recommendation:** Check `response.ok` and handle error responses appropriately. - -#### 5. Inconsistent Validation Location - -**Location:** `app/api/table/route.ts` vs `lib/table/validation/schema.ts` - -The Zod schemas in route handlers duplicate validation logic that also exists in the service layer: - -```typescript -// In route.ts -const ColumnSchema = z.object({ - name: z.string().min(1).max(TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH)... -}) - -// In validation/schema.ts -export function validateColumnDefinition(column: ColumnDefinition): ValidationResult { - if (column.name.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) { - errors.push(`Column name "${column.name}" exceeds maximum length...`) - } -} -``` - -**Recommendation:** Either: -1. Use Zod schemas exclusively and remove the manual validation functions, or -2. Have the route handlers use minimal validation and delegate full validation to the service layer - -### Minor Issues - -#### 6. Magic Numbers - -**Location:** `app/workspace/[workspaceId]/tables/tables.tsx:36` - -```typescript -const debouncedSearchQuery = useDebounce(searchQuery, 300) -``` - -**Recommendation:** Extract to a constant, e.g., `SEARCH_DEBOUNCE_MS = 300` - -#### 7. Unused `createdBy` in Row Insert - -**Location:** `lib/table/service.ts:265-272` - -The service layer's `insertRow` function doesn't track `createdBy`, but the API route does. This inconsistency could cause issues if service functions are called directly. - -```typescript -const newRow = { - id: rowId, - tableId: data.tableId, - // Missing createdBy -} -``` - -**Recommendation:** Add `createdBy` to `InsertRowData` interface and service functions. - -#### 8. Inconsistent Batch Size Constants - -**Location:** `lib/table/constants.ts` - -```typescript -UPDATE_BATCH_SIZE: 100, -DELETE_BATCH_SIZE: 1000, -``` - -**Question:** Why different sizes? If intentional, add a comment explaining the rationale. - -#### 9. Missing Index for Common Query Pattern - -**Location:** `packages/db/schema.ts` - -The `userTableRows` table has a GIN index on `data`, but common queries filter by `tableId` AND specific `data` fields. Consider a compound index pattern if query performance becomes an issue. - -#### 10. Block Definition Type Safety - -**Location:** `blocks/blocks/table.ts:433` - -```typescript -const parseJSON = (value: string | any, fieldName: string): any => { -``` - -Using `any` twice in the same function signature. Consider: -```typescript -const parseJSON = (value: unknown, fieldName: string): unknown => { -``` - ---- - -## Code Style Observations - -### Following Project Standards - -- Uses `createLogger` from `@sim/logger` -- Absolute imports throughout -- Proper use of `cn()` utility for conditional classes -- Components follow the hook ordering convention - -### Minor Deviations - -1. Some inline styles in `tables.tsx` could be Tailwind classes -2. A few places use `String()` where template literals would be clearer - ---- - -## Suggested Test Coverage Additions - -1. **Integration tests** for the full API flow (create table -> insert rows -> query -> delete) -2. **Validation edge cases** in `schema.test.ts`: - - Maximum column count boundary - - Maximum row size boundary - - Unicode/special character handling in string values -3. **Concurrent operation tests** to verify race condition handling -4. **Performance benchmarks** for large tables (approaching 10k rows) - ---- - -## Summary - -| Category | Rating | -|----------|--------| -| Architecture | Excellent | -| Type Safety | Excellent | -| Security | Good | -| Performance | Needs Attention | -| Test Coverage | Good | -| Documentation | Excellent | -| Code Style | Good | - -**Overall Assessment:** Ready for merge with the unique constraint performance issue addressed. The other items can be handled as follow-up improvements. - ---- - -## Action Items - -### Must Fix Before Merge -- [ ] Address unique constraint check performance (Critical #1) - -### Should Fix Soon -- [ ] Fix row count race condition (Critical #2) -- [ ] Add error handling to tool transform responses (Moderate #4) - -### Nice to Have -- [ ] Consolidate access control logic (Moderate #3) -- [ ] Unify validation approach (Moderate #5) -- [ ] Minor code quality improvements (Minor #6-10) diff --git a/apps/sim/app/api/table/[tableId]/route.ts b/apps/sim/app/api/table/[tableId]/route.ts index d1556850b..f2f8eb68b 100644 --- a/apps/sim/app/api/table/[tableId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/route.ts @@ -16,8 +16,7 @@ const logger = createLogger('TableDetailAPI') /** * Zod schema for validating get table requests. * - * The workspaceId is optional for backward compatibility but - * is validated via table access checks when provided. + * The workspaceId is required and validated against the table. */ const GetTableSchema = z.object({ workspaceId: z.string().min(1, 'Workspace ID is required'), @@ -85,15 +84,12 @@ export async function GET(request: NextRequest, { params }: TableRouteParams) { return NextResponse.json({ error: 'Access denied' }, { status: 403 }) } - // Security check: If workspaceId is provided, verify it matches the table's workspace - if (validated.workspaceId) { - const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) - if (!isValidWorkspace) { - logger.warn( - `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` - ) - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } // Get table using service layer @@ -167,6 +163,11 @@ export async function DELETE(request: NextRequest, { params }: TableRouteParams) return NextResponse.json({ error: 'Authentication required' }, { status: 401 }) } + const { searchParams } = new URL(request.url) + const validated = GetTableSchema.parse({ + workspaceId: searchParams.get('workspaceId'), + }) + // Check table write access (similar to knowledge base write access control) const accessCheck = await checkTableWriteAccess(tableId, authResult.userId) @@ -181,6 +182,15 @@ export async function DELETE(request: NextRequest, { params }: TableRouteParams) return NextResponse.json({ error: 'Access denied' }, { status: 403 }) } + // Security check: verify workspaceId matches the table's workspace + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) + } + // Soft delete table using service layer await deleteTable(tableId, requestId) 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 9ce21d73e..2a383e1d6 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts @@ -20,8 +20,7 @@ const logger = createLogger('TableRowAPI') /** * Zod schema for validating get row requests. * - * The workspaceId is optional for backward compatibility but - * is validated via table access checks when provided. + * The workspaceId is required and validated against the table. */ const GetRowSchema = z.object({ workspaceId: z.string().min(1, 'Workspace ID is required'), @@ -102,16 +101,14 @@ export async function GET(request: NextRequest, { params }: RowRouteParams) { return NextResponse.json({ error: 'Access denied' }, { status: 403 }) } - // Security check: If workspaceId is provided, verify it matches the table's workspace - const actualWorkspaceId = validated.workspaceId || accessCheck.table.workspaceId - if (validated.workspaceId) { - const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) - if (!isValidWorkspace) { - logger.warn( - `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` - ) - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + // Security check: verify workspaceId matches the table's workspace + const actualWorkspaceId = validated.workspaceId + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } // Get row @@ -199,16 +196,14 @@ export async function PATCH(request: NextRequest, { params }: RowRouteParams) { const accessResult = await checkAccessOrRespond(tableId, authResult.userId, requestId, 'write') if (accessResult instanceof NextResponse) return accessResult - // Security check: If workspaceId is provided, verify it matches the table's workspace - const actualWorkspaceId = validated.workspaceId || accessResult.table.workspaceId - if (validated.workspaceId) { - const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) - if (!isValidWorkspace) { - logger.warn( - `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessResult.table.workspaceId}` - ) - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + // Security check: verify workspaceId matches the table's workspace + const actualWorkspaceId = validated.workspaceId + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessResult.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } // Get table definition @@ -310,16 +305,14 @@ export async function DELETE(request: NextRequest, { params }: RowRouteParams) { const accessResult = await checkAccessOrRespond(tableId, authResult.userId, requestId, 'write') if (accessResult instanceof NextResponse) return accessResult - // Security check: If workspaceId is provided, verify it matches the table's workspace - const actualWorkspaceId = validated.workspaceId || accessResult.table.workspaceId - if (validated.workspaceId) { - const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) - if (!isValidWorkspace) { - logger.warn( - `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessResult.table.workspaceId}` - ) - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + // Security check: verify workspaceId matches the table's workspace + const actualWorkspaceId = validated.workspaceId + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessResult.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } // Delete row diff --git a/apps/sim/app/api/table/[tableId]/rows/route.ts b/apps/sim/app/api/table/[tableId]/rows/route.ts index 7f6b47b46..0c3a986de 100644 --- a/apps/sim/app/api/table/[tableId]/rows/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/route.ts @@ -29,8 +29,7 @@ const logger = createLogger('TableRowsAPI') /** * Zod schema for inserting a single row into a table. * - * The workspaceId is optional for backward compatibility but is validated - * via table access checks when provided. + * The workspaceId is required and validated against the table. */ const InsertRowSchema = z.object({ workspaceId: z.string().min(1, 'Workspace ID is required'), @@ -140,16 +139,15 @@ async function handleBatchInsert( const table = accessResult.table - // Security check: If workspaceId is provided, verify it matches the table's workspace - if (validated.workspaceId && validated.workspaceId !== table.workspaceId) { + // Security check: verify workspaceId matches the table's workspace + if (validated.workspaceId !== table.workspaceId) { logger.warn( `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${table.workspaceId}` ) return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - // Use the workspaceId from the table (more secure) - const workspaceId = table.workspaceId + const workspaceId = validated.workspaceId // Check row count limit const remainingCapacity = table.maxRows - table.rowCount @@ -271,16 +269,15 @@ export async function POST(request: NextRequest, { params }: TableRowsRouteParam const table = accessResult.table - // Security check: If workspaceId is provided, verify it matches the table's workspace - if (validated.workspaceId && validated.workspaceId !== table.workspaceId) { + // Security check: verify workspaceId matches the table's workspace + if (validated.workspaceId !== table.workspaceId) { logger.warn( `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${table.workspaceId}` ) return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - // Use the workspaceId from the table (more secure) - const workspaceId = table.workspaceId + const workspaceId = validated.workspaceId const rowData = validated.data as RowData // Validate row data (size, schema, unique constraints) @@ -410,20 +407,16 @@ export async function GET(request: NextRequest, { params }: TableRowsRouteParams return NextResponse.json({ error: 'Access denied' }, { status: 403 }) } - // Security check: If workspaceId is provided, verify it matches the table's workspace - if (validated.workspaceId) { - const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) - if (!isValidWorkspace) { - logger.warn( - `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` - ) - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + // Security check: verify workspaceId matches the table's workspace + const actualWorkspaceId = validated.workspaceId + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessCheck.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - // Use the workspaceId from the access check (more secure) - const actualWorkspaceId = validated.workspaceId || accessCheck.table.workspaceId - // Build base where conditions const baseConditions = [ eq(userTableRows.tableId, tableId), @@ -543,16 +536,15 @@ export async function PUT(request: NextRequest, { params }: TableRowsRouteParams const table = accessResult.table - // Security check: If workspaceId is provided, verify it matches the table's workspace - if (validated.workspaceId && validated.workspaceId !== table.workspaceId) { + // Security check: verify workspaceId matches the table's workspace + if (validated.workspaceId !== table.workspaceId) { logger.warn( `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${table.workspaceId}` ) return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - // Use the workspaceId from the table (more secure) - const actualWorkspaceId = table.workspaceId + const actualWorkspaceId = validated.workspaceId const updateData = validated.data as RowData // Validate new data size @@ -749,20 +741,16 @@ export async function DELETE(request: NextRequest, { params }: TableRowsRoutePar const accessResult = await checkAccessOrRespond(tableId, authResult.userId, requestId, 'write') if (accessResult instanceof NextResponse) return accessResult - // Security check: If workspaceId is provided, verify it matches the table's workspace - if (validated.workspaceId) { - const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) - if (!isValidWorkspace) { - logger.warn( - `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessResult.table.workspaceId}` - ) - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + // Security check: verify workspaceId matches the table's workspace + const actualWorkspaceId = validated.workspaceId + const isValidWorkspace = await verifyTableWorkspace(tableId, validated.workspaceId) + if (!isValidWorkspace) { + logger.warn( + `[${requestId}] Workspace ID mismatch for table ${tableId}. Provided: ${validated.workspaceId}, Actual: ${accessResult.table.workspaceId}` + ) + return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - // Use the workspaceId from the access check (more secure) - const actualWorkspaceId = validated.workspaceId || accessResult.table.workspaceId - // Build base where conditions const baseConditions = [ eq(userTableRows.tableId, tableId), diff --git a/apps/sim/lib/table/service.ts b/apps/sim/lib/table/service.ts index ea59d1e7e..7ce1b6bd8 100644 --- a/apps/sim/lib/table/service.ts +++ b/apps/sim/lib/table/service.ts @@ -409,7 +409,7 @@ export async function queryRows( .from(userTableRows) .where(whereClause ?? baseConditions) - const totalCount = countResult[0].count + const totalCount = Number(countResult[0].count) // Build ORDER BY clause let orderByClause @@ -503,17 +503,14 @@ export async function updateRow( throw new Error('Row not found') } - // Merge data - const mergedData = { ...(existingRow.data as RowData), ...data.data } - // Validate size - const sizeValidation = validateRowSize(mergedData) + const sizeValidation = validateRowSize(data.data) if (!sizeValidation.valid) { throw new Error(sizeValidation.errors.join(', ')) } // Validate against schema - const schemaValidation = validateRowAgainstSchema(mergedData, table.schema) + const schemaValidation = validateRowAgainstSchema(data.data, table.schema) if (!schemaValidation.valid) { throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) } @@ -527,7 +524,7 @@ export async function updateRow( .where(eq(userTableRows.tableId, data.tableId)) const uniqueValidation = validateUniqueConstraints( - mergedData, + data.data, table.schema, existingRows.map((r) => ({ id: r.id, data: r.data as RowData })), data.rowId // Exclude current row @@ -541,14 +538,14 @@ export async function updateRow( await db .update(userTableRows) - .set({ data: mergedData, updatedAt: now }) + .set({ data: data.data, updatedAt: now }) .where(eq(userTableRows.id, data.rowId)) logger.info(`[${requestId}] Updated row ${data.rowId} in table ${data.tableId}`) return { id: data.rowId, - data: mergedData, + data: data.data, createdAt: existingRow.createdAt, updatedAt: now, } diff --git a/apps/sim/lib/table/types.ts b/apps/sim/lib/table/types.ts index 1c5e846ed..5ee1f42e9 100644 --- a/apps/sim/lib/table/types.ts +++ b/apps/sim/lib/table/types.ts @@ -217,7 +217,7 @@ export interface UpdateRowData { tableId: string /** Row ID to update */ rowId: string - /** Updated data (merged with existing) */ + /** Full row data replacement */ data: RowData /** Workspace ID */ workspaceId: string