Skip to content

Use mongoose to validate method to check for ID validity #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Use mongoose to validate method to check for ID validity #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
954b021
Use mongoose to validate method to check for ID validity, rather than
carlbennettnz Mar 13, 2016
da805eb
Add NumberId and StringId models for testing
carlbennettnz Mar 14, 2016
6c51686
Refactor the getIdQueryType method to separate the idIsValid logic
carlbennettnz Mar 14, 2016
eb23428
Make the _id of the NumericId and StringId models required
carlbennettnz Mar 14, 2016
08701c1
Tweak behaviour of getIdQueryType when idOrIds is null
carlbennettnz Mar 14, 2016
4653d58
Have idIsValid resolve to false for an invalid ID, not reject
carlbennettnz Mar 14, 2016
c6ce8eb
Update eslint and friends
carlbennettnz Mar 14, 2016
5a1458c
Lint
carlbennettnz Mar 14, 2016
e1a3815
Use function() {} instead of () => {} in the failing integration test
carlbennettnz Mar 14, 2016
2c5a9d3
Switch back to a validateId method which rejects when an ID is invalid
carlbennettnz Mar 14, 2016
3afbb88
Fix incorrect check for input and add test to catch in future
carlbennettnz Mar 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
8 changes: 4 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
364 changes: 186 additions & 178 deletions src/db-adapters/Mongoose/MongooseAdapter.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/steps/pre-query/parse-request-primary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
6 changes: 5 additions & 1 deletion test/app/database/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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", {
Expand Down
10 changes: 10 additions & 0 deletions test/app/database/models/numeric-id.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import mongoose from "mongoose";

const schema = mongoose.Schema({
_id: {
type: Number,
required: true
Copy link
Owner

Choose a reason for hiding this comment

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

In JSON API, a resource's id needs to be a string....so I'm not sure how we want to handle that. For now, it might make sense to only allow strings or ObjectIds??

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe we just need to add something in the serialization layer to make sure that, even if the id is a number, it's always serialized as a string? (including in linkage)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would go with the second option. I can imagine use cases where someone has an existing schema with numeric IDs and they want to put a JSON API-style API in front of it.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good to me!

If we do that, though, I think we should make sure the serialization's working before/as part of merging in support for Number ids. So, do you want to add that serialization stuff to this PR? Or keep this PR just to strings and ObjectIds, and then make a second one that deals with Numbers (validation/queries + serialization)? I don't mind either way, but I could understand if you're feeling anxious to get this merged and so would prefer the former.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't serializedId = String(unserializedId) cover 99% of use cases? If you're thinking of providing a full serialization API, that could be useful, but surely that's a tiny, tiny edge case.

Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't serializedId = String(unserializedId) cover 99% of use cases?

Yup, and that's basically what I'm thinking. That logic just has to be applied (at least in the Document type, but maybe other places I'm not thinking of).

}
});

export default mongoose.model("NumericId", schema);
10 changes: 10 additions & 0 deletions test/app/database/models/string-id.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import mongoose from "mongoose";

const schema = mongoose.Schema({
_id: {
type: String,
required: true
Copy link
Owner

Choose a reason for hiding this comment

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

Just out of curiosity, if _id isn't required here, does it effect the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the empty string test

}
});

export default mongoose.model("StringId", schema);
2 changes: 1 addition & 1 deletion test/integration/patch-relationship/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
210 changes: 173 additions & 37 deletions test/unit/db-adapters/Mongoose/MongooseAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
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", () => {
describe("getModel", () => {
Expand Down Expand Up @@ -69,68 +74,199 @@ 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();
}).catch(done);
});

describe("string", () => {
it("should throw on invalid input", () => {
const fn = function() { MongooseAdapter.getIdQueryType("1"); };
expect(fn).to.throw(APIError);
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 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(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 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", () => {
const fn = function() { MongooseAdapter.getIdQueryType(["1", "552c5e1c604d41e5836bb174"]); };
expect(fn).to.throw(APIError);
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", () => {
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 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();
}).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);
});

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);
});
});
});

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;
describe("validateId", () => {
it("should refuse all == null input", function(done) {
const tests = [
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.state).to.equal("rejected"));
Copy link
Owner

Choose a reason for hiding this comment

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

I'm pretty sure es6 promises don't have a .state property, and I'd like to be able to switch out q for the real ES6 promises soon. So, is there a way to rewrite this test without checking .state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. ES6 Promises don't include an .allSettled method, so the only options I can think of in this case are reimplementing .allSettled in place or continuing to use a promises library for stuff like this along with proper ES6 promises when the library isn't needed. Personally, I've been using RSVP alongside ES6 and it's worked great for me.

Copy link
Owner

Choose a reason for hiding this comment

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

What about something like this?

const tests = [/*... */];
const allTestsRejectedPromise = Q.Promise(function(resolve, reject) {
  let rejectedCount = 0;
  const incrementRejectedCount = () => { 
    rejectedCount++; 
    if(rejectedCount === tests.length) {
      resolve();
    }
  }

  // if any test promise *resolves*, we *reject* the outer promise,
  // which represents whether all the tests were *rejected*. 
  // And, if the test rejects as expected, we increment a counter which 
  // will resolve the outer promise once all the tests are rejected.
  tests.forEach((testPromise) => testPromise.then(reject, incrementRejectedCount));
});

(And once you have a promise for allTestsRejected, then it's easy to tie that to an expect. And if the above is a common pattern, it should be easy to put it in a helper function for the other test cases.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well yes, it's just that's a very common pattern, so common that we have third party libraries that do that and lots of other useful things. I'm not sure what value you get out of reimplementing.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what value you get out of reimplementing.

Having to write just a few lines in order to remove a larger library is worth it imo; it means one less API for me to remember, and for contributors to learn. But there's also a conceptual issue here: while allSettled is useful, and my code is in some sense reimplementing that, it's also removing the need for .state, which is my main concern. The state of a promise is, by design, not meant to be synchronously inspectable, so q exposing that, and us relying on it, seems like bad practice.

done();
}).catch(done);
});

it("should reject bad input type", () => {
expect(MongooseAdapter.idIsValid(true)).to.not.be.ok;
it("should refuse a bad input type", function(done) {
const tests = [
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.state).to.equal("rejected");
Copy link
Owner

Choose a reason for hiding this comment

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

same as above

expect(result.reason.name).to.equal("CastError");
expect(result.reason.path).to.equal("_id");
});

done();
}).catch(done);
});

it("should reject empty string", () => {
expect(MongooseAdapter.idIsValid("")).to.not.be.ok;
it("should refuse an empty string", function(done) {
const tests = [
MongooseAdapter.validateId("", School),
MongooseAdapter.validateId("", NumericId),
MongooseAdapter.validateId("", StringId)
];

Q.allSettled(tests).then((res) => {
res.forEach(result => {
expect(result.state).to.equal("rejected");
Copy link
Owner

Choose a reason for hiding this comment

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

same as above

expect(result.reason.name).to.match(/ValidatorError|CastError/);
expect(result.reason.path).to.equal("_id");
});
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.validateId("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.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);
});
});

Expand Down