From 836f0314c83aca903ca7e357dbb5f9eb182d9a84 Mon Sep 17 00:00:00 2001 From: Matthew Blasius Date: Sat, 20 Nov 2021 13:04:04 -0600 Subject: [PATCH] fix(upsert): fall back to DO NOTHING if no update key values provided (#13594) --- lib/dialects/abstract/query-generator.js | 24 +++++++++++++++++++++++- lib/dialects/mssql/query-generator.js | 10 ++++++---- lib/dialects/mssql/query.js | 5 +++++ lib/dialects/mysql/query-interface.js | 1 + lib/dialects/postgres/query.js | 2 +- lib/model.js | 4 ++-- test/integration/model/upsert.test.js | 24 ++++++++++++++++++++++++ 7 files changed, 62 insertions(+), 8 deletions(-) diff --git a/lib/dialects/abstract/query-generator.js b/lib/dialects/abstract/query-generator.js index 5a438cd3268b..5563771332ae 100644 --- a/lib/dialects/abstract/query-generator.js +++ b/lib/dialects/abstract/query-generator.js @@ -180,14 +180,33 @@ class QueryGenerator { let onDuplicateKeyUpdate = ''; + // `options.updateOnDuplicate` is the list of field names to update if a duplicate key is hit during the insert. It + // contains just the field names. This option is _usually_ explicitly set by the corresponding query-interface + // upsert function. if (this._dialect.supports.inserts.updateOnDuplicate && options.updateOnDuplicate) { if (this._dialect.supports.inserts.updateOnDuplicate == ' ON CONFLICT DO UPDATE SET') { // postgres / sqlite // If no conflict target columns were specified, use the primary key names from options.upsertKeys const conflictKeys = options.upsertKeys.map(attr => this.quoteIdentifier(attr)); const updateKeys = options.updateOnDuplicate.map(attr => `${this.quoteIdentifier(attr)}=EXCLUDED.${this.quoteIdentifier(attr)}`); - onDuplicateKeyUpdate = ` ON CONFLICT (${conflictKeys.join(',')}) DO UPDATE SET ${updateKeys.join(',')}`; + onDuplicateKeyUpdate = ` ON CONFLICT (${conflictKeys.join(',')})`; + // if update keys are provided, then apply them here. if there are no updateKeys provided, then do not try to + // do an update. Instead, fall back to DO NOTHING. + onDuplicateKeyUpdate += _.isEmpty(updateKeys.length) ? ' DO NOTHING ' : ` DO UPDATE SET ${updateKeys.join(',')}`; } else { const valueKeys = options.updateOnDuplicate.map(attr => `${this.quoteIdentifier(attr)}=VALUES(${this.quoteIdentifier(attr)})`); + // the rough equivalent to ON CONFLICT DO NOTHING in mysql, etc is ON DUPLICATE KEY UPDATE id = id + // So, if no update values were provided, fall back to the identifier columns provided in the upsertKeys array. + // This will be the primary key in most cases, but it could be some other constraint. + if (_.isEmpty(valueKeys) && options.upsertKeys) { + valueKeys.push(...options.upsertKeys.map(attr => `${this.quoteIdentifier(attr)}=${this.quoteIdentifier(attr)}`)); + } + + // edge case... but if for some reason there were no valueKeys, and there were also no upsertKeys... then we + // can no longer build the requested query without a syntax error. Let's throw something more graceful here + // so the devs know what the problem is. + if (_.isEmpty(valueKeys)) { + throw new Error('No update values found for ON DUPLICATE KEY UPDATE clause, and no identifier fields could be found to use instead.'); + } onDuplicateKeyUpdate += `${this._dialect.supports.inserts.updateOnDuplicate} ${valueKeys.join(',')}`; } } @@ -286,6 +305,9 @@ class QueryGenerator { tuples.push(`(${values.join(',')})`); } + // `options.updateOnDuplicate` is the list of field names to update if a duplicate key is hit during the insert. It + // contains just the field names. This option is _usually_ explicitly set by the corresponding query-interface + // upsert function. if (this._dialect.supports.inserts.updateOnDuplicate && options.updateOnDuplicate) { if (this._dialect.supports.inserts.updateOnDuplicate == ' ON CONFLICT DO UPDATE SET') { // postgres / sqlite // If no conflict target columns were specified, use the primary key names from options.upsertKeys diff --git a/lib/dialects/mssql/query-generator.js b/lib/dialects/mssql/query-generator.js index 94e643ee0b23..3e44de52da72 100644 --- a/lib/dialects/mssql/query-generator.js +++ b/lib/dialects/mssql/query-generator.js @@ -449,7 +449,7 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator { //IDENTITY_INSERT Condition identityAttrs.forEach(key => { - if (updateValues[key] && updateValues[key] !== null) { + if (insertValues[key] && insertValues[key] !== null) { needIdentityInsertWrapper = true; /* * IDENTITY_INSERT Column Cannot be updated, only inserted @@ -501,16 +501,18 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator { } // Remove the IDENTITY_INSERT Column from update - const updateSnippet = updateKeys.filter(key => !identityAttrs.includes(key)) + const filteredUpdateClauses = updateKeys.filter(key => !identityAttrs.includes(key)) .map(key => { const value = this.escape(updateValues[key]); key = this.quoteIdentifier(key); return `${targetTableAlias}.${key} = ${value}`; - }).join(', '); + }); + const updateSnippet = filteredUpdateClauses.length > 0 ? `WHEN MATCHED THEN UPDATE SET ${filteredUpdateClauses.join(', ')}` : ''; const insertSnippet = `(${insertKeysQuoted}) VALUES(${insertValuesEscaped})`; + let query = `MERGE INTO ${tableNameQuoted} WITH(HOLDLOCK) AS ${targetTableAlias} USING (${sourceTableQuery}) AS ${sourceTableAlias}(${insertKeysQuoted}) ON ${joinCondition}`; - query += ` WHEN MATCHED THEN UPDATE SET ${updateSnippet} WHEN NOT MATCHED THEN INSERT ${insertSnippet} OUTPUT $action, INSERTED.*;`; + query += ` ${updateSnippet} WHEN NOT MATCHED THEN INSERT ${insertSnippet} OUTPUT $action, INSERTED.*;`; if (needIdentityInsertWrapper) { query = `SET IDENTITY_INSERT ${tableNameQuoted} ON; ${query} SET IDENTITY_INSERT ${tableNameQuoted} OFF;`; } diff --git a/lib/dialects/mssql/query.js b/lib/dialects/mssql/query.js index bbead007705a..f35c43d948e7 100644 --- a/lib/dialects/mssql/query.js +++ b/lib/dialects/mssql/query.js @@ -216,6 +216,11 @@ class Query extends AbstractQuery { return data; } if (this.isUpsertQuery()) { + // if this was an upsert and no data came back, that means the record exists, but the update was a noop. + // return the current instance and mark it as an "not an insert". + if (data && data.length === 0) { + return [this.instance || data, false]; + } this.handleInsertQuery(data); return [this.instance || data, data[0].$action === 'INSERT']; } diff --git a/lib/dialects/mysql/query-interface.js b/lib/dialects/mysql/query-interface.js index bcdec9d2e5a7..4c7a43108e70 100644 --- a/lib/dialects/mysql/query-interface.js +++ b/lib/dialects/mysql/query-interface.js @@ -46,6 +46,7 @@ class MySQLQueryInterface extends QueryInterface { options.type = QueryTypes.UPSERT; options.updateOnDuplicate = Object.keys(updateValues); + options.upsertKeys = Object.values(options.model.primaryKeys).map(item => item.field); const model = options.model; const sql = this.queryGenerator.insertQuery(tableName, insertValues, model.rawAttributes, options); diff --git a/lib/dialects/postgres/query.js b/lib/dialects/postgres/query.js index 3640bda38648..8d63110340b2 100644 --- a/lib/dialects/postgres/query.js +++ b/lib/dialects/postgres/query.js @@ -267,7 +267,7 @@ class Query extends AbstractQuery { if (this.instance && this.instance.dataValues) { // If we are creating an instance, and we get no rows, the create failed but did not throw. // This probably means a conflict happened and was ignored, to avoid breaking a transaction. - if (this.isInsertQuery() && rowCount === 0) { + if (this.isInsertQuery() && !this.isUpsertQuery() && rowCount === 0) { throw new sequelizeErrors.EmptyResultError(); } diff --git a/lib/model.js b/lib/model.js index af1f2d29ee16..b904772ccc54 100644 --- a/lib/model.js +++ b/lib/model.js @@ -1672,7 +1672,7 @@ class Model { * @param {object} [options.having] Having options * @param {string} [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only) * @param {boolean|Error} [options.rejectOnEmpty=false] Throws an error when no records found - * @param {boolean} [options.dotNotation] Allows including tables having the same attribute/column names - which have a dot in them. + * @param {boolean} [options.dotNotation] Allows including tables having the same attribute/column names - which have a dot in them. * * @see * {@link Sequelize#query} @@ -2438,7 +2438,7 @@ class Model { * @param {object} values hash of values to upsert * @param {object} [options] upsert options * @param {boolean} [options.validate=true] Run validations before the row is inserted - * @param {Array} [options.fields=Object.keys(this.attributes)] The fields to insert / update. Defaults to all changed fields + * @param {Array} [options.fields=Object.keys(this.attributes)] The fields to update if the record already exists. Defaults to all changed fields. If none of the specified fields are present on the provided `values` object, an insert will still be attempted, but duplicate key conflicts will be ignored. * @param {boolean} [options.hooks=true] Run before / after upsert hooks? * @param {boolean} [options.returning=true] If true, fetches back auto generated values * @param {Transaction} [options.transaction] Transaction to run query under diff --git a/test/integration/model/upsert.test.js b/test/integration/model/upsert.test.js index bfbab5e27e13..10001ce568ca 100644 --- a/test/integration/model/upsert.test.js +++ b/test/integration/model/upsert.test.js @@ -311,6 +311,30 @@ describe(Support.getTestDialectTeaser('Model'), () => { clock.restore(); }); + it('falls back to a noop if no update values are found in the upsert data', async function() { + const User = this.sequelize.define('user', { + username: DataTypes.STRING, + email: { + type: DataTypes.STRING, + field: 'email_address', + defaultValue: 'xxx@yyy.zzz' + } + }, { + // note, timestamps: false is important here because this test is attempting to see what happens + // if there are NO updatable fields (including timestamp values). + timestamps: false + }); + + await User.sync({ force: true }); + // notice how the data does not actually have the update fields. + await User.upsert({ id: 42, username: 'jack' }, { fields: ['email'] }); + await User.upsert({ id: 42, username: 'jill' }, { fields: ['email'] }); + const user = await User.findByPk(42); + // just making sure the user exists, i.e. the insert happened. + expect(user).to.be.ok; + expect(user.username).to.equal('jack'); // second upsert should not have updated username. + }); + it('does not update using default values', async function() { await this.User.create({ id: 42, username: 'john', baz: 'new baz value' }); const user0 = await this.User.findByPk(42);