Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrected the endpoint parameter name generation. #2189

Merged
merged 5 commits into from
Sep 15, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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
}
}
}
````
Expand Down
56 changes: 55 additions & 1 deletion UPGRADING.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,64 @@
Upgrading Grape
===============

### Upgrading to >= 1.5.4

#### Parameter renaming with :as

Prior to 1.5.4 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.

From now on the parameter renaming is not anymore done automatically in the
request payload, but only 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.5.4)
# actual => { b: '5' } (uncasted, unvalidated, <= 1.5.3)
```

Another implication of this is the dependent parameter resolution. Prior to
1.5.4 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:
Expand Down
17 changes: 11 additions & 6 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,20 @@ 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|
params_nested_path_dup = params_nested_path.dup
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)
Expand All @@ -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
Expand Down
10 changes: 1 addition & 9 deletions lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
61 changes: 40 additions & 21 deletions lib/grape/validations/params_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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<Symbol>] 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<String, Symbol>] 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])
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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+
Expand Down
12 changes: 4 additions & 8 deletions lib/grape/validations/validators/as.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading