Skip to content

Commit

Permalink
fixed; prohibit updating arrays selected with $elemMatch
Browse files Browse the repository at this point in the history
An $elemMatch projection only returns the elements of the array in the
database that match the argument. While helpful when working with
large arrays, it means we can no longer depend on any array elements
position being accurate - manipulating these arrays with the
mongoose helper methods would potentially result in incorrect array
elements getting updated or even loss of array elements in the database.

Attempting to update an array selected with an $elemMatch projection
now returns an error that includes the array paths in violation.

To update arrays selected with $elemMatch, manually use Model.update,
Model.findByIdAndUpdate, or another method outside of document.save().

closes #1334
  • Loading branch information
aheckmann committed Mar 4, 2013
1 parent d4a38c6 commit 8b847c0
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 25 deletions.
31 changes: 29 additions & 2 deletions lib/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ var EventEmitter = require('events').EventEmitter
, clone = utils.clone
, isMongooseObject = utils.isMongooseObject
, inspect = require('util').inspect
, ValidationError = require('./errors/validation')
, DocumentError = require('./errors/document')
, ElemMatchError = MongooseError.ElemMatchError
, ValidationError = MongooseError.ValidationError
, DocumentError = MongooseError.DocumentError
, InternalCache = require('./internal')
, deepEqual = utils.deepEqual
, hooks = require('hooks')
Expand Down Expand Up @@ -1204,6 +1205,7 @@ Document.prototype._registerHooks = function _registerHooks () {
DocumentArray || (DocumentArray = require('./types/documentarray'));

this.pre('save', function (next) {
// validate all document arrays.
// we keep the error semaphore to make sure we don't
// call `save` unnecessarily (we only need 1 error)
var subdocs = 0
Expand Down Expand Up @@ -1277,6 +1279,31 @@ Document.prototype._registerHooks = function _registerHooks () {
} 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
1 change: 1 addition & 0 deletions lib/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +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')
36 changes: 36 additions & 0 deletions lib/errors/elemMatch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@

/*!
* 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;
84 changes: 61 additions & 23 deletions test/model.field.selection.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,43 +249,81 @@ describe('model field selection', function(){
});
});

it('casts elemMatch args (gh-1091)', function(done){
describe('with $elemMatch projection', function(){
// mongodb 2.2 support
var db = start()

var postSchema = new Schema({
ids: [{type: Schema.ObjectId}]
});
it('casts elemMatch args (gh-1091)', function(done){
var db = start()

var B = db.model('gh-1091', postSchema);
var _id1 = new mongoose.Types.ObjectId;
var _id2 = new mongoose.Types.ObjectId;
var postSchema = new Schema({
ids: [{type: Schema.ObjectId}]
});

//mongoose.set('debug', true);
B.create({ ids: [_id1, _id2] }, function (err, doc) {
assert.ifError(err);
var B = db.model('gh-1091', postSchema);
var _id1 = new mongoose.Types.ObjectId;
var _id2 = new mongoose.Types.ObjectId;

B
.findById(doc._id)
.select({ ids: { $elemMatch: { $in: [_id2.toString()] }}})
.exec(function (err, found) {
B.create({ ids: [_id1, _id2] }, function (err, doc) {
assert.ifError(err);
assert.ok(found);
assert.equal(found.id, doc.id);
assert.equal(1, found.ids.length);
assert.equal(_id2.toString(), found.ids[0].toString());

B
.find({ _id: doc._id })
.findById(doc._id)
.select({ ids: { $elemMatch: { $in: [_id2.toString()] }}})
.exec(function (err, found) {
assert.ifError(err);
assert.ok(found.length);
found = found[0];
assert.ok(found);
assert.equal(found.id, doc.id);
assert.equal(1, found.ids.length);
assert.equal(_id2.toString(), found.ids[0].toString());
done();

B
.find({ _id: doc._id })
.select({ ids: { $elemMatch: { $in: [_id2.toString()] }}})
.exec(function (err, found) {
assert.ifError(err);
assert.ok(found.length);
found = found[0];
assert.equal(found.id, doc.id);
assert.equal(1, found.ids.length);
assert.equal(_id2.toString(), found.ids[0].toString());
done();
})
})
})
})

it('disallows saving modified elemMatch paths (gh-1334)', function(done){
var db = start()

var postSchema = new Schema({
ids: [{type: Schema.ObjectId}]
, ids2: [{type: Schema.ObjectId}]
});

var B = db.model('gh-1334', postSchema);
var _id1 = new mongoose.Types.ObjectId;
var _id2 = new mongoose.Types.ObjectId;

B.create({ ids: [_id1, _id2], ids2: [_id2, _id1] }, function (err, doc) {
assert.ifError(err);

B
.findById(doc._id)
.select({ ids: { $elemMatch: { $in: [_id2.toString()] }}})
.select({ ids2: { $elemMatch: { $in: [_id1.toString()] }}})
.exec(function (err, found) {
assert.ifError(err);
assert.equal(1, found.ids.length);
assert.equal(1, found.ids2.length);
found.ids = [];
found.ids2.set(0, _id2);
found.save(function (err) {
db.close();
assert.ok(/\$elemMatch projection/.test(err));
assert.ok(/ ids/.test(err));
assert.ok(/ ids2/.test(err));
done()
})
})
})
})
Expand Down

0 comments on commit 8b847c0

Please sign in to comment.