Skip to content

Commit

Permalink
fixed; saving divergent populated arrays
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
aheckmann committed Mar 11, 2013
1 parent 5baac8c commit eaa9fdd
Show file tree
Hide file tree
Showing 7 changed files with 290 additions and 72 deletions.
25 changes: 0 additions & 25 deletions lib/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 1 addition & 1 deletion lib/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
40 changes: 40 additions & 0 deletions lib/errors/divergentArray.js
Original file line number Diff line number Diff line change
@@ -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;
36 changes: 0 additions & 36 deletions lib/errors/elemMatch.js

This file was deleted.

90 changes: 81 additions & 9 deletions lib/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -347,34 +350,95 @@ 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);
}

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.
*
Expand Down Expand Up @@ -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 = {};
Expand All @@ -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;
}
}
Expand Down
19 changes: 18 additions & 1 deletion lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
*
Expand Down
Loading

0 comments on commit eaa9fdd

Please sign in to comment.