diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 4320b39374..14a99edc14 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -167,7 +167,7 @@ Metrics/BlockLength: # Offense count: 11 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 304 + Max: 310 # Offense count: 30 # Configuration parameters: IgnoredMethods. diff --git a/CHANGELOG.md b/CHANGELOG.md index 21ac889533..b1b0d9e865 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -### 1.5.4 (Next) +### 1.6.0 (Next) #### Features @@ -9,7 +9,9 @@ * [#2176](https://github.com/ruby-grape/grape/pull/2176): Fix: OPTIONS fails if matching all routes - [@myxoh](https://github.com/myxoh). * [#2177](https://github.com/ruby-grape/grape/pull/2177): Fix: `default` validator fails if preceded by `as` validator - [@Catsuko](https://github.com/Catsuko). * [#2180](https://github.com/ruby-grape/grape/pull/2180): Call `super` in `API.inherited` - [@yogeshjain999](https://github.com/yogeshjain999). +* [#2189](https://github.com/ruby-grape/grape/pull/2189): Fix: rename parameters when using `:as` (behaviour and grape-swagger documentation) - [@Jack12816](https://github.com/Jack12816). * Your contribution here. + ### 1.5.3 (2021/03/07) #### Fixes diff --git a/README.md b/README.md index 84c6813fb0..9133f1ce13 100644 --- a/README.md +++ b/README.md @@ -801,6 +801,7 @@ Grape allows you to access only the parameters that have been declared by your ` * Filter out the params that have been passed, but are not allowed. * Include any optional params that are declared but not passed. + * Perform any parameter renaming on the resulting hash. Consider the following API endpoint: @@ -995,8 +996,10 @@ curl -X POST -H "Content-Type: application/json" localhost:9292/users/signup -d ````json { "declared_params": { - "first_name": "first name", - "last_name": null + "user": { + "first_name": "first name", + "last_name": null + } } } ```` diff --git a/UPGRADING.md b/UPGRADING.md index 67c8b20ca8..aceed0b512 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,10 +1,50 @@ Upgrading Grape =============== +### Upgrading to >= 1.6.0 + +#### Parameter renaming with :as + +Prior to 1.6.0 the [parameter renaming](https://github.com/ruby-grape/grape#renaming) with `:as` was directly touching the request payload ([`#params`](https://github.com/ruby-grape/grape#parameters)) while duplicating the old and the new key to be both available in the hash. This allowed clients to bypass any validation in case they knew the internal name of the parameter. Unfortunately, in combination with [grape-swagger](https://github.com/ruby-grape/grape-swagger) the internal name (name set with `:as`) of the parameters were documented. + +This behavior was fixed. Parameter renaming is now done when using the [`#declared(params)`](https://github.com/ruby-grape/grape#declared) parameters helper. This stops confusing validation/coercion behavior. + +Here comes an illustration of the old and new behaviour as code: + +```ruby +# (1) Rename a to b, while client sends +a+ +optional :a, type: Integer, as: :b +params = { a: 1 } +declared(params, include_missing: false) +# expected => { b: 1 } +# actual => { b: 1 } + +# (2) Rename a to b, while client sends +b+ +optional :a, type: Integer, as: :b, values: [1, 2, 3] +params = { b: '5' } +declared(params, include_missing: false) +# expected => { } (>= 1.6.0) +# actual => { b: '5' } (uncasted, unvalidated, <= 1.5.3) +``` + +Another implication of this change is the dependent parameter resolution. Prior to 1.6.0 the following code produced an `Grape::Exceptions::UnknownParameter` because `:a` was replace by `:b`: + +```ruby +params do + optional :a, as: :b + given :a do # (<= 1.5.3 you had to reference +:b+ here to make it work) + requires :c + end +end +``` + +This code now works without any errors, as the renaming is just an internal behaviour of the `#declared(params)` parameter helper. + +See [#2189](https://github.com/ruby-grape/grape/pull/2189) for more information. ### Upgrading to >= 1.5.3 -### Nil value and coercion +#### Nil value and coercion Prior to 1.2.5 version passing a `nil` value for a parameter with a custom coercer would invoke the coercer, and not passing a parameter would not invoke it. This behavior was not tested or documented. Version 1.3.0 quietly changed this behavior, in such that `nil` values skipped the coercion. Version 1.5.3 fixes and documents this as follows: diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index d121071993..9038fc60b4 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -48,6 +48,8 @@ def declared_array(passed_params, options, declared_params, params_nested_path) end def declared_hash(passed_params, options, declared_params, params_nested_path) + renamed_params = route_setting(:renamed_params) || {} + declared_params.each_with_object(passed_params.class.new) do |declared_param, memo| if declared_param.is_a?(Hash) declared_param.each_pair do |declared_parent_param, declared_children_params| @@ -55,8 +57,11 @@ def declared_hash(passed_params, options, declared_params, params_nested_path) params_nested_path_dup << declared_parent_param.to_s next unless options[:include_missing] || passed_params.key?(declared_parent_param) + rename_path = params_nested_path + [declared_parent_param.to_s] + renamed_param_name = renamed_params[rename_path] + + memo_key = optioned_param_key(renamed_param_name || declared_parent_param, options) passed_children_params = passed_params[declared_parent_param] || passed_params.class.new - memo_key = optioned_param_key(declared_parent_param, options) memo[memo_key] = handle_passed_param(params_nested_path_dup, passed_children_params.any?) do declared(passed_children_params, options, declared_children_params, params_nested_path_dup) @@ -65,13 +70,13 @@ def declared_hash(passed_params, options, declared_params, params_nested_path) else # If it is not a Hash then it does not have children. # Find its value or set it to nil. - has_renaming = route_setting(:renamed_params) && route_setting(:renamed_params).find { |current| current[declared_param] } - param_renaming = has_renaming[declared_param] if has_renaming + next unless options[:include_missing] || passed_params.key?(declared_param) - next unless options[:include_missing] || passed_params.key?(declared_param) || (param_renaming && passed_params.key?(param_renaming)) + rename_path = params_nested_path + [declared_param.to_s] + renamed_param_name = renamed_params[rename_path] - memo_key = optioned_param_key(param_renaming || declared_param, options) - passed_param = passed_params[param_renaming || declared_param] + memo_key = optioned_param_key(renamed_param_name || declared_param, options) + passed_param = passed_params[declared_param] params_nested_path_dup = params_nested_path.dup params_nested_path_dup << declared_param.to_s diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 068e1ca0af..c28ab61384 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -262,7 +262,6 @@ def run else run_filters before_validations, :before_validation run_validators validations, request - remove_renamed_params run_filters after_validations, :after_validation response_object = execute end @@ -328,14 +327,7 @@ def build_helpers Module.new { helpers.each { |mod_to_include| include mod_to_include } } end - def remove_renamed_params - return unless route_setting(:renamed_params) - route_setting(:renamed_params).flat_map(&:keys).each do |renamed_param| - @params.delete(renamed_param) - end - end - - private :build_stack, :build_helpers, :remove_renamed_params + private :build_stack, :build_helpers def execute @block ? @block.call(self) : nil diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 3f3325ed2a..8a6bee152e 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -13,6 +13,8 @@ class ParamsScope # @param opts [Hash] options for this scope # @option opts :element [Symbol] the element that contains this scope; for # this to be relevant, @parent must be set + # @option opts :element_renamed [Symbol, nil] whenever this scope should + # be renamed and to what, given +nil+ no renaming is done # @option opts :parent [ParamsScope] the scope containing this scope # @option opts :api [API] the API endpoint to modify # @option opts :optional [Boolean] whether or not this scope needs to have @@ -23,13 +25,14 @@ class ParamsScope # validate if this param is present in the parent scope # @yield the instance context, open for parameter definitions def initialize(opts, &block) - @element = opts[:element] - @parent = opts[:parent] - @api = opts[:api] - @optional = opts[:optional] || false - @type = opts[:type] - @group = opts[:group] || {} - @dependent_on = opts[:dependent_on] + @element = opts[:element] + @element_renamed = opts[:element_renamed] + @parent = opts[:parent] + @api = opts[:api] + @optional = opts[:optional] || false + @type = opts[:type] + @group = opts[:group] || {} + @dependent_on = opts[:dependent_on] @declared_params = [] @index = nil @@ -129,18 +132,35 @@ def push_declared_params(attrs, **opts) if lateral? @parent.push_declared_params(attrs, **opts) else - if opts && opts[:as] - @api.route_setting(:renamed_params, @api.route_setting(:renamed_params) || []) - @api.route_setting(:renamed_params) << { attrs.first => opts[:as] } - attrs = [opts[:as]] - end + push_renamed_param(full_path + [attrs.first], opts[:as]) \ + if opts && opts[:as] @declared_params.concat attrs end end + # Get the full path of the parameter scope in the hierarchy. + # + # @return [Array] the nesting/path of the current parameter scope + def full_path + nested? ? @parent.full_path + [@element] : [] + end + private + # Add a new parameter which should be renamed when using the +#declared+ + # method. + # + # @param path [Array] the full path of the parameter + # (including the parameter name as last array element) + # @param new_name [String, Symbol] the new name of the parameter (the + # renamed name, with the +as: ...+ semantic) + def push_renamed_param(path, new_name) + base = @api.route_setting(:renamed_params) || {} + base[Array(path).map(&:to_s)] = new_name.to_s + @api.route_setting(:renamed_params, base) + end + def require_required_and_optional_fields(context, opts) if context == :all optional_fields = Array(opts[:except]) @@ -191,11 +211,12 @@ def new_scope(attrs, optional = false, &block) end self.class.new( - api: @api, - element: attrs[1][:as] || attrs.first, - parent: self, - optional: optional, - type: type || Array, + api: @api, + element: attrs.first, + element_renamed: attrs[1][:as], + parent: self, + optional: optional, + type: type || Array, &block ) end @@ -235,6 +256,8 @@ def new_group_scope(attrs, &block) # Pushes declared params to parent or settings def configure_declared_params + push_renamed_param(full_path, @element_renamed) if @element_renamed + if nested? @parent.push_declared_params [element => @declared_params] else @@ -303,10 +326,6 @@ def validates(attrs, validations) next if order_specific_validations.include?(type) validate(type, options, attrs, doc_attrs, opts) end - - # Apply as validator last so other validations are applied to - # renamed param - validate(:as, validations[:as], attrs, doc_attrs, opts) if validations.key?(:as) end # Validate and comprehend the +:type+, +:types+, and +:coerce_with+ diff --git a/lib/grape/validations/validators/as.rb b/lib/grape/validations/validators/as.rb index 77cef5f1cc..78f8e592e5 100644 --- a/lib/grape/validations/validators/as.rb +++ b/lib/grape/validations/validators/as.rb @@ -3,14 +3,10 @@ module Grape module Validations class AsValidator < Base - def initialize(attrs, options, required, scope, **opts) - @renamed_options = options - super - end - - def validate_param!(attr_name, params) - params[@renamed_options] = params[attr_name] - end + # We use a validator for renaming parameters. This is just a marker for + # the parameter scope to handle the renaming. No actual validation + # happens here. + def validate_param!(*); end end end end diff --git a/spec/grape/endpoint/declared_spec.rb b/spec/grape/endpoint/declared_spec.rb index 6d85dd19b6..e50f45bb8d 100644 --- a/spec/grape/endpoint/declared_spec.rb +++ b/spec/grape/endpoint/declared_spec.rb @@ -598,4 +598,251 @@ def app expect(json.key?(:artist_id)).not_to be_truthy end end + + describe 'parameter renaming' do + context 'with a deeply nested parameter structure' do + let(:params) do + { + i_a: 'a', + i_b: { + i_c: 'c', + i_d: { + i_e: { + i_f: 'f', + i_g: 'g', + i_h: [ + { + i_ha: 'ha1', + i_hb: { + i_hc: 'c' + } + }, + { + i_ha: 'ha2', + i_hb: { + i_hc: 'c' + } + } + ] + } + } + } + } + end + let(:declared) do + { + o_a: 'a', + o_b: { + o_c: 'c', + o_d: { + o_e: { + o_f: 'f', + o_g: 'g', + o_h: [ + { + o_ha: 'ha1', + o_hb: { + o_hc: 'c' + } + }, + { + o_ha: 'ha2', + o_hb: { + o_hc: 'c' + } + } + ] + } + } + } + } + end + let(:params_keys) do + [ + 'i_a', + 'i_b', + 'i_b[i_c]', + 'i_b[i_d]', + 'i_b[i_d][i_e]', + 'i_b[i_d][i_e][i_f]', + 'i_b[i_d][i_e][i_g]', + 'i_b[i_d][i_e][i_h]', + 'i_b[i_d][i_e][i_h][i_ha]', + 'i_b[i_d][i_e][i_h][i_hb]', + 'i_b[i_d][i_e][i_h][i_hb][i_hc]' + ] + end + + before do + subject.format :json + subject.params do + optional :i_a, type: String, as: :o_a + optional :i_b, type: Hash, as: :o_b do + optional :i_c, type: String, as: :o_c + optional :i_d, type: Hash, as: :o_d do + optional :i_e, type: Hash, as: :o_e do + optional :i_f, type: String, as: :o_f + optional :i_g, type: String, as: :o_g + optional :i_h, type: Array, as: :o_h do + optional :i_ha, type: String, as: :o_ha + optional :i_hb, type: Hash, as: :o_hb do + optional :i_hc, type: String, as: :o_hc + end + end + end + end + end + end + subject.post '/test' do + declared(params, include_missing: false) + end + subject.post '/test/no-mod' do + before = params.to_h + declared(params, include_missing: false) + after = params.to_h + { before: before, after: after } + end + end + + it 'generates the correct parameter names for documentation' do + expect(subject.routes.first.params.keys).to match(params_keys) + end + + it 'maps the renamed parameter correctly' do + post '/test', **params + expect(JSON.parse(last_response.body, symbolize_names: true)).to \ + match(declared) + end + + it 'maps no parameters when none are given' do + post '/test' + expect(JSON.parse(last_response.body)).to match({}) + end + + it 'does not modify the request params' do + post '/test/no-mod', **params + result = JSON.parse(last_response.body, symbolize_names: true) + expect(result[:before]).to match(result[:after]) + end + end + + context 'with a renamed root parameter' do + before do + subject.format :json + subject.params do + optional :email_address, type: String, regexp: /.+@.+/, as: :email + end + subject.post '/test' do + declared(params, include_missing: false) + end + end + + it 'generates the correct parameter names for documentation' do + expect(subject.routes.first.params.keys).to match(%w[email_address]) + end + + it 'maps the renamed parameter correctly (original name)' do + post '/test', email_address: 'test@example.com' + expect(JSON.parse(last_response.body)).to \ + match('email' => 'test@example.com') + end + + it 'validates the renamed parameter correctly (original name)' do + post '/test', email_address: 'bad[at]example.com' + expect(JSON.parse(last_response.body)).to \ + match('error' => 'email_address is invalid') + end + + it 'ignores the renamed parameter (as name)' do + post '/test', email: 'test@example.com' + expect(JSON.parse(last_response.body)).to match({}) + end + end + + context 'with a renamed hash with nested parameters' do + before do + subject.format :json + subject.params do + optional :address, type: Hash, as: :address_attributes do + optional :street, type: String, values: ['Street 1', 'Street 2'], + default: 'Street 1' + optional :city, type: String + end + end + subject.post '/test' do + declared(params, include_missing: false) + end + end + + it 'generates the correct parameter names for documentation' do + expect(subject.routes.first.params.keys).to \ + match(%w[address address[street] address[city]]) + end + + it 'maps the renamed parameter correctly (original name)' do + post '/test', address: { city: 'Berlin', street: 'Street 2', t: 't' } + expect(JSON.parse(last_response.body)).to \ + match('address_attributes' => { 'city' => 'Berlin', + 'street' => 'Street 2' }) + end + + it 'validates the renamed parameter correctly (original name)' do + post '/test', address: { street: 'unknown' } + expect(JSON.parse(last_response.body)).to \ + match('error' => 'address[street] does not have a valid value') + end + + it 'ignores the renamed parameter (as name)' do + post '/test', address_attributes: { city: 'Berlin', unknown: '1' } + expect(JSON.parse(last_response.body)).to match({}) + end + end + + context 'with a renamed hash with nested renamed parameter' do + before do + subject.format :json + subject.params do + optional :user, type: Hash, as: :user_attributes do + optional :email_address, type: String, regexp: /.+@.+/, as: :email + end + end + subject.post '/test' do + declared(params, include_missing: false) + end + end + + it 'generates the correct parameter names for documentation' do + expect(subject.routes.first.params.keys).to \ + match(%w[user user[email_address]]) + end + + it 'maps the renamed parameter correctly (original name)' do + post '/test', user: { email_address: 'test@example.com' } + expect(JSON.parse(last_response.body)).to \ + match('user_attributes' => { 'email' => 'test@example.com' }) + end + + it 'validates the renamed parameter correctly (original name)' do + post '/test', user: { email_address: 'bad[at]example.com' } + expect(JSON.parse(last_response.body)).to \ + match('error' => 'user[email_address] is invalid') + end + + it 'ignores the renamed parameter (as name, 1)' do + post '/test', user: { email: 'test@example.com' } + expect(JSON.parse(last_response.body)).to \ + match({ 'user_attributes' => {} }) + end + + it 'ignores the renamed parameter (as name, 2)' do + post '/test', user_attributes: { email_address: 'test@example.com' } + expect(JSON.parse(last_response.body)).to match({}) + end + + it 'ignores the renamed parameter (as name, 3)' do + post '/test', user_attributes: { email: 'test@example.com' } + expect(JSON.parse(last_response.body)).to match({}) + end + end + end end diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index e0ed391691..fdc0357f88 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -144,7 +144,7 @@ def initialize(value) get '/renaming-coerced', foo: ' there we go ' expect(last_response.status).to eq(200) - expect(last_response.body).to eq('there we go-') + expect(last_response.body).to eq('-there we go') end it do @@ -185,7 +185,7 @@ def initialize(value) subject.params do optional :foo, as: :bar, default: 'before' end - subject.get('/rename-before-default') { params[:bar] } + subject.get('/rename-before-default') { declared(params)[:bar] } get '/rename-before-default' expect(last_response.status).to eq(200) @@ -196,7 +196,7 @@ def initialize(value) subject.params do optional :foo, default: 'after', as: :bar end - subject.get('/rename-after-default') { params[:bar] } + subject.get('/rename-after-default') { declared(params)[:bar] } get '/rename-after-default' expect(last_response.status).to eq(200) @@ -590,7 +590,7 @@ def initialize(value) it 'allows renaming of dependent on parameter' do subject.params do optional :a, as: :b - given b: ->(val) { val == 'x' } do + given a: ->(val) { val == 'x' } do requires :c end end @@ -604,7 +604,7 @@ def initialize(value) expect(last_response.status).to eq 200 end - it 'raises an error if the dependent parameter is not the renamed one' do + it 'does not raise if the dependent parameter is not the renamed one' do expect do subject.params do optional :a, as: :b @@ -612,6 +612,17 @@ def initialize(value) requires :c end end + end.not_to raise_error + end + + it 'raises an error if the dependent parameter is the renamed one' do + expect do + subject.params do + optional :a, as: :b + given :b do + requires :c + end + end end.to raise_error(Grape::Exceptions::UnknownParameter) end