'undefined method key?' when calling declared when params contain an empty array.

[astlouisf]

The test added by this commit fails with

Grape::API::Helpers defines parameters
     Failure/Error: return unless options[:include_missing] || passed_params.key?(declared_param)

     NoMethodError:
       undefined method 'key?' for an instance of String
     # ./lib/grape/dsl/inside_route.rb:82:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash_attr'
     # ./lib/grape/dsl/inside_route.rb:57:in 'block in Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash'
     # ./lib/grape/dsl/inside_route.rb:54:in 'Array#each'
     # ./lib/grape/dsl/inside_route.rb:54:in 'Enumerable#each_with_object'
     # ./lib/grape/dsl/inside_route.rb:54:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash'
     # ./lib/grape/dsl/inside_route.rb:35:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared'
     # ./lib/grape/dsl/inside_route.rb:49:in 'block in Grape::DSL::InsideRoute::PostBeforeFilter#declared_array'
     # ./lib/grape/dsl/inside_route.rb:48:in 'Array#map'
     # ./lib/grape/dsl/inside_route.rb:48:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared_array'
     # ./lib/grape/dsl/inside_route.rb:33:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared'
     # ./lib/grape/dsl/inside_route.rb:76:in 'block (2 levels) in Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash_attr'
     # ./lib/grape/dsl/inside_route.rb:99:in 'Grape::DSL::InsideRoute::PostBeforeFilter#handle_passed_param'
     # ./lib/grape/dsl/inside_route.rb:75:in 'block in Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash_attr'
     # ./lib/grape/dsl/inside_route.rb:64:in 'Hash#each_pair'
     # ./lib/grape/dsl/inside_route.rb:64:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash_attr'
     # ./lib/grape/dsl/inside_route.rb:57:in 'block in Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash'
     # ./lib/grape/dsl/inside_route.rb:54:in 'Array#each'
     # ./lib/grape/dsl/inside_route.rb:54:in 'Enumerable#each_with_object'
     # ./lib/grape/dsl/inside_route.rb:54:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared_hash'
     # ./lib/grape/dsl/inside_route.rb:35:in 'Grape::DSL::InsideRoute::PostBeforeFilter#declared'
     # ./spec/grape/api/array_issue_spec.rb:13:in 'block (4 levels) in <top (required)>'
     # ./lib/grape/endpoint.rb:62:in 'UnboundMethod#bind_call'
     # ./lib/grape/endpoint.rb:62:in 'block (2 levels) in Grape::Endpoint.generate_api_method'
     # /usr/local/bundle/gems/ruby/3.4.0/gems/activesupport-8.0.2/lib/active_support/notifications.rb:212:in 'ActiveSupport::Notifications.instrument'
     # ./lib/grape/endpoint.rb:61:in 'block in Grape::Endpoint.generate_api_method'
     # ./lib/grape/endpoint.rb:287:in 'Grape::Endpoint#execute'
     # ./lib/grape/endpoint.rb:267:in 'block in Grape::Endpoint#run'
     # /usr/local/bundle/gems/ruby/3.4.0/gems/activesupport-8.0.2/lib/active_support/notifications.rb:212:in 'ActiveSupport::Notifications.instrument'
     # ./lib/grape/endpoint.rb:250:in 'Grape::Endpoint#run'
     # ./lib/grape/endpoint.rb:394:in 'block in Grape::Endpoint#build_stack'
     # ./lib/grape/middleware/base.rb:33:in 'Grape::Middleware::Base#call!'
     # ./lib/grape/middleware/base.rb:26:in 'Grape::Middleware::Base#call'
     # ./lib/grape/middleware/error.rb:34:in 'block in Grape::Middleware::Error#call!'
     # ./lib/grape/middleware/error.rb:34:in 'Kernel#catch'
     # ./lib/grape/middleware/error.rb:34:in 'Grape::Middleware::Error#call!'
     # ./lib/grape/middleware/base.rb:26:in 'Grape::Middleware::Base#call'
     # /usr/local/bundle/gems/ruby/3.4.0/gems/rack-3.1.12/lib/rack/head.rb:15:in 'Rack::Head#call'
     # ./lib/grape/endpoint.rb:225:in 'Grape::Endpoint#call!'
     # ./lib/grape/endpoint.rb:219:in 'Grape::Endpoint#call'
     # ./lib/grape/router/route.rb:27:in 'Grape::Router::Route#exec'
     # ./lib/grape/router.rb:139:in 'Grape::Router#process_route'
     # ./lib/grape/router.rb:86:in 'block in Grape::Router#identity'
     # ./lib/grape/router.rb:119:in 'Grape::Router#transaction'
     # ./lib/grape/router.rb:84:in 'Grape::Router#identity'
     # ./lib/grape/router.rb:68:in 'block in Grape::Router#call'
     # ./lib/grape/router.rb:155:in 'Grape::Router#with_optimization'
     # ./lib/grape/router.rb:67:in 'Grape::Router#call'
     # ./lib/grape/api/instance.rb:164:in 'Grape::API::Instance#call'
     # ./lib/grape/api/instance.rb:68:in 'Grape::API::Instance.call!'
     # ./lib/grape/api/instance.rb:63:in 'Grape::API::Instance.call'
     # /usr/local/bundle/gems/ruby/3.4.0/gems/rack-test-2.2.0/lib/rack/test.rb:360:in 'Rack::Test::Session#process_request'
     # /usr/local/bundle/gems/ruby/3.4.0/gems/rack-test-2.2.0/lib/rack/test.rb:163:in 'Rack::Test::Session#custom_request'
     # /usr/local/bundle/gems/ruby/3.4.0/gems/rack-test-2.2.0/lib/rack/test.rb:112:in 'Rack::Test::Session#post'
     # ./spec/grape/api/array_issue_spec.rb:28:in 'block (2 levels) in <top (required)>'

