Add support for WHERE clauses to "upsert" queries #4148
Conversation
| ); | ||
| }); | ||
| } catch (err) { | ||
| if (/oracle|mssql/i.test(knex.client.driverName)) { |
There was a problem hiding this comment.
Skipping test altogether same as is done for redshift probably would be more transparent in test results log.
There was a problem hiding this comment.
I don't fully understand the test setup - but it seems like the difference between redshift and oracle/mssql is that for oracle/mssql we expect an error to be thrown, but not redshift? Should I preserve that distinction or just add mssql/oracle to the skip?
There was a problem hiding this comment.
TBH, I think we don't even run redshift in CI, so feel free to just keep ignoring it.
BTW, shouldn't MySQL also be included in error assertion?
There was a problem hiding this comment.
Ah yeah. Added MySQL to have an error assertion:
| @@ -1465,6 +1465,110 @@ module.exports = function (knex) { | |||
| expect(rows[0].name).to.equal('AFTER'); | |||
| }); | |||
|
|
|||
| it('conditionally updates rows when inserting a duplicate key to unique column and merge with where clause matching row(s) is specified', async function () { | |||
| if (/redshift/i.test(knex.client.driverName)) { | |||
There was a problem hiding this comment.
shouldn't mysql be also skipped if you are saying it is not supported?
|
Since you are still planning to add support for SQLite, that will resolve one of the currently failing tests. Could you please add the sanity check for MySQL, which would throw an error if someone tries to use it with WHERE clause, and adjust assertion accordingly? |
|
@kibertoad I think I have addressed your feedback, minus one question I had about the integration tests |
| if (merge) { | ||
| sql += this._merge(merge.updates, insert); | ||
| const wheres = this.where(); | ||
| if (wheres) { |
There was a problem hiding this comment.
It feels like there might be a better way to check whether where is called after a merge statement for MySQL, but I could not figure out what it is
test/integration/builder/inserts.js
Outdated
| @@ -1465,6 +1465,130 @@ module.exports = function (knex) { | |||
| expect(rows[0].name).to.equal('AFTER'); | |||
| }); | |||
|
|
|||
| it('conditionally updates rows when inserting a duplicate key to unique column and merge with where clause matching row(s) is specified', async function () { | |||
| if (/redshift|mysql/i.test(knex.client.driverName)) { | |||
There was a problem hiding this comment.
since you are asserting the error now, probably no longer you need to skip it
There was a problem hiding this comment.
Sorry about that. Fixed: 80f366d
|
This can be merged after documentation is added to https://github.com/knex/documentation |
|
@kibertoad OK, I have created a documentation PR: knex/documentation#300 |
|
Thanks a lot! |
|
Released in 0.21.13 |
This is a follow-up to #3763, which adds support for WHERE clauses to "upsert" queries for the DB engines that support this functionality.
Based on my research on the DB engines that support "upsert" queries, it seems like PostgreSQL and SQLite support WHERE clauses for them, but MySQL does not:
https://www.postgresql.org/docs/10/sql-insert.html
https://sqlite.org/lang_upsert.html
https://dev.mysql.com/doc/refman/8.0/en/insert-on-duplicate.html
To do:
Closes #4145