Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(tables): enforce plan limits in mothership user_table tool#4832
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
1abd7cd96b203a52dd4caFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -14,6 +14,7 @@ import { | ||
| type CsvHeaderMapping, | ||
| CsvImportValidationError, | ||
| coerceRowsForTable, | ||
| getWorkspaceTableLimits, | ||
| inferSchemaFromCsv, | ||
| parseCsvBuffer, | ||
| sanitizeName, | ||
| @@ -263,13 +264,16 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult> | ||
| const requestId = generateId().slice(0, 8) | ||
| assertNotAborted() | ||
| const planLimits = await getWorkspaceTableLimits(workspaceId) | ||
| const table = await createTable( | ||
| { | ||
| name: args.name, | ||
| description: args.description, | ||
| schema: args.schema, | ||
| workspaceId, | ||
| userId: context.userId, | ||
| maxRows: planLimits.maxRowsPerTable, | ||
| maxTables: planLimits.maxTables, | ||
| }, | ||
| requestId | ||
| ) | ||
| @@ -761,31 +765,71 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult> | ||
| const tableName = args.name || file.name.replace(/\.[^.]+$/, '') | ||
| const requestId = generateId().slice(0, 8) | ||
| assertNotAborted() | ||
| const planLimits = await getWorkspaceTableLimits(workspaceId) | ||
| const droppedRows = Math.max(0, rows.length - planLimits.maxRowsPerTable) | ||
| const rowsToImport = droppedRows > 0 ? rows.slice(0, planLimits.maxRowsPerTable) : rows | ||
| const table = await createTable( | ||
| { | ||
| name: tableName, | ||
| description: args.description || `Imported from ${file.name}`, | ||
| schema: { columns }, | ||
| workspaceId, | ||
| userId: context.userId, | ||
| maxRows: planLimits.maxRowsPerTable, | ||
| maxTables: planLimits.maxTables, | ||
| }, | ||
| requestId | ||
| ) | ||
| const coerced = coerceRowsForTable(rows, { columns }, headerToColumn) | ||
| const inserted = await batchInsertAll(table.id, coerced, table, workspaceId, context) | ||
| const coerced = coerceRowsForTable(rowsToImport, { columns }, headerToColumn) | ||
| let inserted: number | ||
| try { | ||
| inserted = await batchInsertAll(table.id, coerced, table, workspaceId, context) | ||
| } catch (insertError) { | ||
| const cleanupRequestId = generateId().slice(0, 8) | ||
| await deleteTable(table.id, cleanupRequestId).catch((cleanupError) => { | ||
| logger.error('Failed to roll back table after import failure', { | ||
| tableId: table.id, | ||
| error: toError(cleanupError).message, | ||
| }) | ||
| }) | ||
| const reason = toError(insertError).message | ||
| const cause = | ||
| insertError instanceof Error && insertError.cause | ||
| ? toError(insertError.cause).message | ||
| : undefined | ||
| logger.error('Failed to import rows into new table', { | ||
| tableId: table.id, | ||
| fileName: file.name, | ||
| error: reason, | ||
| cause, | ||
| }) | ||
| return { | ||
| success: false, | ||
| message: `Failed to import rows from "${file.name}" — the table was rolled back. ${cause ? `${reason} (${cause})` : reason}`, | ||
| } | ||
| } | ||
greptile-apps[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. greptile-apps[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. greptile-apps[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| logger.info('Table created from file', { | ||
| tableId: table.id, | ||
| fileName: file.name, | ||
| columns: columns.length, | ||
| rows: inserted, | ||
| droppedRows, | ||
| userId: context.userId, | ||
| }) | ||
| const createdMessage = `Created table "${table.name}" with ${columns.length} columns and ${inserted.toLocaleString()} rows from "${file.name}"` | ||
| const message = | ||
| droppedRows > 0 | ||
| ? `${createdMessage}. Dropped ${droppedRows.toLocaleString()} row(s) that exceed this plan's limit of ${planLimits.maxRowsPerTable.toLocaleString()} rows per table.` | ||
| : createdMessage | ||
| return { | ||
| success: true, | ||
| message: `Created table "${table.name}" with ${columns.length} columns and ${inserted} rows from "${file.name}"`, | ||
| message, | ||
| data: { | ||
| tableId: table.id, | ||
| tableName: table.name, | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
planLimits.maxRowsPerTablebeing 0 or negative.getTablePlanLimitsreads its values throughenvNumberenv-var overrides without a minimum-value check, and the privategetMaxRowsPerTablehelper explicitly documents-1as the sentinel for "unlimited". If any override is set to 0, all rows are dropped silently and an empty table is created. If it is set to -1,rows.slice(0, -1)drops the last row from every import and produces the nonsensical message "exceeded plan limit of -1 rows".