-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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(postgres): native upsert #12301
Conversation
1c7299d
to
f0de18f
Compare
Codecov Report
@@ Coverage Diff @@
## master #12301 +/- ##
==========================================
- Coverage 96.34% 96.29% -0.05%
==========================================
Files 95 95
Lines 9127 9127
==========================================
- Hits 8793 8789 -4
- Misses 334 338 +4
Continue to review full report at Codecov.
|
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.
Can't say much about the query logic changes, I'm not really a postgres user, some comments on the code though.
* @param {boolean} [options.returning=false] If true, fetches back auto generated values (Postgres only) | ||
* @param {boolean} [options.returning=true] If true, fetches back auto generated values |
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.
Does this work for all 5 dialects now?
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.
Except for mariadb, which does not support RETURINING *
at all, in current 10.1 target version. But it wont fail
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 am confused, if it is not supported by MariaDB, then why could the following code be removed:
if (options.returning === true && primaryKey) {
const record = await this.findByPk(primaryKey, options);
result = [record, created];
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 works but it is buggy, see linked issues. Plus one extra read from database. So I decided to not support this for mariadb.
0c10665
to
7b174eb
Compare
@papb I think I have taken care of all points raised by you, I am merging this PR. Let me know if you have any questions |
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
Closes #11246
Fixes #4132
Fixes #12051
Fixes #9629
Fixes #9216