fix(model): don't alter original scopes when combining them #10722
Conversation
Model was previously altering the original scopes when merging them for queries. This happened because _mergeFunction was in some cases returning references to attributes of the original scope objects instead of cloning them. Also added a new test to prevent this problem from happening again. Closes sequelize#10696
|
@jorgelf - Nice. Thanks also for digging into the |
|
You are probably right, too many flavours of cloning. Maybe this new cloning method can be merged with the already existing 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;
}
// 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;
}
// Preserve special data-types like `fn` across clones. _.get() is used for checking up the prototype chain
if (elem && typeof elem.clone === 'function') {
return elem.clone();
}
});
}Tests are still passing after this change, so nothing seems to break with it. If it looks fine to you, I can just commit those changes to this branch, or make a new PR if you prefer it that way. I probably wouldn't do any changes regarding usage of Lodash's And about the Travis build fail, those tests don't give me any trouble in my machine, so not sure why they failed. |
|
@jorgelf - Sorry to take long. I fully agree with you. Merging with the already existing And don't worry about the travis build fail, it happens. |
Right now it's only used in one place, but it might prove useful in the future, so we now provide a simple way for deep cloning only plain objects. See sequelize#10722 & sequelize#10696
Codecov Report
@@ Coverage Diff @@
## master #10722 +/- ##
===========================================
+ Coverage 74.54% 96.35% +21.81%
===========================================
Files 85 94 +9
Lines 8186 9010 +824
===========================================
+ Hits 6102 8682 +2580
+ Misses 2084 328 -1756
Continue to review full report at Codecov.
|
|
Great, thanks for the comment, @papb. A new commit has been added doing that small refactoring. If something doesn't look good to you, just tell me. |
|
🎉 This PR is included in version 5.9.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…ndle-deadlock * 'master' of github.com:sequelize/sequelize: (22 commits) docs(migrations): use timestamps with seed (sequelize#11160) test: remove redundant test (sequelize#11156) fix(types): add literal to possible where options (sequelize#10990) fix(model): don't alter original scopes when combining them (sequelize#10722) fix(types): relax order typing (sequelize#10802) fix(types): add string to Includeable (sequelize#11003) docs(models-definition): correct spelling mistakes (sequelize#11147) fix(types): silent option for update (sequelize#11115) fix: update sequelize-pool (sequelize#11134) feat(hooks): beforeDisconnect / afterDisconnect (sequelize#11117) refactor: remove unused _templateSettings refactor(query-generation): remove lodash string templates (sequelize#11122) docs: improve datatype docs docs: explain defaults/where behavior for find/create (sequelize#11069) build: remove test*.js from .gitignore (sequelize#11108) docs(data-types): extending types fix(sequelize.close): update sequelize-pool (sequelize#11101) build: update dependencies (sequelize#11099) docs(migrations): foreign key example (sequelize#11097) fix(mariadb): properly escape json path key (sequelize#11089) ...
|
🎉 This PR is included in version 7.0.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
Model was previously altering the original scopes when merging them for queries.
This happened because Model._mergeFunction was in some cases returning references to attributes of the original scope objects instead of cloning them (see line 811).
Also added a new test to prevent this problem from happening again.
Closes #10696
Additional notes
Tried to clone the object with both Utils.cloneDeep and Lodash's
_.cloneDeep, but both result in JavaScript heap out of memory errors when running the tests. My guess is there are some cases where the scopes can include references to complex objects that should not be cloned (maybe references to Sequelize itself?). Because of this, it's cloned with a call to Lodash's_.cloneDeepWithspecifying a function that results in cloning only plain objects. If this function for deep cloning only plain objects can be useful in other places, maybe it could be moved to Utils.