feat: merge scope where conditions using the and operator (#14110)

BREAKING CHANGE: the `where` property of a scope is now merged using `Op.and`

src/model.js

@@ -861,13 +861,7 @@ class Model {
     }
 
     if (['where', 'having'].includes(key)) {
-      if (srcValue instanceof Utils.SequelizeMethod) {
-        srcValue = { [Op.and]: srcValue };
-      }
-
-      if (_.isPlainObject(objValue) && _.isPlainObject(srcValue)) {
-        return Object.assign(objValue, srcValue);
-      }
+      return combineWheresWithAnd(objValue, srcValue);
     } else if (key === 'attributes' && _.isPlainObject(objValue) && _.isPlainObject(srcValue)) {
       return _.assignWith(objValue, srcValue, (objValue, srcValue) => {
         if (Array.isArray(objValue) && Array.isArray(srcValue)) {
@@ -4765,6 +4759,58 @@ class Model {
   static belongsTo(target, options) {}
 }
 
+/**
+ * Unpacks an object that only contains a single Op.and key to the value of Op.and
+ *
+ * Internal method used by {@link combineWheresWithAnd}
+ *
+ * @param {WhereOptions} where The object to unpack
+ * @example `{ [Op.and]: [a, b] }` becomes `[a, b]`
+ * @example `{ [Op.and]: { key: val } }` becomes `{ key: val }`
+ * @example `{ [Op.or]: [a, b] }` remains as `{ [Op.or]: [a, b] }`
+ * @example `{ [Op.and]: [a, b], key: c }` remains as `{ [Op.and]: [a, b], key: c }`
+ * @private
+ */
+function unpackAnd(where) {
+  if (!_.isObject(where)) {
+    return where;
+  }
+
+  const keys = Utils.getComplexKeys(where);
+
+  // object is empty, remove it.
+  if (keys.length === 0) {
+    return;
+  }
+
+  // we have more than just Op.and, keep as-is
+  if (keys.length !== 1 || keys[0] !== Op.and) {
+    return where;
+  }
+
+  const andParts = where[Op.and];
+
+  return andParts;
+}
+
+function combineWheresWithAnd(whereA, whereB) {
+  const unpackedA = unpackAnd(whereA);
+
+  if (unpackedA === undefined) {
+    return whereB;
+  }
+
+  const unpackedB = unpackAnd(whereB);
+
+  if (unpackedB === undefined) {
+    return whereA;
+  }
+
+  return {
+    [Op.and]: [unpackedA, unpackedB].flat(),
+  };
+}
+
 Object.assign(Model, associationsMixin);
 Hooks.applyTo(Model, true);
 

test/integration/model/scope/destroy.test.js

@@ -54,14 +54,6 @@ describe(Support.getTestDialectTeaser('Model'), () => {
         expect(users[1].get('username')).to.equal('fred');
       });
 
-      it('should be able to override default scope', async function () {
-        await this.ScopeMe.destroy({ where: { access_level: { [Op.lt]: 5 } } });
-        const users = await this.ScopeMe.unscoped().findAll();
-        expect(users).to.have.length(2);
-        expect(users[0].get('username')).to.equal('tobi');
-        expect(users[1].get('username')).to.equal('dan');
-      });
-
       it('should be able to unscope destroy', async function () {
         await this.ScopeMe.unscoped().destroy({ where: {} });
         await expect(this.ScopeMe.unscoped().findAll()).to.eventually.have.length(0);
@@ -83,6 +75,15 @@ describe(Support.getTestDialectTeaser('Model'), () => {
         expect(users[2].get('username')).to.equal('fred');
       });
 
+      it('should be able to merge scopes with similar where', async function () {
+        await this.ScopeMe.scope('defaultScope', 'lowAccess').destroy();
+        const users = await this.ScopeMe.unscoped().findAll();
+        expect(users).to.have.length(3);
+        expect(users[0].get('username')).to.equal('tony');
+        expect(users[1].get('username')).to.equal('tobi');
+        expect(users[2].get('username')).to.equal('fred');
+      });
+
       it('should work with empty where', async function () {
         await this.ScopeMe.scope('lowAccess').destroy();
         const users = await this.ScopeMe.unscoped().findAll();

test/integration/model/scope/find.test.js

@@ -87,6 +87,13 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       expect(['tony', 'fred'].includes(users[1].username)).to.be.true;
     });
 
+    it('should be able to combine multiple scopes', async function () {
+      const users = await this.ScopeMe.scope('defaultScope', 'highValue').findAll();
+      expect(users).to.have.length(2);
+      expect(['tobi', 'dan'].includes(users[0].username)).to.be.true;
+      expect(['tobi', 'dan'].includes(users[1].username)).to.be.true;
+    });
+
     it('should be able to use a defaultScope if declared', async function () {
       const users = await this.ScopeMe.findAll();
       expect(users).to.have.length(2);

test/integration/model/scope/findAndCountAll.test.js

@@ -82,6 +82,13 @@ describe(Support.getTestDialectTeaser('Model'), () => {
         expect(result.count).to.equal(1);
       });
 
+      it('should be able to merge multiple scopes', async function () {
+        const result = await this.ScopeMe.scope('defaultScope', 'lowAccess')
+          .findAndCountAll();
+
+        expect(result.count).to.equal(1);
+      });
+
       it('should ignore the order option if it is found within the scope', async function () {
         const result = await this.ScopeMe.scope('withOrder').findAndCountAll();
         expect(result.count).to.equal(4);

test/integration/model/scope/update.test.js

@@ -54,14 +54,6 @@ describe(Support.getTestDialectTeaser('Model'), () => {
         expect(users[1].get('email')).to.equal('dan@sequelizejs.com');
       });
 
-      it('should be able to override default scope', async function () {
-        await this.ScopeMe.update({ username: 'ruben' }, { where: { access_level: { [Op.lt]: 5 } } });
-        const users = await this.ScopeMe.unscoped().findAll({ where: { username: 'ruben' } });
-        expect(users).to.have.length(2);
-        expect(users[0].get('email')).to.equal('tony@sequelizejs.com');
-        expect(users[1].get('email')).to.equal('fred@foobar.com');
-      });
-
       it('should be able to unscope destroy', async function () {
         await this.ScopeMe.unscoped().update({ username: 'ruben' }, { where: {} });
         const rubens = await this.ScopeMe.unscoped().findAll();
@@ -82,6 +74,13 @@ describe(Support.getTestDialectTeaser('Model'), () => {
         expect(users[0].get('email')).to.equal('dan@sequelizejs.com');
       });
 
+      it('should be able to merge scopes with similar where', async function () {
+        await this.ScopeMe.scope('defaultScope', 'lowAccess').update({ username: 'fakeName' });
+        const users = await this.ScopeMe.unscoped().findAll({ where: { username: 'fakeName' } });
+        expect(users).to.have.length(1);
+        expect(users[0].get('email')).to.equal('dan@sequelizejs.com');
+      });
+
       it('should work with empty where', async function () {
         await this.ScopeMe.scope('lowAccess').update({
           username: 'ruby',

test/support.js

@@ -2,7 +2,7 @@
 
 const fs = require('fs');
 const path = require('path');
-const { isDeepStrictEqual } = require('util');
+const { inspect, isDeepStrictEqual } = require('util');
 const _ = require('lodash');
 
 const Sequelize = require('@sequelize/core');
@@ -18,6 +18,12 @@ chai.use(require('chai-datetime'));
 chai.use(require('chai-as-promised'));
 chai.use(require('sinon-chai'));
 
+// Using util.inspect to correctly assert objects with symbols
+// Because expect.deep.equal does not test non iterator keys such as symbols (https://github.com/chaijs/chai/issues/1054)
+chai.Assertion.addMethod('deepEqual', function (expected, depth = 5) {
+  expect(inspect(this._obj, { depth })).to.deep.equal(inspect(expected, { depth }));
+});
+
 chai.config.includeStack = true;
 chai.should();
 

test/unit/model/include.test.js

@@ -4,7 +4,7 @@ const chai = require('chai');
 
 const expect = chai.expect;
 const Support   = require('../support');
-const Sequelize = require('@sequelize/core');
+const { Sequelize, Op, Utils } = require('@sequelize/core');
 
 const current   = Support.sequelize;
 
@@ -207,7 +207,10 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           include: [{ where: { active: false }, model: this.Project.scope('that') }],
         });
 
-        expect(options.include[0]).to.have.property('where').which.deep.equals({ active: false, that: false });
+        // TODO [chai>5]: simplify once '.deep.equals' includes support for symbols (https://github.com/chaijs/chai/issues/1054)
+        expect(options.include[0]).to.have.property('where');
+        expect(Utils.getComplexKeys(options.include[0].where)).to.deep.equal([Op.and]);
+        expect(options.include[0].where[Op.and]).to.deep.equal([{ that: false }, { active: false }]);
       });
 
       it('add the where from a scoped associated model', function () {