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

Support multiple validator errors #2612

Closed
vkarpov15 opened this issue Jan 22, 2015 · 30 comments
Closed

Support multiple validator errors #2612

vkarpov15 opened this issue Jan 22, 2015 · 30 comments
Labels
discussion If you have any thoughts or comments on this issue, please share them! enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature plugin

Comments

@vkarpov15
Copy link
Collaborator

No description provided.

@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Jan 22, 2015
@elbuo8
Copy link

elbuo8 commented Jun 2, 2015

Was this added to v4?

@vkarpov15
Copy link
Collaborator Author

Nope. This would be a really big backwards breaking change and I'm not entirely convinced it's the right choice. I could be convinced by a well-reasoned argument though.

@phiphou
Copy link

phiphou commented Jul 11, 2015

Here is my "well-reasoned" argument : as I can see, we get multiple validation errors when multiple fields of the model are invalid, but not when multiple valdation test failed for just one field. So here is my question : what are we validating ? Fields or models ? The fact that we can get multiple validation errors when multiple fields of the model are invalid make me think you want to validate the model. So what is a valid model ? I would say a model that pass ALL the validation, don't you think ?

@vkarpov15
Copy link
Collaborator Author

Yep it should, but that's not really the point of this discussion. If a document is valid, of course it will pass validation. The point of contention here is how validators are composed. Right now validators run one after another in the order they're declared (with the special exception that required validators always come first) and validation of a field stops when one validator fails. Multiple validator errors would require changing that logic so that validation of a single field doesn't stop when one validator fails, which would be a big change to the way mongoose works.

@phiphou
Copy link

phiphou commented Jul 11, 2015

Not sure I understand (not the changes implied but the concept), or maybe I didn't explain well what I think (I'am french and have not a well english ;)) .

Let say we have a simple model with two fields with two validators for each of them. Now, imagine a payload that fails on the first validators of each field. Despite the whole model isn't valid, you don't stop validation on the first fail encoutered (we get two validation errors).

So my question is more about why models and fields validation process do not follow the sames rules. Whatever the choice you made, don't you think it will be the same for both fields and models ?

More clearly, in this case, regarding of the choice you make, we should have either only one validation error (current fields validation behaviour) or four validation errors (the big change).

Hope I success in making you now understand better what I'm trying to say ;)

@vkarpov15
Copy link
Collaborator Author

The reason is to make it easy to compose validators. If you declare one validator, you don't need to do that check again in subsequent validators. For instance, if you declare a required validator, you don't have to worry about TypeErrors crashing your server because subsequent validators won't get run if the field is undefined

@misterdai
Copy link

👍 I'm currently rewriting some code and moving my validation logic from the controller to the models, but I'm starting to worry about the lack of support for multiple validation errors on a single field. I don't want to increase the number of form submission attempts from a user, in order to pass validation, if I'm drip feeding them each error at a time. Rather than showing each validation rule they've failed.

To avoid breaking existing code, perhaps a flag to toggle support should be added and then produce a different type of error instead of "ValidationError", to denote the difference. For example, validateMulti: true against the schema, failures of validation would then return a ValidationMultiError containing an array per field.

@vkarpov15 vkarpov15 added the discussion If you have any thoughts or comments on this issue, please share them! label Sep 22, 2015
@vkarpov15
Copy link
Collaborator Author

Worth discussing more. You can always just write a custom validator that executes all the validations you want. I'm open to changing if there's a lot of demand, but right now the fact that validators are executed in sequence with required validators coming first is really integral to mongoose's API, and changing will require a lot of headache for a lot of people.

@misterdai
Copy link

I just took a run at that, modified an existing plugin I was using (mongoose-validator) to run through the multiple rules but appear to mongoose as a single validator. My only issue is return multiple messages for the various rules that were failed.

ValidatorError.prototype.formatMessage is expecting the message to be a string, so I'm forced to stringify my array to JSON to get them formatted correctly (property replacements) and avoid throwing an error. That's really the only step that I feel uncomfortable with if no official support was added.

@vkarpov15
Copy link
Collaborator Author

Stringifying errors in general is pretty complicated when you support multiple things going wrong. Would it be helpful if you could associate an arbitrary object with a ValidatorError?

@misterdai
Copy link

That would probably be the simplest approach. Currently I've tried it out using a similar approach to mongoose-validate-all, which uses a dirty trick based on the face that mongoose calls .replace on the error message, taking that opportunity to provide (and replace) the multiple validation error messages.

Are you suggesting that the validation format of [ValidationFn, msgString, validationType] be expanded to allow for an object reference? That'd be a perfect way to build up multiple validation messages or even allow deeper information for single validations.

👍

@vkarpov15
Copy link
Collaborator Author

It already supports an object format, should be easy to extend to add custom properties to the error.

@misterdai
Copy link

Just checking, do you mean that it's possible currently and I've missed something obvious ;) or that it should be easy to add support for this into a future mongoose release?

@vkarpov15
Copy link
Collaborator Author

It's currently only possible to add custom properties when you create the schema, but no way for validation logic to modify properties. It's just useful for formatting error messages. General plan is for a future release.

@misterdai
Copy link

Ah, I wasn't aware of that feature. Took a look but, as you say, there's no way for validation logic to modify properties. I tried out passing an array or object that may maintain a reference that I could update, but it seems that it creates a copy at some point so the reference to my object is gone. Shame, almost feels like that'd do the job otherwise.

@vkarpov15
Copy link
Collaborator Author

The other way would be to make your validator set properties on the document, like:

var breakfastSchema = new mongoose.Schema({
  description: {
    type: String,
    validate: [{
      validator: function(v) {
        var res = v.length <= 70;
        if (!res) {
          this.$customErrors['description'] = 'blah';
        }
        return res;
      }
    }]
  }
});

You can then check $customErrors for extra information related to errors.

@misterdai
Copy link

Interesting, that could work for now. But that wouldn't cover firing the validators on an update would it?

@vkarpov15
Copy link
Collaborator Author

Nope but update validators can help with that.

@misterdai
Copy link

True and that's where I'm kind of stuck. Since Model.update, while I can toggle validation on and it works, I have no avenue for returning my validation errors as only an error object would be returned, the additional property set on the document approach wouldn't work.

@misterdai
Copy link

Ended up producing a plugin called mongoose-validator-all which provides the behaviour I'm after. However, it's a little hacky as I'm returning an array of error messages instead of a single string, by tricking mongoose with a custom .replace method on the array returned.

Not ideal but it'll do for now, until hopefully some point where this kind of behaviour is catered for in mongoose.

@vkarpov15
Copy link
Collaborator Author

I'll watch that plugin

@donnut
Copy link

donnut commented Oct 8, 2015

I was working on validation of some models when I read this issue. I think the current validation capabilities of mongoose are probably sufficient and I'm curious to hear your opinion on why I think it is.

The objective to validate data in the (mongoose) models is different from the objective to validate for the business logic. The objective of validation in the models is to keep the database valid and consistent. Validation as part of the business logic has a somewhat different objective -- generating error messages, warnings or hints that can be send to the user. The latter is not the job of mongoose. Mongoose models must make sure no invalid data can be stored in the collections. From that perspective the data is either valid or invalid, and the validation can terminate on the first error. The current validation capabilities in mongoose make this 'raw' validation possible.

One could argue that you have to duplicate validation functions, on for the model and for the business logic. However, if you start with the more complicated and fine grained validation rules for the business, those rule can be used in the models as well, wrapped in some function to make the api match. That way you are still working DRY.

@misterdai
Copy link

I can see your point, just figured that mongoose was designed to help with the fact that

writing MongoDB validation, casting and business logic boilerplate is a drag

Having validation occur twice, at two different layers, even if it's making use of the same code, doesn't feel right to me. I'm not saying we should force mongoose to behave this way by default, but I think it would be useful if support was added via a flag or some other method.

@damianstasik
Copy link

Will support for multiple error messages for a single field be implemented in v5?

@vkarpov15 vkarpov15 added this to the 4.6 milestone Feb 9, 2016
@vkarpov15
Copy link
Collaborator Author

Will probably just write a plugin to support this behavior. It's easy enough to do, you just need a single validator that wraps a bunch of other validators.

@misterdai
Copy link

Be fantastic if there could just be an alteration to mongoose to support either a string message or an array of messages. Just to avoid the need for such a plugin to fake a replace function on the array, to avoid mongoose error'ing if passing an array of messages back.

@damianstasik
Copy link

damianstasik commented Jun 30, 2016

@vkarpov15

It's easy enough to do, you just need a single validator that wraps a bunch of other validators.

Could you guide me, what said plugin would look like? And would that solution allow for multiple errors from a single field?

@misterdai
Copy link

@visualfanatic The only solution I've found with current Mongoose is to add a custom replace method to the error array. Mongoose would normally expect a String type and then use the standard replace method to do any error formatting. Since we want to return an array of errors for a single field, that array needs the method or Mongoose will fall over at that point and complain about the array.

https://github.com/misterdai/mongoose-validator-all/blob/master/lib/mongoose-validator-all.js#L69

It'd only be a minor change to support in my opinion, to allow a string or an array of strings, but who knows if we'll ever get it ;)

@vkarpov15
Copy link
Collaborator Author

@visualfanatic the plugin linked above looks like it does essentially what I suggested, just a validator composed of multiple validators to be executed in parallel.

@vkarpov15 vkarpov15 removed this from the 4.8 milestone Dec 22, 2016
@vkarpov15
Copy link
Collaborator Author

I'm gonna close this, looks like mongoose-validator-all solves this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them! enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature plugin
Projects
None yet
Development

No branches or pull requests

6 participants