From 954b0211942ac1de48f3f1664b9315aedc3e89e0 Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Mon, 14 Mar 2016 11:42:29 +1300
Subject: [PATCH 01/11] Use mongoose to validate method to check for ID
 validity, rather than assuming every _id field needs an ObjectId

This method is slightly inefficient as it does use try to validate every
other field in the schema as well. If there's a way to limit mongoose to
validating a single field, we should do that, but this is the best
method I've found so far.

Potential solution to #80
---
 src/db-adapters/Mongoose/MongooseAdapter.js   | 367 +++++++++---------
 .../db-adapters/Mongoose/MongooseAdapter.js   |  56 ++-
 2 files changed, 227 insertions(+), 196 deletions(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index a83b3c50..d499e917 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -32,108 +32,108 @@ export default class MongooseAdapter {
   find(type, idOrIds, fields, sorts, filters, includePaths) {
     const model = this.getModel(this.constructor.getModelName(type));
     const queryBuilder = new mongoose.Query(null, null, model, model.collection);
-    const [mode, idQuery] = this.constructor.getIdQueryType(idOrIds);
     const pluralizer = this.inflector.plural;
     let primaryDocumentsPromise, includedResourcesPromise = Q(null);
 
+    return this.constructor.getIdQueryType(idOrIds, model).then(([ mode, idQuery]) => {
+      queryBuilder[mode](idQuery);
 
-    queryBuilder[mode](idQuery);
-
-    // do sorting
-    if(Array.isArray(sorts)) {
-      queryBuilder.sort(sorts.join(" "));
-    }
-
-    // filter out invalid records with simple fields equality.
-    // note that there's a non-trivial risk of sql-like injection here.
-    // we're mostly protected by the fact that we're treating the filter's
-    // value as a single string, though, and not parsing as JSON.
-    if(typeof filters === "object" && !Array.isArray(filters)) {
-      queryBuilder.where(filters);
-    }
-
-    // in an ideal world, we'd use mongoose here to filter the fields before
-    // querying. But, because the fields to filter can be scoped by type and
-    // we don't always know about a document's type until after query (becuase
-    // of discriminator keys), and because filtering out fields can really
-    // complicate population for includes, we don't yet filter at query time but
-    // instead just hide filtered fields in @docToResource. There is a more-
-    // efficient way to do this down the road, though--something like taking the
-    // provided fields and expanding them just enough (by looking at the type
-    // heirarachy and the relationship paths) to make sure that we're not going
-    // to run into any of the problems outlined above, while still querying for
-    // less data than we would without any fields restriction. For reference, the
-    // code for safely using the user's `fields` input, by putting them into a
-    // mongoose `.select()` object so that the user can't prefix a field with a
-    // minus on input to affect the query, is below.
-    // Reference: http://mongoosejs.com/docs/api.html#query_Query-select.
-    // let arrToSelectObject = (prev, curr) => { prev[curr] = 1; return prev; };
-    // for(let type in fields) {
-    //   fields[type] = fields[type].reduce(arrToSelectObject, {});
-    // }
-
-    // support includes, but only a level deep for now (recursive includes,
-    // especially if done in an efficient way query wise, are a pain in the ass).
-    if(includePaths) {
-      const populatedPaths = [];
-      const refPaths = util.getReferencePaths(model);
-
-      includePaths = includePaths.map((it) => it.split("."));
-      includePaths.forEach((pathParts) => {
-        // first, check that the include path is valid.
-        if(!arrayContains(refPaths, pathParts[0])) {
-          let title = "Invalid include path.";
-          let detail = `Resources of type "${type}" don't have a(n) "${pathParts[0]}" relationship.`;
-          throw new APIError(400, undefined, title, detail);
-        }
+      // do sorting
+      if(Array.isArray(sorts)) {
+        queryBuilder.sort(sorts.join(" "));
+      }
 
-        if(pathParts.length > 1) {
-          throw new APIError(501, undefined, "Multi-level include paths aren't yet supported.");
-        }
+      // filter out invalid records with simple fields equality.
+      // note that there's a non-trivial risk of sql-like injection here.
+      // we're mostly protected by the fact that we're treating the filter's
+      // value as a single string, though, and not parsing as JSON.
+      if(typeof filters === "object" && !Array.isArray(filters)) {
+        queryBuilder.where(filters);
+      }
 
-        // Finally, do the population
-        populatedPaths.push(pathParts[0]);
-        queryBuilder.populate(pathParts[0]);
-      });
+      // in an ideal world, we'd use mongoose here to filter the fields before
+      // querying. But, because the fields to filter can be scoped by type and
+      // we don't always know about a document's type until after query (becuase
+      // of discriminator keys), and because filtering out fields can really
+      // complicate population for includes, we don't yet filter at query time but
+      // instead just hide filtered fields in @docToResource. There is a more-
+      // efficient way to do this down the road, though--something like taking the
+      // provided fields and expanding them just enough (by looking at the type
+      // heirarachy and the relationship paths) to make sure that we're not going
+      // to run into any of the problems outlined above, while still querying for
+      // less data than we would without any fields restriction. For reference, the
+      // code for safely using the user's `fields` input, by putting them into a
+      // mongoose `.select()` object so that the user can't prefix a field with a
+      // minus on input to affect the query, is below.
+      // Reference: http://mongoosejs.com/docs/api.html#query_Query-select.
+      // let arrToSelectObject = (prev, curr) => { prev[curr] = 1; return prev; };
+      // for(let type in fields) {
+      //   fields[type] = fields[type].reduce(arrToSelectObject, {});
+      // }
+
+      // support includes, but only a level deep for now (recursive includes,
+      // especially if done in an efficient way query wise, are a pain in the ass).
+      if(includePaths) {
+        const populatedPaths = [];
+        const refPaths = util.getReferencePaths(model);
+
+        includePaths = includePaths.map((it) => it.split("."));
+        includePaths.forEach((pathParts) => {
+          // first, check that the include path is valid.
+          if(!arrayContains(refPaths, pathParts[0])) {
+            let title = "Invalid include path.";
+            let detail = `Resources of type "${type}" don't have a(n) "${pathParts[0]}" relationship.`;
+            throw new APIError(400, undefined, title, detail);
+          }
+
+          if(pathParts.length > 1) {
+            throw new APIError(501, undefined, "Multi-level include paths aren't yet supported.");
+          }
+
+          // Finally, do the population
+          populatedPaths.push(pathParts[0]);
+          queryBuilder.populate(pathParts[0]);
+        });
 
-      let includedResources = [];
-      primaryDocumentsPromise = Q(queryBuilder.exec()).then((docs) => {
-        forEachArrayOrVal(docs, (doc) => {
-          // There's no gaurantee that the doc (or every doc) was found
-          // and we can't populate paths on a non-existent doc.
-          if(!doc) return;
-
-          populatedPaths.forEach((path) => {
-            // if it's a toOne relationship, doc[path] will be a doc or undefined;
-            // if it's a toMany relationship, we have an array (or undefined).
-            let refDocs = Array.isArray(doc[path]) ? doc[path] : [doc[path]];
-            refDocs.forEach((it) => {
-              // only include if it's not undefined.
-              if(it) {
-                includedResources.push(
-                  this.constructor.docToResource(it, pluralizer, fields)
-                );
-              }
+        let includedResources = [];
+        primaryDocumentsPromise = Q(queryBuilder.exec()).then((docs) => {
+          forEachArrayOrVal(docs, (doc) => {
+            // There's no gaurantee that the doc (or every doc) was found
+            // and we can't populate paths on a non-existent doc.
+            if(!doc) return;
+
+            populatedPaths.forEach((path) => {
+              // if it's a toOne relationship, doc[path] will be a doc or undefined;
+              // if it's a toMany relationship, we have an array (or undefined).
+              let refDocs = Array.isArray(doc[path]) ? doc[path] : [doc[path]];
+              refDocs.forEach((it) => {
+                // only include if it's not undefined.
+                if(it) {
+                  includedResources.push(
+                    this.constructor.docToResource(it, pluralizer, fields)
+                  );
+                }
+              });
             });
           });
-        });
 
-        return docs;
-      });
+          return docs;
+        });
 
-      includedResourcesPromise = primaryDocumentsPromise.then(() =>
-        new Collection(includedResources)
-      );
-    }
+        includedResourcesPromise = primaryDocumentsPromise.then(() =>
+          new Collection(includedResources)
+        );
+      }
 
-    else {
-      primaryDocumentsPromise = Q(queryBuilder.exec());
-    }
+      else {
+        primaryDocumentsPromise = Q(queryBuilder.exec());
+      }
 
-    return Q.all([primaryDocumentsPromise.then((it) => {
-      const makeCollection = !idOrIds || Array.isArray(idOrIds) ? true : false;
-      return this.constructor.docsToResourceOrCollection(it, makeCollection, pluralizer, fields);
-    }), includedResourcesPromise]).catch(util.errorHandler);
+      return Q.all([primaryDocumentsPromise.then((it) => {
+        const makeCollection = !idOrIds || Array.isArray(idOrIds) ? true : false;
+        return this.constructor.docsToResourceOrCollection(it, makeCollection, pluralizer, fields);
+      }), includedResourcesPromise]).catch(util.errorHandler);
+    });
   }
 
   /**
@@ -202,88 +202,89 @@ export default class MongooseAdapter {
       return it.id;
     });
 
-    const [mode, idQuery] = this.constructor.getIdQueryType(idOrIds);
-
-    return Q(model[mode](idQuery).exec()).then((docs) => {
-      const successfulSavesPromises = [];
-
-      // if some ids were invalid/deleted/not found, we can't let *any* update
-      // succeed. this is the beginning of our simulation of transactions.
-      // There are two types of invalid cases here: we looked up one or more
-      // docs and got none back (i.e. docs === null) or we looked up an array of
-      // docs and got back docs that were missing some requested ids.
-      if(docs === null) {
-        throw new APIError(404, undefined, "No matching resource found.");
-      }
-      else {
-        const idOrIdsAsArray = Array.isArray(idOrIds) ? idOrIds : [idOrIds];
-        const docIdOrIdsAsArray = Array.isArray(docs) ? docs.map(it => it.id) : [docs.id];
-
-        if(!arrayValuesMatch(idOrIdsAsArray, docIdOrIdsAsArray)) {
-          let title = "Some of the resources you're trying to update could not be found.";
-          throw new APIError(404, undefined, title);
+    return this.constructor.getIdQueryType(idOrIds, model)
+      .then(([ mode, idQuery]) => model[mode](idQuery).exec())
+      .then((docs) => {
+        const successfulSavesPromises = [];
+
+        // if some ids were invalid/deleted/not found, we can't let *any* update
+        // succeed. this is the beginning of our simulation of transactions.
+        // There are two types of invalid cases here: we looked up one or more
+        // docs and got none back (i.e. docs === null) or we looked up an array of
+        // docs and got back docs that were missing some requested ids.
+        if(docs === null) {
+          throw new APIError(404, undefined, "No matching resource found.");
         }
-      }
+        else {
+          const idOrIdsAsArray = Array.isArray(idOrIds) ? idOrIds : [idOrIds];
+          const docIdOrIdsAsArray = Array.isArray(docs) ? docs.map(it => it.id) : [docs.id];
 
-      forEachArrayOrVal(docs, (currDoc) => {
-        let newResource = changeSets[currDoc.id];
-
-        // Allowing the type to change is a bit of a pain. If the type's
-        // changed, it means the mongoose Model representing the doc must be
-        // different too. So we have to get the data from the old doc with
-        // .toObject(), change its discriminator, and then create an instance
-        // of the new model with that data. We also have to mark that new
-        // instance as not representing a new document, so that mongoose will
-        // do an update query rather than a save. Finally, we have to do all
-        // this before updating other attributes, so that they're correctly
-        // marked as modified when changed.
-        const currentModelName = currDoc.constructor.modelName;
-        const newModelName = this.constructor.getModelName(newResource.type, singular);
-        if(currentModelName !== newModelName) {
-          const newDoc = currDoc.toObject({virtuals: true, getters: true});
-          const NewModelConstructor = this.getModel(newModelName);
-          newDoc[currDoc.constructor.schema.options.discriminatorKey] = newModelName;
-
-          // replace the currDoc with our new creation.
-          currDoc = new NewModelConstructor(newDoc);
-          currDoc.isNew = false;
+          if(!arrayValuesMatch(idOrIdsAsArray, docIdOrIdsAsArray)) {
+            let title = "Some of the resources you're trying to update could not be found.";
+            throw new APIError(404, undefined, title);
+          }
         }
 
-        // update all attributes and links provided, ignoring type/meta/id.
-        currDoc.set(util.resourceToDocObject(newResource));
-
-        successfulSavesPromises.push(
-          Q.Promise(function (resolve, reject) {
-            currDoc.save((err, doc) => {
-              if(err) reject(err);
-              resolve(doc);
-            });
-          })
-        );
-      });
+        forEachArrayOrVal(docs, (currDoc) => {
+          let newResource = changeSets[currDoc.id];
+
+          // Allowing the type to change is a bit of a pain. If the type's
+          // changed, it means the mongoose Model representing the doc must be
+          // different too. So we have to get the data from the old doc with
+          // .toObject(), change its discriminator, and then create an instance
+          // of the new model with that data. We also have to mark that new
+          // instance as not representing a new document, so that mongoose will
+          // do an update query rather than a save. Finally, we have to do all
+          // this before updating other attributes, so that they're correctly
+          // marked as modified when changed.
+          const currentModelName = currDoc.constructor.modelName;
+          const newModelName = this.constructor.getModelName(newResource.type, singular);
+          if(currentModelName !== newModelName) {
+            const newDoc = currDoc.toObject({virtuals: true, getters: true});
+            const NewModelConstructor = this.getModel(newModelName);
+            newDoc[currDoc.constructor.schema.options.discriminatorKey] = newModelName;
+
+            // replace the currDoc with our new creation.
+            currDoc = new NewModelConstructor(newDoc);
+            currDoc.isNew = false;
+          }
+
+          // update all attributes and links provided, ignoring type/meta/id.
+          currDoc.set(util.resourceToDocObject(newResource));
+
+          successfulSavesPromises.push(
+            Q.Promise(function (resolve, reject) {
+              currDoc.save((err, doc) => {
+                if(err) reject(err);
+                resolve(doc);
+              });
+            })
+          );
+        });
 
-      return Q.all(successfulSavesPromises);
-    }).then((docs) => {
-      let makeCollection = resourceOrCollection instanceof Collection;
-      return this.constructor.docsToResourceOrCollection(docs, makeCollection, plural);
-    }).catch(util.errorHandler);
+        return Q.all(successfulSavesPromises);
+      }).then((docs) => {
+        let makeCollection = resourceOrCollection instanceof Collection;
+        return this.constructor.docsToResourceOrCollection(docs, makeCollection, plural);
+      }).catch(util.errorHandler);
   }
 
   delete(parentType, idOrIds) {
     const model = this.getModel(this.constructor.getModelName(parentType));
-    const [mode, idQuery] = this.constructor.getIdQueryType(idOrIds);
 
-    if(!idOrIds) {
-      return Q.Promise((resolve, reject) => {
-        reject(new APIError(400, undefined, "You must specify some resources to delete"));
-      });
-    }
+    return this.constructor.getIdQueryType(idOrIds, model).then(([ mode, idQuery ]) => {
+      if(!idOrIds) {
+        return Q.Promise((resolve, reject) => {
+          reject(new APIError(400, undefined, "You must specify some resources to delete"));
+        });
+      }
 
-    return Q(model[mode](idQuery).exec()).then((docs) => {
-      if(!docs) throw new APIError(404, undefined, "No matching resource found.");
-      forEachArrayOrVal(docs, (it) => { it.remove(); });
-      return docs;
-    }).catch(util.errorHandler);
+      return Q(model[mode](idQuery).exec()).then((docs) => {
+        if(!docs) throw new APIError(404, undefined, "No matching resource found.");
+        forEachArrayOrVal(docs, (it) => { it.remove(); });
+        return docs;
+      }).catch(util.errorHandler);
+    });
   }
 
   /**
@@ -603,27 +604,41 @@ export default class MongooseAdapter {
     return words.join(" ");
   }
 
-  static getIdQueryType(idOrIds) {
-    const mode = typeof idOrIds === "string" ? "findOne" : "find";
-    let idQuery;
+  static getIdQueryType(idOrIds, model) {
+    return Q.Promise((resolve, reject) => {
+      const mode = typeof idOrIds === "string" ? "findOne" : "find";
+      let ids = Array.isArray(idOrIds) ? idOrIds : [ idOrIds ];
+      let idQuery;
+      let tests = [];
 
-    if (typeof idOrIds === "string") {
-      if (!this.idIsValid(idOrIds)) {
-        throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
-      }
+      ids = ids.filter(id => Boolean(id));
 
-      idQuery = {_id: idOrIds};
-    }
+      ids.forEach(id => {
+        const testDoc = new model({ _id: id })
+        tests.push(testDoc.validate());
+      });
 
-    else if (Array.isArray(idOrIds)) {
-      if (!idOrIds.every(this.idIsValid)) {
-        throw new APIError(400, undefined, "Invalid ID.");
-      }
+      Q.allSettled(tests).then(results => {
+        let invalid = false;
 
-      idQuery = {_id: {"$in": idOrIds}};
-    }
+        results.forEach(result => {
+          if (result.reason && result.reason.errors._id) {
+            invalid = true;
+          }
+        });
 
-    return [mode, idQuery];
+        if (!invalid) {
+          const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
+          resolve([ mode, results.length ? idQuery : undefined ]);
+        } else {
+          if (mode === "findOne") {
+            reject(new APIError(404, undefined, "No matching resource found.", "Invalid ID."));
+          } else {
+            reject(new APIError(400, undefined, "Invalid ID."));
+          }
+        }
+      }).catch(reject);
+    });
   }
 
   static idIsValid(id) {
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index 36f7af8d..c146af6c 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -3,6 +3,8 @@ import APIError from "../../../../src/types/APIError";
 import mongoose from "mongoose";
 import MongooseAdapter from "../../../../src/db-adapters/Mongoose/MongooseAdapter";
 
+const School = mongoose.model('School');
+
 describe("Mongoose Adapter", () => {
   describe("its instances methods", () => {
     describe("getModel", () => {
@@ -69,37 +71,51 @@ describe("Mongoose Adapter", () => {
     });
 
     describe("getIdQueryType", () => {
-      it("should handle null input", () => {
-        const res = MongooseAdapter.getIdQueryType();
-        expect(res[0]).to.equal("find");
-        expect(res[1]).to.be.undefined;
+      it("should handle null input", function(done) {
+        MongooseAdapter.getIdQueryType(null, School).then(([ mode, idQuery ]) => {
+          expect(mode).to.equal("find");
+          expect(idQuery).to.be.undefined;
+          done();
+        }, done);
       });
 
       describe("string", () => {
-        it("should throw on invalid input", () => {
-          const fn = function() { MongooseAdapter.getIdQueryType("1"); };
-          expect(fn).to.throw(APIError);
+        it("should throw on invalid input", function(done) {
+          MongooseAdapter.getIdQueryType("1", School).then(res => {
+            expect(false).to.be.ok;
+          }, err => {
+            expect(err).to.be.instanceof(APIError);
+            done();
+          });
         });
 
-        it("should produce query on valid input", () => {
-          const res = MongooseAdapter.getIdQueryType("552c5e1c604d41e5836bb174");
-          expect(res[0]).to.equal("findOne");
-          expect(res[1]._id).to.equal("552c5e1c604d41e5836bb174");
+        it("should produce query on valid input", function(done) {
+          MongooseAdapter.getIdQueryType("552c5e1c604d41e5836bb174", School).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("findOne");
+            expect(idQuery._id).to.equal("552c5e1c604d41e5836bb174");
+            done();
+          }).catch(err => { console.log(err); done(err); });
         });
       });
 
       describe("array", () => {
-        it("should throw if any ids are invalid", () => {
-          const fn = function() { MongooseAdapter.getIdQueryType(["1", "552c5e1c604d41e5836bb174"]); };
-          expect(fn).to.throw(APIError);
+        it("should throw if any ids are invalid", function(done) {
+          MongooseAdapter.getIdQueryType(["1", "552c5e1c604d41e5836bb174"], School).then(res => {
+            expect(false).to.be.ok;
+          }, err => {
+            expect(err).to.be.instanceof(APIError);
+            done();
+          });
         });
 
-        it("should produce query on valid input", () => {
-          const res = MongooseAdapter.getIdQueryType(["552c5e1c604d41e5836bb174", "552c5e1c604d41e5836bb175"]);
-          expect(res[0]).to.equal("find");
-          expect(res[1]._id.$in).to.be.an.Array;
-          expect(res[1]._id.$in[0]).to.equal("552c5e1c604d41e5836bb174");
-          expect(res[1]._id.$in[1]).to.equal("552c5e1c604d41e5836bb175");
+        it("should produce query on valid input", function(done) {
+          MongooseAdapter.getIdQueryType(["552c5e1c604d41e5836bb174", "552c5e1c604d41e5836bb175"], School).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("find");
+            expect(idQuery._id.$in).to.be.an.Array;
+            expect(idQuery._id.$in[0]).to.equal("552c5e1c604d41e5836bb174");
+            expect(idQuery._id.$in[1]).to.equal("552c5e1c604d41e5836bb175");
+            done();
+          }, done);
         });
       });
     });

From da805ebd0e6944b11d1ddf19b6691d9b9306abeb Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Mon, 14 Mar 2016 21:40:38 +1300
Subject: [PATCH 02/11] Add NumberId and StringId models for testing

---
 test/app/database/index.js             | 6 +++++-
 test/app/database/models/numeric-id.js | 7 +++++++
 test/app/database/models/string-id.js  | 7 +++++++
 3 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100755 test/app/database/models/numeric-id.js
 create mode 100755 test/app/database/models/string-id.js

diff --git a/test/app/database/index.js b/test/app/database/index.js
index ca748b9d..31679cad 100644
--- a/test/app/database/index.js
+++ b/test/app/database/index.js
@@ -5,6 +5,8 @@ import fixtures from "node-mongoose-fixtures";
 import PersonModel from "./models/person";
 import makeSchoolModelConstructor from "./models/school";
 import OrganizationModelSchema from "./models/organization";
+import NumericId from "./models/numeric-id";
+import StringId from "./models/string-id";
 
 /*eslint-disable new-cap */
 const ObjectId = mongoose.Types.ObjectId;
@@ -19,7 +21,9 @@ const OrganizationSchema = OrganizationModelSchema.schema;
 const models = {
   Person: PersonModel,
   Organization: OrganizationModel,
-  School: makeSchoolModelConstructor(OrganizationModel, OrganizationSchema)
+  School: makeSchoolModelConstructor(OrganizationModel, OrganizationSchema),
+  NumericId,
+  StringId
 };
 
 fixtures.save("all", {
diff --git a/test/app/database/models/numeric-id.js b/test/app/database/models/numeric-id.js
new file mode 100755
index 00000000..e45c3ab5
--- /dev/null
+++ b/test/app/database/models/numeric-id.js
@@ -0,0 +1,7 @@
+import mongoose from "mongoose";
+
+const schema = mongoose.Schema({
+  _id: Number
+});
+
+export default mongoose.model("NumericId", schema);
diff --git a/test/app/database/models/string-id.js b/test/app/database/models/string-id.js
new file mode 100755
index 00000000..3da5f3f4
--- /dev/null
+++ b/test/app/database/models/string-id.js
@@ -0,0 +1,7 @@
+import mongoose from "mongoose";
+
+const schema = mongoose.Schema({
+  _id: String
+});
+
+export default mongoose.model("StringId", schema);

From 6c51686da7b2fe645aceb1c0e11103b8827ab68f Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Mon, 14 Mar 2016 21:41:28 +1300
Subject: [PATCH 03/11] Refactor the getIdQueryType method to separate the
 idIsValid logic

---
 src/db-adapters/Mongoose/MongooseAdapter.js   |  61 +++----
 .../db-adapters/Mongoose/MongooseAdapter.js   | 155 +++++++++++++++---
 2 files changed, 156 insertions(+), 60 deletions(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index d499e917..1ffb1cfc 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -605,43 +605,34 @@ export default class MongooseAdapter {
   }
 
   static getIdQueryType(idOrIds, model) {
-    return Q.Promise((resolve, reject) => {
-      const mode = typeof idOrIds === "string" ? "findOne" : "find";
-      let ids = Array.isArray(idOrIds) ? idOrIds : [ idOrIds ];
-      let idQuery;
-      let tests = [];
-
-      ids = ids.filter(id => Boolean(id));
-
-      ids.forEach(id => {
-        const testDoc = new model({ _id: id })
-        tests.push(testDoc.validate());
-      });
-
-      Q.allSettled(tests).then(results => {
-        let invalid = false;
-
-        results.forEach(result => {
-          if (result.reason && result.reason.errors._id) {
-            invalid = true;
-          }
-        });
-
-        if (!invalid) {
-          const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
-          resolve([ mode, results.length ? idQuery : undefined ]);
-        } else {
-          if (mode === "findOne") {
-            reject(new APIError(404, undefined, "No matching resource found.", "Invalid ID."));
-          } else {
-            reject(new APIError(400, undefined, "Invalid ID."));
-          }
-        }
-      }).catch(reject);
+    const mode = typeof idOrIds === "string" ? "findOne" : "find";
+    const ids = Array.isArray(idOrIds) ? idOrIds : [ idOrIds ];
+    const tests = ids
+      .filter(id => id != null) // mongo treats these as to-be-generated and therefore valid
+      .map(id => this.idIsValid(id, model));
+
+    return Q.all(tests).then((validIds) => {
+      const idQuery = { _id: mode === "find" ? { $in: validIds } : validIds[0] };
+      return [ mode, validIds.length ? idQuery : undefined ];
+    }, (err) => {
+      if (mode === "findOne") {
+        throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
+      } else {
+        throw new APIError(400, undefined, "Invalid ID.");
+      }
     });
   }
 
-  static idIsValid(id) {
-    return typeof id === "string" && /^[0-9a-fA-F]{24}$/.test(id);
+  static idIsValid(id, model) {
+    return Q.Promise((resolve, reject) => {
+      if (id == null) {
+        return reject();
+      }
+
+      return (new model({ _id: id })).validate().then(
+        (res) => resolve(id),
+        (err) => err.errors && err.errors._id ? reject(err.errors._id) : resolve(id)
+      );
+    });
   }
 }
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index c146af6c..565aca14 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -1,9 +1,12 @@
+import Q from "q";
 import {expect} from "chai";
 import APIError from "../../../../src/types/APIError";
 import mongoose from "mongoose";
 import MongooseAdapter from "../../../../src/db-adapters/Mongoose/MongooseAdapter";
 
 const School = mongoose.model('School');
+const NumericId = mongoose.model('NumericId');
+const StringId = mongoose.model('StringId');
 
 describe("Mongoose Adapter", () => {
   describe("its instances methods", () => {
@@ -76,77 +79,179 @@ describe("Mongoose Adapter", () => {
           expect(mode).to.equal("find");
           expect(idQuery).to.be.undefined;
           done();
-        }, done);
+        }).catch(done);
       });
 
       describe("string", () => {
-        it("should throw on invalid input", function(done) {
+        it("should reject on invalid ObjectId input", function(done) {
           MongooseAdapter.getIdQueryType("1", School).then(res => {
             expect(false).to.be.ok;
           }, err => {
             expect(err).to.be.instanceof(APIError);
             done();
-          });
+          }).catch(done);
         });
 
-        it("should produce query on valid input", function(done) {
+        it("should reject on invalid numeric ID input", function(done) {
+          MongooseAdapter.getIdQueryType("123abc", NumericId).then(res => {
+            expect(false).to.be.ok;
+          }, err => {
+            expect(err).to.be.instanceof(APIError);
+            done();
+          }).catch(done);
+        });
+
+        it("should produce query on valid ObjectId input", function(done) {
           MongooseAdapter.getIdQueryType("552c5e1c604d41e5836bb174", School).then(([ mode, idQuery ]) => {
             expect(mode).to.equal("findOne");
             expect(idQuery._id).to.equal("552c5e1c604d41e5836bb174");
             done();
-          }).catch(err => { console.log(err); done(err); });
+          }).catch(done);
+        });
+
+        it("should produce query on valid numeric ID input", function(done) {
+          MongooseAdapter.getIdQueryType("0", NumericId).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("findOne");
+            expect(idQuery._id).to.equal("0");
+            done();
+          }).catch(done);
+        });
+
+        it("should produce query on valid string ID input", function(done) {
+          MongooseAdapter.getIdQueryType("null", StringId).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("findOne");
+            expect(idQuery._id).to.equal("null");
+            done();
+          }).catch(done);
         });
       });
 
       describe("array", () => {
-        it("should throw if any ids are invalid", function(done) {
+        it("should throw if any ObjectIds are invalid", function(done) {
           MongooseAdapter.getIdQueryType(["1", "552c5e1c604d41e5836bb174"], School).then(res => {
             expect(false).to.be.ok;
           }, err => {
             expect(err).to.be.instanceof(APIError);
             done();
-          });
+          }).catch(done);
+        });
+
+        it("should throw if any numeric ids are invalid", function(done) {
+          MongooseAdapter.getIdQueryType(["abc", "123"], NumericId).then(res => {
+            expect(false).to.be.ok;
+          }, err => {
+            expect(err).to.be.instanceof(APIError);
+            done();
+          }).catch(done);
         });
 
-        it("should produce query on valid input", function(done) {
+        it("should produce query on valid ObjectId input", function(done) {
           MongooseAdapter.getIdQueryType(["552c5e1c604d41e5836bb174", "552c5e1c604d41e5836bb175"], School).then(([ mode, idQuery ]) => {
             expect(mode).to.equal("find");
             expect(idQuery._id.$in).to.be.an.Array;
             expect(idQuery._id.$in[0]).to.equal("552c5e1c604d41e5836bb174");
             expect(idQuery._id.$in[1]).to.equal("552c5e1c604d41e5836bb175");
             done();
-          }, done);
+          }).catch(done);
+        });
+
+        it("should produce query on valid numeric ID input", function(done) {
+          MongooseAdapter.getIdQueryType(["0", "1"], NumericId).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("find");
+            expect(idQuery._id.$in).to.be.an.Array;
+            expect(idQuery._id.$in[0]).to.equal("0");
+            expect(idQuery._id.$in[1]).to.equal("1");
+            done();
+          }).catch(done);
+        });
+
+        it("should produce query on valid string ID input", function(done) {
+          MongooseAdapter.getIdQueryType(["a", "null"], StringId).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("find");
+            expect(idQuery._id.$in).to.be.an.Array;
+            expect(idQuery._id.$in[0]).to.equal("a");
+            expect(idQuery._id.$in[1]).to.equal("null");
+            done();
+          }).catch(done);
         });
       });
     });
 
     describe("idIsValid", () => {
-      it("should reject all == null input", () => {
-        expect(MongooseAdapter.idIsValid()).to.not.be.ok;
-        expect(MongooseAdapter.idIsValid(null)).to.not.be.ok;
-        expect(MongooseAdapter.idIsValid(undefined)).to.not.be.ok;
+      it("should reject all == null input", function(done) {
+        const tests = [
+          MongooseAdapter.idIsValid(School),
+          MongooseAdapter.idIsValid(NumericId),
+          MongooseAdapter.idIsValid(StringId),
+          MongooseAdapter.idIsValid(null, School),
+          MongooseAdapter.idIsValid(null, NumericId),
+          MongooseAdapter.idIsValid(null, StringId),
+          MongooseAdapter.idIsValid(undefined, School),
+          MongooseAdapter.idIsValid(undefined, NumericId),
+          MongooseAdapter.idIsValid(undefined, StringId),
+        ];
+
+        Q.allSettled(tests).then((res) => {
+          res.forEach(result => expect(result.state).to.equal("rejected"));
+          done();
+        }).catch(done);
       });
 
-      it("should reject bad input type", () => {
-        expect(MongooseAdapter.idIsValid(true)).to.not.be.ok;
+      it("should reject bad input type", function(done) {
+        const tests = [
+          MongooseAdapter.idIsValid(true, School),
+          MongooseAdapter.idIsValid(false, School),
+          MongooseAdapter.idIsValid("not hex", School),
+          MongooseAdapter.idIsValid([], School),
+          MongooseAdapter.idIsValid("1234567890abcdef", School),
+          MongooseAdapter.idIsValid(1234567890, School),
+          MongooseAdapter.idIsValid("NaN", NumericId),
+          MongooseAdapter.idIsValid("one", NumericId),
+          MongooseAdapter.idIsValid([], NumericId),
+          MongooseAdapter.idIsValid(true, NumericId),
+          MongooseAdapter.idIsValid(false, NumericId)
+          // StringId should except anything != null
+        ];
+
+        Q.allSettled(tests).then((res) => {
+          res.forEach(result => expect(result.state).to.equal("rejected"));
+          done();
+        }).catch(done);
       });
 
-      it("should reject empty string", () => {
-        expect(MongooseAdapter.idIsValid("")).to.not.be.ok;
+      it("should reject empty string", function(done) {
+        const tests = [
+          MongooseAdapter.idIsValid("", School),
+          MongooseAdapter.idIsValid("", NumericId)
+          // StringId should except anything != null
+        ];
+
+        Q.allSettled(tests).then((res) => {
+          res.forEach(result => expect(result.state).to.equal("rejected"));
+          done();
+        }).catch(done);
       });
 
       // the string coming into the MongooseAdapter needs to be the 24-character,
       // hex encoded version of the ObjectId, not an arbitrary 12 byte string.
-      it("should reject 12-character strings", () => {
-        expect(MongooseAdapter.idIsValid("aaabbbccc111")).to.not.be.ok;
-      });
-
-      it("should reject numbers", () => {
-        expect(MongooseAdapter.idIsValid(1)).to.not.be.ok;
+      it("should reject 12-character strings", function(done) {
+        MongooseAdapter.idIsValid("aaabbbccc111", School)
+          .then(() => expect(false).to.be.ok)
+          .catch(() => done());
       });
 
-      it("should accpet valid hex string", () => {
-        expect(MongooseAdapter.idIsValid("552c5e1c604d41e5836bb175")).to.be.ok;
+      it("should accpet valid IDs", function(done) {
+        const tests = [
+          MongooseAdapter.idIsValid("552c5e1c604d41e5836bb175", School),
+          MongooseAdapter.idIsValid("123", NumericId),
+          MongooseAdapter.idIsValid(123, NumericId),
+          MongooseAdapter.idIsValid("0", NumericId),
+          MongooseAdapter.idIsValid(0, NumericId),
+          MongooseAdapter.idIsValid("0", StringId),
+          MongooseAdapter.idIsValid("null", StringId)
+        ];
+
+        Q.all(tests).then((res) => done(), done);
       });
     });
 

From eb23428ed7781e0abb7600084c5cfe75beb12869 Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Mon, 14 Mar 2016 21:47:11 +1300
Subject: [PATCH 04/11] Make the _id of the NumericId and StringId models
 required

---
 test/app/database/models/numeric-id.js            | 5 ++++-
 test/app/database/models/string-id.js             | 5 ++++-
 test/unit/db-adapters/Mongoose/MongooseAdapter.js | 4 ++--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/test/app/database/models/numeric-id.js b/test/app/database/models/numeric-id.js
index e45c3ab5..da1fac82 100755
--- a/test/app/database/models/numeric-id.js
+++ b/test/app/database/models/numeric-id.js
@@ -1,7 +1,10 @@
 import mongoose from "mongoose";
 
 const schema = mongoose.Schema({
-  _id: Number
+  _id: {
+    type: Number,
+    required: true
+  }
 });
 
 export default mongoose.model("NumericId", schema);
diff --git a/test/app/database/models/string-id.js b/test/app/database/models/string-id.js
index 3da5f3f4..fb8deadf 100755
--- a/test/app/database/models/string-id.js
+++ b/test/app/database/models/string-id.js
@@ -1,7 +1,10 @@
 import mongoose from "mongoose";
 
 const schema = mongoose.Schema({
-  _id: String
+  _id: {
+    type: String,
+    required: true
+  }
 });
 
 export default mongoose.model("StringId", schema);
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index 565aca14..00a550c4 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -222,8 +222,8 @@ describe("Mongoose Adapter", () => {
       it("should reject empty string", function(done) {
         const tests = [
           MongooseAdapter.idIsValid("", School),
-          MongooseAdapter.idIsValid("", NumericId)
-          // StringId should except anything != null
+          MongooseAdapter.idIsValid("", NumericId),
+          MongooseAdapter.idIsValid("", StringId)
         ];
 
         Q.allSettled(tests).then((res) => {

From 08701c1fd13ef90bae848d777237a9d13fc09c1d Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Mon, 14 Mar 2016 22:35:29 +1300
Subject: [PATCH 05/11] Tweak behaviour of getIdQueryType when idOrIds is null

---
 src/db-adapters/Mongoose/MongooseAdapter.js | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index 1ffb1cfc..36fad775 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -605,15 +605,16 @@ export default class MongooseAdapter {
   }
 
   static getIdQueryType(idOrIds, model) {
-    const mode = typeof idOrIds === "string" ? "findOne" : "find";
-    const ids = Array.isArray(idOrIds) ? idOrIds : [ idOrIds ];
-    const tests = ids
-      .filter(id => id != null) // mongo treats these as to-be-generated and therefore valid
-      .map(id => this.idIsValid(id, model));
-
-    return Q.all(tests).then((validIds) => {
-      const idQuery = { _id: mode === "find" ? { $in: validIds } : validIds[0] };
-      return [ mode, validIds.length ? idQuery : undefined ];
+    if (idOrIds == null) {
+      return Q.resolve([ "find", undefined ]);
+    }
+
+    const [ mode, ids ] = Array.isArray(idOrIds) ? [ "find", idOrIds ] : [ "findOne", [ idOrIds ] ];
+    const tests = ids.map(id => this.idIsValid(id, model));
+
+    return Q.all(tests).then(() => {
+      const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
+      return [ mode, ids.length ? idQuery : undefined ];
     }, (err) => {
       if (mode === "findOne") {
         throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
@@ -630,7 +631,7 @@ export default class MongooseAdapter {
       }
 
       return (new model({ _id: id })).validate().then(
-        (res) => resolve(id),
+        (res) => resolve(true),
         (err) => err.errors && err.errors._id ? reject(err.errors._id) : resolve(id)
       );
     });

From 4653d58fca94f983650921e9dac4e6a9a0dfe040 Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Tue, 15 Mar 2016 08:54:31 +1300
Subject: [PATCH 06/11] Have idIsValid resolve to false for an invalid ID, not
 reject

---
 src/db-adapters/Mongoose/MongooseAdapter.js   | 24 ++++++++++---------
 .../db-adapters/Mongoose/MongooseAdapter.js   | 14 +++++------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index 36fad775..6345b68b 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -606,20 +606,22 @@ export default class MongooseAdapter {
 
   static getIdQueryType(idOrIds, model) {
     if (idOrIds == null) {
-      return Q.resolve([ "find", undefined ]);
+      return Q([ "find", undefined ]);
     }
 
     const [ mode, ids ] = Array.isArray(idOrIds) ? [ "find", idOrIds ] : [ "findOne", [ idOrIds ] ];
-    const tests = ids.map(id => this.idIsValid(id, model));
-
-    return Q.all(tests).then(() => {
-      const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
-      return [ mode, ids.length ? idQuery : undefined ];
-    }, (err) => {
-      if (mode === "findOne") {
-        throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
+    const idValidationPromises = ids.map(id => this.idIsValid(id, model));
+
+    return Q.all(idValidationPromises).then((idsAreValid) => {
+      if (idsAreValid.every(valid => valid)) {
+        const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
+        return [ mode, ids.length ? idQuery : undefined ];
       } else {
-        throw new APIError(400, undefined, "Invalid ID.");
+        if (mode === "findOne") {
+          throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
+        } else {
+          throw new APIError(400, undefined, "Invalid ID.");
+        }
       }
     });
   }
@@ -632,7 +634,7 @@ export default class MongooseAdapter {
 
       return (new model({ _id: id })).validate().then(
         (res) => resolve(true),
-        (err) => err.errors && err.errors._id ? reject(err.errors._id) : resolve(id)
+        (err) => err.errors && err.errors._id ? resolve(false) : resolve(true)
       );
     });
   }
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index 00a550c4..e27fb4a2 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -178,7 +178,7 @@ describe("Mongoose Adapter", () => {
     });
 
     describe("idIsValid", () => {
-      it("should reject all == null input", function(done) {
+      it("should refuse all == null input", function(done) {
         const tests = [
           MongooseAdapter.idIsValid(School),
           MongooseAdapter.idIsValid(NumericId),
@@ -188,16 +188,16 @@ describe("Mongoose Adapter", () => {
           MongooseAdapter.idIsValid(null, StringId),
           MongooseAdapter.idIsValid(undefined, School),
           MongooseAdapter.idIsValid(undefined, NumericId),
-          MongooseAdapter.idIsValid(undefined, StringId),
+          MongooseAdapter.idIsValid(undefined, StringId)
         ];
 
         Q.allSettled(tests).then((res) => {
-          res.forEach(result => expect(result.state).to.equal("rejected"));
+          res.forEach(result => expect(result.result).to.not.be.ok);
           done();
         }).catch(done);
       });
 
-      it("should reject bad input type", function(done) {
+      it("should refuse a bad input type", function(done) {
         const tests = [
           MongooseAdapter.idIsValid(true, School),
           MongooseAdapter.idIsValid(false, School),
@@ -214,12 +214,12 @@ describe("Mongoose Adapter", () => {
         ];
 
         Q.allSettled(tests).then((res) => {
-          res.forEach(result => expect(result.state).to.equal("rejected"));
+          res.forEach(result => expect(result.result).to.not.be.ok);
           done();
         }).catch(done);
       });
 
-      it("should reject empty string", function(done) {
+      it("should refuse an empty string", function(done) {
         const tests = [
           MongooseAdapter.idIsValid("", School),
           MongooseAdapter.idIsValid("", NumericId),
@@ -227,7 +227,7 @@ describe("Mongoose Adapter", () => {
         ];
 
         Q.allSettled(tests).then((res) => {
-          res.forEach(result => expect(result.state).to.equal("rejected"));
+          res.forEach(result => expect(result.result).to.not.be.ok);
           done();
         }).catch(done);
       });

From c6ce8eb0c35c133ca6fff8ce614ec2db90b4169b Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Tue, 15 Mar 2016 09:18:21 +1300
Subject: [PATCH 07/11] Update eslint and friends

eslint@latest and babel-eslint@latest are currently incompatible and so
I have pinned eslint@2.2 for now, as per the advice in
babel/babel-eslint#267
---
 .eslintrc    | 2 +-
 package.json | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/.eslintrc b/.eslintrc
index 569f060b..99ad4231 100644
--- a/.eslintrc
+++ b/.eslintrc
@@ -4,7 +4,7 @@
     "strict": 0,
 
     "curly": 0,
-    "indent": [2, 2, {indentSwitchCase: true}],
+    "indent": [2, 2, {"SwitchCase": 1}],
     "brace-style": [2, "stroustrup", { "allowSingleLine": true }],
     "new-cap": [2, {"capIsNewExceptions": ["Q", "Promise", "ValueObject", "Maybe", "mongoose.Schema", "Schema"]}],
     "quotes": [2, "double", "avoid-escape"],
diff --git a/package.json b/package.json
index 73f1f6a4..10e06b5a 100644
--- a/package.json
+++ b/package.json
@@ -14,7 +14,7 @@
   "license": "LGPL-3.0",
   "main": "index.js",
   "dependencies": {
-    "babel-runtime": "5.6.17",
+    "babel-runtime": "^5.8.35",
     "co": "4.5.x",
     "content-type": "1.x.x",
     "dasherize": "2.0.x",
@@ -32,11 +32,11 @@
   },
   "devDependencies": {
     "babel": "5.6.14",
-    "babel-eslint": "3.x.x",
+    "babel-eslint": "^5.0.0",
     "chai": "^1.9.2",
-    "coveralls": "^2.11.4",
     "chai-subset": "^1.1.0",
-    "eslint": "0.x.x",
+    "coveralls": "^2.11.4",
+    "eslint": "~2.2.0",
     "express": "4.x.x",
     "gulp": "^3.8.6",
     "istanbul": "0.3.x",

From 5a1458c3ac7093bc05ec8a439fc76fc3e98a64e2 Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Tue, 15 Mar 2016 09:18:29 +1300
Subject: [PATCH 08/11] Lint

---
 src/db-adapters/Mongoose/MongooseAdapter.js       | 14 ++++++++------
 src/steps/pre-query/parse-request-primary.js      |  2 +-
 test/unit/db-adapters/Mongoose/MongooseAdapter.js |  6 +++---
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index 6345b68b..fec22a47 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -604,35 +604,37 @@ export default class MongooseAdapter {
     return words.join(" ");
   }
 
-  static getIdQueryType(idOrIds, model) {
+  static getIdQueryType(idOrIds, Model) {
     if (idOrIds == null) {
       return Q([ "find", undefined ]);
     }
 
     const [ mode, ids ] = Array.isArray(idOrIds) ? [ "find", idOrIds ] : [ "findOne", [ idOrIds ] ];
-    const idValidationPromises = ids.map(id => this.idIsValid(id, model));
+    const idValidationPromises = ids.map(id => this.idIsValid(id, Model));
 
     return Q.all(idValidationPromises).then((idsAreValid) => {
       if (idsAreValid.every(valid => valid)) {
         const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
         return [ mode, ids.length ? idQuery : undefined ];
-      } else {
+      }
+      else {
         if (mode === "findOne") {
           throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
-        } else {
+        }
+        else {
           throw new APIError(400, undefined, "Invalid ID.");
         }
       }
     });
   }
 
-  static idIsValid(id, model) {
+  static idIsValid(id, Model) {
     return Q.Promise((resolve, reject) => {
       if (id == null) {
         return reject();
       }
 
-      return (new model({ _id: id })).validate().then(
+      return (new Model({ _id: id })).validate().then(
         (res) => resolve(true),
         (err) => err.errors && err.errors._id ? resolve(false) : resolve(true)
       );
diff --git a/src/steps/pre-query/parse-request-primary.js b/src/steps/pre-query/parse-request-primary.js
index 4eaedc56..44f3e949 100644
--- a/src/steps/pre-query/parse-request-primary.js
+++ b/src/steps/pre-query/parse-request-primary.js
@@ -37,7 +37,7 @@ export default function(data, parseAsLinkage) {
 
 function relationshipFromJSON(json) {
   if (typeof json.data === "undefined") {
-    throw new APIError(400, undefined, `Missing relationship linkage.`);
+    throw new APIError(400, undefined, "Missing relationship linkage.");
   }
 
   return new Relationship(linkageFromJSON(json.data));
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index e27fb4a2..690051b7 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -4,9 +4,9 @@ import APIError from "../../../../src/types/APIError";
 import mongoose from "mongoose";
 import MongooseAdapter from "../../../../src/db-adapters/Mongoose/MongooseAdapter";
 
-const School = mongoose.model('School');
-const NumericId = mongoose.model('NumericId');
-const StringId = mongoose.model('StringId');
+const School = mongoose.model("School");
+const NumericId = mongoose.model("NumericId");
+const StringId = mongoose.model("StringId");
 
 describe("Mongoose Adapter", () => {
   describe("its instances methods", () => {

From e1a38150fdfdf0a1d8ee83d5be630b5ba4e95e18 Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Tue, 15 Mar 2016 09:24:14 +1300
Subject: [PATCH 09/11] Use function() {} instead of () => {} in the failing
 integration test

---
 test/integration/patch-relationship/index.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/integration/patch-relationship/index.js b/test/integration/patch-relationship/index.js
index a5f5e60c..513db9f5 100644
--- a/test/integration/patch-relationship/index.js
+++ b/test/integration/patch-relationship/index.js
@@ -15,7 +15,7 @@ describe("Patching a relationship", () => {
     }).then(done, done);
   });
 
-  it("should support full replacement at a to-many relationship endpoint", (done) => {
+  it("should support full replacement at a to-many relationship endpoint", function(done) {
     const url = `/organizations/${orgId}/relationships/liaisons`;
 
     const setRelationship = (data, url) => { //eslint-disable-line no-shadow

From 2c5a9d34ad982615fb51b526af43f8948753d28d Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Tue, 15 Mar 2016 10:07:40 +1300
Subject: [PATCH 10/11] Switch back to a validateId method which rejects when
 an ID is invalid

---
 src/db-adapters/Mongoose/MongooseAdapter.js   | 25 +++---
 .../db-adapters/Mongoose/MongooseAdapter.js   | 76 ++++++++++---------
 2 files changed, 52 insertions(+), 49 deletions(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index fec22a47..dd17736d 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -610,25 +610,22 @@ export default class MongooseAdapter {
     }
 
     const [ mode, ids ] = Array.isArray(idOrIds) ? [ "find", idOrIds ] : [ "findOne", [ idOrIds ] ];
-    const idValidationPromises = ids.map(id => this.idIsValid(id, Model));
-
-    return Q.all(idValidationPromises).then((idsAreValid) => {
-      if (idsAreValid.every(valid => valid)) {
-        const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
-        return [ mode, ids.length ? idQuery : undefined ];
+    const idValidationPromises = ids.map(id => this.validateId(id, Model));
+
+    return Q.all(idValidationPromises).then(() => {
+      const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
+      return [ mode, ids.length ? idQuery : undefined ];
+    }, err => {
+      if (mode === "findOne") {
+        throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
       }
       else {
-        if (mode === "findOne") {
-          throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
-        }
-        else {
-          throw new APIError(400, undefined, "Invalid ID.");
-        }
+        throw new APIError(400, undefined, "Invalid ID.");
       }
     });
   }
 
-  static idIsValid(id, Model) {
+  static validateId(id, Model) {
     return Q.Promise((resolve, reject) => {
       if (id == null) {
         return reject();
@@ -636,7 +633,7 @@ export default class MongooseAdapter {
 
       return (new Model({ _id: id })).validate().then(
         (res) => resolve(true),
-        (err) => err.errors && err.errors._id ? resolve(false) : resolve(true)
+        (err) => err.errors && err.errors._id ? reject(err.errors._id) : resolve(true)
       );
     });
   }
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index 690051b7..21175c0b 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -177,57 +177,63 @@ describe("Mongoose Adapter", () => {
       });
     });
 
-    describe("idIsValid", () => {
+    describe("validateId", () => {
       it("should refuse all == null input", function(done) {
         const tests = [
-          MongooseAdapter.idIsValid(School),
-          MongooseAdapter.idIsValid(NumericId),
-          MongooseAdapter.idIsValid(StringId),
-          MongooseAdapter.idIsValid(null, School),
-          MongooseAdapter.idIsValid(null, NumericId),
-          MongooseAdapter.idIsValid(null, StringId),
-          MongooseAdapter.idIsValid(undefined, School),
-          MongooseAdapter.idIsValid(undefined, NumericId),
-          MongooseAdapter.idIsValid(undefined, StringId)
+          MongooseAdapter.validateId(null, School),
+          MongooseAdapter.validateId(null, NumericId),
+          MongooseAdapter.validateId(null, StringId),
+          MongooseAdapter.validateId(undefined, School),
+          MongooseAdapter.validateId(undefined, NumericId),
+          MongooseAdapter.validateId(undefined, StringId)
         ];
 
         Q.allSettled(tests).then((res) => {
-          res.forEach(result => expect(result.result).to.not.be.ok);
+          res.forEach(result => expect(result.state).to.equal("rejected"));
           done();
         }).catch(done);
       });
 
       it("should refuse a bad input type", function(done) {
         const tests = [
-          MongooseAdapter.idIsValid(true, School),
-          MongooseAdapter.idIsValid(false, School),
-          MongooseAdapter.idIsValid("not hex", School),
-          MongooseAdapter.idIsValid([], School),
-          MongooseAdapter.idIsValid("1234567890abcdef", School),
-          MongooseAdapter.idIsValid(1234567890, School),
-          MongooseAdapter.idIsValid("NaN", NumericId),
-          MongooseAdapter.idIsValid("one", NumericId),
-          MongooseAdapter.idIsValid([], NumericId),
-          MongooseAdapter.idIsValid(true, NumericId),
-          MongooseAdapter.idIsValid(false, NumericId)
+          MongooseAdapter.validateId(true, School),
+          MongooseAdapter.validateId(false, School),
+          MongooseAdapter.validateId("not hex", School),
+          MongooseAdapter.validateId([], School),
+          MongooseAdapter.validateId("1234567890abcdef", School),
+          MongooseAdapter.validateId(1234567890, School),
+          MongooseAdapter.validateId("NaN", NumericId),
+          MongooseAdapter.validateId("one", NumericId),
+          MongooseAdapter.validateId([], NumericId),
+          MongooseAdapter.validateId(true, NumericId),
+          MongooseAdapter.validateId(false, NumericId)
           // StringId should except anything != null
         ];
 
         Q.allSettled(tests).then((res) => {
-          res.forEach(result => expect(result.result).to.not.be.ok);
+          res.forEach(result => {
+            expect(result.state).to.equal("rejected");
+            expect(result.reason.name).to.equal("CastError");
+            expect(result.reason.path).to.equal("_id");
+          });
+
           done();
         }).catch(done);
       });
 
       it("should refuse an empty string", function(done) {
         const tests = [
-          MongooseAdapter.idIsValid("", School),
-          MongooseAdapter.idIsValid("", NumericId),
-          MongooseAdapter.idIsValid("", StringId)
+          MongooseAdapter.validateId("", School),
+          MongooseAdapter.validateId("", NumericId),
+          MongooseAdapter.validateId("", StringId)
         ];
 
         Q.allSettled(tests).then((res) => {
-          res.forEach(result => expect(result.result).to.not.be.ok);
+          res.forEach(result => {
+            expect(result.state).to.equal("rejected");
+            expect(result.reason.name).to.match(/ValidatorError|CastError/);
+            expect(result.reason.path).to.equal("_id");
+          });
           done();
         }).catch(done);
       });
@@ -235,20 +241,20 @@ describe("Mongoose Adapter", () => {
       // the string coming into the MongooseAdapter needs to be the 24-character,
       // hex encoded version of the ObjectId, not an arbitrary 12 byte string.
       it("should reject 12-character strings", function(done) {
-        MongooseAdapter.idIsValid("aaabbbccc111", School)
+        MongooseAdapter.validateId("aaabbbccc111", School)
           .then(() => expect(false).to.be.ok)
           .catch(() => done());
       });
 
       it("should accpet valid IDs", function(done) {
         const tests = [
-          MongooseAdapter.idIsValid("552c5e1c604d41e5836bb175", School),
-          MongooseAdapter.idIsValid("123", NumericId),
-          MongooseAdapter.idIsValid(123, NumericId),
-          MongooseAdapter.idIsValid("0", NumericId),
-          MongooseAdapter.idIsValid(0, NumericId),
-          MongooseAdapter.idIsValid("0", StringId),
-          MongooseAdapter.idIsValid("null", StringId)
+          MongooseAdapter.validateId("552c5e1c604d41e5836bb175", School),
+          MongooseAdapter.validateId("123", NumericId),
+          MongooseAdapter.validateId(123, NumericId),
+          MongooseAdapter.validateId("0", NumericId),
+          MongooseAdapter.validateId(0, NumericId),
+          MongooseAdapter.validateId("0", StringId),
+          MongooseAdapter.validateId("null", StringId)
         ];
 
         Q.all(tests).then((res) => done(), done);

From 3afbb889f53e7e475f8bbfd8135f884902842cdc Mon Sep 17 00:00:00 2001
From: Carl Bennett <carlbennettnz@gmail.com>
Date: Tue, 15 Mar 2016 20:12:39 +1300
Subject: [PATCH 11/11] Fix incorrect check for input and add test to catch in
 future

---
 src/db-adapters/Mongoose/MongooseAdapter.js       | 2 +-
 test/unit/db-adapters/Mongoose/MongooseAdapter.js | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/db-adapters/Mongoose/MongooseAdapter.js b/src/db-adapters/Mongoose/MongooseAdapter.js
index dd17736d..a20c7e63 100644
--- a/src/db-adapters/Mongoose/MongooseAdapter.js
+++ b/src/db-adapters/Mongoose/MongooseAdapter.js
@@ -614,7 +614,7 @@ export default class MongooseAdapter {
 
     return Q.all(idValidationPromises).then(() => {
       const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
-      return [ mode, ids.length ? idQuery : undefined ];
+      return [ mode, idOrIds != null ? idQuery : undefined ];
     }, err => {
       if (mode === "findOne") {
         throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
diff --git a/test/unit/db-adapters/Mongoose/MongooseAdapter.js b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
index 21175c0b..b9e3823e 100644
--- a/test/unit/db-adapters/Mongoose/MongooseAdapter.js
+++ b/test/unit/db-adapters/Mongoose/MongooseAdapter.js
@@ -174,6 +174,15 @@ describe("Mongoose Adapter", () => {
             done();
           }).catch(done);
         });
+
+        it("should produce an empty query when passed an empty array of ids", function(done) {
+          MongooseAdapter.getIdQueryType([], School).then(([ mode, idQuery ]) => {
+            expect(mode).to.equal("find");
+            expect(idQuery._id.$in).to.be.an.Array
+            expect(idQuery._id.$in).to.have.lengthOf(0);
+            done();
+          }).catch(done);
+        });
       });
     });