grape icon indicating copy to clipboard operation
grape copied to clipboard

Array of hashes not failing correctly when array is empty

Open ZeroInputCtrl opened this issue 8 years ago • 6 comments

grape: 1.0.1 ruby: 2.4.2

When you have:

params do
          requires :paired_values, type: Array do
            requires :name, allow_blank: false, type: String
            requires :semver, allow_blank: false, type: String, semver_constraint: true
          end
        end

And semver_constraint custom validator looks like

class SemverConstraint < Grape::Validations::Base
  def validate_param!(attr_name, params)
    Semverse::Constraint.new(params[attr_name])
  rescue Semverse::InvalidConstraintFormat => e
    raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: e.message
  end
end

When you call into the api_endpoint with params: { paired_values: [] } grape ends up calling validate_param! with attr_name being :semver and params being "", so this throws the exception TypeError: no implicit conversion of Symbol into Integer. Because these values are required shouldn't the grape validation for required fail out before this is even called?

I can easily work around this but before i upgraded to 1.0.0/1.0.1 this was not throwing this exception.

ZeroInputCtrl avatar Oct 11 '17 18:10 ZeroInputCtrl

I encourage you to write a spec for this and maybe a fix?

dblock avatar Oct 12 '17 17:10 dblock

I have a very similar issue, which I believe is a bug. I've written a test:

When using the combination of a required param and a custom validator, the custom validator raises errors if the the required param is not present.

https://github.com/rjaus/grape/commit/d372811b452c222f90b4c9ded11b445447a19605

it 'text is not present in request' do
      get '/'
      expect(last_response.status).to eq 400
      expet(last_response.body).to eq 'text is missing'
end

rjaus avatar Jun 03 '20 04:06 rjaus

Setting fail_fast: true & allow_blank: false (ensuring that allow_blank: false comes before)

params do
    requires :date, allow_blank: false, performance_date: true, type: Date, desc: "Date of the performance data", fail_fast: true
end

Still, it seem like this is a bug, as required on it's own shouldn't cause errors.

rjaus avatar Jun 03 '20 07:06 rjaus

@rjaus What's the actual error that you get?

dblock avatar Jun 03 '20 15:06 dblock

params do
  requires :date, performance_date: true, type: Date, desc: "Date of the performance data"
end

Custom Validator

class API::Validations::PerformanceDate < Grape::Validations::Base
  def validate_param!(attr_name, params)

	unless params[attr_name] < Date.tomorrow
		fail Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: 'must not be in the future'
	end
  end
end

If the :date param is NOT included in the request.

Error:

{
  "status": 500,
  "error": "Internal Server Error",
  "exception": "#<NoMethodError: undefined method `<' for nil:NilClass>",
}

Same issue as the initial post. Considering the param is set to requires, I'd expect it not to pass it on to the validator only to fail due to the param not existing.

But now I've discovered fail_fast, I can get it to work as I expected.

rjaus avatar Jun 03 '20 15:06 rjaus

So the custom validator shouldn't be invoked here, this makes sense to me.

dblock avatar Jun 03 '20 17:06 dblock