Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.95k stars 3.84k forks source link

Improve error handling #7215

Closed pronebird closed 6 years ago

pronebird commented 6 years ago

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

Feature

What is the current behavior?

Currently Mongoose does not provide any way to error handling. Consider the following example from documentation:

// Handler **must** take 3 parameters: the error that occurred, the document
// in question, and the `next()` function
schema.post('save', function(error, doc, next) {
  if (error.name === 'MongoError' && error.code === 11000) {
    next(new Error('There was a duplicate key error'));
  } else {
    next();
  }
});

The use of so many magic values is unacceptable 🤮. Perhaps some of them come from the underlying driver. Would be awesome to either re-export those values or define constants to improve the situation.

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

What is the expected behavior?

Please mention your node.js, mongoose and MongoDB version.

arm22 commented 6 years ago

This is a contrived example, and one that appears in a handful closed issues. #3059, #2284

Normally Mongoose will throw one or an array of predefined Mongoose.Error objects.

In the case where a document passes the validator middleware, and attempts to be saved, MongoDB throws the above MongoError with code === 11000 because:

the unique Option Is Not a Validator // Will error, but will not be a mongoose validation error, it will be // a duplicate key error.

New users of Mongoose (myself included) expect that setting a field as unique on the schema will validate that any documents created will have been validated for their uniqueness before saving, which isn't the case. https://mongoosejs.com/docs/api.html#schematype_SchemaType-unique

The example provided in the docs and referenced in this issue is a way to handle errors thrown outside of the scope of Mongoose, in this case by Mongo itself. Theres a nifty plugin to abstract this one specific instance of "magic values" out of your codebase. Under the hood this plugin just does the legwork of checking if any document exists with the given unique field and forgoes calling save() to return a Mongoose.ValidationError.

vkarpov15 commented 6 years ago

Be wary of using mongoose-unique-validator because it comes with caveats. For a more robust solution, check out this plugin: https://www.npmjs.com/package/mongoose-beautiful-unique-validation

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

const connectionString = `mongodb://localhost:27017/test`;
const { Schema } = mongoose;

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

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

  const schema = new Schema({
    name: {
      type: String,
      unique: true
    }
  });

  schema.plugin(require('mongoose-beautiful-unique-validation'));

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

  await Model.init();

  // Throws a validation error with 'ValidatorError: Path `name` (test) is not unique'
  await Model.create([{ name: 'test' }, { name: 'test' }]);
}
bricekams commented 1 year ago

This is still an issue to me. I think the error should be thrown as custom dedicated classes so that we can handle exceptions in a more concise way using instance of or @Catch<T> in NestJS. Instead of that, Mongoose throws errors as Object. It's still very difficult to catch errors globally in that way.

Uzlopak commented 1 year ago

@bricekams

MongooseErrors are inheriting from Error, so instanceof will work. Nestjs uses jest as testrunner, which is known to have issues with instanceof Error.

https://github.com/jestjs/jest/issues/2549

bricekams commented 1 year ago

But i still think instanceof MongooseError should work to catch every error thrown by query at runtime. For example:

schema.post('save', function(error, doc, next) {
  if (error.name === 'MongoError' && error.code === 11000) {
    next(new Error('There was a duplicate key error'));
  } else {
    next();
  }
});

This is still what I use to catch this type of erors. Even when I pass an invalid mongoId it just throw an object

Uzlopak commented 1 year ago

MongooseError and MongoError are extending from Error.

https://github.com/mongodb/node-mongodb-native/blob/52cd649caf2e64aef6d3984c5f2d24af03db4c51/src/error.ts#L122

So you claim that our thrown errors are just Objects is false.

Your code snippet seems wrong as you always expect error to be always not being undefined or null.

bricekams commented 1 year ago

Well I apologize, I'll just take a deep look into it