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): coerce row values to column types on write instead of failing#4761
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
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
fe6ea03
fix(tables): coerce row values to column types on write instead of fa…
TheodoreSpeaks 4f72933
fix(tables): persist coerced values in upsert match + bulk update, no…
TheodoreSpeaks 42dd8e2
fix(tables): guard date coercion against out-of-range values
TheodoreSpeaks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
2 changes: 2 additions & 0 deletions
2 apps/sim/lib/table/__tests__/service-filter-threading.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -61,8 +61,9 @@ import type { | ||
| import { | ||
| checkBatchUniqueConstraintsDb, | ||
| checkUniqueConstraintsDb, | ||
| coerceRowToSchema, | ||
| coerceRowValues, | ||
| getUniqueColumns, | ||
| validateRowAgainstSchema, | ||
| validateRowSize, | ||
| validateTableName, | ||
| validateTableSchema, | ||
| @@ -913,7 +914,7 @@ export async function insertRow( | ||
| } | ||
| // Validate against schema | ||
| const schemaValidation = validateRowAgainstSchema(data.data, table.schema) | ||
| const schemaValidation = coerceRowToSchema(data.data, table.schema) | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| @@ -1060,7 +1061,7 @@ export async function batchInsertRowsWithTx( | ||
| throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`) | ||
| } | ||
| const schemaValidation = validateRowAgainstSchema(row, table.schema) | ||
| const schemaValidation = coerceRowToSchema(row, table.schema) | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| @@ -1201,7 +1202,7 @@ export async function replaceTableRowsWithTx( | ||
| throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`) | ||
| } | ||
| const schemaValidation = validateRowAgainstSchema(row, table.schema) | ||
| const schemaValidation = coerceRowToSchema(row, table.schema) | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| @@ -1331,22 +1332,24 @@ export async function upsertRow( | ||
| ) | ||
| } | ||
| const targetValue = data.data[targetColumnName] | ||
| if (targetValue === undefined || targetValue === null) { | ||
| throw new Error(`Upsert requires a value for the conflict target column "${targetColumnName}"`) | ||
| } | ||
| // Validate row data | ||
| const sizeValidation = validateRowSize(data.data) | ||
| if (!sizeValidation.valid) { | ||
| throw new Error(sizeValidation.errors.join(', ')) | ||
| } | ||
| const schemaValidation = validateRowAgainstSchema(data.data, schema) | ||
| const schemaValidation = coerceRowToSchema(data.data, schema) | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| // Read the conflict-target value *after* coercion so `matchFilter` branches on | ||
| // the persisted type (e.g. a coerced `"123"` → `123` matches existing rows). | ||
| const targetValue = data.data[targetColumnName] | ||
| if (targetValue === undefined || targetValue === null) { | ||
| throw new Error(`Upsert requires a value for the conflict target column "${targetColumnName}"`) | ||
| } | ||
| // `data->` and `data->>` accept the JSON key as a parameterized text value; | ||
| // no need for `sql.raw` interpolation. | ||
| const matchFilter = | ||
| @@ -1957,7 +1960,7 @@ export async function updateRow( | ||
| } | ||
| // Validate against schema | ||
| const schemaValidation = validateRowAgainstSchema(mergedData, table.schema) | ||
| const schemaValidation = coerceRowToSchema(mergedData, table.schema) | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| @@ -2167,6 +2170,12 @@ export async function updateRowsByFilter( | ||
| return { affectedCount: 0, affectedRowIds: [] } | ||
| } | ||
| // Coerce the patch itself in place — the write below persists `data.data` | ||
| // (as `patchJson`), so coercing only the per-row merged copies would be | ||
| // discarded. The merged validation in the loop still enforces required | ||
| // fields against the full row. | ||
| coerceRowValues(data.data, table.schema) | ||
| for (const row of matchingRows) { | ||
| const existingData = row.data as RowData | ||
| const mergedData = { ...existingData, ...data.data } | ||
| @@ -2176,7 +2185,7 @@ export async function updateRowsByFilter( | ||
| throw new Error(`Row ${row.id}: ${sizeValidation.errors.join(', ')}`) | ||
| } | ||
| const schemaValidation = validateRowAgainstSchema(mergedData, table.schema) | ||
| const schemaValidation = coerceRowToSchema(mergedData, table.schema) | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Row ${row.id}: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| @@ -2334,7 +2343,7 @@ export async function batchUpdateRows( | ||
| throw new Error(`Row ${update.rowId}: ${sizeValidation.errors.join(', ')}`) | ||
| } | ||
| const schemaValidation = validateRowAgainstSchema(merged, table.schema) | ||
| const schemaValidation = coerceRowToSchema(merged, table.schema) | ||
| if (!schemaValidation.valid) { | ||
| throw new Error(`Row ${update.rowId}: ${schemaValidation.errors.join(', ')}`) | ||
| } | ||
| @@ -3247,8 +3256,8 @@ export async function updateWorkflowGroup( | ||
| // Resolve the new leaf type for each remap so the column's declared type | ||
| // matches what the new mapping produces. Without this, a string→number | ||
| // remap would keep `type: 'string'` and validateRowAgainstSchema would | ||
| // reject every backfilled value. | ||
| // remap would keep `type: 'string'` and coerceRowToSchema would coerce | ||
| // every backfilled value toward the wrong type. | ||
| try { | ||
| const [ | ||
| { loadWorkflowFromNormalizedTables }, | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.