fix(upsert): fall back to DO NOTHING if no update key values provided (…

…sequelize#13594)

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

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;`;
     }

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'];
     }

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);

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();
         }
 

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

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);