-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
feat: add upsert methods for the drivers that support onUpdate #8104
Changes from 2 commits
5f37460
3c0a5d9
9703549
343d0f0
c31d0e1
c30408d
9e4f50f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,6 +147,29 @@ await manager.update(User, 1, { firstName: "Rizzrak" }); | |
// executes UPDATE user SET firstName = Rizzrak WHERE id = 1 | ||
``` | ||
|
||
* `upsert` - Inserts a new entity or array of entities unless they already exist in which case they are updated instead. Supported by AuroraDataApi, Cockroach, Mysql, Postgres, and Sqlite database drivers. | ||
|
||
```typescript | ||
await manager.upsert(User, { externalId: "abc123" }, { firstName: "Rizzrak" }); | ||
/** executes | ||
* INSERT INTO user | ||
* VALUES (externalId = abc123, firstName = Rizzrak) | ||
* ON CONFLICT (externalId) DO UPDATE firstName = EXCLUDED.firstName | ||
**/ | ||
|
||
await manager.update(User, ["externalId"], [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all set here |
||
{ externalId:"abc123", firstName: "Rizzrak" }, | ||
{ externalId:"bca321", firstName: "Karzzir" }, | ||
]); | ||
/** executes | ||
* INSERT INTO user | ||
* VALUES | ||
* (externalId = abc123, firstName = Rizzrak), | ||
* (externalId = cba321, firstName = Karzzir), | ||
* ON CONFLICT (externalId) DO UPDATE firstName = EXCLUDED.firstName | ||
**/ | ||
``` | ||
|
||
* `delete` - Deletes entities by entity id, ids or given conditions: | ||
|
||
```typescript | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,6 +154,29 @@ await repository.update(1, { firstName: "Rizzrak" }); | |
// executes UPDATE user SET firstName = Rizzrak WHERE id = 1 | ||
``` | ||
|
||
* `upsert` - Inserts a new entity or array of entities unless they already exist in which case they are updated instead. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also need to specify list of supported drivers here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. incoming |
||
|
||
```typescript | ||
await repository.upsert({ externalId: "abc123" }, { firstName: "Rizzrak" }); | ||
/** executes | ||
* INSERT INTO user | ||
* VALUES (externalId = abc123, firstName = Rizzrak) | ||
* ON CONFLICT (externalId) DO UPDATE firstName = EXCLUDED.firstName | ||
**/ | ||
|
||
await repository.update(["externalId"], [ | ||
{ externalId:"abc123", firstName: "Rizzrak" }, | ||
{ externalId:"bca321", firstName: "Karzzir" }, | ||
]); | ||
/** executes | ||
* INSERT INTO user | ||
* VALUES | ||
* (externalId = abc123, firstName = Rizzrak), | ||
* (externalId = cba321, firstName = Karzzir), | ||
* ON CONFLICT (externalId) DO UPDATE firstName = EXCLUDED.firstName | ||
**/ | ||
``` | ||
|
||
* `delete` - Deletes entities by entity id, ids or given conditions: | ||
|
||
```typescript | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export type UpsertType = "on-conflict-do-update" | "on-duplicate-key-update"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import { QueryDeepPartialEntity } from "../query-builder/QueryPartialEntity"; | ||
|
||
/** | ||
* Special options passed to Repository#upsert | ||
*/ | ||
export interface UpsertOptions<Entity> { | ||
|
||
/** | ||
* Allow potentially unsafe insert only | ||
*/ | ||
allowUnsafeInsertOnly?: boolean; | ||
|
||
/** | ||
* Some values that will be written on insert but not on update | ||
*/ | ||
insertOnly?: QueryDeepPartialEntity<Entity> | ||
} |
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.
TBH I find this signature confusing. I prefer to have only second signature:
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.
Also I would swap parameters order, it makes more logically sense to have:
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.
I put a lot of thought into the signature and think it is best to match the style used by https://docs.mongodb.com/manual/reference/method/db.collection.findOneAndUpdate/. In addition, since the definition is essentially
upsert(Entity, ${things to match/conflict on}, ${things to update})
I felt that parameter order made more sense for the second signature. I feel strongly about keeping the first signature, but a little less strongly about the order of the second signature.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.
${things to match/conflict on}
- but when there is no conflict, this value will be used for insertion, right? So it's actually${things to match/conflict on/insert if no conflict merged with next parameter values}
- and this is what it makes this signature complicated in understanding.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.
this is where i'm hung up:
await postRepository.upsert({ externalId }, { title: "Post title initial" });
vs
await postRepository.upsert({ externalId, title: "Post title initial" }, ["externalId"]);
I feel that the first signature flows significantly better than the second. The second feels unnecessarily redundant.
MongoRepository#updateOne
has the same basic signature and I don't believe it's complicated to understand:await mongoRepository.updateOne({ externalId }, { title: "Post title initial" }, { upsert: true });
Perhaps there's a middle ground here? Maybe rather than 1 upsert method with 2 signatures I should introduce 2 upsert methods?
Repository#upsertOne(conditions, partial)
, a signature similar toMongoRepository#updateOne(conditions, partial, { upsert: true })
and alsoRepository#upsertMany(conflictPaths: string[], partialEntities: DeepPartial<Entity>[])
I really don't want to lose the MongoDb style signature.
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.
I'm looking for both simplicity and obviousness. Adding new method won't help. Following MongoDB convention doesn't have to be there, maybe some people can appreciate it, but most prefer simplicity and obviousness. And I don't find
${things to match/conflict on/insert if no conflict merged with next parameter values}
parameter obvious. Maybe because I'm not MongoDB user. I would prefer to type extra code.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.
Well, I don't like it, but you're in charge here. I'll update the PR with your preferred style.
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.
sorry about that =( If you want we can wait for other people who are interested in this feature to provide their feedback.
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.
no worries, I'm going to make the change. we can always reintroduce the second signature if feedback warrants it. thanks for everything you do for typeorm.
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.
thank you 🤞