Permalink
Browse files

Fixed FindAndModify to correctly handle pooling and getlasterror retu…

…rning the expected values
  • Loading branch information...
christkv committed Sep 8, 2011
1 parent 247f96c commit d8030259894c10f52e562051e8f5da2e574a5d48
Showing with 253 additions and 192 deletions.
  1. +22 −22 lib/mongodb/collection.js
  2. +4 −0 lib/mongodb/connection.js
  3. +14 −5 lib/mongodb/db.js
  4. +65 −41 test/find_test.js
  5. +148 −124 test/replicaset/query_secondaries_test.js
View
@@ -557,35 +557,35 @@ Collection.prototype.findAndModify = function findAndModify (query, sort, doc, o
var errorOptions = (options && options.safe != null) ? options.safe : null;
errorOptions = errorOptions == null && this.opts.safe != null ? this.opts.safe : errorOptions;
errorOptions = errorOptions == null && this.db.strict != null ? this.db.strict : errorOptions;
- var commandOptions = errorOptions != null ? {safe:errorOptions} : {};
- this.db.executeDbCommand(queryObject, commandOptions, function (err, result) {
+ // Checkout a writer and fetch the raw connection
+ var connection = this.db.serverConfig.checkoutWriter();
+ var rawConnection = connection.getConnection().connection;
+
+ // Ensure we execute against the same raw connection so we can get the correct error
+ // result after the execution of the findAndModify finishes
+ this.db.executeDbCommand(queryObject, {writer:rawConnection}, function (err, result) {
result = result && result.documents;
if(!callback) return;
- //
- // if (err) {
- // callback(err);
- // } else if (error[0].err) {
- // callback(self.db.wrap(error[0]));
- // } else {
- // callback(null, error[0].n);
- // }
- //
- // debug("-------------------------------------------------- hello")
- // debug(inspect(err))
- // debug(inspect(result))
- if (err) {
+ if(err) {
callback(err);
- } else if (result[0].err) {
+ } else if(!result[0].ok) {
callback(self.db.wrap(result[0]));
- } else if (result[0].n) {
- callback(null, result[0].n);
- } else if (result[0].ok != 1) {
- callback(new Error(result[0].errmsg));
} else {
- callback(err, result[0].value)
- }
+ // If we have a safe statement set up we need to execute a getLastErrorCommand first
+ if(errorOptions != null) {
+ self.db.lastError({safe:true}, {writer:rawConnection}, function(err, errResult) {
+ if(errResult[0].ok) {
+ return callback(null, result[0].value);
+ } else {
+ return callback(self.db.wrap(errResult[0]));
+ }
+ })
+ } else {
+ return callback(null, result[0].value);
+ }
+ }
});
}
@@ -204,6 +204,10 @@ Connection.prototype.open = function() {
this.pool = setupConnectionPool(this, this.poolSize);
}
+Connection.prototype.getConnection = function() {
+ return getConnection(this);
+}
+
Connection.prototype.close = function(callback) {
this.connected = false;
// Close all entries in the pool
View
@@ -445,16 +445,25 @@ Db.prototype.renameCollection = function(fromCollection, toCollection, callback)
/**
Return last error message for the given connection
**/
-Db.prototype.lastError = function(options, callback) {
- if ('function' === typeof options) callback = options, options = {};
+Db.prototype.lastError = function(options, connectionOptions, callback) {
+ // Unpack calls
+ var args = Array.prototype.slice.call(arguments, 0);
+ callback = args.pop();
+ options = args.length ? args.shift() : {};
+ connectionOptions = args.length ? args.shift() : {};
- this.executeCommand(DbCommand.createGetLastErrorCommand(options, this), function(err, error) {
+ this.executeCommand(DbCommand.createGetLastErrorCommand(options, this), connectionOptions, function(err, error) {
callback(err, error && error.documents);
});
};
Db.prototype.error = function(options, callback) {
- this.lastError(options, callback);
+ // Unpack call parameters
+ var args = Array.prototype.slice.call(arguments, 0);
+ callback = args.pop();
+ options = args.length ? args.shift() : {};
+ // Execute last error
+ this.lastError(options, {}, callback);
};
/**
@@ -697,7 +706,7 @@ Db.prototype.executeCommand = function(db_command, options, callback) {
writer.send(db_command, rawConnection);
}
}
- } catch(err){
+ } catch(err){
// Set server config to disconnected if it's a replicaset
if(self.serverConfig instanceof ReplSetServers && err == "notConnected") {
// Just clear up all connections as we need to perform a complete reconnect for the call
View
@@ -564,7 +564,7 @@ var tests = testCase({
test.equal(doc.a,doc.b); // making sure empty field select returns properly
test.equal((14-idx),doc.a); // checking skip and limit in args
});
-
+
test.done();
});
});
@@ -578,46 +578,46 @@ var tests = testCase({
// Test return new document on change
collection.insert({'a':1, 'b':2}, {safe:true}, function(err, doc) {
// Let's modify the document in place
- collection.findAndModify({'a':1}, [['a', 1]], {'$set':{'b':3}}, {'new': true}, function(err, updated_doc) {
+ collection.findAndModify({'a':1}, [['a', 1]], {'$set':{'b':3}}, {'new':true}, function(err, updated_doc) {
test.equal(1, updated_doc.a);
test.equal(3, updated_doc.b);
+
+ // Test return old document on change
+ collection.insert({'a':2, 'b':2}, {safe:true}, function(err, doc) {
+ // Let's modify the document in place
+ collection.findAndModify({'a':2}, [['a', 1]], {'$set':{'b':3}}, {safe:true}, function(err, result) {
+ test.equal(2, result.a);
+ test.equal(2, result.b);
+
+ // Test remove object on change
+ collection.insert({'a':3, 'b':2}, {safe:true}, function(err, doc) {
+ // Let's modify the document in place
+ collection.findAndModify({'a':3}, [], {'$set':{'b':3}}, {'new': true, remove: true}, function(err, updated_doc) {
+ test.equal(3, updated_doc.a);
+ test.equal(2, updated_doc.b);
+
+ // // Let's upsert!
+ collection.findAndModify({'a':4}, [], {'$set':{'b':3}}, {'new': true, upsert: true}, function(err, updated_doc) {
+ test.equal(4, updated_doc.a);
+ test.equal(3, updated_doc.b);
+
+ // Test selecting a subset of fields
+ collection.insert({a: 100, b: 101}, {safe:true}, function (err, ids) {
+ collection.findAndModify({'a': 100}, [], {'$set': {'b': 5}}, {'new': true, fields: {b: 1}}, function (err, updated_doc) {
+ test.equal(2, Object.keys(updated_doc).length);
+ test.equal(ids[0]['_id'].toHexString(), updated_doc._id.toHexString());
+ test.equal(5, updated_doc.b);
+ test.equal("undefined", typeof updated_doc.a);
+ test.done();
+ });
+ });
+ });
+ })
+ });
+ })
+ });
})
});
-
- // Test return old document on change
- collection.insert({'a':2, 'b':2}, {safe:true}, function(err, doc) {
- // Let's modify the document in place
- collection.findAndModify({'a':2}, [['a', 1]], {'$set':{'b':3}}, {safe:true}, function(err, result) {
- test.equal(1, result);
- })
- });
-
- // Test remove object on change
- collection.insert({'a':3, 'b':2}, {safe:true}, function(err, doc) {
- // Let's modify the document in place
- collection.findAndModify({'a':3}, [], {'$set':{'b':3}}, {'new': true, remove: true}, function(err, updated_doc) {
- test.equal(3, updated_doc.a);
- test.equal(2, updated_doc.b);
- })
- });
-
- // Let's upsert!
- collection.findAndModify({'a':4}, [], {'$set':{'b':3}}, {'new': true, upsert: true}, function(err, updated_doc) {
- test.equal(4, updated_doc.a);
- test.equal(3, updated_doc.b);
- });
-
- // Test selecting a subset of fields
- collection.insert({a: 100, b: 101}, {safe:true}, function (err, ids) {
- collection.findAndModify({'a': 100}, [], {'$set': {'b': 5}}, {'new': true, fields: {b: 1}}, function (err, updated_doc) {
- test.equal(2, Object.keys(updated_doc).length);
- test.equal(ids[0]['_id'].toHexString(), updated_doc._id.toHexString());
- test.equal(5, updated_doc.b);
- test.equal("undefined", typeof updated_doc.a);
-
- test.done();
- });
- });
});
},
@@ -788,7 +788,7 @@ var tests = testCase({
});
},
- // Test findAndModify a document
+ // Test findAndModify a document with strict mode enabled
shouldCorrectlyFindAndModifyDocumentWithDBStrict : function(test) {
var p_client = new Db(MONGODB, new Server("127.0.0.1", 27017, {auto_reconnect: true}), {strict:true, native_parser: (process.env['TEST_NATIVE'] != null)});
p_client.bson_deserializer = client.bson_deserializer;
@@ -797,19 +797,43 @@ var tests = testCase({
p_client.open(function(err, p_client) {
p_client.createCollection('shouldCorrectlyFindAndModifyDocumentWithDBStrict', function(err, collection) {
+ // Test return old document on change
+ collection.insert({'a':2, 'b':2}, {safe:true}, function(err, doc) {
+ // Let's modify the document in place
+ collection.findAndModify({'a':2}, [['a', 1]], {'$set':{'b':3}}, {new:true}, function(err, result) {
+ test.equal(2, result.a)
+ test.equal(3, result.b)
+ p_client.close();
+ test.done();
+ })
+ });
+ });
+ });
+ },
+
+ // Test findAndModify a document that fails in first step before safe
+ shouldCorrectlyFindAndModifyDocumentThatFailsInFirstStep : function(test) {
+ var p_client = new Db(MONGODB, new Server("127.0.0.1", 27017, {auto_reconnect: true}), {strict:true, native_parser: (process.env['TEST_NATIVE'] != null)});
+ p_client.bson_deserializer = client.bson_deserializer;
+ p_client.bson_serializer = client.bson_serializer;
+ p_client.pkFactory = client.pkFactory;
+
+ p_client.open(function(err, p_client) {
+ p_client.createCollection('shouldCorrectlyFindAndModifyDocumentThatFailsInFirstStep', function(err, collection) {
// Test return old document on change
collection.insert({'a':2, 'b':2}, function(err, doc) {
// Let's modify the document in place
- collection.findAndModify({'a':2}, [['a', 1]], {'$set':{'b':3}}, function(err, result) {
- test.equal(1, result);
+ collection.findAndModify({'c':2}, [['a', 1]], {'$set':{'b':3}}, function(err, result) {
+ test.ok(err != null);
+ test.equal(null, result);
p_client.close();
test.done();
})
});
});
});
},
-
+
noGlobalsLeaked : function(test) {
var leaks = gleak.detectNew();
test.equal(0, leaks.length, "global var leak detected: " + leaks.join(', '));
Oops, something went wrong.

0 comments on commit d803025

Please sign in to comment.