Values validator fails fast when below root scope, violating documentation

[eriklovmo]

In the documentation, it is currently written:

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.

My interpretation of that statement is that, in order to skip subsequent validations when a specific param is found invalid, one must set fail_fast to true (though that interpretation might be far-reaching). However, I have found one example that behaves differently. When using the values validator in a nested scope, the validation is not exhaustive:

grape/lib/grape/validations/validators/values_validator.rb

Line 32 in 3f01d03

return if val.nil? && !required_for_root_scope?

The following snippets demonstrate what this entails.

A

class Foo < Grape::API
  params do
    requires :bar, values: ->(v) { v.zero? || v.positive? }
    requires :baz, values: ->(v) { v.positive? }
  end

  post "/foo" do
    "bar"
  end
end

With example A, the validation includes the following errors when no params are provided:

"bar is missing, bar does not have a valid value, baz is missing, baz does not have a valid value"

B

class Foo < Grape::API
  params do
    with(type: Integer) do
      requires :bar, values: ->(v) { v.zero? || v.positive? }
      requires :baz, values: ->(v) { v.positive? }
    end
  end

  post "/foo" do
    "bar"
  end
end

With example B, the validation includes the following errors when no params are provided:

"bar is missing, baz is missing"

It would be good to either fix this behavior if possible or to update the documentation to mention this exception, either in the section about the values validator or in the general paragraph mentioning "fast failing", which I have cited.

[dblock]

Looks like a bug, appreciate a fix!

[numbata]

Looks like unexpected expected behavior #671

[eriklovmo]

Looks like unexpected expected behavior #671

Ah, that's it! It seems like the code conflates sets of params bundled using with and sets of params bundled with optional, that the PR seemingly targeted. We need to differentiate between "nesting" and "grouping", since the short-circuiting behavior is only desired sometimes.

[dblock]

@eriklovmo Are you trying to fix it? Write some specs at least that fail?

[numbata]

@dblock an example of spec that fails:

diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb
index d8ef17c9..5d8daab9 100644
--- a/spec/grape/validations/validators/values_spec.rb
+++ b/spec/grape/validations/validators/values_spec.rb
@@ -261,6 +261,13 @@ describe Grape::Validations::Validators::ValuesValidator do
         optional :name, type: String, values: %w[a b], allow_blank: true
       end
       get '/allow_blank'
+
+      params do
+        with(type: String) do
+          requires :type, values: ValuesModel.values
+        end
+      end
+      get 'values_wrapped_by_with_block'
     end
   end

@@ -730,4 +737,13 @@ describe Grape::Validations::Validators::ValuesValidator do
       end
     end
   end
+
+  context 'when wrapped by with block' do
+    it 'rejects an invalid value' do
+      get 'values_wrapped_by_with_block'
+
+      expect(last_response.status).to eq 400
+      expect(last_response.body).to eq({ error: 'type is missing, type does not have a valid value' }.to_json)
+    end
+  end
 end

Fails with:

Failures:

 1) Grape::Validations::Validators::ValuesValidator when wrapped by with block rejects an invalid value
    Failure/Error: expect(last_response.body).to eq({ error: 'type is missing, type does not have a valid value' }.to_json)

      expected: "{\"error\":\"type is missing, type does not have a valid value\"}"
           got: "{\"error\":\"type is missing\"}"

      (compared using ==)
    # ./spec/grape/validations/validators/values_spec.rb:746:in `block (3 levels) in <top (required)>'

Finished in 4.14 seconds (files took 1.71 seconds to load)
2189 examples, 1 failure

[numbata]

Possible fix ⬆️

[eriklovmo]

@eriklovmo Are you trying to fix it? Write some specs at least that fail?

I have not had time, unfortunately, but please see the fix from @numbata! @dblock

[eriklovmo]

Done in #2382. Thanks for acknowledging @dblock and thanks for the fix @numbata!