Error responses contain strange indexes

[renra]

Hello,

I have the following grape endpoint:

params do
  requires :user, type: Hash do
    requires :access_start, type: Date, allow_blank: false
    # ... more required attributes
  end

  require :filters, type: Array
end

When I send the following data: {"user":{"access_start": null},"filters":[{"id":{"equals":[1, 2]}}]} and I inspect the error raised by grape(calling as_json on it), I sometimes get errors like this:

[{:params=>["user[access_start]"], :messages=>["is empty"]}]

and sometimes like this:

[{:params=>["user[0][access_start]"], :messages=>["is empty"]}]

I am not able to get a consistent behaviour, but the first way seems correct to me. Using grape 0.16.2

Thanks!

[dblock]

This should be reproducible. Are you sending correct content-type application/json?

[pvmeerbe]

I'm also experiencing this issue : an RSpec test file with multiple tests (happy and failure paths) throws in extra indexes in error messages. The 'Content-Type' => 'application/json' header is correctly set.

I was able to simulate the issue when adding debug points, although the issue does not always appears for the same tests (so similar to the original reported the issue appears sometimes & sometimes not).

Grape params declaration :

    params do
      requires :company, type: Hash do
        requires :number, allow_blank: false, type: String, desc: "Company A-number."
        requires :name, allow_blank: false, type: String, desc: "Company name"
      end
      requires :delivery_contact, type: Hash do
        requires :email, allow_blank: false, type: String, desc: "Delivery_email address.", regexp: /.+@.+/
      end

      requires :description, allow_blank: false, type: Hash, desc: "Deliverable description" do
        requires :ico_number, allow_blank: false, type: String
        requires :ico_line_number, allow_blank: false, type: String # explicitly converted to although ayuda model has Integer
        requires :po_number, allow_blank: false, type: String
        requires :ordered_quantity, allow_blank: false, type: Float # keep in sync with ayuda model type
        requires :ordered_product, allow_blank: false, type: Hash do
          requires :number, allow_blank: false, type: String
          requires :description, allow_blank: false, type: String
        end
        optional :contract_ref, type: String
        optional :reseller, type: Hash do
          requires :name, type: String
          requires :number, type: String
          optional :email, type: String, regexp: /.+@.+/
        end
        optional :end_user, type: Hash do
          requires :name, type: String
          requires :number, type: String
        end
        optional :customer, type: Hash do
          requires :name, type: String
          requires :number, type: String
        end
      end
    end

I was able to trace it to the 'grape-0.16.2/lib/grape/validations/params_scope.rb:51' path (screenshots taken using rubymine debugger during handling of the 'ordered_quantity' key).

Sometimes the @parent.index is incorrectly set to '0' for a declared param of type Hash resulting in the incorrect message "description[0][ordered_quantity]":

Most of the times @parent.index is not set which is the correct behavior resulting in the correct message "description[ordered_quantity]":

[pvmeerbe]

seems to be caused by cec42f2 which contains the following code

/lib/grape/validations/attributes_iterator.rb:17

if resource_params.is_a?(Hash) && resource_params[attr_name].is_a?(Array)
          scope.index = index
end

In one of my negative tests I provide an empty Array as data for the 'description' parameter of type Hash, which should be detected as invalid data. However as a side-effect of the above code the 'index' key is added, which results in the behavior described in this ticket.

Since RSpec switches the test execution order, the behavior is inconsistent, i.e. only occurs after this specific empty Array test was executed...

[pvmeerbe]

I had a look at the original pull request cec42f2. IMHO this implementation is not correct/incomplete:

• as shown by this ticket unwanted side-effects can occur if global config is used/changed (in this case the Grape::Validations::ParamsScope objects) to construct error messages for specific validation actions
• detecting whether the value is an array is done before coercion, causing the implementation to fail when using JSON arrays ; this can be demonstrated using an existing test : https://github.com/ruby-grape/grape/blob/master/spec/grape/validations/validators/coerce_spec.rb#L385
  • comment out lines 397-399 (these trigger the setting of the index attribute on @parent ) & run only this test
  • produces the following error

   expected: "ints[0][i] is missing, ints[0][i] is invalid, ints[0][j] is missing"
      got: "ints[i] is missing, ints[i] is invalid, ints[j] is missing"  

• the error message should be constructed based on the expected type & not on the type of the provided value for validation;
  • Example https://github.com/ruby-grape/grape/blob/master/spec/grape/validations_spec.rb#L338 :
    presenting an Array value [{ key: 'hash in array' }] for an expected hash key 'items'
  • produces the weird error 'items is invalid, items[0][key] does not have a valid value' instead of the more logic error 'items does not have a valid value'

A naive simple fix for the issue described by this ticket could be : https://github.com/ruby-grape/grape/blob/master/lib/grape/validations/attributes_iterator.rb#L17

         if resource_params.is_a?(Hash) && resource_params[attr_name].is_a?(Array)
          scope.index = index
         else
           scope.index = nil
         end

This way:

• the global scope object is still abused but reset when needed
• the 2 other issues described above are still unsolved

[dblock]

I think you should write a bunch of specs @pvmeerbe and try to implement fixes for the failures.

[dblock]

@renra Check out HEAD now that #1463 has been merged. Is this still a problem?

[pvmeerbe]

I can confirm the reported issue does not occur anymore using grape 0.17.0

[dblock]

Closing.