Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds UI and API support for mutating deployment database tables from the Deployment page, enabling inserting new rows and updating existing rows (with shared access checks on the backend).
Changes:
- Add an “Insert Row” drawer UI and a row “Update” form in
DeploymentPage.tsx, plus a simple toast notification mechanism. - Add new API routes for table insert/update with shared deployment table access checks.
- Add
insertTableDataandupdateTableDatahelpers inapi/sql.ts, including write-transformer support.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
web/pages/DeploymentPage.tsx |
Adds Insert Row + Update Row forms and toast notifications in the deployment DB UI. |
api/sql.ts |
Implements insert/update SQL helpers and hooks in write-transformer pipeline. |
api/routes.ts |
Exposes new insert/update endpoints and refactors shared deployment table access checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const query = `INSERT INTO ${table} (${columns.join(', ')}) VALUES (${ | ||
| values.join(', ') | ||
| })` |
There was a problem hiding this comment.
The INSERT query interpolates the table name and column identifiers directly into SQL without validating/quoting them. Even with auth, this is still SQL injection-prone and can break on reserved words; validate identifiers against the cached schema (or strictly whitelist) and/or quote identifiers appropriately for the detected dialect.
| const sets = Object.entries(transformedData).map(([k, v]) => { | ||
| const val = v === null | ||
| ? 'NULL' | ||
| : typeof v === 'string' | ||
| ? `'${v.replace(/'/g, "''")}'` | ||
| : String(v) | ||
| return `${k} = ${val}` |
There was a problem hiding this comment.
updateTableData can generate an invalid query when data is empty: sets.join(', ') becomes an empty string, resulting in UPDATE <table> SET WHERE .... Add a guard to reject empty updates early (or return a no-op result) before building the SQL.
| effect(() => { | ||
| const dep = url.params.dep | ||
| if (dep) { | ||
| schema.fetch({ url: dep }) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
There are two identical @preact/signals effects that call schema.fetch() when url.params.dep changes (this one and the one starting at line 77). This will double-fetch the schema on every dependency change; remove one of the effects and keep a single schema-fetch effect.
| effect(() => { | |
| const dep = url.params.dep | |
| if (dep) { | |
| schema.fetch({ url: dep }) | |
| } | |
| }) |
| } else if (type.includes('Bool')) { | ||
| data[key] = val === 'on' | ||
| } else if ( |
There was a problem hiding this comment.
Checkbox values are not present in FormData when unchecked, so boolean columns cannot be set to false via this form. To support toggling off, read checkbox state from the form element (checked) or add a hidden field/default value so a false value is submitted when unchecked.
| try { | ||
| await api['POST/api/deployment/table/update'].fetch({ | ||
| deployment: url.params.dep!, | ||
| table: tableName, | ||
| pk: { key: pk, value: row[pk] as unknown as string }, |
There was a problem hiding this comment.
If the user submits without making changes (or all values are filtered out), data can be empty, which will produce an invalid UPDATE on the backend (empty SET clause). Add a guard before calling the update API: if there are no fields to update, show a toast and return.
| const sets = Object.entries(transformedData).map(([k, v]) => { | ||
| const val = v === null | ||
| ? 'NULL' | ||
| : typeof v === 'string' | ||
| ? `'${v.replace(/'/g, "''")}'` | ||
| : String(v) | ||
| return `${k} = ${val}` | ||
| }) | ||
|
|
||
| const pkVal = typeof pk.value === 'string' | ||
| ? `'${String(pk.value).replace(/'/g, "''")}'` | ||
| : String(pk.value) | ||
|
|
||
| const query = `UPDATE ${table} SET ${ | ||
| sets.join(', ') | ||
| } WHERE ${pk.key} = ${pkVal}` |
There was a problem hiding this comment.
Like insert, the UPDATE query interpolates table and pk.key directly into SQL; pk.value is only string-escaped. Validate/whitelist the table and pk.key identifiers against the cached schema and prefer parameter binding for pk.value and update values to avoid injection and quoting bugs.
| const sets = Object.entries(transformedData).map(([k, v]) => { | |
| const val = v === null | |
| ? 'NULL' | |
| : typeof v === 'string' | |
| ? `'${v.replace(/'/g, "''")}'` | |
| : String(v) | |
| return `${k} = ${val}` | |
| }) | |
| const pkVal = typeof pk.value === 'string' | |
| ? `'${String(pk.value).replace(/'/g, "''")}'` | |
| : String(pk.value) | |
| const query = `UPDATE ${table} SET ${ | |
| sets.join(', ') | |
| } WHERE ${pk.key} = ${pkVal}` | |
| const sanitizeIdentifier = (identifier: string): string => { | |
| // Allow only standard SQL identifier characters to avoid injection. | |
| // Identifiers must start with a letter or underscore, followed by letters, digits, or underscores. | |
| if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(identifier)) { | |
| throw new SQLQueryError('Invalid SQL identifier', JSON.stringify({ identifier })) | |
| } | |
| // Quote the identifier and escape any embedded double quotes. | |
| return `"${identifier.replace(/"/g, '""')}"` | |
| } | |
| const safeTable = sanitizeIdentifier(table) | |
| const sets = Object.entries(transformedData).map(([k, v]) => { | |
| const column = sanitizeIdentifier(k) | |
| const val = v === null | |
| ? 'NULL' | |
| : typeof v === 'string' | |
| ? `'${v.replace(/'/g, "''")}'` | |
| : String(v) | |
| return `${column} = ${val}` | |
| }) | |
| const pkColumn = sanitizeIdentifier(pk.key) | |
| const pkVal = typeof pk.value === 'string' | |
| ? `'${String(pk.value).replace(/'/g, "''")}'` | |
| : String(pk.value) | |
| const query = `UPDATE ${safeTable} SET ${ | |
| sets.join(', ') | |
| } WHERE ${pkColumn} = ${pkVal}` |
| if (row[key] === val) continue | ||
| const col = tableDef.columns.find((c) => c.name === key) | ||
| if (!col) continue | ||
| const type = col.type | ||
| if ( | ||
| type.includes('Int') || type.includes('Float') || | ||
| type.includes('Decimal') | ||
| ) { | ||
| data[key] = Number(val) | ||
| } else if (type.includes('Bool')) { | ||
| data[key] = val === 'on' | ||
| } else if ( | ||
| type.includes('JSON') || type.includes('Array') || type.includes('Map') | ||
| ) { | ||
| try { | ||
| data[key] = JSON.parse(val as string) | ||
| } catch { | ||
| data[key] = val | ||
| } | ||
| } else { | ||
| data[key] = val | ||
| } |
There was a problem hiding this comment.
The "unchanged" check if (row[key] === val) continue compares the current row value (often number/boolean/object) against FormData values (always string/File), so it will rarely skip anything and can cause unnecessary updates. Consider normalizing/parsing val to the column type before comparing, or comparing against the input element's typed value (e.g., valueAsNumber/checked).
| if (row[key] === val) continue | |
| const col = tableDef.columns.find((c) => c.name === key) | |
| if (!col) continue | |
| const type = col.type | |
| if ( | |
| type.includes('Int') || type.includes('Float') || | |
| type.includes('Decimal') | |
| ) { | |
| data[key] = Number(val) | |
| } else if (type.includes('Bool')) { | |
| data[key] = val === 'on' | |
| } else if ( | |
| type.includes('JSON') || type.includes('Array') || type.includes('Map') | |
| ) { | |
| try { | |
| data[key] = JSON.parse(val as string) | |
| } catch { | |
| data[key] = val | |
| } | |
| } else { | |
| data[key] = val | |
| } | |
| const col = tableDef.columns.find((c) => c.name === key) | |
| if (!col) continue | |
| const type = col.type | |
| let parsedVal: unknown | |
| if ( | |
| type.includes('Int') || type.includes('Float') || | |
| type.includes('Decimal') | |
| ) { | |
| parsedVal = Number(val) | |
| } else if (type.includes('Bool')) { | |
| parsedVal = val === 'on' | |
| } else if ( | |
| type.includes('JSON') || type.includes('Array') || type.includes('Map') | |
| ) { | |
| try { | |
| parsedVal = JSON.parse(val as string) | |
| } catch { | |
| parsedVal = val | |
| } | |
| } else { | |
| parsedVal = val | |
| } | |
| if (row[key] === parsedVal) continue | |
| data[key] = parsedVal |
| for (const [key, val] of formData.entries()) { | ||
| const col = tableDef.columns.find((c) => c.name === key) | ||
| if (!col) continue | ||
| const type = col.type | ||
| if ( | ||
| type.includes('Int') || type.includes('Float') || | ||
| type.includes('Decimal') | ||
| ) { | ||
| data[key] = Number(val) | ||
| } else if (type.includes('Bool')) { | ||
| data[key] = val === 'on' |
There was a problem hiding this comment.
Insert uses the same checkbox/FormData pattern as update: unchecked boolean inputs won't appear in FormData, so you can't explicitly insert false (the key will be missing). Read checkbox states via form.elements or ensure a default value is submitted for unchecked checkboxes.
| for (const [key, val] of formData.entries()) { | |
| const col = tableDef.columns.find((c) => c.name === key) | |
| if (!col) continue | |
| const type = col.type | |
| if ( | |
| type.includes('Int') || type.includes('Float') || | |
| type.includes('Decimal') | |
| ) { | |
| data[key] = Number(val) | |
| } else if (type.includes('Bool')) { | |
| data[key] = val === 'on' | |
| // Ensure boolean checkbox fields are always present in `data` with an explicit | |
| // true/false value, regardless of whether they are checked (and thus present | |
| // in FormData) or not. | |
| for (const col of tableDef.columns) { | |
| if (!col.type.includes('Bool')) continue | |
| const elem = form.elements.namedItem(col.name) | |
| if (elem instanceof HTMLInputElement && elem.type === 'checkbox') { | |
| data[col.name] = elem.checked | |
| } | |
| } | |
| for (const [key, val] of formData.entries()) { | |
| const col = tableDef.columns.find((c) => c.name === key) | |
| if (!col) continue | |
| const type = col.type | |
| // If this is a boolean field rendered as a checkbox, we've already set it | |
| // from `elem.checked` above, and the FormData value (`"on"`) is redundant. | |
| const elem = form.elements.namedItem(key) | |
| if ( | |
| type.includes('Bool') && | |
| elem instanceof HTMLInputElement && | |
| elem.type === 'checkbox' | |
| ) { | |
| continue | |
| } | |
| if ( | |
| type.includes('Int') || type.includes('Float') || | |
| type.includes('Decimal') | |
| ) { | |
| data[key] = Number(val) |
| 'POST/api/deployment/table/insert': route({ | ||
| authorize: withUserSession, | ||
| fn: (ctx, { deployment, table, data }) => { | ||
| const dep = withDeploymentTableAccess(ctx, deployment) |
There was a problem hiding this comment.
The insert route does not validate that table exists in the cached schema (or that data only contains known columns) like POST /api/deployment/table/data does. This allows arbitrary table/column names to reach SQL construction; fetch the cached schema for the deployment and reject unknown table/columns before calling insertTableData.
| const dep = withDeploymentTableAccess(ctx, deployment) | |
| const dep = withDeploymentTableAccess(ctx, deployment) | |
| const schema = DatabaseSchemasCollection.get(deployment) | |
| if (!schema) { | |
| throw respond.NotFound({ message: 'Schema not cached yet' }) | |
| } | |
| const tableDef = schema.tables.find((t) => t.table === table) | |
| if (!tableDef) { | |
| throw respond.NotFound({ message: 'Table not found in schema' }) | |
| } | |
| const allowedColumns = new Set(tableDef.columns.map((c) => c.name)) | |
| for (const columnName of Object.keys(data)) { | |
| if (!allowedColumns.has(columnName)) { | |
| throw respond.BadRequest({ | |
| message: `Unknown column "${columnName}" for table "${table}"`, | |
| }) | |
| } | |
| } |
| 'POST/api/deployment/table/update': route({ | ||
| authorize: withUserSession, | ||
| fn: (ctx, { deployment, table, pk, data }) => { | ||
| const dep = withDeploymentTableAccess(ctx, deployment) | ||
| return updateTableData(dep, table, pk, data) | ||
| }, |
There was a problem hiding this comment.
The update route trusts table and pk.key from the request without checking them against the deployment's cached schema. Validate the table exists, that pk.key is a column on that table, and that the update data keys are all valid columns before calling updateTableData.
…nt page, including new SQL methods and toast feedback.
…and primary key type support, and enhance table data editing in the UI.
… and UI component.
02f6f03 to
b0f21fc
Compare
98 implement insert database functionality