Skip to content
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.

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

Best way to validate an ObjectId #1959

Closed
nikilster opened this issue Mar 12, 2014 · 29 comments
Closed

Best way to validate an ObjectId #1959

nikilster opened this issue Mar 12, 2014 · 29 comments

Comments

@nikilster
Copy link

I'm inserting some objectIds as references and I want to make sure that they are valid objectids (right now I'm getting a crash when they aren't valid ids).

I'm checking to see if they refer to valid objects by just doing a find() call.

How should I check if the strings are in the correct format? I've seen some regexs that check if the string is a 24 character string and stuff like that - seems like a hack. Is there an internal validator on the ObjectId? I couldn't figure out how to use it. Thanks so much!

@avishaan
Copy link

I'm having the same problem. I tried mongoose.Schema.ObjectId.isValid() as well as mongoose.Types.ObjectId.isValid() but neither of those properties have an isValid method. How did you end up solving this problem? I also see mongodb has one and there is also regex as another option. I would prefer to not use regex nor have to require('mongodb')

@atcwells
Copy link

This works for me:

var mongoose = require('./node_modules/mongoose');
console.log(mongoose.Types.ObjectId.isValid);
// [Function: isValid]
console.log(mongoose.Types.ObjectId.isValid('53cb6b9b4f4ddef1ad47f943'));
// true
console.log(mongoose.Types.ObjectId.isValid('bleurgh'));
// false

@atcwells
Copy link

Incidentally the method in mongodb is the bson lib method, which just checks for not null, 12 or 24 char hex string - it's a regex like this:

var checkForHexRegExp = new RegExp("^[0-9a-fA-F]{24}$");

so it can't be too hacky if it's used in there

@aslepushkin
Copy link

sValid() always returns True if string contains 12 letters

console.log(mongoose.Types.ObjectId.isValid("zzzzzzzzzzzz")); // true

@vkarpov15
Copy link
Collaborator

Because "zzzzzzzzzzzz" is technically a valid ObjectId - an object id's only defining feature is that its 12 bytes long. See mongodb/js-bson#112. A JS string of length 12 has 12 bytes, modulo unicode weirdnesses. If you want to check for a length 24 hex string, just check the string matches /^[a-fA-F0-9]{24}$/

@aslepushkin
Copy link

"zzzzzzzzzzzz" is not valid ObjectId. For example Mongo shell listiong (mongodb version - 3.0.2):

> ObjectId('zzzzzzzzzzzz')
2015-04-29T18:05:20.705+0300 E QUERY    Error: invalid object id: length
    at (shell):1:1
> ObjectId('zzzzzzzzzzzzzzzzzzzzzzzz')
2015-04-29T18:06:09.773+0300 E QUERY    Error: invalid object id: not hex
    at (shell):1:1
> ObjectId('ffffffffffff')
2015-04-29T18:09:17.303+0300 E QUERY    Error: invalid object id: length
    at (shell):1:1
> ObjectId('ffffffffffffffffffffffff')
ObjectId("ffffffffffffffffffffffff")

@vkarpov15
Copy link
Collaborator

Because the mongodb shell's ObjectId constructor is written such that it only accepts hex strings. It's a restriction in the mongo shell for convenience, not with the BSON type ObjectId. Admittedly this is a somewhat counterintuitive case given that hex strings are how ObjectIds are usually represented, but if you don't like it then just use the regex /^[a-fA-F0-9]{24}$/ :)

@niftylettuce
Copy link
Contributor

Why do we get false when we try to do isValid on an ObjectId itself and not a String? Shouldn't this return true since and ObjectId is a valid ObjectId? That doesn't make sense -- maybe call .toString() if it's an object being passed to isValid?

@vkarpov15
Copy link
Collaborator

@niftylettuce comments welcome at #3365. Right now we just defer down to the bson package's ObjectId.isValid() function, which doesn't line up exactly with how people think of ObjectIds in mongoose. I'll open up a PR for returning true if you're given an ObjectId though, that seems perfectly reasonable.

@shankie-codes
Copy link

