Mongoose: Handle cast error on find when objectId is invalid

[rafaelgrilli92]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
I'm trying to find an item by its _id, but when I try to handle the possible "error", when the client sends an id that doesn't exist and then return it as an user friendly error, it's getting into the "catch" function and resulting in an Error 500 to the user with a very big error message.

I tried to find a method to handle it on google and even here, but I didn't find anything.

If the current behavior is a bug, please provide the steps to reproduce.

ToDo.findById(todoId)
.then(result => {
    return httpResponse.success(res, "Data successfully fetched", result)
})
.catch(err => {
    // When the ID isn't valid, it shows up as an error
    return httpResponse.error(res, err, "Error while fetching the ToDo data on the database");
})

What is the expected behavior?
I want to handle this kind of "error" that for me isn't an error. It's just an invalid item to search.

Please mention your node.js, mongoose and MongoDB version.
Node: v7.4.0
Mongoose: 4.10.5
MongoDB: v3.4.4

[varunjayaraman]

so your problem isnt that the id doesn't exist, but that it's an invalid objectid?

I guess you could handle this with require('mongodb').ObjectId.isValid(todoId) but its imperfect (i.e. 9 will return true for that but mongoose will throw an error).

Cant you just handle the error if its an invalid mongo id error and let the user know the doc couldn't be found?

[rafaelgrilli92]

Yes, I can do that. I'm already doing something similar actually. But, I don't know... For me, a better approach for those cases would be returning an empty user instead of throwing an error.

This is my currently approach:

ToDo.findOne({ _id: todoId, createdBy: userId })
.populate(['createdBy', 'updatedBy'])
.then(result => {
    if (!result) return httpResponse.wrong(res, statusCode.error.NOT_FOUND, `Data was not found`);

    return httpResponse.success(res, "Data successfully fetched", result)
})
.catch(err => {
    if (err.message.indexOf('Cast to ObjectId failed') !== -1)
        return httpResponse.success(res, "Data was not found", null);

    return httpResponse.error(res, err, "Error while fetching the data on the database");
})

[varunjayaraman]

sorry for the late response. anyway, i think this is potentially backwards breaking but ill label it an enhancement for now, will wait for @vkarpov15 's thoughts

[vkarpov15]

This is expected behavior and your way of handling the error is mostly correct. I'd recommend instead doing:

.catch(err => {
    if (err.message instanceof mongoose.Error.CastError)
        return httpResponse.success(res, "Data was not found", null);

    return httpResponse.error(res, err, "Error while fetching the data on the database");
})

Checking for instanceof CastError is more robust because the error message may change.

If you don't want to execute this check for every find operation you can use error handling middleware to transform the error into something that is appropriate for your application.

[aletorrado]

I know this is currently expected behavior, but it neither seems nice having to check everywhere for validity.

That being said, in JS trying to cast an invalid number from a string just returns NaN, and we have Invalid Date as well, for example. I think this is the paradigm that we as JS users are used to. As such, we shouldn't fight this but embrace it.

Just my two cents.

[vkarpov15]

You don't have to check everywhere, thats what error handling middleware is for. Also, I don't really see how this relates to NaN and invalid date...

[aletorrado]

Would it be possible to create an error handling middleware to get empty results when an invalid ObjectId is queried?

[vkarpov15]

No unfortunately not. FWIW invalid date and NaN are two concepts that mongoose tries to fight against, it'll error rather than let you save NaN or invalid date.

[youssefsharief]

@vkarpov15

instanceof mongoose.Error.CastError

This is not working with me

[vkarpov15]

@youssefsharief can you provide code samples please?

[mdutra]

@youssefsharief It's this way:

if (err instanceof mongoose.CastError) {
  // Handle this error
}

[youssefsharief]

Thank you sir

On Tue, Jan 2, 2018 at 3:07 PM, Murilo Dutra [e-mail]
wrote:

@youssefsharief https://github.com/youssefsharief It's this way:

if (err instanceof mongoose.CastError) {
// Handle this error
}

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Automattic/mongoose/issues/5354#issuecomment-354762767,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AULYc2KvIgRRVD2PwAj1b1FEIqCM3tYBks5tGioXgaJpZM4N4rdt
.

--
Best Regards,
Youssef Sherif

[lzszone]

@vkarpov15 Hi, I'm using mongoose v5 +, and my case is that I want to have an error handle middleware that deals with the Cast Error. But I can't find a stage to put it into.
I think it should be put into the pre of the init stage, is that right?
If that's correct, why is that I get an error next is not a function?
This is my code

plugMiddlewares(schema, ops) {
    schema.pre('init', function(err, doc, next) {
      if(err) {
        err.status = 400;
        next(err)
      } else {
        next()
      }
    });
}

And I tested it with a invalid ObjectId, the result did not match my expectation.
It throws something like next is not a function.

[vkarpov15]

@lzszone what cast error are you trying to handle? In a query, in save()?

Also, init hooks unfortunately need to be synchronous, here are the docs on the subject: https://mongoosejs.com/docs/middleware.html#synchronous

