feat: restrict instance methods if no primary key

src/dialects/abstract/query-interface.js

@@ -885,6 +885,10 @@ export class QueryInterface {
    *
    * @returns {Promise<boolean,?number>} Resolves an array with <created, primaryKey>
    */
+  // Note: "where" is only used by DB2 and MSSQL. This is because these dialects do not propose any "ON CONFLICT UPDATE" mechanisms
+  // The UPSERT pattern in SQL server requires providing a WHERE clause
+  // TODO: the user should be able to configure the WHERE clause for upsert instead of the current default which
+  //  is using the primary keys.
   async upsert(tableName, insertValues, updateValues, where, options) {
     if (options?.bind) {
       assertNoReservedBind(options.bind);

src/model.js

@@ -901,7 +901,6 @@ Specify a different name for either index to resolve this issue.`);
       underscored: false,
       paranoid: false,
       rejectOnEmpty: false,
-      whereCollection: null,
       schema: '',
       schemaDelimiter: '',
       defaultScope: {},
@@ -1836,9 +1835,6 @@ Specify a different name for either index to resolve this issue.`);
       options.originalAttributes = this._injectDependentVirtualAttributes(options.attributes);
     }
 
-    // whereCollection is used for non-primary key updates
-    this.options.whereCollection = options.where || null;
-
     Utils.mapFinderOptions(options, this);
 
     options = this._paranoidClause(this, options);
@@ -2577,6 +2573,8 @@ Specify a different name for either index to resolve this issue.`);
     // Db2 does not allow NULL values for unique columns.
     // Add dummy values if not provided by test case or user.
     if (this.sequelize.options.dialect === 'db2') {
+      // TODO: remove. This is fishy and is going to be a source of bugs (because it replaces null values with arbitrary values that could be actual data).
+      //  If DB2 doesn't support NULL in unique columns, then it should error if the user tries to insert NULL in one.
       this.uniqno = this.sequelize.dialect.queryGenerator.addUniqueFields(
         insertValues, this.rawAttributes, this.uniqno,
       );
@@ -2593,15 +2591,23 @@ Specify a different name for either index to resolve this issue.`);
       await this.hooks.runAsync('beforeUpsert', values, options);
     }
 
-    const result = await this.queryInterface.upsert(this.getTableName(options), insertValues, updateValues, instance.where(), options);
+    const result = await this.queryInterface.upsert(
+      this.getTableName(options),
+      insertValues,
+      updateValues,
+      // TODO: this is only used by DB2 & MSSQL, as these dialects require a WHERE clause in their UPSERT implementation.
+      //  but the user should be able to specify a WHERE clause themselves (because we can't perfectly include all UNIQUE constraints in our implementation)
+      //  there is also some incoherence in our implementation: This "where" returns the Primary Key constraint, but all other unique constraints
+      //  are added inside of QueryInterface. Everything should be done inside of QueryInterface instead.
+      instance.where(false, true) ?? {},
+      options,
+    );
 
     const [record] = result;
     record.isNewRecord = false;
 
     if (options.hooks) {
       await this.hooks.runAsync('afterUpsert', result, options);
-
-      return result;
     }
 
     return result;
@@ -3567,21 +3573,42 @@ Instead of specifying a Model, either:
   }
 
   /**
-   * Returns an object representing the query for this instance, use with `options.where`
+   * Returns a Where Object that can be used to uniquely select this instance, using the instance's primary keys.
    *
    * @param {boolean} [checkVersion=false] include version attribute in where hash
+   * @param {boolean} [nullIfImpossible=false] return null instead of throwing an error if the instance is missing its primary keys and therefore no Where object can be built.
    *
    * @returns {object}
    */
