feat(scopes): use AND merging scope strategy as default (and unique)

src/model.d.ts

@@ -1579,16 +1579,6 @@ export interface ModelOptions<M extends Model = Model> {
    * @default false
    */
   version?: boolean | string;
-
-  /**
-   * Enable where scopes merging using Op.and operator.
-   * When enabled, scopes containing the same attribute in a where clause will be grouped with the Op.and operator
-   * For instance merging scopes containing `{ where: { myField: 1 }}` and `{ where: { myField: 2 }}` will result in
-   * `{ where: { [Op.and]: [{ myField: 1 }, { myField: 2 }] } }`.
-   *
-   * @default false
-   */
-  mergeWhereScopesWithAndOperator?: boolean;
 }
 
 /**

src/model.js

@@ -855,32 +855,26 @@ class Model {
     return args[0];
   }
 
-  static _mergeOverlappingAttributeConditions(objValue, srcValue) {
-    return {
-      [Op.and]: [
-        ...((objValue && objValue[Op.and]) || []),
-        srcValue,
-      ],
-    };
-  }
-
   static _mergeFunction(objValue, srcValue, key) {
     if (Array.isArray(objValue) && Array.isArray(srcValue)) {
       return _.union(objValue, srcValue);
     }
 
     if (['where', 'having'].includes(key)) {
-      if (srcValue instanceof Utils.SequelizeMethod) {
-        srcValue = { [Op.and]: srcValue };
+      let objKeysToKeep;
+      if (objValue) {
+        const objKeys = Object.getOwnPropertyNames(objValue).concat(Object.getOwnPropertySymbols(objValue));
+        objKeysToKeep = _.filter(objKeys, key => key !== Op.and);
       }
 
-      if (this.options?.mergeWhereScopesWithAndOperator) {
-        return this._mergeOverlappingAttributeConditions(objValue, srcValue);
-      }
+      return {
+        [Op.and]: _.compact([
+          ...(objValue?.[Op.and] ?? []),
+          !_.isEmpty(objKeysToKeep) && _.pick(objValue, objKeysToKeep),
+          srcValue,
+        ]),
+      };
 
-      if (_.isPlainObject(objValue) && _.isPlainObject(srcValue)) {
-        return Object.assign(objValue, srcValue);
-      }
     } else if (key === 'attributes' && _.isPlainObject(objValue) && _.isPlainObject(srcValue)) {
       return _.assignWith(objValue, srcValue, (objValue, srcValue) => {
         if (Array.isArray(objValue) && Array.isArray(srcValue)) {
@@ -900,7 +894,7 @@ class Model {
   }
 
   static _assignOptions(...args) {
-    return this._baseMerge(...args, this._mergeFunction.bind(this));
+    return this._baseMerge(...args, this._mergeFunction);
   }
 
   static _defaultsOptions(target, opts) {
@@ -997,7 +991,6 @@ class Model {
    * @param {string}                  [options.initialAutoIncrement] Set the initial AUTO_INCREMENT value for the table in MySQL.
    * @param {object}                  [options.hooks] An object of hook function that are called before and after certain lifecycle events. The possible hooks are: beforeValidate, afterValidate, validationFailed, beforeBulkCreate, beforeBulkDestroy, beforeBulkUpdate, beforeCreate, beforeDestroy, beforeUpdate, afterCreate, beforeSave, afterDestroy, afterUpdate, afterBulkCreate, afterSave, afterBulkDestroy and afterBulkUpdate. See Hooks for more information about hook functions and their signatures. Each property can either be a function, or an array of functions.
    * @param {object}                  [options.validate] An object of model wide validations. Validations have access to all model values via `this`. If the validator function takes an argument, it is assumed to be async, and is called with a callback that accepts an optional error.
-   * @param {boolean}                 [options.mergeWhereScopesWithAndOperator] Merge `where` properties of scopes together by adding `Op.and` at the top-most level.
    *
    * @returns {Model}
    */
@@ -1047,7 +1040,6 @@ class Model {
       defaultScope: {},
       scopes: {},
       indexes: [],
-      mergeWhereScopesWithAndOperator: false,
       ...options,
     };
 

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

@@ -83,6 +83,13 @@ describe(Support.getTestDialectTeaser('Model'), () => {
     it('should be able to combine scope and findAll where clauses', async function () {
       const users = await this.ScopeMe.scope({ where: { parent_id: 1 } }).findAll({ where: { access_level: 3 } });
       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 combine multiple scopes', async function () {
+      const users = await this.ScopeMe.scope('defaultScope', 'highValue').findAll();
+      expect(users).to.have.length(2);
       expect(['tony', 'fred'].includes(users[0].username)).to.be.true;
       expect(['tony', 'fred'].includes(users[1].username)).to.be.true;
     });

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/unit/model/include.test.js

@@ -1,14 +1,23 @@
 'use strict';
 
 const chai = require('chai');
+const util = require('util');
 
 const expect = chai.expect;
 const Support   = require('../support');
 const Sequelize = require('sequelize');
 
+const Op = Sequelize.Op;
+
 const current   = Support.sequelize;
 
 describe(Support.getTestDialectTeaser('Model'), () => {
+  // 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('deepEquals', function (expected, depth = 5) {
+    expect(util.inspect(this._obj, { depth })).to.deep.equal(util.inspect(expected, { depth }));
+  });
+
   describe('all', () => {
     const Referral = current.define('referal');
 
@@ -179,7 +188,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           include: [{ model: this.Project }],
         });
 
-        expect(options.include[0]).to.have.property('where').which.deep.equals({ active: true });
+        expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ active: true }] });
       });
 
       it('adds the where from a scoped model', function () {
@@ -188,7 +197,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           include: [{ model: this.Project.scope('that') }],
         });
 
-        expect(options.include[0]).to.have.property('where').which.deep.equals({ that: false });
+        expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ that: false }] });
         expect(options.include[0]).to.have.property('limit').which.equals(12);
       });
 
@@ -198,7 +207,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           include: [{ model: this.Project.scope('attr') }],
         });
 
-        expect(options.include[0]).to.have.property('attributes').which.deep.equals(['baz']);
+        expect(options.include[0]).to.have.property('attributes').which.deepEquals(['baz']);
       });
 
       it('merges where with the where from a scoped model', function () {
@@ -207,7 +216,7 @@ 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 });
+        expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ that: false }, { active: false }] });
       });
 
       it('add the where from a scoped associated model', function () {
@@ -216,7 +225,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           include: [{ model: this.Project, as: 'thisProject' }],
         });
 
-        expect(options.include[0]).to.have.property('where').which.deep.equals({ this: true });
+        expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ this: true }] });
       });
 
       it('handles a scope with an aliased column (.field)', function () {
@@ -225,7 +234,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           include: [{ model: this.Project.scope('foobar') }],
         });
 
-        expect(options.include[0]).to.have.property('where').which.deep.equals({ foo: 42 });
+        expect(options.include[0]).to.have.property('where').which.deepEquals({ [Op.and]: [{ foo: 42 }] });
       });
     });