[lzszone]

@vkarpov15 I'm trying to handle the schema.findById method globally,
And yes, I've read about that... And what is really confusing me is that the cast error should happen before querying the database, so I think it should be pre of init, but the cast error seems not catchable in that stage.
I want to handle schema.findById 's cast error globally
I've read about the doc many times, cannot solve this.

[vkarpov15]

"init" refers to initializing the document when loading it from the database. That presumes you successfully executed a query. But Mongoose doesn't execute your query if it failed to cast. You should use error handling middleware for this.

const assert = require('assert');
const mongoose = require('mongoose');
mongoose.set('debug', true);

const GITHUB_ISSUE = `gh5354`;
const connectionString = `mongodb://localhost:27017/${ GITHUB_ISSUE }`;
const { Schema } = mongoose;

run().then(() => console.log('done')).catch(error => console.error(error.stack));

async function run() {
  await mongoose.connect(connectionString);
  await mongoose.connection.dropDatabase();

  const schema = new Schema({});

  schema.post('findOne', function(error, doc, next) {
    console.log('Error handling middleware:', error.message);
    next(); 
  });

  const M = await mongoose.model('Test', schema);

  await M.findById('foobar');
}

[lzszone]

@vkarpov15 Thank you for the explanation, it's very clear :D

[maxs15]

Hey @vkarpov15,
I stumbled on this post, this is exactly what I'm looking for but it looks like we cannot remove the error and return null instead using an error middleware, what did you end up doing @lzszone ?
I'm just trying to return null instead of throwing a CastError, it's kinda annoying to have to check if the id is valid before every findById..

Thanks

[lzszone]

@maxs15 I added some business code above @vkarpov15 's, If you just want to ignore the CastError, I think the following would work.

// After mongoose connected
mongoose.plugin(function(schema, options) {
  schema.post('findOne', function(error, doc, next) {
      if(error) {
        if(error.name === 'CastError') {
          console.log('Failed to cast, but not throw');
          return next()
        }
        return next(error)
      }
      return next()
    })
})

[maxs15]

Thanks @lzszone !
Although it's weird it works on your side, I tried exactly the same it doesn't work.
The documentation(https://mongoosejs.com/docs/middleware.html#error-handling) says:
Error handling middleware can transform an error, but it can't remove the error. Even if you call next() with no error as shown above, the function call will still error out.

[lzszone]

@maxs15 I haven't tested the code above, my business code will add some property in the error and pass the error to the next, so you are right, thank you for telling me that.

[vkarpov15]

@maxs15 right now the only way to handle that is to do Model.findById().catch(), or by wrapping .findById().catch() in a static. Mongoose doesn't support 'exiting out' of error handling middleware. Error handling middleware can change the error, but it can't change the fact that there is an error.

Allowing exiting out of error handling middleware is tricky. Returning null in all cases seems strange and isn't really compatible with operations like updateOne().

[maxs15]

Thanks for the explanation @vkarpov15.

Tricky to implement in the code ?
In my opinion I don't think that returning null for Model.findById(123) is strange, but you're right it can be useful in some cases (especially in dev) to know exactly what's the error.

But let's say you have an API, if you don't want your code to throw internal errors every time an API user use a wrong parameter (/song/:wrong_song_id), you would have to validate the parameters before every Model.find() or create a new static that handles that.
It would be very useful to have such a basic thing already in mongoose.

Just a suggestion but maybe a property throwCastError: false (true by default) in the mongoose config could be an option ?
That way the people who wants to handle the cast errors like a model not found could do so, In my case I would be able to enable this option in production.
Being able to prevent an error in the error handling middleware would be a much powerful feature but I can understand it can be tricky to implement.

[vkarpov15]

The tough part isn't findById, its functions like updateOne() that always resolve to an object when no error occurs. What should updateOne() return in the event of aborting a cast error in error handling middleware?

[maxs15]

If you decide to allow the error middleware to prevent an error we could target find so no issue about the update.

If you decide to go for an option like throwCastError: false, for the update we could return the same response as an update without success: { n: 0, nModified: 0, ok: 1 }
That makes sense in my opinion since it's an id that cannot be found.

[vkarpov15]

I'm not thrilled about that approach because that means Mongoose has to create a fake response that looks like a MongoDB server response, which may change between MongoDB server versions.

[maxs15]

Alright I see your point, this is only an optimization though, if you do not throw a cast error and call the find method of the driver you'll end up with the response no ?

[vkarpov15]

No, the MongoDB driver has a separate response for updates. That's why Mongoose actually does fake the MongoDB server response in one place, but that's not something I want to rely on as part of Mongoose's public API, because if the MongoDB server changes we would need to modify the server response to match Mongoose's API.

[maxs15]

Alright thanks @vkarpov15, guess I'm gonna check all my ObjectIds manually

[H4kim]

if the id is a valid but doesn't exist it will automatically send an empty object
but if the id is not valid (less char or more ) then you can handle it by the error name

if(err.name === 'CastError') { err.statusCode = 400; err.status = err.status || 'error'; }