Skip to content

Commit 8b4255a

Browse files
kmaharmbroadst
kmahar
authored and
mbroadst
committed
fix(updateOne/updateMany): ensure that update documents contain atomic operators
NODE-965
1 parent e9c4ffc commit 8b4255a

6 files changed

+127
-9
lines changed

‎lib/collection.js

+28
Original file line numberDiff line numberDiff line change
@@ -938,7 +938,15 @@ define.classMethod('insert', { callback: true, promise: true });
938938
*/
939939
Collection.prototype.updateOne = function(filter, update, options, callback) {
940940
var self = this;
941+
941942
if (typeof options === 'function') (callback = options), (options = {});
943+
944+
var err = checkForAtomicOperators(update);
945+
if (err) {
946+
if (typeof callback === 'function') return callback(err);
947+
return this.s.promiseLibrary.reject(err);
948+
}
949+
942950
options = shallowClone(options);
943951

944952
// Add ignoreUndfined
@@ -959,6 +967,19 @@ Collection.prototype.updateOne = function(filter, update, options, callback) {
959967
});
960968
};
961969

970+
var checkForAtomicOperators = function(update) {
971+
var keys = Object.keys(update);
972+
973+
// same errors as the server would give for update doc lacking atomic operators
974+
if (keys.length === 0) {
975+
return toError('The update operation document must contain at least one atomic operator.');
976+
}
977+
978+
if (keys[0][0] !== '$') {
979+
return toError('the update operation document must contain atomic operators.');
980+
}
981+
};
982+
962983
var updateOne = function(self, filter, update, options, callback) {
963984
// Set single document update
964985
options.multi = false;
@@ -1061,6 +1082,13 @@ define.classMethod('replaceOne', { callback: true, promise: true });
10611082
Collection.prototype.updateMany = function(filter, update, options, callback) {
10621083
var self = this;
10631084
if (typeof options === 'function') (callback = options), (options = {});
1085+
1086+
var err = checkForAtomicOperators(update);
1087+
if (err) {
1088+
if (typeof callback === 'function') return callback(err);
1089+
return this.s.promiseLibrary.reject(err);
1090+
}
1091+
10641092
options = shallowClone(options);
10651093

10661094
// Add ignoreUndfined

‎test/functional/crud_api_tests.js

+80-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
2-
var test = require('./shared').assert;
3-
var setupDatabase = require('./shared').setupDatabase;
2+
var test = require('./shared').assert,
3+
setupDatabase = require('./shared').setupDatabase,
4+
expect = require('chai').expect;
45

56
// instanceof cannot be use reliably to detect the new models in js due to scoping and new
67
// contexts killing class info find/distinct/count thus cannot be overloaded without breaking
@@ -1011,4 +1012,81 @@ describe('CRUD API', function() {
10111012
});
10121013
}
10131014
});
1015+
1016+
it('should correctly throw error if update doc for updateOne lacks atomic operator', {
1017+
// Add a tag that our runner can trigger on
1018+
// in this case we are setting that node needs to be higher than 0.10.X to run
1019+
metadata: {
1020+
requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] }
1021+
},
1022+
1023+
// The actual test we wish to run
1024+
test: function(done) {
1025+
var configuration = this.configuration;
1026+
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
1027+
client.connect(function(err, client) {
1028+
expect(err).to.not.exist;
1029+
var db = client.db(configuration.db);
1030+
var col = db.collection('t21_1');
1031+
col.insertOne({ a: 1, b: 2, c: 3 }, function(err, r) {
1032+
expect(err).to.not.exist;
1033+
expect(r.insertedCount).to.equal(1);
1034+
1035+
// empty update document
1036+
col.updateOne({ a: 1 }, {}, function(err, r) {
1037+
expect(err).to.exist;
1038+
expect(r).to.not.exist;
1039+
1040+
// update document non empty but still lacks atomic operator
1041+
col.updateOne({ a: 1 }, { b: 5 }, function(err, r) {
1042+
expect(err).to.exist;
1043+
expect(r).to.not.exist;
1044+
1045+
client.close();
1046+
done();
1047+
});
1048+
});
1049+
});
1050+
});
1051+
}
1052+
});
1053+
1054+
it('should correctly throw error if update doc for updateMany lacks atomic operator', {
1055+
// Add a tag that our runner can trigger on
1056+
// in this case we are setting that node needs to be higher than 0.10.X to run
1057+
metadata: {
1058+
requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] }
1059+
},
1060+
1061+
// The actual test we wish to run
1062+
test: function(done) {
1063+
var configuration = this.configuration;
1064+
var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
1065+
client.connect(function(err, client) {
1066+
expect(err).to.not.exist;
1067+
var db = client.db(configuration.db);
1068+
var col = db.collection('t22_1');
1069+
col.insertMany([{ a: 1, b: 2 }, { a: 1, b: 3 }, { a: 1, b: 4 }], function(err, r) {
1070+
console.dir(err);
1071+
expect(err).to.not.exist;
1072+
expect(r.insertedCount).to.equal(3);
1073+
1074+
// empty update document
1075+
col.updateMany({ a: 1 }, {}, function(err, r) {
1076+
expect(err).to.exist;
1077+
expect(r).to.not.exist;
1078+
1079+
// update document non empty but still lacks atomic operator
1080+
col.updateMany({ a: 1 }, { b: 5 }, function(err, r) {
1081+
expect(err).to.exist;
1082+
expect(r).to.not.exist;
1083+
1084+
client.close();
1085+
done();
1086+
});
1087+
});
1088+
});
1089+
});
1090+
}
1091+
});
10141092
});

‎test/functional/examples_tests.js

+10-2
Original file line numberDiff line numberDiff line change
@@ -1035,8 +1035,16 @@ describe('Examples', function() {
10351035
.updateOne(
10361036
{ item: 'paper' },
10371037
{
1038-
item: 'paper',
1039-
instock: [{ warehouse: 'A', qty: 60 }, { warehouse: 'B', qty: 40 }]
1038+
$set: {
1039+
item: 'paper',
1040+
instock: [{ warehouse: 'A', qty: 60 }, { warehouse: 'B', qty: 40 }]
1041+
},
1042+
$unset: {
1043+
qty: '',
1044+
size: '',
1045+
status: '',
1046+
lastModified: ''
1047+
}
10401048
}
10411049
)
10421050
.then(function(result) {

‎test/functional/operation_example_tests.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -3531,7 +3531,7 @@ describe('Operation Examples', function() {
35313531
// Get a collection
35323532
var collection = db.collection('update_a_simple_document_upsert');
35333533
// Update the document using an upsert operation, ensuring creation if it does not exist
3534-
collection.updateOne({ a: 1 }, { b: 2, a: 1 }, { upsert: true, w: 1 }, function(
3534+
collection.updateOne({ a: 1 }, { $set: { b: 2, a: 1 } }, { upsert: true, w: 1 }, function(
35353535
err,
35363536
result
35373537
) {
@@ -6777,7 +6777,7 @@ describe('Operation Examples', function() {
67776777

67786778
db
67796779
.collection('mongoclient_test')
6780-
.updateOne({ a: 1 }, { b: 1 }, { upsert: true }, function(err, result) {
6780+
.updateOne({ a: 1 }, { $set: { b: 1 } }, { upsert: true }, function(err, result) {
67816781
test.equal(null, err);
67826782
test.equal(1, result.result.n);
67836783

‎test/functional/operation_generators_example_tests.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -2697,7 +2697,11 @@ describe('Operation (Generators)', function() {
26972697
// Get a collection
26982698
var collection = db.collection('update_a_simple_document_upsert_with_generators');
26992699
// Update the document using an upsert operation, ensuring creation if it does not exist
2700-
var result = yield collection.updateOne({ a: 1 }, { b: 2, a: 1 }, { upsert: true, w: 1 });
2700+
var result = yield collection.updateOne(
2701+
{ a: 1 },
2702+
{ $set: { b: 2, a: 1 } },
2703+
{ upsert: true, w: 1 }
2704+
);
27012705
test.equal(1, result.result.n);
27022706

27032707
// Fetch the document that we modified and check if it got inserted correctly

‎test/functional/operation_promises_example_tests.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -2789,7 +2789,7 @@ describe('Operation (Promises)', function() {
27892789

27902790
// Update the document using an upsert operation, ensuring creation if it does not exist
27912791
return collection
2792-
.updateOne({ a: 1 }, { b: 2, a: 1 }, { upsert: true, w: 1 })
2792+
.updateOne({ a: 1 }, { $set: { b: 2, a: 1 } }, { upsert: true, w: 1 })
27932793
.then(function(result) {
27942794
test.equal(1, result.result.n);
27952795

@@ -5077,7 +5077,7 @@ describe('Operation (Promises)', function() {
50775077
// BEGIN
50785078
return db
50795079
.collection('mongoclient_test_with_promise')
5080-
.updateOne({ a: 1 }, { b: 1 }, { upsert: true })
5080+
.updateOne({ a: 1 }, { $set: { b: 1 } }, { upsert: true })
50815081
.then(function(result) {
50825082
test.equal(1, result.result.n);
50835083
client.close();

0 commit comments

Comments
 (0)