Somehow¹, params contains {"z" => [""]}.

I'm new to ruby and I'm not sure how Grape actually works. My current assumptions are that Grape validates params regardless of if declared is called or not; declared is only used to extract declared params according to the params schema. With that it mind, it feels like validation let through an array that shouldn't have passed validation.

Applying

diff --git a/spec/grape/api/array_issue_spec.rb b/spec/grape/api/array_issue_spec.rb
index d8ff4af1..5f1a5bbf 100644
--- a/spec/grape/api/array_issue_spec.rb
+++ b/spec/grape/api/array_issue_spec.rb
@@ -4,7 +4,7 @@ describe Grape::API::Helpers do
   subject do
     Class.new(Grape::API) do
       params do
-        optional :z, type: Array do
+        requires :z, type: Array do
           requires :a, type: Integer
         end
       end

result in a 400 status code. This points to an issue specific to the use of both optional and type: Array.

Footnotes

1. I suspect it has to do with how form encoded bodies can't effectively encode an empty array that's not absent. ↩

[ericproulx]

@astlouisf I've look into your test and I do confirm a bug. Since there's no media type in the post, the original payload is parsed as application/x-www-form-urlencoded and { "z": [] } gives { "z": [""] } since the actual received payload is z[]=.

If you specify a content-type like application/json like the following test, it does not crash since params will be equal to { "z": [] } not { "z": [""] }

describe '.declared_params' do
  let(:app) do
    Class.new(Grape::API) do
      format :json
      params do
        requires :z, type: Array do
          optional :a, type: Integer
        end
      end

      post do
        declared(params, include_missing: false)
        status 200
      end
    end
  end


  it 'defines parameters' do
    post '/', { "z": [] }.to_json, { 'CONTENT_TYPE' => 'application/json' }
    expect(last_response.body).to eq '200'
  end
end

[astlouisf]

I'm not sure the fix is right. It leads to unintended side effects such as

  describe 'optional_array' do
    subject { last_response }

    let(:app) do
      Class.new(Grape::API) do
        params do
          requires :z, type: Array do
            optional :c, type: Integer
          end
        end

        post do
          d = declared(params, include_missing: false)
          pp d
        end
      end
    end

    before { post '/', { z: ["a", "b"] }, { "Content-Type" => "application/x-www-form-urlencoded" } }

    it { is_expected.to be_successful }
  end

where d gets assigned {"z" => ["", ""]}. I would expect parameters that don't match the schema to fail validation instead.

[ericproulx]

I will check this out and try to understand whats happening. Meanwhile, are you expecting an array of Integers like { z: [1,2,3] } or an array of hashes that contains a key c with an Integer value. Something like {z: [{c: 1}, {c: 2}]}. If its just an array of integers, you can declare Array[Integer]. Nonetheless, there's still an issue, I just want to help to understand :)

[astlouisf]

@ericproulx My use case was my initial example of an optional array of (more complex) hashes. I knew that setting the content-type to application/json would result in parameters being properly validated and this is our intended happy path. My issue was that a user could still set the content-type to application/x-www-form-urlencoded and bypass part of the validation¹. Letting through unexpected values could lead to bugs or crashes.

I'm not sure that the empty array case should be handled for application/x-www-form-urlencoded instead of rejected, but maybe it's possible if the schema is not ambiguous when url-encoded.

Meanwhile, are you expecting an array of Integers like { z: [1,2,3] } or an array of hashes that contains a key c with an Integer value.

Right now, I'm pulling theoretical examples that highlight a problem in the way I expect schema-based validation to work. So it's a bit less about solving my specific use case and more about improving schema validation.

Hope that helps/clarifies :)

Footnotes

1. I could probably reject url-encoded bodies, but it would only be required because of a bug in how url-encoded params are validated. ↩

[ericproulx]

Cool, thanks for the update. I'll take a closer look this week.

[dblock]

Let's reopen this so we don't lose it.

@astlouisf feel free to open PRs even if it's just with a new failing spec