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

carlbennettnz · 2016-03-13T22:45:57Z

Which is better than assuming every _id field needs an ObjectId.

This method is slightly inefficient as it does 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

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 ethanresnick#80

ethanresnick · 2016-03-14T02:14:31Z

Thanks for submitting this. I'll try to take a look at it soon but, as I'm sure you've noticed, I haven't had much time for this project recently. Is there any chance you can reformat these changes to make the diff as small as possible? That will make it much easier for me to follow what's changed and get things merged quickly.

carlbennettnz · 2016-03-14T03:15:08Z

Is there any chance you can reformat these changes to make the diff as small as possible?

It's not actually as bad as it looks. I had to change getIdQueryType to return a promise, so now half the file is inside .then() calls and therefore indented further. Pull the branch and run git diff --ignore-space-change HEAD^1 HEAD and you'll get a much nicer diff.

Not sure why that one test is failing. I've spent so much time trying to figure out this project's tests and CI system, but it still does stuff I don't understand. They pass for me locally, even with the same node and npm versions. The only other thing I can think of to reproduce the failure is using Ubuntu rather than OS X, but I have no idea why that would be an issue.

carlbennettnz · 2016-03-14T03:18:21Z

TIL you can add w=1 to GitHub's URL params to see the diff with whitespace ignored

https://github.com/ethanresnick/json-api/pull/113/files?diff=unified&w=1

ethanresnick · 2016-03-14T04:12:08Z

Thanks Carl!

The smaller diff definitely helps. I'll try to add comments inline. Also, can you add some new tests for the case that this change is supposed to address?

ethanresnick · 2016-03-14T04:13:04Z

src/db-adapters/Mongoose/MongooseAdapter.js

-      if (!this.idIsValid(idOrIds)) {
-        throw new APIError(404, undefined, "No matching resource found.", "Invalid ID.");
-      }
+      ids = ids.filter(id => Boolean(id));


What is this line for?

Remove falsely values. because if we passed null as the _id when validating then the validation passes. Could maybe be changed to

ids = ids.filter(id => typeof id === "string");

or we could change the change the line 617 to

const testDoc = new model({ _id: String(id) });

Both of those seem like they'd reject the integer 0 as an id. But 0 should be allowed right, if the point is to go beyond ObjectIds? I think it's legal in mongo.

Honestly, null, false, etc could arguably be allowed too, but I'm not sure if mongoose treats them specially (e.g. as though the user hasn't provided a value), or if we have code elsewhere that assumes the id isn't present if it's falsey.

The perfect is the enemy of the good, though, so I could also be persuaded to leave this as is (i.e. with just the falsey check), so long as we documented that. (I.e., "Ids can be ObjectIds or any truthy value".)

Edit: actually, we probably shouldn't just silently filter out invalid ids; instead, they should trigger an error too, right? In which case this check could also be incorporated into the definition of idIsValid

For idIsValid, yes, null should reject. It's a bit more complicated for getIdQueryType though because a null input is used to say no ID query should be returned, and therefore all records will eventually be resolved.

Good call. But I think that case only applies if a single id is passed in. (I.e., if idOrIds is [null], that should be an error, right?) So I think I'd replace the filter with a conditional that only checks when a single ids is passed in. (I'm not sure what the resolution there should be though; see comment.)

Also, it'd be nice to clarify that mode and ids are based on the same isArray check. Finally, I think idIsValid should resolve to true or false, not to the id/error (since it has "is" in the name). Edit: Or, idIsValid should be renamed, which might be better!

Putting those together:

static getIdQueryType(idOrIds) { 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)); return Q.all(idValidationPromises).then(() => { const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] }; return [ mode, idQuery]; }, (err) => { // .... }); });

Okay, that method is nicer. Pushed.

I'm not sure which resolution makes more sense, but the latter makes the tests pass.

Thanks Carl! Re the resolution, maybe resolving with ["find", undefined] is correct? (i.e. not using findOne at all). Does that make the tests pass?

(I updated code the in comment above accordingly.)

carlbennettnz · 2016-03-14T08:49:57Z

Somehow that test that used to fail on Circle has started passing. It's just eslint that's failing now.

eslint@latest and babel-eslint@latest are currently incompatible and so I have pinned [email protected] for now, as per the advice in babel/babel-eslint#267

carlbennettnz · 2016-03-14T20:34:23Z

Linting now passes, but coverall is failing. I've never used coverall, so I haven't got a clue what the problem is. This might be helpful: lemurheavy/coveralls-public#632

ethanresnick · 2016-03-14T20:43:13Z

src/db-adapters/Mongoose/MongooseAdapter.js


-    return [mode, idQuery];
+    return Q.all(idValidationPromises).then((idsAreValid) => {


Ugh, now we're back to waiting for all the validations to finish before we run the then block, even if we could skip to throwing an error earlier.

This is my fault... I said earlier that, if we're going to keep isIdValid named as it is, it should resolve with true or false. But, what I meant was that it should resolve with true if the id is valid but reject with false otherwise. That way, the basic structure of getIdQueryType can be as it was in my last comment.

I realize that rejecting with false is a bit weird semantically, though, which is why I edited my earlier comment to say that it's probably better to just rename isIdValid. So that's what I'd do now.

I.e., I'd rename isIdValid to validateId, which can then resolve/reject as you had originally (i.e., with the valid id on success, and the validation error on failure). And then the getIdQueryType can work with the two different handlers.

Ahhh, right. I think I finally understand what you were saying earlier. Yes, we should be failing as soon as possible. We could maintain the semantics of idIsValid() by doing something like this:

const idValidationPromises = ids.map(id => this.idIsValid(id, Model).then(isValid => { if (!isValid) { throw new Error('ID is invalid'); } }));

Do you think retaining the semantics of idIsValid is worth the extra complexity in getIdQueryType?

We could maintain the semantics of idIsValid() by doing something like this

That's a pretty clever solution! I like it.

Still, though, I think I'd slightly prefer just renaming idIsValid so that settling the promise with richer values (i.e. the id and the error, respectively) then makes sense. I lean that way only because we have the validation error information within that function, so we might as well not throw it away (in case future callers want to do something with it).

👍

Pushed

ethanresnick · 2016-03-14T20:45:27Z

@carlbennettnz Don't worry about coveralls. It's going to fail for you no matter what you do, because of a permissions error. (Specifically, my coveralls API key isn't available to your code when CircleCI runs it.) This makes some sense, but it's annoying. I opened an issue somewhere to see if this could be worked around, but there hasn't been any movement.

As long as the tests pass, you're fine.

ethanresnick · 2016-03-14T21:13:51Z

Awesome! Thanks @carlbennettnz! I'm heading into a meeting now, but I'll take a final look at this later tonight. I think it should be good to go!

carlbennettnz · 2016-03-14T21:14:50Z

Sweet. Do you want me to merge commits now, or after you get a chance to review?

ethanresnick · 2016-03-15T06:51:05Z

src/db-adapters/Mongoose/MongooseAdapter.js

-      if (!this.idIsValid(idOrIds)) {
+    return Q.all(idValidationPromises).then(() => {
+      const idQuery = { _id: mode === "find" ? { $in: ids } : ids[0] };
+      return [ mode, ids.length ? idQuery : undefined ];


Is this ids.length ? idQuery : undefined ternary needed?

If I read correctly, it means that calling getIdQueryOrType with [] will resolve to ["find", undefined], which I don't think is what we want, since I'd interpret an array of ids—even if it's empty—to be a query for docs whose ids are from that array, rather than interpreting it as a findAll query. (This seems pedantic, but I can imagine cases where it might come up.)

You're right. It's left over from a previous version

Wait, wouldn't the proper test be idOrIds != null ? idQuery : undefined? If you pass [] you get an empty query, if you pass null you get undefined.

Pretty sure that's right. Pushed.

ethanresnick · 2016-03-15T07:00:43Z

Just finished making notes! Re merging the commits now or later: I don't mind either way, but it probably makes sense to wait until all the code's finalized, just so you don't have to do it again if we catch any last bits to change

ethanresnick reviewed Mar 14, 2016
View reviewed changes

carlbennettnz added 3 commits March 14, 2016 21:40

Add NumberId and StringId models for testing

da805eb

Refactor the getIdQueryType method to separate the idIsValid logic

6c51686

Make the _id of the NumericId and StringId models required

eb23428

carlbennettnz added 5 commits March 14, 2016 22:35

Tweak behaviour of getIdQueryType when idOrIds is null

08701c1

Have idIsValid resolve to false for an invalid ID, not reject

4653d58

Update eslint and friends

c6ce8eb

eslint@latest and babel-eslint@latest are currently incompatible and so I have pinned [email protected] for now, as per the advice in babel/babel-eslint#267

Lint

5a1458c

Use function() {} instead of () => {} in the failing integration test

e1a3815

ethanresnick reviewed Mar 14, 2016
View reviewed changes

Switch back to a validateId method which rejects when an ID is invalid

2c5a9d3

ethanresnick reviewed Mar 15, 2016
View reviewed changes

Fix incorrect check for input and add test to catch in future

3afbb88

snyk-bot mentioned this pull request Jan 17, 2022

[Snyk] Upgrade pluralize from 0.0.11 to 8.0.0 majacQ/json-api-1#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carlbennettnz commented Mar 13, 2016 •

edited

Loading

ethanresnick commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

ethanresnick commented Mar 14, 2016

ethanresnick Mar 14, 2016

carlbennettnz Mar 14, 2016 •

edited

Loading

ethanresnick Mar 14, 2016

ethanresnick Mar 14, 2016

ethanresnick Mar 14, 2016

carlbennettnz Mar 14, 2016

ethanresnick Mar 14, 2016

carlbennettnz Mar 14, 2016

ethanresnick Mar 14, 2016

carlbennettnz Mar 14, 2016

carlbennettnz commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

ethanresnick Mar 14, 2016

carlbennettnz Mar 14, 2016

ethanresnick Mar 14, 2016

carlbennettnz Mar 14, 2016

ethanresnick commented Mar 14, 2016

ethanresnick commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

ethanresnick Mar 15, 2016

carlbennettnz Mar 15, 2016

carlbennettnz Mar 15, 2016

carlbennettnz Mar 15, 2016

ethanresnick commented Mar 15, 2016


		return [mode, idQuery];
		return Q.all(idValidationPromises).then((idsAreValid) => {

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

Are you sure you want to change the base?

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

Conversation

carlbennettnz commented Mar 13, 2016 • edited Loading

ethanresnick commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

ethanresnick commented Mar 14, 2016

Choose a reason for hiding this comment

carlbennettnz Mar 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlbennettnz commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanresnick commented Mar 14, 2016

ethanresnick commented Mar 14, 2016

carlbennettnz commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanresnick commented Mar 15, 2016

carlbennettnz commented Mar 13, 2016 •

edited

Loading

carlbennettnz Mar 14, 2016 •

edited

Loading