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); + }); + }) + }) +})