-  where(checkVersion) {
-    const where = this.constructor.primaryKeyAttributes.reduce((result, attribute) => {
-      result[attribute] = this.get(attribute, { raw: true });
+  where(checkVersion, nullIfImpossible) {
+    if (this.constructor.primaryKeyAttributes.length === 0) {
+      if (nullIfImpossible) {
+        return null;
+      }
 
-      return result;
-    }, {});
+      throw new Error(
+        `This model instance method needs to be able to identify the entity in a stable way, but the model does not have a primary key attribute definition. Either add a primary key to this model, or use one of the following alternatives:
+
+- instance methods "save", "update", "decrement", "increment": Use the static "update" method instead.
+- instance method "reload": Use the static "findOne" method instead.
+- instance methods "destroy" and "restore": use the static "destroy" and "restore" methods instead.
+        `.trim(),
+      );
+    }
+
+    const where = {};
+
+    for (const attribute of this.constructor.primaryKeyAttributes) {
+      const attrVal = this.get(attribute, { raw: true });
+      if (attrVal == null) {
+        if (nullIfImpossible) {
+          return null;
+        }
 
-    if (_.size(where) === 0) {
-      return this.constructor.options.whereCollection;
+        throw new TypeError(`This model instance method needs to be able to identify the entity in a stable way, but this model instance is missing the value of its primary key "${attribute}". Make sure that attribute was not excluded when retrieving the model from the database.`);
+      }
+
+      where[attribute] = attrVal;
     }
 
     const versionAttr = this.constructor._versionAttribute;
@@ -4160,20 +4187,13 @@ Instead of specifying a Model, either:
       return this;
     }
 
-    if (!this.changed() && !this.isNewRecord) {
-      return this;
-    }
-
     const versionFieldName = _.get(this.constructor.rawAttributes[versionAttr], 'field') || versionAttr;
     const values = Utils.mapValueFieldNames(this.dataValues, options.fields, this.constructor);
     let query;
     let args;
     let where;
 
-    if (this.isNewRecord) {
-      query = 'insert';
-      args = [this, this.constructor.getTableName(options), values, options];
-    } else {
+    if (!this.isNewRecord) {
       where = this.where(true);
       if (versionAttr) {
         values[versionFieldName] = Number.parseInt(values[versionFieldName], 10) + 1;
@@ -4183,6 +4203,15 @@ Instead of specifying a Model, either:
       args = [this, this.constructor.getTableName(options), values, where, options];
     }
 
+    if (!this.changed() && !this.isNewRecord) {
+      return this;
+    }
+
+    if (this.isNewRecord) {
+      query = 'insert';
+      args = [this, this.constructor.getTableName(options), values, options];
+    }
+
     const [result, rowsUpdated] = await this.constructor.queryInterface[query](...args);
 
     if (versionAttr) {
@@ -4349,6 +4378,10 @@ Instead of specifying a Model, either:
 
     const changedBefore = this.changed() || [];
 
+    if (this.isNewRecord) {
+      throw new Error('You attempted to update an instance that is not persisted.');
+    }
+
     options = options || {};
     if (Array.isArray(options)) {
       options = { fields: options };

test/integration/instance/decrement.test.js

@@ -189,5 +189,13 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
 
       await expect(User.findByPk(1)).to.eventually.have.property('updatedAt').equalTime(oldDate);
     });
+
+    it('is disallowed if no primary key is present', async function () {
+      const Foo = this.sequelize.define('Foo', {}, { noPrimaryKey: true });
+      await Foo.sync({ force: true });
+
+      const instance = await Foo.create({});
+      await expect(instance.decrement()).to.be.rejectedWith('but the model does not have a primary key attribute definition.');
+    });
   });
 });

test/integration/instance/destroy.test.js

@@ -248,6 +248,14 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
       expect(user.username).to.equal('bar');
     });
 
