fix(model): don't alter original scopes when combining them (#10722)

jorgelf · sushantdhiman · commit f2b0aec14c98 · 2019-07-06T13:45:32.000+05:30
diff --git a/lib/model.js b/lib/model.js
@@ -811,6 +811,12 @@ class Model {
         }
       });
     }
+    // If we have a possible object/array to clone, we try it.
+    // Otherwise, we return the original value when it's not undefined,
+    // or the resulting object in that case.
+    if (srcValue) {
+      return Utils.cloneDeep(srcValue, true);
+    }
     return srcValue === undefined ? objValue : srcValue;
   }
 
diff --git a/lib/utils.js b/lib/utils.js
@@ -123,16 +123,17 @@ function formatNamedParameters(sql, parameters, dialect) {
 }
 exports.formatNamedParameters = formatNamedParameters;
 
-function cloneDeep(obj) {
+function cloneDeep(obj, onlyPlain) {
   obj = obj || {};
   return _.cloneDeepWith(obj, elem => {
     // Do not try to customize cloning of arrays or POJOs
     if (Array.isArray(elem) || _.isPlainObject(elem)) {
       return undefined;
     }
 
-    // Don't clone stuff that's an object, but not a plain one - fx example sequelize models and instances
-    if (typeof elem === 'object') {
+    // If we specified to clone only plain objects & arrays, we ignore everyhing else
+    // In any case, don't clone stuff that's an object, but not a plain one - fx example sequelize models and instances
+    if (onlyPlain || typeof elem === 'object') {
       return elem;
     }
 
diff --git a/test/unit/model/scope.test.js b/test/unit/model/scope.test.js
@@ -112,6 +112,10 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       it('should unite attributes with array', () => {
         expect(User.scope('aScope', 'defaultScope')._scope.attributes).to.deep.equal({ exclude: ['value', 'password'] });
       });
+
+      it('should not modify the original scopes when merging them', () => {
+        expect(User.scope('defaultScope', 'aScope').options.defaultScope.attributes).to.deep.equal({ exclude: ['password'] });
+      });
     });
 
     it('defaultScope should be an empty object if not overridden', () => {

Original file line number	Diff line number	Diff line change
`@@ -811,6 +811,12 @@ class Model {`
`811`	`811`	`}`
`812`	`812`	`});`
`813`	`813`	`}`
	`814`	`+ // If we have a possible object/array to clone, we try it.`
	`815`	`+ // Otherwise, we return the original value when it's not undefined,`
	`816`	`+ // or the resulting object in that case.`
	`817`	`+ if (srcValue) {`
	`818`	`+ return Utils.cloneDeep(srcValue, true);`
	`819`	`+ }`
`814`	`820`	`return srcValue === undefined ? objValue : srcValue;`
`815`	`821`	`}`
`816`	`822`
Original file line number	Diff line number	Diff line change
`@@ -123,16 +123,17 @@ function formatNamedParameters(sql, parameters, dialect) {`
`123`	`123`	`}`
`124`	`124`	`exports.formatNamedParameters = formatNamedParameters;`
`125`	`125`
`126`		`-function cloneDeep(obj) {`
	`126`	`+function cloneDeep(obj, onlyPlain) {`
`127`	`127`	`obj = obj \|\| {};`
`128`	`128`	`return _.cloneDeepWith(obj, elem => {`
`129`	`129`	`// Do not try to customize cloning of arrays or POJOs`
`130`	`130`	`if (Array.isArray(elem) \|\| _.isPlainObject(elem)) {`
`131`	`131`	`return undefined;`
`132`	`132`	`}`
`133`	`133`
`134`		`- // Don't clone stuff that's an object, but not a plain one - fx example sequelize models and instances`
`135`		`- if (typeof elem === 'object') {`
	`134`	`+ // If we specified to clone only plain objects & arrays, we ignore everyhing else`
	`135`	`+ // In any case, don't clone stuff that's an object, but not a plain one - fx example sequelize models and instances`
	`136`	`+ if (onlyPlain \|\| typeof elem === 'object') {`
`136`	`137`	`return elem;`
`137`	`138`	`}`
`138`	`139`