From f5781afb47f62cb16e72289e6798bec561e68f33 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Sun, 15 Jun 2014 15:25:28 -0600
Subject: [PATCH 1/3] Updating MongoDB service, improving tests and cleaning up
 code.

---
 lib/mongodb.js | 209 +++++++++++++++++++++----------------------------
 package.json   |   8 +-
 test/test.js   | 142 ++++++++++++++++-----------------
 3 files changed, 167 insertions(+), 192 deletions(-)

diff --git a/lib/mongodb.js b/lib/mongodb.js
index c26f1e5..d1bab65 100644
--- a/lib/mongodb.js
+++ b/lib/mongodb.js
@@ -3,58 +3,21 @@ var mongo = require('mongoskin');
 var errors = require('feathers').errors.types;
 var _ = require('lodash');
 
-// TODO (EK): Does order matter for how these filters
-// are applied? I think it does or at least it should.
-
-var filters = {
-  sort: function (value) {
-    return { 'sort': [[value, 1]] };
-  },
-  order: function (value) {
-    return { 'sort': [[value, -1]] };
-  },
-  skip: function (value) {
-    return { 'skip': value };
-  },
-  limit: function (value) {
-    return { 'limit': value };
-  }
-};
-
 var MongoService = Proto.extend({
-
   // TODO (EK): How do we handle indexes?
-  init: function (options) {
+  init: function(options) {
     options = options || {};
 
-    this.type = 'mongodb';
-    this._id = options.idField || '_id';
-    this.connectionString = options.connectionString || null;
-    this.collection = options.collection || null;
-    this.ackOptions = {
-      w: options.w || 1,                  // write acknowledgment
-      journal: options.journal || false,  // doesn't wait for journal before acknowledgment
-      fsync: options.fsync || false       // doesn't wait for syncing to disk before acknowledgment
-    };
-
-    if (options.safe) {
-      this.ackOptions = { safe: options.safe || true };
+    if(!options.collection) {
+      throw new errors.GeneralError('No MongoDB collection name specified.');
     }
 
-    // NOTE (EK): We need to get the collection somehow.
-    // We have 3 options:
-    //   1. Pass in the path on each request
-    //   2. Initialize separate instances and pass it in there
-    //   3. Set the collection when we register each service
-    //
-    // We are currently using option number 3. This could be a bad assumption.
+    this.type = 'mongodb';
+    this.options = _.extend({
+      _id: '_id'
+    }, options);
 
-    if (this.connectionString){
-      this.store = mongo.db(this.connectionString, this.ackOptions);
-    }
-    else {
-      this._connect(options);
-    }
+    this._connect(this.options);
   },
 
   // NOTE (EK): We create a new database connection for every MongoService.
@@ -63,137 +26,147 @@ var MongoService = Proto.extend({
   // app residing on a totally different server.
 
   // TODO (EK): We need to handle replica sets.
-  _connect: function(options){
-    this.host = options.host || process.env.MONGODB_HOST || 'localhost';
-    this.port = options.port || process.env.MONGODB_PORT || 27017;
-    this.db = options.db || process.env.MONGODB_DB || 'feathers';
+  _connect: function(options) {
+    var connectionString = options.connectionString;
+    var ackOptions = {
+      w: options.w || 1,                  // write acknowledgment
+      journal: options.journal || false,  // doesn't wait for journal before acknowledgment
+      fsync: options.fsync || false,       // doesn't wait for syncing to disk before acknowledgment
+      safe: options.safe || false
+    };
+
+    if(!connectionString) {
+      var config = _.extend({
+        host: 'localhost',
+        port: 27017,
+        db: 'feathers'
+      }, options);
 
-    var connectionString = this.host + ':' + this.port + '/' + this.db;
+      connectionString = config.host + ':' + config.port + '/' + config.db;
+    }
 
-    if (options.username && options.password){
-      connectionString =+ options.username + ':' + options.password + '@';
+    if(options.username && options.password) {
+      connectionString += options.username + ':' + options.password + '@';
     }
 
-    if (options.reconnect) {
+    if(options.reconnect) {
       connectionString += '?auto_reconnect=true';
     }
 
-    this.store = mongo.db(connectionString, this.ackOptions);
+    this.store = mongo.db(connectionString, ackOptions);
+    this.collection = this.store.collection(options.collection);
   },
 
-  find: function (params, cb) {
-    var id = null;
-
-    if (_.isFunction(params)){
+  find: function(params, cb) {
+    if(_.isFunction(params)) {
       cb = params;
       params = {};
     }
-    else {
-      id = params.id;
-    }
-
-    var query = params.query || {};
-    var options = {};
 
-    if (!this.collection) {
-      return cb(new Error('No collection specified'));
-    }
-
-    // TODO (EK): sort out filters.
-    // ie. sort, limit, fields, skip, etc...
-
-    _.each(filters, function(handler, name){
-      if (query[name]){
-        _.extend( options, handler(query[name]) );
-        delete query[name];
-      }
-    });
-
-    if (id){
-      this.store.collection(this.collection).findById(id, options, cb);
-    }
-    else {
-      this.store.collection(this.collection).find(query, options).toArray(cb);
-    }
+    this.collection.find(params.query || {}, params.options || {}).toArray(cb);
   },
 
-  get: function (id, params, cb) {
-
-    if (_.isFunction(id)){
+  get: function(id, params, cb) {
+    if(_.isFunction(id)) {
       cb = id;
-      return cb(new errors.BadRequest('An string or number id must be provided'));
+      return cb(new errors.BadRequest('A string or number id must be provided'));
     }
 
-    if (_.isFunction(params)){
+    if(_.isFunction(params)) {
       cb = params;
       params = {};
     }
 
-    params.query = params.query || {};
-
-    if (_.isString(id)){
+    if(_.isString(id)) {
       id = id.toLowerCase();
     }
 
-    if (!this.collection) {
-      return cb(new errors.GeneralError('No collection specified'));
-    }
+    this.collection.findById(id, params.query || {}, function(error, data) {
+      if(error) {
+        return cb(error);
+      }
+
+      if(!data) {
+        return cb(new errors.NotFound('No record found for id ' + id));
+      }
 
-    this.store.collection(this.collection).findById(id, params.query, cb);
+      return cb(null, data);
+    });
   },
 
   // TODO (EK): Batch support for create, update, delete.
-  create: function (data, params, cb) {
-    if (_.isFunction(params)){
+  create: function(data, params, cb) {
+    if(_.isFunction(params)) {
       cb = params;
       params = {};
     }
 
-    if (!this.collection) {
-      return cb(new errors.GeneralError('No collection specified'));
-    }
-
-    this.store.collection(this.collection).insert(data, params, function(error, data) {
+    this.collection.insert(data, params, function(error, data) {
       if(error || !data) {
-        cb(error);
+        return cb(error);
       }
 
       cb(null, data.length === 1 ? data[0] : data);
     });
   },
 
-  // TODO (EK): Return the updated document(s)
-  update: function (id, data, params, cb) {
-    if (_.isFunction(params)){
+  patch: function(id, data, params, cb) {
+    if(_.isFunction(params)) {
       cb = params;
       params = {};
     }
 
-    if (!this.collection) {
-      return cb(new errors.GeneralError('No collection specified'));
-    }
+    var _get = this.get.bind(this);
 
-    this.store.collection(this.collection).updateById(id, data, params, function(error) {
+    this.collection.updateById(id, { $set: data }, params, function(error) {
       if(error) {
         return cb(error);
       }
 
-      this.get(id, {}, cb);
+      _get(id, {}, cb);
     }.bind(this));
   },
 
-  // TODO (EK): Return removed document(s)
-  remove: function (id, params, cb) {
-    if (_.isFunction(params)){
+  update: function(id, data, params, cb) {
+    if(_.isFunction(params)) {
       cb = params;
       params = {};
     }
 
-    if (!this.collection) {
-      return cb(new errors.GeneralError('No collection specified'));
+    var _get = this.get.bind(this);
+
+    this.collection.updateById(id, data, params, function(error) {
+      // TODO (DL) maybe we should throw a NotFound error already but
+      // that doesn't seem to be necessary
+      if(error) {
+        return cb(error);
+      }
+
+      _get(id, {}, cb);
+    }.bind(this));
+  },
+
+  remove: function(id, params, cb) {
+    if(_.isFunction(params)) {
+      cb = params;
+      params = {};
     }
 
-    this.store.collection(this.collection).removeById(id, params.query, cb);
+    var collection = this.collection;
+
+    this.get(id, params, function(error, data) {
+      if(error) {
+        return cb(error);
+      }
+
+      collection.removeById(id, params.query || {}, function(error) {
+        if(error) {
+          return cb(error);
+        }
+
+        cb(null, data);
+      });
+    });
   }
 });
 
diff --git a/package.json b/package.json
index c0c1c2e..17da610 100644
--- a/package.json
+++ b/package.json
@@ -26,8 +26,8 @@
     "test": "grunt test"
   },
   "engines": {
-    "node": "*",
-    "npm": "*"
+    "node": "~0.10.0",
+    "npm": "~1.4.0"
   },
   "dependencies": {
     "lodash": "~2.4.1",
@@ -35,10 +35,10 @@
     "uberproto": "~1.1.0"
   },
   "peerDependencies": {
-    "feathers": ">=1.0.x"
+    "feathers": ">= 1.0.0-pre.1"
   },
   "devDependencies": {
-    "feathers": ">=1.0.x",
+    "feathers": ">= 1.0.0-pre.1",
     "chai": "^1.7.2",
     "database-cleaner": "~0.7.0",
     "grunt": "^0.4.5",
diff --git a/test/test.js b/test/test.js
index 3a3a3a6..a2bc645 100644
--- a/test/test.js
+++ b/test/test.js
@@ -2,6 +2,7 @@ var chai = require('chai');
 var expect = chai.expect;
 var DatabaseCleaner = require('database-cleaner');
 var databaseCleaner = new DatabaseCleaner('mongodb');
+var errors = require('feathers').errors.types;
 
 var mongodb = require('./../lib/mongodb');
 var service = mongodb({
@@ -42,6 +43,46 @@ describe('Mongo Service', function() {
     it('should setup a mongo connection based on a connection string');
   });
 
+  describe('get', function() {
+    it('should return an instance that exists', function(done) {
+      service.get(_ids.Doug, function(error, data) {
+        expect(error).to.be.null;
+        expect(data._id.toString()).to.equal(_ids.Doug.toString());
+        expect(data.name).to.equal('Doug');
+        done();
+      });
+    });
+
+    it('should return an error when no id is provided', function(done) {
+      service.get(function(error, data) {
+        expect(error).to.be.ok;
+        expect(error instanceof errors.BadRequest).to.be.ok;
+        expect(data).to.be.undefined;
+        done();
+      });
+    });
+
+    it('should return NotFound error for non-existing id', function(done) {
+      service.get('abc', function(error) {
+        expect(error).to.be.ok;
+        expect(error instanceof errors.NotFound).to.be.ok;
+        expect(error.message).to.equal('No record found for id abc');
+        done();
+      });
+    });
+  });
+
+  describe('remove', function() {
+    it('should delete an existing instance and return the deleted instance', function(done) {
+      service.remove(_ids.Doug, function(error, data) {
+        expect(error).to.be.null;
+        expect(data).to.be.ok;
+        expect(data.name).to.equal('Doug');
+        done();
+      });
+    });
+  });
+
   describe('find', function() {
     beforeEach(function(done) {
       service.create({
@@ -72,7 +113,6 @@ describe('Mongo Service', function() {
 
     it('should return all items', function(done) {
       service.find({}, function(error, data) {
-
         expect(error).to.be.null;
         expect(data).to.be.instanceof(Array);
         expect(data.length).to.equal(3);
@@ -80,18 +120,11 @@ describe('Mongo Service', function() {
       });
     });
 
-    it('should return an item by id', function(done) {
-      service.find({ id: _ids.Doug }, function(error, data) {
-
-        expect(error).to.be.null;
-        expect(data.name).to.equal('Doug');
-        done();
-      });
-    });
-
-    it('should return all items sorted in ascending order', function(done) {
-
-      service.find({ query: { sort: 'name' } }, function(error, data) {
+    it('passes in options', function(done) {
+      service.find({
+        query: {},
+        options: { sort: [['name', 1]] }
+      }, function(error, data) {
         expect(error).to.be.null;
         expect(data.length).to.equal(3);
         expect(data[0].name).to.equal('Alice');
@@ -101,60 +134,57 @@ describe('Mongo Service', function() {
       });
     });
 
-    it('should return all items sorted in descending order', function(done) {
-
-      service.find({ query: { order: 'age' } }, function(error, data) {
+    it('query filters by parameter', function(done) {
+      service.find({ query: { name: 'Alice' } }, function(error, data) {
         expect(error).to.be.null;
-        expect(data.length).to.equal(3);
-        expect(data[0].name).to.equal('Doug');
-        expect(data[1].name).to.equal('Bob');
-        expect(data[2].name).to.equal('Alice');
+        expect(data).to.be.instanceof(Array);
+        expect(data.length).to.equal(1);
+        expect(data[0].name).to.equal('Alice');
         done();
       });
     });
+  });
 
-    it('should return the number of items set by the limit', function(done) {
-
-      service.find({ query: { limit: 2 } }, function(error, data) {
-
+  describe('update', function() {
+    it('should replace an existing instance', function(done) {
+      service.update(_ids.Doug, { name: 'Dougler' }, function(error, data) {
         expect(error).to.be.null;
-        expect(data.length).to.equal(2);
+        expect(data._id.toString()).to.equal(_ids.Doug.toString());
+        expect(data.name).to.equal('Dougler');
+        expect(data.age).to.be.undefined;
         done();
       });
     });
 
-    it('should skip over the number of items set by skip', function(done) {
-
-      service.find({ query: { skip: 2 } }, function(error, data) {
-        expect(error).to.be.null;
-        expect(data.length).to.equal(1);
-        expect(data[0].name).to.equal('Alice');
+    it('should throw an error when updating non-existent instances', function(done) {
+      service.update('bla', { name: 'NotFound' }, function(error) {
+        expect(error).to.be.ok;
+        expect(error instanceof errors.NotFound).to.be.ok;
+        expect(error.message).to.equal('No record found for id bla');
         done();
       });
     });
   });
 
-  describe('get', function() {
-    it('should return an instance that exists', function(done) {
-      service.get(_ids.Doug, function(error, data) {
-
+  describe('patch', function() {
+    it('should replace an existing instance', function(done) {
+      service.patch(_ids.Doug, { name: 'PatchDoug' }, function(error, data) {
         expect(error).to.be.null;
         expect(data._id.toString()).to.equal(_ids.Doug.toString());
-        expect(data.name).to.equal('Doug');
+        expect(data.name).to.equal('PatchDoug');
+        expect(data.age).to.equal(32);
         done();
       });
     });
 
-    it('should return an error when no id is provided', function(done) {
-      service.get(function(error, data) {
-
+    it('should throw an error when updating non-existent instances', function(done) {
+      service.patch('bla', { name: 'NotFound' }, function(error) {
         expect(error).to.be.ok;
-        expect(data).to.be.undefined;
+        expect(error instanceof errors.NotFound).to.be.ok;
+        expect(error.message).to.equal('No record found for id bla');
         done();
       });
     });
-
-    it('should return an error on db error');
   });
 
   describe('create', function() {
@@ -189,33 +219,5 @@ describe('Mongo Service', function() {
         done();
       });
     });
-
-    it('should return an error on db error');
-  });
-
-  describe('update', function() {
-    it('should update an existing instance', function(done) {
-      service.update(_ids.Doug, { name: 'Doug', age: 12 }, function(error, data) {
-        expect(error).to.be.null;
-        expect(data.name).to.equal('Doug');
-        expect(data.age).to.equal(12);
-        done();
-      });
-    });
-    it('should update multiple existing instances');
-    it('should return an error on db error');
-  });
-
-  describe('remove', function() {
-    it('should delete an existing instance', function(done) {
-      service.remove(_ids.Doug, function(error, data) {
-        expect(error).to.be.null;
-        expect(data).to.be.ok;
-        done();
-      });
-    });
-
-    it('should delete multiple existing instances');
-    it('should return an error on db error');
   });
 });

From 4474db416174e14f2b81926271d3c79dbabaec93 Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Sun, 15 Jun 2014 15:44:28 -0600
Subject: [PATCH 2/3] Adding some tests for intialization.

---
 test/test.js | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/test/test.js b/test/test.js
index a2bc645..8c3e187 100644
--- a/test/test.js
+++ b/test/test.js
@@ -38,9 +38,37 @@ describe('Mongo Service', function() {
   });
 
   describe('init', function() {
-    it('should setup a mongo connection based on config');
-    it('should setup a mongo connection based on ENV vars');
-    it('should setup a mongo connection based on a connection string');
+    it('should setup a mongo connection based on config', function(done) {
+      var myService = mongodb({
+        db: 'some-database',
+        collection: 'other-test'
+      });
+
+      myService.create({ name: 'David' }, function(error, data) {
+        expect(error).to.be.null;
+        expect(data._id).to.be.ok;
+        databaseCleaner.clean(myService.store, function() {
+          myService.store.close();
+          done();
+        });
+      });
+    });
+
+    it('should setup a mongo connection based on a connection string', function(done) {
+      var otherService = mongodb({
+        connectionString: 'localhost:27017/dummy-db',
+        collection: 'other-test'
+      });
+
+      otherService.create({ name: 'David' }, function(error, data) {
+        expect(error).to.be.null;
+        expect(data._id).to.be.ok;
+        databaseCleaner.clean(otherService.store, function() {
+          otherService.store.close();
+          done();
+        });
+      });
+    });
   });
 
   describe('get', function() {
@@ -123,7 +151,9 @@ describe('Mongo Service', function() {
     it('passes in options', function(done) {
       service.find({
         query: {},
-        options: { sort: [['name', 1]] }
+        options: { sort: [
+          ['name', 1]
+        ] }
       }, function(error, data) {
         expect(error).to.be.null;
         expect(data.length).to.equal(3);

From 5a0283efafa479cefba674195d406a720f9d410e Mon Sep 17 00:00:00 2001
From: David Luecke <daff@neyeon.com>
Date: Mon, 23 Jun 2014 17:41:18 -0600
Subject: [PATCH 3/3] Adding better documentation. Closes #6.

---
 README.md | 174 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 174 insertions(+)

diff --git a/README.md b/README.md
index f0abdb8..fbe8331 100644
--- a/README.md
+++ b/README.md
@@ -5,3 +5,177 @@ feathers-mongodb
 [![Code Climate](https://codeclimate.com/github/feathersjs/feathers-mongodb.png)](https://codeclimate.com/github/feathersjs/feathers-mongodb)
 
 A MongoDB CRUD service for [feathers](http://feathersjs.com)
+
+## Getting Started
+
+To install feathers-hooks from [npm](https://www.npmjs.org/), run:
+
+```bash
+$ npm install feathers-mongodb --save
+```
+
+Finally, to use the plugin in your Feathers app:
+
+```javascript
+var feathers = require('feathers');
+var mongodb = require('feathers-mongodb');
+
+// Initialize a MongoDB service with the users collection on a local MongoDB instance
+var app = feathers()
+  .use('/users', mongodb({
+    collection: 'users'
+  });
+  
+app.listen(8080);
+```
+
+## Options
+
+The following options can be passed when creating a new MongoDB service:
+
+General options:
+
+- `collection` - The name of the collection
+- `connectionString` - A MongoDB connection string
+- `[_id]` (default: `"_id"`) - The id property
+- `username` - MongoDB username
+- `password` - MongoDB password
+
+Connection options (when `connectionString` is not set):
+
+- `host` (default: `"localhost"`) - The MongoDB host
+- `port` (default: `27017`) - The MongoDB port
+- `db` (default: `"feathers"`) - The name of the database
+
+MongoDB options:
+
+- `w` (default: `1`) - Write acknowledgements
+- `journal` (default: `false`) - Don't wait for journal before acknowledgement
+- `fsync` (default: `false`) - Don't wait for syncing to disk before acknowledgment
+- `safe` (default: `false`) - Safe mode 
+
+## Extending MongoDB services
+
+To extend the basic MongoDB service there are two options. Either through using [Uberproto's](https://github.com/daffl/uberproto) inheritance mechanism or by using [feathers-hooks](https://github.com/feathersjs/feathers-hooks).
+
+### With Uberproto
+
+The basic MongoDB Feathers service is implemented using [Uberproto](https://github.com/daffl/uberproto), a small EcmaScript 5 inheritance library so you can use the Uberproto syntax to add your custom functionality.
+For example, you might want `update` and `patch` to behave the same (the basic implementation of `update` replaces the entire object instead of merging it) and add an `updatedAt` and `createdAt` flag to your data:
+
+```js
+// myservice.js
+var mongodb = require('feathers-mongodb');
+var Proto = require('uberproto');
+
+var TimestampPatchService = mongodb.Service.extend({
+  create: function(data, params, callback) {
+    data.createdAt = new Date();
+    
+    // Call the original `create`
+    return this._super(data, params, callback);
+  },
+  
+  update: function() {
+    // Call `patch` instead so that PUT calls merge
+    // instead of replace data, too
+    this.patch(id, data, params, callback);
+  },
+  
+  patch: function(id, data, params, callback) {
+    data.updatedAt = new Date();
+    
+    // Call the original `patch`
+    this._super(id, data, params, callback);
+  }
+});
+
+// Export a simple function that instantiates this new service like
+// var myservice = require('myservice');
+// app.use('/users', myservice(options));
+module.exports = function(options) {
+  // We need to call `Proto.create` explicitly here since we are overriding
+  // the original `create` method
+  return Proto.create.call(TimestampPatchService, options);
+}
+
+module.exports.Service = TimestampPatchService;
+```
+
+### With hooks
+
+Another options would be to weave functionality into your existing services using [feathers-hooks](https://github.com/feathersjs/feathers-hooks), for example the above `createdAt` and `updatedAt` functionality:
+
+```javascript
+var feathers = require('feathers');
+var hooks = require('feathers-hooks');
+var mongodb = require('feathers-mongodb');
+
+// Initialize a MongoDB service with the users collection on a local MongoDB instance
+var app = feathers()
+  .configure(hooks())
+  .use('/users', mongodb({
+    collection: 'users'
+  });
+  
+app.lookup('users').before({
+  create: function(hook, next) {
+    hook.data.createdAt = new Date();
+  },
+  
+  update: function(hook, next) {
+    hook.data.updatedAt = new Date();
+    next();
+  }
+});
+
+app.listen(8080);
+```
+
+## MongoDB options and filtering
+
+Internally the MongoDB service uses [MongoSkin](https://github.com/kissjs/node-mongoskin).
+To set the query options, set `params.options` in your service calls. Using [feathers-hooks](https://github.com/feathersjs/feathers-hooks), for example, a sort mechanism based on query parameters
+(e.g. `/users?sort=name`) can be implemented as a hook like this:
+
+```js
+var app = feathers()
+  .configure(hooks())
+  .use('/users', mongodb({
+    collection: 'users'
+  });
+  
+app.lookup('users').before({
+  find: function(hook, next) {
+    // Grab the fieldname from `params.query`
+    var field = hook.params.query.sort;
+    if(field) {
+      hook.params.options = { sort: [[field, -1]] };
+    }
+  }
+});
+```
+
+
+## Changelog
+
+__0.3.0__
+
+- Implement `.patch` support ([#5](https://github.com/feathersjs/feathers-mongodb/issues/5))
+- Better documentation
+- Refactoring that removes pre-implemented MongoSkin options
+
+__0.2.x__
+
+- Pre-releases
+
+## Authors
+
+- [Eric Kryski](https://github.com/ekryski)
+- [David Luecke](https://github.com/daffl)
+
+## License
+
+Copyright (c) 2014 Eric Kryski, David Luecke
+
+Licensed under the [MIT license](LICENSE).
\ No newline at end of file