Collection of validation errors

[stevschmid]

We added the ability to collect the validation errors in order to display them at once (e.g. in a form).

Usage:

collect_validation_errors true

rescue_from Grape::Exceptions::Validations do |e|
  Rack::Response.new({
    'status' => 422,
    'message' => e.message,
    'errors' => e.errors
  }.to_json, 422)
end

We made sure that the exception handling is backwards compatible.

What do you think about it?

[dblock]

I wonder whether we should break backwards compatibility and raise the collection instead of adding the option and raising the first validation error? Another option is to raise the first error if there's one or the collection if there're many. Thoughts?

[dblock]

Please also update CHANGELOG and README.

[stevschmid]

Thanks for the feedback. I would preserve backwards compatibility if you don't plan to release a new major version in the near future.

In the next major version we could raise the collection by default (and remove the flag collect_validation_errors altogether).

[dblock]

I'd rather break backward compatibility here than introduce an option that will have to be removed. Will bump the major version.

[stevschmid]

I'm okay with that. I just removed the backwards compatibility and found a bug along the way where each presence validator was registered twice (cf. 3723c99).

The only gripe (no pun intended) I have is that the base Grape::Exceptions::Validation class has a status attribute, which makes no sense in the error collection. Rescuing and responding to validations errors as outlined in the README would include a status attribute for each validation error.

[dblock]

I think :status should go away too. If you raise a validation error, it gets collected and we return a fixed 400 status. There's a way to change that status by doing a rescue block.

Am I missing anything?

[stevschmid]

Ok. I'll look into it as soon as I find the time.

[spariev]

@dblock, @stevschmid , any updates ? This feature would be really handy.

[dblock]

@stevschmid If you don't have time to finish it, LMK, I'll try to find some this week.

[stevschmid]

@dblock I was absent the last 3 weeks. I should be able to finish it this week.

[stevschmid]

I removed the status, but I'm very unhappy with the result. In my opinion, the whole validation system is badly designed. My pull request would be a hack on top of it and make the system even more fragile:

• In my eyes, exceptions should not be used for normal application flow. Here is a good explanation.
• The system in place is misused by the DefaultValidator to set the default values.

At the end of the day, you are interested in all the validation errors, optimally grouped by the parameter name. Rails solves the collection of the validation errors very elegantly with ActiveModel::Errors. I think it would be better to replace the whole validation system by something more robust and use the Rails way as an inspiration. What do you think?

[dblock]

I think this implementation is better than what we had before, so I am going to merge it. I don't want to launch a debate on exceptions vs. non-exceptions, but my philosophy is that the burden of checking should not be put on the developer, at least not by default. I side with the idea that "developers should write less code", and isn't that the whole purpose of the library like Grape?

I would be open to replacing the validation system with something better if it's actually better :)

[stevschmid]

I have a more polished version. I'll push it tomorrow.

[siong1987]

A new version of the gem should be released as soon as possible after this merge. I spent hours (more like 15 minutes) to figure out that the documentation is wrong after this merge.

Only the old "Grape::Exceptions::Validation" (without s) works in the current release of the gem.