Fix alias on dependent param bug

[darren987469]

Fixes #1808

It seems like we cannot access to the value inside the proc for an alias param

requires :type, as: :action
given type: ->(val) { ActionService::ACTIONS.include?(val) } do #val not present inside the proc
  requires :comment, type: String
end

The root cause is as: :action alias would change params from { type: 'some type' } to { action: 'some type' } in as validator.

grape/lib/grape/validations/validators/as.rb

Lines 9 to 12 in 3bb835a

	def validate_param!(attr_name, params)
	params[@alias] = params[attr_name]
	params.delete(attr_name)
	end

When checking ->(val) { ActionService::ACTIONS.include?(val) } in the following snippet(line 61), dependency_key is still :type, so params[:type] is nil because the params is changed in previous validator.

grape/lib/grape/validations/params_scope.rb

Lines 57 to 65 in 3bb835a

	@dependent_on.each do |dependency|
	if dependency.is_a?(Hash)
	dependency_key = dependency.keys[0]
	proc = dependency.values[0]
	return false unless proc.call(params.try(:[], dependency_key))
	elsif params.respond_to?(:key?) && params.try(:[], dependency).blank?
	return false
	end
	end

The idea to fix is let attribute name in given the same as the aliased one. That means the usage now becomes:

reuiqres :type, as: :action
given action: ->(val) { ... }

[dblock]

Thanks! So what happens if I was using the unaliased param before, that is given a: in the spec below instead of given b:?

subject.params do
        optional :a, as: :b
        given b: ->(val) { val == 'x' } do
          requires :c
        end
      end

I am going to guess that without this fix given a: works, but given b: fails, and vice-versa. Could you please add a spec for this?

If that's the case we should either support both or clearly state what happens here in UPGRADING.

[dblock]

Maybe say something more explicit? Eg. "Fix support in given for aliased params"?

[darren987469]

Yeah, it is more clear.

[darren987469]

Let me describe given a:, given b: cases with or without this fix.

Without this fix case given a:

subject.params do
  optional :a, as: :b                # 'as: :b' change params form { a: 'something' } to { b: 'something' }
  given a: ->(val) { val == 'x' } do # val = params[:a] = nil, condition doesn't meet
    requires :c
  end
end
subject.get('/') { declared(params) }

get '/', a: 'x'
expect(last_response.status).to eq 400              # fail, since given condition doesn't meet 
expect(last_response.body).to eq 'c is missing' 

Without this fix case given b:

subject.params do
  optional :a, as: :b                           
  given b: ->(val) { val == 'x' } do # raise Grape::Exceptions::UnknownParameter. @declared_params in grape/lib/grape/dsl/parameters.rb is [:a], which doesn't contain :b. That why this fix try to solve.
    requires :c
  end
end

With this fix case given a:

subject.params do
  optional :a, as: :b                # 'as: :b' change params form { a: 'somthing' } to { b: 'something' }
  given a: ->(val) { val == 'x' } do # raise Grape::Exceptions::UnknownParameter.  @declared_params in grape/lib/grape/dsl/parameters.rb is now [:b], which doesn't contain :a. 
    requires :c
  end
end

With this fix case given b:

subject.params do
  optional :a, as: :b            # 'as: :b' change params form { a: 'somthing' } to { b: 'something' }               
  given b: ->(val) { val == 'x' } do # val = params[:b] 
    requires :c
  end
end

get '/', a: 'x'
expect(last_response.status).to eq 400              # success, since given condition meets
expect(last_response.body).to eq 'c is missing' 

I think it makes sense to force to use given b:. Because we change params from { a: 'something' } to { b: 'something' }.
What do you think?

[darren987469]

I add a spec to show with this fix, given :a(not the aliased param :b) case.

[dblock]

Perfect, thanks for the specs.