Skip to content

Commit

Permalink
Change normalized callback arguments.
Browse files Browse the repository at this point in the history
After this commit the callbacks for the write functions are as follows.

```
db.users.insert(document, callback);    // => callback(err, [document])
db.users.save(document, callback);  // => callback(err, document)
db.users.update({ ... }, { ... }, callback); // => callback(err, lastErrorObject)
db.users.remove({ ... }, callback); // => callback(err, {n: amountRemoved})
db.users.findAndModify({ query: { ... }, update: { ... }}, callback); // => callback(err, document, lastErrorObject)
```

The reason for these changes is that sometimes you might want to save a document and retrieve the
ObjectId it was saved with.

The problem of `save` returning '1' when the operation was an update is solved
here, and a test case was added for this as well.
  • Loading branch information
sorribas committed Jan 9, 2014
1 parent 4e72aab commit 1c1dbd5
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 13 deletions.
24 changes: 12 additions & 12 deletions index.js
Expand Up @@ -37,12 +37,6 @@ var getCallback = function(args) {
return typeof callback === 'function' ? callback : noop;
};

var truncateCallback = function(callback) {
return function(err) {
callback(err);
};
};

// Proxy for the native cursor prototype that normalizes method names and
// arguments to fit the mongo shell.

Expand Down Expand Up @@ -196,13 +190,19 @@ Collection.prototype.remove = function() {
this._apply(DRIVER_COLLECTION_PROTO.remove, arguments.length === 0 ? [{}, noop] : replaceCallback(arguments, callback));
};

Collection.prototype.insert = function() {
var callback = truncateCallback(getCallback(arguments));
this._apply(DRIVER_COLLECTION_PROTO.insert, replaceCallback(arguments, callback));
};

Collection.prototype.save = function() {
var callback = truncateCallback(getCallback(arguments));
var self = this;
var args = arguments;
var fn = getCallback(arguments);

var callback = function(err, doc) {
if (err) return fn(err);
if (doc === 1) {
fn(err, args[0]);
} else {
fn(err, doc);
}
}
this._apply(DRIVER_COLLECTION_PROTO.save, replaceCallback(arguments, callback));
};

Expand Down
10 changes: 10 additions & 0 deletions tests/test-insert.js
@@ -0,0 +1,10 @@
var assert = require('assert');
var mongojs = require('../index');
var db = mongojs('test', ['a','b']);

db.a.insert([{name: "Squirtle"}, {name: "Charmander"}, {name: "Bulbasaur"}], function(err, docs) {
assert.ok(docs[0]._id);
assert.ok(docs[1]._id);
assert.ok(docs[2]._id);
db.close();
});
17 changes: 17 additions & 0 deletions tests/test-save.js
@@ -0,0 +1,17 @@
var assert = require('assert');
var mongojs = require('../index');
var db = mongojs('test', ['a','b']);

db.a.save({hello: "world"}, function(err, doc) {
assert.equal(doc.hello, "world");
assert.ok(doc._id);

doc.hello = "verden";
db.a.save(doc, function(err, doc) {
assert.ok(doc._id);
assert.equal(doc.hello, "verden")
db.a.remove(function() {
db.close();
});
});
});
2 changes: 1 addition & 1 deletion tests/test-update-and-callback.js
Expand Up @@ -14,4 +14,4 @@ insert([{
done();
});
sync = false;
});
});

6 comments on commit 1c1dbd5

@mafintosh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an lastErrorObject for .save as well containing information whether or it did an upsert? If so we should added as the third argument to the callback

@sorribas
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .save function doesn't have a lastErrorObject as far as I know.

@sorribas
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I tested it I think that it has a lastErrorObject when it does an update. I will add it to the arguments as well. In the case of a save I will fake it the same way it is done on .findAndModify.

@kapetan
Copy link
Contributor

@kapetan kapetan commented on 1c1dbd5 Jan 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't insert be made intuitiv? When passing a single document return that document, instead of an array. Else if array is passed, return the array.

db.users.insert(documentOrArray, callback);    // => callback(err, documentOrArray)

And also, why not fake lastErrorObject for insert?

@kapetan
Copy link
Contributor

@kapetan kapetan commented on 1c1dbd5 Jan 9, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see save can either performe an update or insert a new document, lastErrorObject makes sense there.

@mafintosh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kapetan Yes to the insert thingy. I'll fix that. save has got a lastErrorObject in master now.

Please sign in to comment.