feat: implement D1 upsert - #55
Conversation
Adds `upsert` and `upsertp` methods to `D1` in `d1.js` leveraging SQLite's `INSERT ... ON CONFLICT(id) DO UPDATE SET ...` syntax. Properly merges object properties via `json_patch`. Also adds a test file (`test_upsert.js`) to verify functionality. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces upsert functionality to the D1 library, including the core _upsertp, upsertp, and upsert methods, along with a new API endpoint and comprehensive tests. Feedback focuses on improving the robustness of the ON CONFLICT clause by using model-defined primary keys, refactoring duplicated logic between insertion and upsertion methods, and relaxing field name validation to allow underscores.
| s += ` ON CONFLICT(id) DO UPDATE SET ${updates.join(', ')}` | ||
| } else { | ||
| s += ` ON CONFLICT(id) DO NOTHING` |
There was a problem hiding this comment.
The ON CONFLICT clause is hardcoded to use id as the conflict target. While the library seems to favor id as the primary key, it would be more robust to use the primary key defined in the model if available, similar to how jsonObjectCol handles it.
| async _upsertp(table, obj, opts = {}) { | ||
| if (typeof table != 'string') { | ||
| opts.model = table | ||
| } | ||
| let ob = null | ||
| let fields = [] | ||
| let values = [] | ||
| if (obj instanceof Object && !Array.isArray(obj)) { | ||
| ob = obj | ||
| values = [] | ||
| let f2 = [] | ||
| for (const f in obj) { | ||
| f2.push(f) | ||
| values.push(obj[f]) | ||
| } | ||
| fields = f2 | ||
| } else { | ||
| // deprecated | ||
| fields = obj | ||
| values = opts | ||
| ob = {} | ||
| opts = {} | ||
| } | ||
| let id | ||
| if (!fields.includes('id')) { | ||
| id = nanoid() | ||
| fields.push('id') | ||
| values.push(id) | ||
| ob.id = id | ||
| } else { | ||
| id = values[fields.indexOf('id')] | ||
| if (!id) { | ||
| id = nanoid() | ||
| values[fields.indexOf('id')] = id | ||
| ob.id = id | ||
| } | ||
| } | ||
| let now = new Date() | ||
| if (!fields.includes('createdAt')) { | ||
| fields.push('createdAt') | ||
| values.push(now) | ||
| ob.createdAt = now | ||
| } | ||
| if (!fields.includes('updatedAt')) { | ||
| fields.push('updatedAt') | ||
| values.push(now) | ||
| ob.updatedAt = now | ||
| } else { | ||
| // If updating, we should probably set updatedAt to now anyway, or keep what they provided? | ||
| // Let's keep what they provided for consistency with insert, but typically upsert would update the date. | ||
| // Actually we'll let ON CONFLICT handle updating it to `excluded.updatedAt`. | ||
| } | ||
| for (let f of fields) { | ||
| if (!/^[a-zA-Z0-9]+$/.test(f)) { | ||
| throw new Error('Field must be alphanumeric') | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between _upsertp and _insertp (lines 374-426). Most of the logic for extracting fields, generating IDs, and setting timestamps is identical. Consider refactoring this shared logic into a private helper method to improve maintainability.
| // Actually we'll let ON CONFLICT handle updating it to `excluded.updatedAt`. | ||
| } | ||
| for (let f of fields) { | ||
| if (!/^[a-zA-Z0-9]+$/.test(f)) { |
There was a problem hiding this comment.
The alphanumeric check !/^[a-zA-Z0-9]+$/.test(f) is quite restrictive as it prevents the use of underscores in field names, which is a common convention in SQL (e.g., user_id). While this matches the existing _insertp implementation, consider allowing underscores to support a wider range of database schemas.
| if (!/^[a-zA-Z0-9]+$/.test(f)) { | |
| if (!/^[a-zA-Z0-9_]+$/.test(f)) { |
1 participant
Adds
upsertandupsertpmethods toD1ind1.jsleveraging SQLite'sINSERT ... ON CONFLICT(id) DO UPDATE SET ...syntax. Properly merges object properties viajson_patch. Also adds a test file (test_upsert.js) to verify functionality.PR created automatically by Jules for task 5877242264351571818 started by @treeder