leepowelldev / mongoose-validator

Validators for mongoose models utilising validator.js
MIT License
379 stars 43 forks source link

Messages not coming through #31

Closed nathanhornby closed 9 years ago

nathanhornby commented 9 years ago

I'm having a hard time getting anything out of the validation process.

I'm using Passport and the failureFlash: true argument. The flash object is being populated, but all I'm getting is: { error: [ 'Pro validation failed' ] } (Pro being the name of the model/schema) - none of the actual error messages I'm setting using this package.

I don't have this issue when using standard inline validation in the schema.

Am I doing something obviously wrong?

leepowelldev commented 9 years ago

Do you have a simplified code example of the issue?

nathanhornby commented 9 years ago

Sure, hopefully this leaves enough detail in place: https://gist.github.com/nathanhornby/60fcf1f37a8c7ad2fd20

nathanhornby commented 9 years ago

Ugh, I just noticed after looking over that gist that I was declaring one of the validators as isAscii when it should be isLength - I'm going to go check that wasn't the issue!

nathanhornby commented 9 years ago

Darn, unfortunately that wasn't it - I still just get { error: [ 'Pro validation failed' ] } in the flash object.

Although if I output the err object just before I pass it back using return done(null, false, err); then it outputs correctly. So actually I think this issue is specific to the Passport workflow…

It seems like it's just grabbing the message:

message: 'Pro validation failed', name: 'ValidationError', errors: { … }

So I may have to raise this with the Passport people - unless you have any insight! Namely why it would work fine with the standard mongoose response (which I thought this package also used).

leepowelldev commented 9 years ago

Could you post the output of the error object for me please from the save method.

nathanhornby commented 9 years ago

Of course, find the full object here: https://gist.github.com/nathanhornby/a01931b9001792aa0ff6

leepowelldev commented 9 years ago

Ah, ok. So it's using the error.message property. Maybe try this if you want a more detailed output:

// Passport

passport.use('local-signup', new LocalStrategy({
  usernameField: 'email',
  passwordField: 'password',
  passReqToCallback: true
},
function(req, email, password, done) {
  …
    newPro.save(function(err) {
      if (err) {
        return done(null, false, { message: err.toString() });
      }
      return done(null, newPro);
    });
  …
})

This is simply making use of mongoose's validation Error object: https://github.com/Automattic/mongoose/blob/master/lib/error/validation.js

Alternatively if you wanted to just display the first error message you could grab the first ValidatorError in the err.errors object.

nathanhornby commented 9 years ago

Ah OK - that change gave me the two errors that triggered as a comma separated string (just like it's asking) - so thanks! How can I simply return the errors object as-is? I just had a couple tries but am struggling to return what I need (sorry still new to node/mongoose!). I was hoping to walk through the errors and output them with their fields, so I do need it as an object ideally.

Edit:

OK I think I get what's going on - the way passport sets those flash error messages they need to be a string (right?), passing back an object just results in an empty flash message. So I think ultimately this is going to be an issue with Passport.

Thanks a lot for your help by the way.

leepowelldev commented 9 years ago

Well, the ValidationError object contains a property called errors which is a object of path (key) value to ValidatorError objects. I'm not quite sure what you're trying to do.

leepowelldev commented 9 years ago

If you pass an error object to Passport then the flash message will display error.message, if you want to pass a custom message for the flash message, then you need to pass an object with a property of message (essentially simulating an Error object).

nathanhornby commented 9 years ago

Yes that's the object I want to pass back - but I can't as I can only pass back a string (which is oddly limiting, but on Passports end).

I'm looking at walking through the errors and writing each one to req.flash myself - as then at least I'll be able to access them individually - just all feels very very clunky!

Edit: "If you pass an error object to Passport then the flash message will display error.message". When I pass an object to Passport's done() the req.flash is just blank.

leepowelldev commented 9 years ago

Where do you want to pass it back to? Into your route handler?

nathanhornby commented 9 years ago

Yep (although all I'll be doing there is passing it to the view)

leepowelldev commented 9 years ago

One dirty way would just be to attach it to the req object.

// Passport

passport.use('local-signup', new LocalStrategy({
  usernameField: 'email',
  passwordField: 'password',
  passReqToCallback: true
},
function(req, email, password, done) {
  …
    newPro.save(function(err) {
      if (err) {
        req.myPassportError = err;
        return done(null, false, { message: err.toString() });
      }
      return done(null, newPro);
    });
  …
})

Then It should be available to you in the req object in your route.

nathanhornby commented 9 years ago

Yea I'm currently looking at feeding it directly to the flash object, almost the same techinque:

// Passport

passport.use('local-signup', new LocalStrategy({
  usernameField: 'email',
  passwordField: 'password',
  passReqToCallback: true
},
function(req, email, password, done) {
  …
    newPro.save(function(err) {
      if (err) {
        if (err.errors) {
          req.flash('errors', err.errors);
        }
        return done(null, false, "Errors found");
      }
      return done(null, newPro);
    });
  …
})

Although I'm now having issues extracting the values in the view - so I'm not sure that's a valid approach!

My flash object (passed in as messages) now looks more like this:

[{ name:
    { properties: [Object],
      message: 'Your company name needs to be between 3 and 64 characters long',

But

<% if (messages) { %>
  <p><%= messages.name %></p>
  <p><%= messages.name.message %></p>
<% } %>

Is outputting nothing (not even [object Object]), and messages.name.message just generates errors because name isn't defined >.<

Gah! I'll battle on anyway - you've certainly gone above and beyond getting me in the right direction! Cheers.

leepowelldev commented 9 years ago

Just out of interest - is it really required to pass the full error object across. What is the end result you're looking for? To display all the error messages to the user as a list?

nathanhornby commented 9 years ago

I'll be displaying them inline, i.e. along with each respective input field. So having them as a comma separated list isn't ideal.

leepowelldev commented 9 years ago

OK, which version of Express as you using? 3?

nathanhornby commented 9 years ago

Latest, so 4.x

leepowelldev commented 9 years ago

Cool. Give me a sec to have a think. If you use connect-flash directly you maybe able to bypass Passport's usage of it, and that might give you a bit more flexibility.

nathanhornby commented 9 years ago

Thanks so much Lee - unnecessary but very much appreciated!

leepowelldev commented 9 years ago

Try this ... https://gist.github.com/leepowellcouk/17d25e5938e014077148

Turn off Passports flash feature, and don't pass the err object into it. This uses req.flash directly to stash the error messages per path that's got an error. Then in your route handler pass it into the view. So you should be able to access your messages with something like errors.name and errors.uri.

leepowelldev commented 9 years ago

Ah, just seen that Passport is handling the failure redirect too ... you might need to change that and use your own callback, or something similar.

nathanhornby commented 9 years ago

Awesome thanks Lee - I'll have a play!

leepowelldev commented 9 years ago

No worries - looking again, you should be fine to use the failure redirect as it'll grab the flash messages on the get request after the redirect has happened :)

Let me know how you get on.

nathanhornby commented 9 years ago

Woop - works a treat!

Edit: And no I didn't need to change the failure redirect!

leepowelldev commented 9 years ago

Great stuff!