From eaa9fddb28993efca7eb4efa1ab07dd05b6b4371 Mon Sep 17 00:00:00 2001 From: Aaron Heckmann Date: Mon, 11 Mar 2013 08:34:58 -0700 Subject: [PATCH] fixed; saving divergent populated arrays When a $set of an entire array or a $pop operation of that array is produced through document.save() and the array was populated using limit, skip, query condition, or deselection of the _id field, we now return an error. The view mongoose has of the array has diverged from the array in the database and these operations would have unknown consequences on that data. $pop: // database { _id: 1, array: [3,5,7,9] } // mongoose var query = Doc.findOne(); query.populate({ path: 'array', match: { name: 'three' }}) query.exec(function (err, doc) { console.log(doc.array) // [{ _id: 3, name: 'three' }] doc.$pop(); console.log(doc.array) // [] doc.save() // <== Error }) This $pop removed the document with the _id of 3 from the array locally but when sent to the database would have removed the number 9 (since $pop removes the last element of the array). Instead, one could use doc.array.pull({ _id: 3 }) or perform this update manually using doc.update(..); $set: // database { _id: 1, array: [9,3,7,5] } // mongoose var query = Doc.findOne(); query.populate({ path: 'array', match: { _id: { $gt: 7 }}}) query.exec(function (err, doc) { console.log(doc.array) // [{ _id: 9 }] doc.array.unshift({ _id: 2 }) console.log(doc.array) // [{ _id: 2 }, { _id: 9 }] doc.save() // <== Error }) The would result in a $set of the entire array (there is no equivalent atomic operator for `unshift`) which would overwrite the other un-matched elements in the array in the database. Use doc.update() instead. --- lib/document.js | 25 ----- lib/error.js | 2 +- lib/errors/divergentArray.js | 40 +++++++ lib/errors/elemMatch.js | 36 ------- lib/model.js | 90 ++++++++++++++-- lib/utils.js | 19 +++- test/model.populate.divergent.test.js | 150 ++++++++++++++++++++++++++ 7 files changed, 290 insertions(+), 72 deletions(-) create mode 100644 lib/errors/divergentArray.js delete mode 100644 lib/errors/elemMatch.js create mode 100644 test/model.populate.divergent.test.js diff --git a/lib/document.js b/lib/document.js index 2b36207cfb7..b793ee5b3a4 100644 --- a/lib/document.js +++ b/lib/document.js @@ -1282,31 +1282,6 @@ Document.prototype.$__registerHooks = function () { } else { next(); } - }).pre('save', function checkIfUpdatingElemMatchedArrays (next) { - if (this.isNew) return next(); - if (!this.$__.selected) return next(); - - // walk through modified paths for arrays, - // if any array was selected using an $elemMatch projection, deny the update. - // see https://github.com/LearnBoost/mongoose/issues/1334 - // NOTE: MongoDB only supports projected $elemMatch on top level array. - - var self = this; - var elemMatched = []; - - this.$__.activePaths.forEach('modify', function (path) { - var top = path.split('.')[0]; - if (self.$__.selected[top] && self.$__.selected[top].$elemMatch) { - elemMatched.push(top); - } - }); - - if (elemMatched.length) { - next(new ElemMatchError(elemMatched)); - } else { - next(); - } - }).pre('save', function validation (next) { return this.validate(next); }); diff --git a/lib/error.js b/lib/error.js index 9b21234bad6..cec0e492bfc 100644 --- a/lib/error.js +++ b/lib/error.js @@ -36,4 +36,4 @@ MongooseError.ValidatorError = require('./errors/validator') MongooseError.VersionError =require('./errors/version') MongooseError.OverwriteModelError = require('./errors/overwriteModel') MongooseError.MissingSchemaError = require('./errors/missingSchema') -MongooseError.ElemMatchError = require('./errors/elemMatch') +MongooseError.DivergentArrayError = require('./errors/divergentArray') diff --git a/lib/errors/divergentArray.js b/lib/errors/divergentArray.js new file mode 100644 index 00000000000..45809bc49d5 --- /dev/null +++ b/lib/errors/divergentArray.js @@ -0,0 +1,40 @@ + +/*! + * Module dependencies. + */ + +var MongooseError = require('../error'); + +/*! + * DivergentArrayError constructor. + * + * @inherits MongooseError + */ + +function DivergentArrayError (paths) { + var msg = 'For your own good, using `document.save()` to update an array ' + + 'which was selected using an $elemMatch projection OR ' + + 'populated using skip, limit, query conditions, or exclusion of ' + + 'the _id field when the operation results in a $pop or $set of ' + + 'the entire array is not supported. The following ' + + 'path(s) would have been modified unsafely:\n' + + ' ' + paths.join('\n ') + '\n' + + 'Use Model.update() to update these arrays instead.' + // TODO write up a docs page (FAQ) and link to it + + MongooseError.call(this, msg); + Error.captureStackTrace(this, arguments.callee); + this.name = 'DivergentArrayError'; +}; + +/*! + * Inherits from MongooseError. + */ + +DivergentArrayError.prototype.__proto__ = MongooseError.prototype; + +/*! + * exports + */ + +module.exports = DivergentArrayError; diff --git a/lib/errors/elemMatch.js b/lib/errors/elemMatch.js deleted file mode 100644 index 65b7833a0fa..00000000000 --- a/lib/errors/elemMatch.js +++ /dev/null @@ -1,36 +0,0 @@ - -/*! - * Module dependencies. - */ - -var MongooseError = require('../error'); - -/*! - * ElemMatch Error constructor. - * - * @inherits MongooseError - */ - -function ElemMatchError (paths) { - var msg = 'Using `document.save()` to update an array which was selected ' - + 'using an $elemMatch projection is not supported. The following ' - + 'path(s) were were selected with an $elemMatch projection:\n' - + ' ' + paths.join('\n ') + '\n' - + 'Use Model.update() to update these arrays instead.' - - MongooseError.call(this, msg); - Error.captureStackTrace(this, arguments.callee); - this.name = 'ElemMatchError'; -}; - -/*! - * Inherits from MongooseError. - */ - -ElemMatchError.prototype.__proto__ = MongooseError.prototype; - -/*! - * exports - */ - -module.exports = ElemMatchError; diff --git a/lib/model.js b/lib/model.js index ff6dd0100ce..bbccd7aaa99 100644 --- a/lib/model.js +++ b/lib/model.js @@ -6,11 +6,13 @@ var Document = require('./document') , MongooseArray = require('./types/array') , MongooseBuffer = require('./types/buffer') , MongooseError = require('./error') - , VersionError = require('./errors/version') + , VersionError = MongooseError.VersionError + , DivergentArrayError = MongooseError.DivergentArrayError , Query = require('./query') , Schema = require('./schema') , Types = require('./schema/index') , utils = require('./utils') + , hasOwnProperty = utils.object.hasOwnProperty , isMongooseObject = utils.isMongooseObject , EventEmitter = require('events').EventEmitter , merge = utils.merge @@ -182,6 +184,7 @@ Model.prototype.save = function save (fn) { var delta = this.$__delta(); if (delta) { + if (delta instanceof Error) return complete(delta); var where = this.$__where(delta[0]); this.$__reset(); this.collection.update(where, delta[1], options, complete); @@ -334,10 +337,10 @@ Model.prototype.$__delta = function () { var dirty = this.$__dirty(); if (!dirty.length) return; - var self = this - , where = {} + var where = {} , delta = {} , len = dirty.length + , divergent = [] , d = 0 , val , obj @@ -347,27 +350,39 @@ Model.prototype.$__delta = function () { var value = data.value var schema = data.schema + var match = checkDivergentArray(this, data.path, value); + if (match) { + divergent.push(match); + continue; + } + + if (divergent.length) continue; + if (undefined === value) { - operand(self, where, delta, data, 1, '$unset'); + operand(this, where, delta, data, 1, '$unset'); } else if (null === value) { - operand(self, where, delta, data, null); + operand(this, where, delta, data, null); } else if (value._path && value._atomics) { // arrays and other custom types (support plugins etc) - handleAtomics(self, where, delta, data, value); + handleAtomics(this, where, delta, data, value); } else if (value._path && Buffer.isBuffer(value)) { // MongooseBuffer value = value.toObject(); - operand(self, where, delta, data, value); + operand(this, where, delta, data, value); } else { value = utils.clone(value, { convertToId: 1 }); - operand(self, where, delta, data, value); + operand(this, where, delta, data, value); } } + if (divergent.length) { + return new DivergentArrayError(divergent); + } + if (this.$__.version) { this.$__version(where, delta); } @@ -375,6 +390,55 @@ Model.prototype.$__delta = function () { return [where, delta]; } +/*! + * Determine if array was populated with some form of filter and is now + * being updated in a manner which could overwrite data unintentionally. + * + * @see https://github.com/LearnBoost/mongoose/issues/1334 + * @param {Document} doc + * @param {String} path + * @return {String|undefined} + */ + +function checkDivergentArray (doc, path, array) { + // see if we populated this path + var pop = doc.populated(path, true); + + if (!pop && doc.$__.selected) { + // If any array was selected using an $elemMatch projection, we deny the update. + // NOTE: MongoDB only supports projected $elemMatch on top level array. + var top = path.split('.')[0]; + if (doc.$__.selected[top] && doc.$__.selected[top].$elemMatch) { + return top; + } + } + + if (!(pop && array instanceof MongooseArray)) return; + + // If the array was populated using options that prevented all + // documents from being returned (match, skip, limit) or they + // deselected the _id field, $pop and $set of the array are + // not safe operations. If _id was deselected, we do not know + // how to remove elements. $pop will pop off the _id from the end + // of the array in the db which is not guaranteed to be the + // same as the last element we have here. $set of the entire array + // would be similarily destructive as we never received all + // elements of the array and potentially would overwrite data. + var check = pop.options.match || + pop.options.options && hasOwnProperty(pop.options.options, 'limit') || // 0 is not permitted + pop.options.options && pop.options.options.skip || // 0 is permitted + pop.options.select && // deselected _id? + (0 === pop.options.select._id || + /\s?-_id\s?/.test(pop.options.select)) + + if (check) { + var atomics = array._atomics; + if (0 === Object.keys(atomics).length || atomics.$set || atomics.$pop) { + return path; + } + } +} + /** * Appends versioning to the where and update clauses. * @@ -1671,7 +1735,13 @@ function populate (model, docs, options, cb) { return cb(); } - match || (match = {}); + // preserve original match conditions by copying + if (match) { + match = utils.object.shallowCopy(match); + } else { + match = {}; + } + match._id || (match._id = { $in: ids }); var assignmentOpts = {}; @@ -1685,6 +1755,8 @@ function populate (model, docs, options, cb) { if ('string' == typeof select) { select = null; } else { + // preserve original select conditions by copying + select = utils.object.shallowCopy(select); delete select._id; } } diff --git a/lib/utils.js b/lib/utils.js index a971427f122..81fffa4941b 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -302,7 +302,7 @@ function cloneArray (arr, options) { }; /** - * Copies and merges options with defaults. + * Shallow copies defaults into options. * * @param {Object} defaults * @param {Object} options @@ -602,6 +602,23 @@ exports.object.vals = function vals (o) { return ret; } +/*! + * @see exports.options + */ + +exports.object.shallowCopy = exports.options; + +/*! + * Safer helper for hasOwnProperty checks + * + * @param {Object} obj + * @param {String} prop + */ + +exports.object.hasOwnProperty = function (obj, prop) { + return Object.prototype.hasOwnProperty.call(obj, prop); +} + /*! * Determine if `val` is null or undefined * diff --git a/test/model.populate.divergent.test.js b/test/model.populate.divergent.test.js new file mode 100644 index 00000000000..e2eadcbf95c --- /dev/null +++ b/test/model.populate.divergent.test.js @@ -0,0 +1,150 @@ + +/** + * Test dependencies. + */ + +var start = require('./common') + , assert = require('assert') + , mongoose = start.mongoose + , DivergentArrayError = mongoose.Error.DivergentArrayError + , utils = require('../lib/utils') + , random = utils.random + , Schema = mongoose.Schema + , ObjectId = Schema.ObjectId + +/** + * Tests. + */ + +describe('model: populate: divergent arrays', function(){ + // match + // skip + // limit + // -_id + // + // $set + // $pop -1 + // $pop 1 + + var db, C, M, a, b, c; + + before(function(done){ + db = start(); + C = db.model("Child", { _id: Number, name: String }); + M = db.model("Parent", { array: { type: [{ type: Number, ref: 'Child' }] }}); + + C.create( + { _id: 0, name: 'zero' } + , { _id: 1, name: 'one' } + , { _id: 2, name: 'two' }, function (err, a_, b_, c_) { + assert.ifError(err); + a = a_; + b = b_; + c = c_; + M.create({ array: [0, 1, 2] }, function (err, doc) { + assert.ifError(err); + done(); + }) + }) + }) + + after(function(done){ + db.close(done) + }) + + function test (check, fn) { + it('using $set', function(done){ + fn(function (err, doc) { + assert.ifError(err); + doc.array.unshift({ _id: 10, name: 'ten' }); + doc.save(function (err) { + check(err); + done(); + }) + }); + }) + it('using $pop 1', function(done){ + fn(function (err, doc) { + assert.ifError(err); + doc.array.$pop(); + doc.save(function (err) { + check(err); + done(); + }) + }); + }) + it('using $pop -1', function(done){ + fn(function (err, doc) { + assert.ifError(err); + doc.array.$shift(); + doc.save(function (err) { + check(err); + done(); + }) + }); + }) + } + + function testOk (fn) { + test(assert.ifError.bind(assert), fn); + } + + function testFails (fn) { + test(function (err) { + assert.ok(err instanceof DivergentArrayError); + assert.ok(/\sarray/.test(err.message)); + }, fn); + } + + describe('from match', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', match: { name: 'one' }}).exec(cb); + }); + }) + describe('from skip', function(){ + describe('2', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', options: { skip: 2 }}).exec(cb); + }); + }) + describe('0', function(){ + testOk(function (cb) { + M.findOne().populate({ path: 'array', options: { skip: 0 }}).exec(cb); + }); + }) + }) + describe('from limit', function(){ + describe('0', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', options: { limit: 0 }}).exec(cb); + }); + }) + describe('1', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', options: { limit: 1 }}).exec(cb); + }); + }) + }) + describe('from deselected _id', function(){ + describe('using string and only -_id', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', select: '-_id'}).exec(cb); + }); + }) + describe('using string', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', select: 'name -_id'}).exec(cb); + }); + }) + describe('using object and only _id: 0', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', select: { _id: 0 }}).exec(cb); + }); + }) + describe('using object', function(){ + testFails(function (cb) { + M.findOne().populate({ path: 'array', select: { _id: 0, name: 1 }}).exec(cb); + }); + }) + }) +})