+    it('is disallowed if no primary key is present', async function () {
+      const Foo = this.sequelize.define('Foo', {}, { noPrimaryKey: true });
+      await Foo.sync({ force: true });
+
+      const instance = await Foo.create({});
+      await expect(instance.destroy()).to.be.rejectedWith('but the model does not have a primary key attribute definition.');
+    });
+
     it('allows sql logging of delete statements', async function () {
       const UserDelete = this.sequelize.define('UserDelete', {
         name: DataTypes.STRING,

test/integration/instance/increment.test.js

@@ -118,6 +118,14 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
       expect(user2.aNumber).to.be.equal(1);
     });
 
+    it('is disallowed if no primary key is present', async function () {
+      const Foo = this.sequelize.define('Foo', {}, { noPrimaryKey: true });
+      await Foo.sync({ force: true });
+
+      const instance = await Foo.create({});
+      await expect(instance.increment()).to.be.rejectedWith('but the model does not have a primary key attribute definition.');
+    });
+
     it('should still work right with other concurrent updates', async function () {
       const user1 = await this.User.findByPk(1);
       // Select the user again (simulating a concurrent query)

test/integration/instance/reload.test.js

@@ -107,6 +107,14 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
       expect(updatedUser.username).to.equal('Doe John');
     });
 
+    it('is disallowed if no primary key is present', async function () {
+      const Foo = this.sequelize.define('Foo', {}, { noPrimaryKey: true });
+      await Foo.sync({ force: true });
+
+      const instance = await Foo.create({});
+      await expect(instance.reload()).to.be.rejectedWith('but the model does not have a primary key attribute definition.');
+    });
+
     it('should support updating a subset of attributes', async function () {
       const user1 = await this.User.create({
         aNumber: 1,

test/integration/instance/restore.test.ts

@@ -0,0 +1,15 @@
+import { DataTypes } from '@sequelize/core';
+import { expect } from 'chai';
+import { sequelize } from '../support';
+
+describe('Model#restore', () => {
+  it('is disallowed if no primary key is present', async () => {
+    const User = sequelize.define('User', {
+      name: { type: DataTypes.STRING },
+    }, { noPrimaryKey: true, paranoid: true });
+    await User.sync({ force: true });
+
+    const instance = User.build({}, { isNewRecord: false });
+    await expect(instance.restore()).to.be.rejectedWith('but the model does not have a primary key attribute definition.');
+  });
+});

test/integration/instance/save.test.js

@@ -129,6 +129,14 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
       });
     });
 
+    it('is disallowed if no primary key is present', async function () {
+      const Foo = this.sequelize.define('Foo', {});
+      await Foo.sync({ force: true });
+
+      const instance = await Foo.build({}, { isNewRecord: false });
+      await expect(instance.save()).to.be.rejectedWith('You attempted to save an instance with no primary key');
+    });
+
     describe('hooks', () => {
       it('should update attributes added in hooks when default fields are used', async function () {
         const User = this.sequelize.define(`User${Support.rand()}`, {

test/integration/instance/update.test.js

@@ -174,14 +174,24 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
     });
 
     it('should save attributes affected by setters', async function () {
-      const user = this.User.build();
+      const user = await this.User.create();
       await user.update({ validateSideEffect: 5 });
       expect(user.validateSideEffect).to.be.equal(5);
       await user.reload();
       expect(user.validateSideAffected).to.be.equal(10);
       expect(user.validateSideEffect).not.to.be.ok;
     });
 
+    it('fails if the update was made to a new record which is not persisted', async function () {
+      const Foo = this.sequelize.define('Foo', {
+        name: { type: DataTypes.STRING },
+      }, { noPrimaryKey: true });
+      await Foo.sync({ force: true });
+
+      const instance = Foo.build({ name: 'FooBar' }, { isNewRecord: true });
+      await expect(instance.update()).to.be.rejectedWith('You attempted to update an instance that is not persisted.');
+    });
+
     describe('hooks', () => {
       it('should update attributes added in hooks when default fields are used', async function () {
         const User = this.sequelize.define(`User${Support.rand()}`, {

test/unit/instance/decrement.test.js

@@ -11,6 +11,13 @@ const sinon     = require('sinon');
 
 describe(Support.getTestDialectTeaser('Instance'), () => {
   describe('decrement', () => {
+    it('is not allowed if the instance does not have a primary key defined', async () => {
+      const User = current.define('User', {});
+      const instance = User.build({});
+
+      await expect(instance.decrement()).to.be.rejectedWith('but this model instance is missing the value of its primary key');
+    });
+
     describe('options tests', () => {
       let stub; let instance;
       const Model = current.define('User', {

test/unit/instance/destroy.test.js

@@ -11,6 +11,13 @@ const sinon     = require('sinon');
 
 describe(Support.getTestDialectTeaser('Instance'), () => {
   describe('destroy', () => {
+    it('is not allowed if the instance does not have a primary key defined', async () => {
+      const User = current.define('User', {});
+      const instance = User.build({});
+
+      await expect(instance.destroy()).to.be.rejectedWith('but this model instance is missing the value of its primary key');
+    });
+
     describe('options tests', () => {
       let stub; let instance;
       const Model = current.define('User', {

test/unit/instance/increment.test.js

@@ -11,6 +11,13 @@ const sinon     = require('sinon');
 
 describe(Support.getTestDialectTeaser('Instance'), () => {
   describe('increment', () => {
+    it('is not allowed if the instance does not have a primary key defined', async () => {
+      const User = current.define('User', {});
+      const instance = User.build({});
+
+      await expect(instance.increment()).to.be.rejectedWith('but this model instance is missing the value of its primary key');
+    });
+
     describe('options tests', () => {
       let stub; let instance;
       const Model = current.define('User', {

test/unit/instance/reload.test.js

@@ -11,6 +11,13 @@ const sinon     = require('sinon');
 
 describe(Support.getTestDialectTeaser('Instance'), () => {
   describe('reload', () => {
+    it('is not allowed if the instance does not have a primary key defined', async () => {
+      const User = current.define('User', {});
+      const instance = User.build({});
+
+      await expect(instance.reload()).to.be.rejectedWith('but this model instance is missing the value of its primary key');
+    });
+
     describe('options tests', () => {
       let stub; let instance;
       const Model = current.define('User', {

test/unit/instance/restore.test.js

@@ -11,6 +11,13 @@ const sinon     = require('sinon');
 
 describe(Support.getTestDialectTeaser('Instance'), () => {
   describe('restore', () => {
+    it('is not allowed if the instance does not have a primary key defined', async () => {
+      const User = current.define('User', {}, { paranoid: true });
+      const instance = User.build({}, { isNewRecord: false });
+
+      await expect(instance.restore()).to.be.rejectedWith('save an instance with no primary key, this is not allowed since it would');
+    });
+
     describe('options tests', () => {
       let stub; let instance;
       const Model = current.define('User', {

test/unit/instance/save.test.js

@@ -11,11 +11,11 @@ const sinon     = require('sinon');
 
 describe(Support.getTestDialectTeaser('Instance'), () => {
   describe('save', () => {
-    it('should disallow saves if no primary key values is present', async () => {
-      const Model = current.define('User', {});
-      const instance = Model.build({}, { isNewRecord: false });
+    it('is not allowed if the instance does not have a primary key defined', async () => {
+      const User = current.define('User', {});
+      const instance = User.build({}, { isNewRecord: false });
 
-      await expect(instance.save()).to.be.rejected;
+      await expect(instance.save()).to.be.rejectedWith('You attempted to save an instance with no primary key, this is not allowed since it would result in a global update');
     });
 
     describe('options tests', () => {

test/unit/instance/update.test.ts

@@ -0,0 +1,23 @@
+import { DataTypes } from '@sequelize/core';
+import { expect } from 'chai';
+import { sequelize } from '../../support';
+
+describe('Model#update', () => {
+  it('is not allowed if the primary key is not defined', async () => {
+    const User = sequelize.define('User', {
+      name: { type: DataTypes.STRING },
+    });
+    const instance = User.build({}, { isNewRecord: false });
+
+    await expect(instance.update({ name: 'john' })).to.be.rejectedWith('You attempted to save an instance with no primary key, this is not allowed since');
+  });
+
+  it('is not allowed if the primary key is not defined and is a newly created record', async () => {
+    const User = sequelize.define('User', {
+      name: { type: DataTypes.STRING },
+    });
+    const instance = User.build({}, { isNewRecord: true });
+
+    await expect(instance.update({ name: 'john' })).to.be.rejectedWith('You attempted to update an instance that is not persisted.');
+  });
+});