Skip to content

Commit 4071378

Browse files
slickmbslickmb
authored
fix(upsert): fall back to DO NOTHING if no update key values provided (#13594)
1 parent fc0b19e commit 4071378

7 files changed

Lines changed: 62 additions & 8 deletions

File tree

‎lib/dialects/abstract/query-generator.js‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,14 +180,33 @@ class QueryGenerator {
180180

181181
let onDuplicateKeyUpdate = '';
182182

183+
// `options.updateOnDuplicate` is the list of field names to update if a duplicate key is hit during the insert. It
184+
// contains just the field names. This option is _usually_ explicitly set by the corresponding query-interface
185+
// upsert function.
183186
if (this._dialect.supports.inserts.updateOnDuplicate && options.updateOnDuplicate) {
184187
if (this._dialect.supports.inserts.updateOnDuplicate == ' ON CONFLICT DO UPDATE SET') { // postgres / sqlite
185188
// If no conflict target columns were specified, use the primary key names from options.upsertKeys
186189
const conflictKeys = options.upsertKeys.map(attr => this.quoteIdentifier(attr));
187190
const updateKeys = options.updateOnDuplicate.map(attr => `${this.quoteIdentifier(attr)}=EXCLUDED.${this.quoteIdentifier(attr)}`);
188-
onDuplicateKeyUpdate = ` ON CONFLICT (${conflictKeys.join(',')}) DO UPDATE SET ${updateKeys.join(',')}`;
191+
onDuplicateKeyUpdate = ` ON CONFLICT (${conflictKeys.join(',')})`;
192+
// if update keys are provided, then apply them here. if there are no updateKeys provided, then do not try to
193+
// do an update. Instead, fall back to DO NOTHING.
194+
onDuplicateKeyUpdate += _.isEmpty(updateKeys.length) ? ' DO NOTHING ' : ` DO UPDATE SET ${updateKeys.join(',')}`;
189195
} else {
190196
const valueKeys = options.updateOnDuplicate.map(attr => `${this.quoteIdentifier(attr)}=VALUES(${this.quoteIdentifier(attr)})`);
197+
// the rough equivalent to ON CONFLICT DO NOTHING in mysql, etc is ON DUPLICATE KEY UPDATE id = id
198+
// So, if no update values were provided, fall back to the identifier columns provided in the upsertKeys array.
199+
// This will be the primary key in most cases, but it could be some other constraint.
200+
if (_.isEmpty(valueKeys) && options.upsertKeys) {
201+
valueKeys.push(...options.upsertKeys.map(attr => `${this.quoteIdentifier(attr)}=${this.quoteIdentifier(attr)}`));
202+
}
203+
204+
// edge case... but if for some reason there were no valueKeys, and there were also no upsertKeys... then we
205+
// can no longer build the requested query without a syntax error. Let's throw something more graceful here
206+
// so the devs know what the problem is.
207+
if (_.isEmpty(valueKeys)) {
208+
throw new Error('No update values found for ON DUPLICATE KEY UPDATE clause, and no identifier fields could be found to use instead.');
209+
}
191210
onDuplicateKeyUpdate += `${this._dialect.supports.inserts.updateOnDuplicate} ${valueKeys.join(',')}`;
192211
}
193212
}
@@ -286,6 +305,9 @@ class QueryGenerator {
286305
tuples.push(`(${values.join(',')})`);
287306
}
288307

308+
// `options.updateOnDuplicate` is the list of field names to update if a duplicate key is hit during the insert. It
309+
// contains just the field names. This option is _usually_ explicitly set by the corresponding query-interface
310+
// upsert function.
289311
if (this._dialect.supports.inserts.updateOnDuplicate && options.updateOnDuplicate) {
290312
if (this._dialect.supports.inserts.updateOnDuplicate == ' ON CONFLICT DO UPDATE SET') { // postgres / sqlite
291313
// If no conflict target columns were specified, use the primary key names from options.upsertKeys

‎lib/dialects/mssql/query-generator.js‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,7 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator {
449449

450450
//IDENTITY_INSERT Condition
451451
identityAttrs.forEach(key => {
452-
if (updateValues[key] && updateValues[key] !== null) {
452+
if (insertValues[key] && insertValues[key] !== null) {
453453
needIdentityInsertWrapper = true;
454454
/*
455455
* IDENTITY_INSERT Column Cannot be updated, only inserted
@@ -501,16 +501,18 @@ class MSSQLQueryGenerator extends AbstractQueryGenerator {
501501
}
502502

503503
// Remove the IDENTITY_INSERT Column from update
504-
const updateSnippet = updateKeys.filter(key => !identityAttrs.includes(key))
504+
const filteredUpdateClauses = updateKeys.filter(key => !identityAttrs.includes(key))
505505
.map(key => {
506506
const value = this.escape(updateValues[key]);
507507
key = this.quoteIdentifier(key);
508508
return `${targetTableAlias}.${key} = ${value}`;
509-
}).join(', ');
509+
});
510+
const updateSnippet = filteredUpdateClauses.length > 0 ? `WHEN MATCHED THEN UPDATE SET ${filteredUpdateClauses.join(', ')}` : '';
510511

511512
const insertSnippet = `(${insertKeysQuoted}) VALUES(${insertValuesEscaped})`;
513+
512514
let query = `MERGE INTO ${tableNameQuoted} WITH(HOLDLOCK) AS ${targetTableAlias} USING (${sourceTableQuery}) AS ${sourceTableAlias}(${insertKeysQuoted}) ON ${joinCondition}`;
513-
query += ` WHEN MATCHED THEN UPDATE SET ${updateSnippet} WHEN NOT MATCHED THEN INSERT ${insertSnippet} OUTPUT $action, INSERTED.*;`;
515+
query += ` ${updateSnippet} WHEN NOT MATCHED THEN INSERT ${insertSnippet} OUTPUT $action, INSERTED.*;`;
514516
if (needIdentityInsertWrapper) {
515517
query = `SET IDENTITY_INSERT ${tableNameQuoted} ON; ${query} SET IDENTITY_INSERT ${tableNameQuoted} OFF;`;
516518
}

‎lib/dialects/mssql/query.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,11 @@ class Query extends AbstractQuery {
216216
return data;
217217
}
218218
if (this.isUpsertQuery()) {
219+
// if this was an upsert and no data came back, that means the record exists, but the update was a noop.
220+
// return the current instance and mark it as an "not an insert".
221+
if (data && data.length === 0) {
222+
return [this.instance || data, false];
223+
}
219224
this.handleInsertQuery(data);
220225
return [this.instance || data, data[0].$action === 'INSERT'];
221226
}

‎lib/dialects/mysql/query-interface.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ class MySQLQueryInterface extends QueryInterface {
4646

4747
options.type = QueryTypes.UPSERT;
4848
options.updateOnDuplicate = Object.keys(updateValues);
49+
options.upsertKeys = Object.values(options.model.primaryKeys).map(item => item.field);
4950

5051
const model = options.model;
5152
const sql = this.queryGenerator.insertQuery(tableName, insertValues, model.rawAttributes, options);

‎lib/dialects/postgres/query.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ class Query extends AbstractQuery {
267267
if (this.instance && this.instance.dataValues) {
268268
// If we are creating an instance, and we get no rows, the create failed but did not throw.
269269
// This probably means a conflict happened and was ignored, to avoid breaking a transaction.
270-
if (this.isInsertQuery() && rowCount === 0) {
270+
if (this.isInsertQuery() && !this.isUpsertQuery() && rowCount === 0) {
271271
throw new sequelizeErrors.EmptyResultError();
272272
}
273273

‎lib/model.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1672,7 +1672,7 @@ class Model {
16721672
* @param {object} [options.having] Having options
16731673
* @param {string} [options.searchPath=DEFAULT] An optional parameter to specify the schema search_path (Postgres only)
16741674
* @param {boolean|Error} [options.rejectOnEmpty=false] Throws an error when no records found
1675-
* @param {boolean} [options.dotNotation] Allows including tables having the same attribute/column names - which have a dot in them.
1675+
* @param {boolean} [options.dotNotation] Allows including tables having the same attribute/column names - which have a dot in them.
16761676
*
16771677
* @see
16781678
* {@link Sequelize#query}
@@ -2438,7 +2438,7 @@ class Model {
24382438
* @param {object} values hash of values to upsert
24392439
* @param {object} [options] upsert options
24402440
* @param {boolean} [options.validate=true] Run validations before the row is inserted
2441-
* @param {Array} [options.fields=Object.keys(this.attributes)] The fields to insert / update. Defaults to all changed fields
2441+
* @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.
24422442
* @param {boolean} [options.hooks=true] Run before / after upsert hooks?
24432443
* @param {boolean} [options.returning=true] If true, fetches back auto generated values
24442444
* @param {Transaction} [options.transaction] Transaction to run query under

‎test/integration/model/upsert.test.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,30 @@ describe(Support.getTestDialectTeaser('Model'), () => {
311311
clock.restore();
312312
});
313313

314+
it('falls back to a noop if no update values are found in the upsert data', async function() {
315+
const User = this.sequelize.define('user', {
316+
username: DataTypes.STRING,
317+
email: {
318+
type: DataTypes.STRING,
319+
field: 'email_address',
320+
defaultValue: '[email protected]'
321+
}
322+
}, {
323+
// note, timestamps: false is important here because this test is attempting to see what happens
324+
// if there are NO updatable fields (including timestamp values).
325+
timestamps: false
326+
});
327+
328+
await User.sync({ force: true });
329+
// notice how the data does not actually have the update fields.
330+
await User.upsert({ id: 42, username: 'jack' }, { fields: ['email'] });
331+
await User.upsert({ id: 42, username: 'jill' }, { fields: ['email'] });
332+
const user = await User.findByPk(42);
333+
// just making sure the user exists, i.e. the insert happened.
334+
expect(user).to.be.ok;
335+
expect(user.username).to.equal('jack'); // second upsert should not have updated username.
336+
});
337+
314338
it('does not update using default values', async function() {
315339
await this.User.create({ id: 42, username: 'john', baz: 'new baz value' });
316340
const user0 = await this.User.findByPk(42);

0 commit comments

Comments
 (0)