-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Comments
Was this added to v4? |
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. |
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 ? |
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. |
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 ;) |
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 |
👍 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, |
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. |
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.
|
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? |
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 👍 |
It already supports an object format, should be easy to extend to add custom properties to the error. |
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? |
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. |
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. |
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 |
Interesting, that could work for now. But that wouldn't cover firing the validators on an update would it? |
Nope but update validators can help with that. |
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. |
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 Not ideal but it'll do for now, until hopefully some point where this kind of behaviour is catered for in mongoose. |
I'll watch that plugin |
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. |
I can see your point, just figured that mongoose was designed to help with the fact that
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. |
Will support for multiple error messages for a single field be implemented in v5? |
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. |
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. |
Could you guide me, what said plugin would look like? And would that solution allow for multiple errors from a single field? |
@visualfanatic The only solution I've found with current Mongoose is to add a custom 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 ;) |
@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. |
I'm gonna close this, looks like mongoose-validator-all solves this issue. |
No description provided.
The text was updated successfully, but these errors were encountered: