Support fail_fast param validation option #1499

dgasper · 2016-09-28T14:55:55Z

#1423
This change will enable

params do
  required :beer, fail_fast: true
  required :wine
end

which will result in skipping of subsequent validation checks if a fail_fast required or optional fail validation.

dblock · 2016-09-30T18:31:08Z

lib/grape/validations/params_scope.rb

@@ -140,9 +140,13 @@ def require_optional_fields(context, opts)
      end

      def validate_attributes(attrs, opts, &block)
+        # check fail_fast flag
+        fail_fast = opts.delete(:fail_fast) || false


You should not be modifying opts being passed in here, combine the clone below.

dblock · 2016-09-30T18:31:12Z

README.md

@@ -1355,6 +1355,23 @@ subject.rescue_from Grape::Exceptions::ValidationErrors do |e|
 end
 ```

+Grape returns all validation and coercion errors found by default. To skip all subsequent validation checks when a specific param is found invalid, use `fail_fast: true`.
+For example:


Maybe space things up a bit. Minor.

dblock · 2016-09-30T18:31:50Z

README.md

+  required :wine
+end
+```
+will not check if `:wine` is present unless it finds `:beer`. The result of empty params would be a single `Grape::Exceptions::ValidationErrors` error.


I think a lot of grape examples try to make full sentences, so prefer "The following example will not ..." to having "For example: [code] will not".

dblock · 2016-09-30T18:34:07Z

lib/grape/validations/params_scope.rb

@@ -201,7 +205,7 @@ def configure_declared_params
        end
      end

-      def validates(attrs, validations)
+      def validates(attrs, validations, fail_fast = false)


I think the argument should be an options hash here, so we can make it extensible for the future. So def validates(attrs, validations, opts = {}).

dblock · 2016-09-30T18:35:48Z

lib/grape/validations/validators/base.rb

@@ -10,10 +10,11 @@ class Base
      # @param options [Object] implementation-dependent Validator options
      # @param required [Boolean] attribute(s) are required or optional
      # @param scope [ParamsScope] parent scope for this Validator
-      def initialize(attrs, options, required, scope)
+      def initialize(attrs, options, required, fail_fast, scope)


Should fail_fast be rolled into one of these existing params? I am worried about custom validators other people built.

@dblock I've thought about it. It could be a part of the scope, but I am not sure if it is the best place for validation options. Thoughts?

I've changed the constructor to the following to keep existing custom validators working.
def initialize(attrs, options, required, scope, opts = {})

dblock · 2016-09-30T18:36:55Z

This is excellent. Most of my comments are small implementation-related things, I'd like fail_fast to roll in as yet another option on the validator rather than its own big kind of thing. I am sure we'll have future other options.

…tions

dblock · 2016-10-03T18:33:42Z

lib/grape/validations/params_scope.rb

@@ -140,9 +140,14 @@ def require_optional_fields(context, opts)
      end

      def validate_attributes(attrs, opts, &block)


Have you tried to do this?

def validate_attributes(attrs, validations, opts = {}, &block)

This way there wouldn't be any reason to do the selection below.

Either way this seems to be a bit too much future-proof ;)

validations = opts.clone validations[:type] ||= Array if block options = {} options[:fail_fast] = validations.delete(:fail_fast)

... which begs the question, why is fail_fast so special? Why does it need to be split at all? Why can't it just be @fail_fast = validations[:fail_fast] and not pass anything more around?

Then I would have to split the options here, no?

Hmm, I guess I got a bit carried away with the future other options idea :)

In my opinion, the difference is that the rest of the validations refer to a specific validation type with options whereas fail_fast (and possible future validation options) influences the validation process across multiple validations.

But I could be wrong. I just dove into grape code for the first time :)

Try to make it simpler for now, if not splitting is less code, we should go with that.

dblock · 2016-10-05T16:08:29Z

This is better. Merging.

dgasper added 4 commits September 28, 2016 14:15

Add fail_fast tests

b3a75c4

Add fail_fast param validation option

f217627

fix tests boken by fail_fast

8bfd1eb

Update readme with fail_fast option info

32de18e

dgasper force-pushed the fail-fast-on-param-validation branch from 18c27b5 to 32de18e Compare September 28, 2016 15:01

dblock reviewed Sep 30, 2016

View reviewed changes

dgasper added 3 commits October 3, 2016 13:23

leave base validation constructor intact + add optional validation op…

8542d6e

…tions

Use full sentences and add some spacing to fail_fast documentation part

89ec9a7

replace fail_fast with opts in validator constructors

0a4c3e7

dblock reviewed Oct 3, 2016

View reviewed changes

simplify things

5695f14

dblock merged commit d734415 into ruby-grape:master Oct 5, 2016

dgasper deleted the fail-fast-on-param-validation branch October 7, 2016 13:23

dblock mentioned this pull request Oct 7, 2016

Ability to fail fast on params validation #1423

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fail_fast param validation option #1499

Support fail_fast param validation option #1499

dgasper commented Sep 28, 2016 •

edited

Loading

dblock Sep 30, 2016

dblock Sep 30, 2016

dblock Sep 30, 2016

dblock Sep 30, 2016

dgasper Oct 3, 2016

dblock Sep 30, 2016 •

edited

Loading

dgasper Oct 3, 2016 •

edited

Loading

dblock commented Sep 30, 2016

dblock Oct 3, 2016

dblock Oct 3, 2016

dblock Oct 3, 2016 •

edited

Loading

dgasper Oct 4, 2016

dblock Oct 4, 2016

dblock commented Oct 5, 2016

		@@ -140,9 +140,14 @@ def require_optional_fields(context, opts)
		end

		def validate_attributes(attrs, opts, &block)

Support fail_fast param validation option #1499

Support fail_fast param validation option #1499

Conversation

dgasper commented Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

dgasper Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

dblock commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Oct 5, 2016

dgasper commented Sep 28, 2016 •

edited

Loading

dblock Sep 30, 2016 •

edited

Loading

dgasper Oct 3, 2016 •

edited

Loading

dblock Oct 3, 2016 •

edited

Loading