Coming back to a bit of an old issue here... @atcwells solution mongoose.Types.ObjectId.isValid('53cb6b9b4f4ddef1ad47f943') works well enough for me, but it still seems a bit hacky to have to have my controller check whether an object ID is valid – when surely it's a pretty common use case to want to be able to send a mal-formed ID to the server and not have it crash.

Ideally, it would simply return something in the err in the callback so that we could handle it correctly and send the correct HTTP status using our controller.

Is there a use-case where this wouldn't be useful functionality in core? If not, perhaps we can make a plugin. I've had a quick search and there doesn't seem to be anything the does the job – https://github.com/CampbellSoftwareSolutions/mongoose-id-validator is for validating that the ID actually exists, which isn't what we want to do here – we simply want to make sure that we don't generate an uncaught error.

@shankie-codes
Copy link

Right now, in my Express controller, every time I go a request that has an ObjectId in it, e.g. GET to https://myproject/organisations/{id}, I have to do something like:

if( !mongoose.Types.ObjectId.isValid(id) ){
    return res.sendStatus(400); // They didn't send an object ID
  }

... before then going on to do Organisation.findOne();

Seems pretty boilerplate. I'm happy to write a plugin or something if someone can point me in the right direction of where to start. Doesn't seem like a plugin as it's not really a schema thing...

@vkarpov15
Copy link
Collaborator

@shankiesan you don't need to do that, mongoose will reject the query promise if the id is invalid.

var assert = require('assert');
var mongoose = require('mongoose');
var Schema = mongoose.Schema;

mongoose.connect('mongodb://localhost/test');
mongoose.set('debug', true);

var MyModel = mongoose.model('test', new Schema({ name: String }));

MyModel.findOne({ _id: 'invalid' }).exec().catch(error => console.error('error', error));

Output:

$ node gh-1959.js 
error { CastError: Cast to ObjectId failed for value "invalid" at path "_id"
    at MongooseError.CastError (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/error/cast.js:19:11)
    at ObjectId.cast (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/schema/objectid.js:147:13)
    at ObjectId.castForQuery (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/schema/objectid.js:187:15)
    at cast (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/cast.js:174:32)
    at Query.cast (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/query.js:2563:10)
    at Query.findOne (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/query.js:1239:10)
    at /home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/query.js:2163:21
    at new Promise.ES6 (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/promise.js:45:3)
    at Query.exec (/home/val/Workspace/10gen/troubleshoot-mongoose/node_modules/mongoose/lib/query.js:2156:10)
    at Object.<anonymous> (/home/val/Workspace/10gen/troubleshoot-mongoose/gh-1959.js:10:37)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
  message: 'Cast to ObjectId failed for value "invalid" at path "_id"',
  name: 'CastError',
  kind: 'ObjectId',
  value: 'invalid',
  path: '_id',
  reason: undefined }
^C
$ 

@vkarpov15
Copy link
Collaborator

@shankie-codes
Copy link

Ah what a muppet I've been! Of course, handle the promise being rejected... arg, my brain. Thanks @vkarpov15

@vkarpov15
Copy link
Collaborator

Callbacks work too if promises are too much of a headache MyModel.findOne({ _id: 'invalid' }).exec(error => console.error('error', error)); :)

@johnculviner
Copy link

DO NOT USE ObjectId.isValid() if it isn't already clear from the above 12 bytes thing Valeri said. Just got burned pretty good by this one:
ObjectId.isValid('The Flagship') === true

@johnculviner
Copy link

@atcwells if you could update your highly upvoted comment to include that bit I think other people might appreciate it since I was initially doing it based on what you said: ObjectId.isValid('The Flagship') === true

@devtaha
Copy link

devtaha commented Sep 29, 2017

The funny part is:
the following statement is returning true
mongoose.Types.ObjectId.isValid("South Africa")

@joelcolucci
Copy link

joelcolucci commented Oct 10, 2017

What I'm rolling with (for now) is checking the error type in the promise catch. If it's 'ObjectId' I am returned a 404. I am returning a 404 because to the consumer of the API/Web service the resource was not found or doesn't exist.

See example:

  Widget.findByIdAndRemove(resourceId)
    .then((result) => {
      if (!result) {
        let error = new Error('Resource not found');
        error.status = 404;
        next(error);
      } else {
        res.redirect(303, '/');
      }
    })
    .catch((error) => {
      if (error.kind === 'ObjectId') {
        let error = new Error('Resource not found');
        error.status = 404;

        next(error);
      } else {
        next(error);
      }
    });

UPDATE:
Instead of adding this to each route handler in the controller. I'm adding to the global catch all handler.

See example:

app.use(function(err, req, res, next) {
  // set locals, only providing error in development
  res.locals.message = err.message;
  res.locals.error = req.app.get('env') === 'development' ? err : {};

  if (err.kind === 'ObjectId') {
    err.status = 404;
  }

  // render the error page
  res.status(err.status || 500);
  res.render('error');
});

@fdorantesm
Copy link

fdorantesm commented Oct 14, 2017

Is it possible validate in the schema? Is not a best practice repeat that if, but i want to log cast error event.

@shubham-agrawal
Copy link

Why can't mongoose implement the regex /^[a-fA-F0-9]{24}$/ for isValid when it is only dealing with MongoDB. This is so confusing. We just spend an hour debugging the issue and it came out to be such a silly thing that you will never notice

@devtaha
Copy link

devtaha commented Dec 5, 2017

I would recommend to use this npm package https://www.npmjs.com/package/valid-objectid
It works perfectly

@mstmustisnt
Copy link

mstmustisnt commented Dec 5, 2017

I've created a workaround for this:

function isObjectId(value) {
  try {
      const { ObjectId } = mongoose.Types;
      const asString = value.toString(); // value is either ObjectId or string or anything
      const asObjectId = new ObjectId(asString);
      const asStringifiedObjectId = asObjectId.toString();
      return asString === asStringifiedObjectId;
    } catch (error) {
      return false;
    }
}

@victorbadila
Copy link

@mstmustisnt I think that needs to be try/catched, if value is "123" this will throw an error.

@shankie-codes
Copy link

In case it helps anyone, I've been doing an adaptation of the ObjectId.isValid() approach above. Because in my use case, I want to be able to get a resource either by its ID or its url-slug, e.g.:

GET /users/59f5e7257a92d900168ce49a ... or ...
GET /users/andrew-shankie

...I've found that this works well in my controller:

  const { id } = req.params;

  const query = {
    $or: [{ slug: id }],
  };

  if (mongoose.Types.ObjectId.isValid(id)) query.$or.push({ _id: id });

  User.findOne(query)
    .exec((err, user) => { ... }

In this case, a 12-byte string is still a valid Object ID, searching for it simply returns a zero-length array, rather than throwing an error. And because I'm using an $or query, it then searches by the URL slug (my other possible option).

Might not be the most elegant solution, but it works for me.

@mstmustisnt
Copy link

@victorbadila yes, exactly. I just gave a hint. Edited my comment, actually just places the code I actually use.

@no-stack-dub-sack
Copy link

validator.js has a built-in isMongoId method

@adamreisnz
Copy link
Contributor

adamreisnz commented Mar 9, 2019

Whenever I use mongoose, I always extend it with a few static helper methods:

const mongoose = require('mongoose');
const {Types: {ObjectId}} = mongoose;

//Helper to check if an ID is an object ID
mongoose.isObjectId = function(id) {
  return (id instanceof ObjectId);
};

//Helper to validate a string as object ID
mongoose.isValidObjectId = function(str) {
  if (typeof str !== 'string') {
    return false;
  }
  return str.match(/^[a-f\d]{24}$/i);
};

You can run this as a part of your database initialisation scripts, so the methods are always available in your app.

timkuijsten pushed a commit to timkuijsten/js-bson that referenced this issue Mar 14, 2019
fix(ObjectID): isValid(new ObjectID()) returns true
@chenhebing
Copy link

If ObjectId.isValid(id) is true, we can judge (new ObjectId(id).toString()) 's value and id.

const mongoose = require('mongoose');
const {Types: {ObjectId}} = mongoose;
const validateObjectId = (id) => ObjectId.isValid(id) && (new ObjectId(id)).toString() === id;

@Automattic Automattic locked as resolved and limited conversation to collaborators May 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests