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

Custom (user-friendly) ValidatorError message #753

Closed
wants to merge 17 commits into from

Conversation

ManInTheBox
Copy link

Related to #747

Validators now accepts custom error message or if not provided, built-in error message will be used.
Error messages are borrowed from Yii PHP Framework and are stored in errormessages.json.

var UserSchema = new Schema({
  name: {
    first: {
      type: String,
      required: [ true, 'Field {path} is required.' ]
    },
    last: {
      type: String,
      max: [ 30, '{path|Your dear last name} is limited to {max} characters.' ]
    }
  },
  age: {
    type: Number,
    max: 100,
    min: [ 5, 'Minimum value for {path|Your age} is {min}.' ]
  }
});

This will be translated as:

  • Field Name First is required
  • Your dear last name is limited to 30 characters.
  • Age is too big (maximum is 100).
  • Minimum value for Your age is 5.

Also, CastError is improved with built-in cast error message like:
Age must be a number.

I added three new String validators, min, max and length, because I think these are common.

@chanced
Copy link

chanced commented Apr 6, 2012

Thanks!

@ghost
Copy link

ghost commented Apr 8, 2012

+1 Thanks

@aheckmann
Copy link
Collaborator

why use 'Minimum value for {path|Your age} is {min}.' and not Minimum value for your age is {min}?

i'd prefer to keep the new string validators out of mongoose and instead have those available as a plugin.

can you add tests for these changes including cast error tests? then merge with latest master please.

thanks!

@ManInTheBox
Copy link
Author

Actually there's no need for {path|Your label}. Ofc it will work as you mentioned above... I realized that right after sending pull request :D It will be removed.

Sure, I'll add tests soon.

Cheers

@aheckmann
Copy link
Collaborator

great!

@kof
Copy link

kof commented May 4, 2012

@ManInTheBox what do you think about #887 ?

@ManInTheBox
Copy link
Author

@kof ValidatorError.toString() and CastError.toString() will output user-friendly messages (see below):

{ message: 'Validation failed',
  name: 'ValidationError',
  errors: 
   { 'name.username': 
      { message: 'Username cannot be blank.',
        name: 'ValidatorError',
        path: 'name.username',
        type: 'required' },
     email: 
      { message: 'E-mail cannot be blank.',
        name: 'ValidatorError',
        path: 'email',
        type: 'required' },
     password: 
      { message: 'Password cannot be blank.',
        name: 'ValidatorError',
        path: 'password',
        type: 'required' } } }

For CastError there's a slightly different format (it stops on first cast error, so there's no errors object):

{ message: 'Test must be a valid date.',
  name: 'CastError',
  type: 'date',
  value: 'asdf' }

If you need to find out what caused an error you can check err.name -> name: "ValidationError".

if (err) {
  if (err.name === 'ValidationError') {
    // display validation errors to user
  } else if (err.name === 'CastError') {
    // display cast error message to user
  } else {
    // something related to db happened... crap!
  }
}

If you want to display nice error summary box, just iterate through err.errors object:

// eg jade
  - if (user.errors)
    div(class='error')
      ul
        - each err in user.errors
          li= err // <-- ValidatorError.toString()

real example:
https://github.com/ManInTheBox/nodejs.rs/blob/master/routes/user.js#L47
https://github.com/ManInTheBox/nodejs.rs/blob/master/views/user/register.jade#L5

I think this will fit your needs.

btw @aheckmann I've stuck for a while with MongooseArray. Concrete with this test. Can't get path of embedded document. It's always 'undefined', though arr expected.
In a couple of next days, I hope I will push back added tests. I'm just too busy with another project right now.

@kof
Copy link

kof commented May 5, 2012

I am still missing the value of the field in the error, this is what I expect to find in logs:

{ message: 'Validation failed',
  name: 'ValidationError',
  errors: 
   { email: 
      { message: 'E-mail is not valide.',
        name: 'ValidatorError',
        path: 'email',
        type: 'required',
        value: '123234r@web.c' },
     password: 
      { message: 'Password cannot be blank.',
        name: 'ValidatorError',
        path: 'password',
        type: 'required' } } }

In this example if you don't get the email string, you will not be able to understand why is the issue happened. An email regex will not say it to you.

@ManInTheBox
Copy link
Author

Yup, you're right!
I'll include value key in ValidatorError and also path key in CastError.
Your thoughts?

@kof
Copy link

kof commented May 5, 2012

fine, the second thing is ValidatorError.prototype.toString method. It should return all error informations, not just the message, for me it is fine if you just use util.format so the result will look like in your example in previous comment (formatted json). The reason is to make it convinent (no need to log errors array separately) and consistent relative to other error handlings:

doc.save(function(err) { 
    if (err) {
        return next(new Error(err));
    }
})

Error constructor will point in stack trace to this code and will call #toString method so get the stringified version of ValidationError. I believe this is the way how the most user handle errors in node.

@kof
Copy link

kof commented May 5, 2012

I am not sure if the stringified version of error should/can be mulitiline formatted. Probably "ValidationError: Validation failed. {email:{message:'E-mailisnotvalide.',name:'ValidatorError',path:'email',type:'required',value:'123234r@web.c'},password:{message:'Passwordcannotbeblank.',name:'ValidatorError',path:'password',type:'required'}}}" is fine.

@kof
Copy link

kof commented May 5, 2012

probably something like this

ValidatorError.prototype.toString = function() {
    return this.name + ': ' + this.message + ' ' + JSON.stringify(this.errors);   
};

@ManInTheBox
Copy link
Author

@kof what's the difference between this:

if (err) return next(err);

and this:

if (err) return next(new Error(err));

???

err is an instance of Error object so there's no need to instantiate another one...

I'm using first approach in Express and it's error handler catches that just fine.

If you want to print err object its #toString() is called, and it's reasonable (at least for me) to output its message. If you need detailed information about error object (e.g. for logging purposes), you have that whole error object.

Let's hear what others think about this.

Cheers

@kof
Copy link

kof commented May 6, 2012

@kof
Copy link

kof commented May 6, 2012

You need detailed information about the error if the error is happened. If it is happend and logged message is not complete you need to add detailed information in code and wait until the error happens again. This process is messy, everyone want to get the whole description at the moment error is happened.

One could log .errors array in one central place, but there is this new Error(err) thingy, which will only pass the stringified version of the error. Sometimes one can pass just the original err, but very often you want to know which function has got the error instance.

@kof
Copy link

kof commented May 6, 2012

of course one could call everywere captureStackTrace instead of new Error(), then pass just the original error instance, so the endpoint logger can check for err.errors and log them all, but this is not handy.

if (err) {
    Error.captureStackTrace(err);
    return next(err);
}

@aheckmann
Copy link
Collaborator

@kof are you saying you want error objects to record where they were logged?

@kof
Copy link

kof commented May 8, 2012

@aheckmann yes, but this is not because I want to know where they are logged, but to know in which callback they were passed, to see which function has called the function which has passed to my callback the error object.

need example?

@kof
Copy link

kof commented May 9, 2012

The point is we need both:

  1. information about were does the error happend
  2. how does it comes to happen

The 1. one is surely the original Error instance, but you haven't the 2. one if you don't add call points to stack. Actually it is a misuse of Error constructor in this case, because we actually don't want to create a new error, we just want to add a stack to this point, but it is handy and it wokred for all errors which return complete information from #toString.

@kof
Copy link

kof commented Aug 2, 2012

@ManInTheBox thanks, really waiting for this one :)

@aheckmann
Copy link
Collaborator

sounds good. i was planning on revisiting this pull and a few others after 3.0 stable is out.

@ManInTheBox
Copy link
Author

@aheckmann congrats to v3 release!
this pull will require a little more adjustments according to v3 (it's really old code now :) ) so please be patient... think it will be available in next few days... thanks!

@aheckmann
Copy link
Collaborator

just merged locally but there were a ton of failing tests. this looks pretty good but we need all tests to pass and be backwards compatible before we can merge it in.

@ManInTheBox
Copy link
Author

@aheckmann yeah, i will provide all passing tests (don't have spare time right now). please try/review now b/c i fixed existing tests broken by this feature. also i will write better docs according to this pull. i have few failing test and IMO they aren't related to this pull:
https://gist.github.com/3794045
https://gist.github.com/3794030

@kof
Copy link

kof commented Sep 28, 2012

I just had the case, where knowing the query which was send to mongo would save a lot of time. Can we add it to the err object? (conditions, options, new values)

This could be also the same string like one we get if mongoose.set('debug', true)

@ecdeveloper
Copy link

Hi all, guys!

Just wanted to ask - when can we wait for this pull request being merged to master? I'd like to use this awesome feature of User-Friendly errors stuff, without checking out some non-master branches.

Thanks in advance!

@ManInTheBox
Copy link
Author

Hi,
it's not ready for master yet since some tests still missing. I'm planning to add them in about ~10 days. Out of time ATM :S
Thanks!

@ecdeveloper
Copy link

Thanks for a quick reply!
Will wait for release :)

@dexcell
Copy link

dexcell commented Jan 16, 2013

No update news?
I think it's pretty crucial, right now i have to manually check and return error messages accordingly. (which make double job / redundant since it's already defined in the schema)

@ecdeveloper
Copy link

+1

@richzw
Copy link

richzw commented Jan 17, 2013

+1

This is we needed.

@RohovDmytro
Copy link

We need this very much.

@vizo
Copy link

vizo commented Jan 31, 2013

+1

@aheckmann aheckmann closed this Mar 18, 2013
@vizo
Copy link

vizo commented Apr 20, 2013

Can i ask what is the status of this?

@daslicht
Copy link

I would also like to know this please

@daslicht
Copy link

Such kind of feature should be built in

@otang
Copy link

otang commented Nov 27, 2013

+1

2 similar comments
@tjworks
Copy link

tjworks commented Dec 31, 2013

+1

@daslicht
Copy link

+1

@aheckmann
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet