Add support for passing a unique column to queryBuilder for UPSERT st… #63
Conversation
…atements The default primary key does work but is not suitable for all situations, there are use cases that need an email or an api key to be used as the conflicting column
|
| @@ -32,6 +32,7 @@ export type GenerateQueryOptions<T extends object> = { | |||
| orderBy?: OrderBy<T> | OrderBy<T>[]; | |||
| data?: Partial<T>; | |||
| upsertOnlyUpdateData?: Partial<T>; | |||
| uniqueColumn?: string; | |||
There was a problem hiding this comment.
Have you run this code against D1 to confirm it works?
There was a problem hiding this comment.
I think the type can be a little more strict but otherwise I have tested this on a D1 preview and it works fine.
| @@ -171,9 +172,10 @@ export function GenerateQuery<T extends object>( | |||
| .split("") | |||
| .join(", ")})`; | |||
|
|
|||
| const primaryKeyStr = Array.isArray(primaryKeys) | |||
| const primaryKeyStr = options.uniqueColumn || (Array.isArray(primaryKeys) | |||
There was a problem hiding this comment.
Is there a reason we can't just use the primary key value for this? It could use a rename - but functionally it appears to do exactly the same
There was a problem hiding this comment.
I think this covers a completely different use case, for example I have a table where the primary key is id but when upserting I want to use a different unique column like email, api_key or something similar.
I probably would not want to update the primary key to be email or api_key instead of id. does that make sense?
There was a problem hiding this comment.
If we instead renamed the variable to something like conflictConstraint, that would demonstrate it's purpose better, and still allow you to do this.
There was a problem hiding this comment.
Are you suggesting to change options.uniqueColumn to options.conflictConstraint or you are thinking more like renaming the primaryKeys in the options passed to the model constructor?
There was a problem hiding this comment.
If it's the later then do you not think that would cause issues with the migrations? I was under the impression that you can also create the table using the model configurations.
There was a problem hiding this comment.
I do mean the latter. This would ideally:
- Rename the variable in the Query builder
- Edit the Model#Upsert method to have a new optional parameter for constraint columns, which will default to the primary key(s) if not already present
| @@ -291,6 +291,15 @@ describe("Query Builder", () => { | |||
| upsertOnlyUpdateData: { id: 1 }, | |||
| }) | |||
| ).to.not.throw(); | |||
|
|
|||
| expect(() => | |||
| GenerateQuery(QueryType.UPSERT, "test", { | |||
There was a problem hiding this comment.
Not a fault of your test in particular, but we really should be testing the output of these queries too
|
Closing this PR as I don't have enough time to finish the work required here. 😞 |
The default primary key does work but is not suitable for all situations, there are use cases that need an email or an api key to be used as the conflicting column.