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

try to fix the declared with not given #2285

Merged
merged 33 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
* [#2256](https://github.com/ruby-grape/grape/pull/2256): Raise `Grape::Exceptions::MultipartPartLimitError` from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck).
* [#2266](https://github.com/ruby-grape/grape/pull/2266): Fix code coverage - [@duffn](https://github.com/duffn).
* [#2284](https://github.com/ruby-grape/grape/pull/2284): Fix an unexpected backtick - [@zysend](https://github.com/zysend).
* [#2285](https://github.com/ruby-grape/grape/pull/2285): Try to fix the declared with not given - [@zysend](https://github.com/zysend).
zysend marked this conversation as resolved.
Show resolved Hide resolved
* Your contribution here.

### 1.6.2 (2021/12/30)
Expand Down
19 changes: 19 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,25 @@ See [#2227](https://github.com/ruby-grape/grape/pull/2227) for more information.

Rack supports a configurable limit on the number of files created from multipart parameters (`Rack::Utils.multipart_part_limit`) and raises an error if params are received that create too many files. If you were handling the Rack error directly, Grape now wraps that error in `Grape::Execeptions::TooManyMultipartFiles`. Additionally, Grape will return a 413 status code if the exception goes unhandled.

#### Declared params can choose not to evaluate given

The [`#declared(params)`] will return parameters even if depend on another parameters not given,but it will not return these if `evaluate_given=false` was set.
zysend marked this conversation as resolved.
Show resolved Hide resolved
Here comes an illustration of the old and new behaviour as code:

```ruby
# b depends on a, while a is not given
optional :a
given :a { optional :b }

params = { b: 'b' }
declared(params)
declared(params, evaluate_given: false)
# expect => {}
# actual => { b: 'b' }
# evaluate_given: false => {}

```

### Upgrading to >= 1.6.0

#### Parameter renaming with :as
Expand Down
62 changes: 34 additions & 28 deletions lib/grape/dsl/inside_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def self.post_filter_methods(type)
# has completed
module PostBeforeFilter
def declared(passed_params, options = {}, declared_params = nil, params_nested_path = [])
options = options.reverse_merge(include_missing: true, include_parent_namespaces: true)
options = options.reverse_merge(include_missing: true, include_parent_namespaces: true, evaluate_given: true, request_params: passed_params)
declared_params ||= optioned_declared_params(**options)

if passed_params.is_a?(Array)
Expand All @@ -47,41 +47,47 @@ 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_attr, memo|
# Check given
zysend marked this conversation as resolved.
Show resolved Hide resolved
next if !options[:evaluate_given] && !declared_param_attr.meets_dependency?(options[:request_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)
declared_hash_attr(passed_params, options, declared_param_attr.key, params_nested_path, memo)
end
end

rename_path = params_nested_path + [declared_parent_param.to_s]
renamed_param_name = renamed_params[rename_path]
def declared_hash_attr(passed_params, options, declared_param, params_nested_path, memo)
renamed_params = route_setting(:renamed_params) || {}
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(renamed_param_name || declared_parent_param, options)
passed_children_params = passed_params[declared_parent_param] || passed_params.class.new

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)
end
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)
end
else
# If it is not a Hash then it does not have children.
# Find its value or set it to nil.
next unless options[:include_missing] || passed_params.key?(declared_param)
end
else
# If it is not a Hash then it does not have children.
# Find its value or set it to nil.
return unless options[:include_missing] || passed_params.key?(declared_param)

rename_path = params_nested_path + [declared_param.to_s]
renamed_param_name = renamed_params[rename_path]
rename_path = params_nested_path + [declared_param.to_s]
renamed_param_name = renamed_params[rename_path]

memo_key = optioned_param_key(renamed_param_name || declared_param, options)
passed_param = passed_params[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
memo[memo_key] = passed_param || handle_passed_param(params_nested_path_dup) do
passed_param
end
params_nested_path_dup = params_nested_path.dup
params_nested_path_dup << declared_param.to_s
memo[memo_key] = passed_param || handle_passed_param(params_nested_path_dup) do
passed_param
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/grape/dsl/parameters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,8 @@ def declared_param?(param)
else
# @declared_params also includes hashes of options and such, but those
# won't be flattened out.
@declared_params.flatten.any? do |declared_param|
first_hash_key_or_param(declared_param) == param
@declared_params.flatten.any? do |declared_param_attr|
first_hash_key_or_param(declared_param_attr.key) == param
end
end
end
Expand Down
38 changes: 37 additions & 1 deletion lib/grape/validations/params_scope.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,41 @@ class ParamsScope

include Grape::DSL::Parameters

class Attr
attr_accessor :key, :scope

# Open up a new ParamsScope::Attr
# @param key [Hash, Symbol] key of attr
# @param scope [Grape::Validations::ParamsScope] scope of attr
def initialize(key, scope)
@key = key
@scope = scope
end

def meets_dependency?(request_params)
return true if scope.nil?

scope.meets_dependency?(scope.params(request_params), request_params)
end

# @return Array[Symbol, Hash[Symbol => Array]] declared_params with symbol instead of Attr
def self.attrs_keys(declared_params)
declared_params.map do |declared_param_attr|
attr_key(declared_param_attr)
end
end

def self.attr_key(declared_param_attr)
return attr_key(declared_param_attr.key) if declared_param_attr.is_a?(self)

if declared_param_attr.is_a?(Hash)
declared_param_attr.transform_values { |value| attrs_keys(value) }
else
declared_param_attr
end
end
end

# Open up a new ParamsScope, allowing parameter definitions per
# Grape::DSL::Params.
# @param opts [Hash] options for this scope
Expand Down Expand Up @@ -130,13 +165,14 @@ def required?
# Adds a parameter declaration to our list of validations.
# @param attrs [Array] (see Grape::DSL::Parameters#requires)
def push_declared_params(attrs, **opts)
opts[:declared_params_scope] ||= self
zysend marked this conversation as resolved.
Show resolved Hide resolved
if lateral?
@parent.push_declared_params(attrs, **opts)
else
push_renamed_param(full_path + [attrs.first], opts[:as]) \
if opts && opts[:as]

@declared_params.concat attrs
@declared_params.concat(attrs.map { |attr| ::Grape::Validations::ParamsScope::Attr.new(attr, opts[:declared_params_scope]) })
end
end

Expand Down
126 changes: 126 additions & 0 deletions spec/grape/validations/params_scope_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,132 @@ def initialize(value)
get '/nested', bar: { a: true, c: { b: 'yes' } }
expect(JSON.parse(last_response.body)).to eq('bar' => { 'a' => 'true', 'c' => { 'b' => 'yes' } })
end

context 'when the dependent parameter is not present #declared(params)' do
context 'lateral parameter' do
before do
[true, false].each do |evaluate_given|
subject.params do
optional :a
given :a do
optional :b
end
end
subject.get("/evaluate_given_#{evaluate_given}") { declared(params, evaluate_given: evaluate_given).to_json }
end
end

it 'evaluate_given_true' do
get '/evaluate_given_true', b: 'b'
expect(JSON.parse(last_response.body)).to eq('a' => nil, 'b' => 'b')
end

it 'evaluate_given_false' do
get '/evaluate_given_false', b: 'b'
expect(JSON.parse(last_response.body)).to eq('a' => nil)
end
end

context 'nested parameter' do
before do
[true, false].each do |evaluate_given|
subject.params do
optional :a, values: %w[x y]
given a: ->(a) { a == 'x' } do
optional :b, type: Hash do
optional :c
end
optional :e
end
given a: ->(a) { a == 'y' } do
optional :b, type: Hash do
optional :d
end
optional :f
end
end
subject.get("/evaluate_given_#{evaluate_given}") { declared(params, evaluate_given: evaluate_given).to_json }
end
end

it 'evaluate_given_true' do
get '/evaluate_given_true', a: 'x'
expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => { 'd' => nil }, 'e' => nil, 'f' => nil)

get '/evaluate_given_true', a: 'y'
expect(JSON.parse(last_response.body)).to eq('a' => 'y', 'b' => { 'd' => nil }, 'e' => nil, 'f' => nil)
end

it 'evaluate_given_false' do
get '/evaluate_given_false', a: 'x'
expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => { 'c' => nil }, 'e' => nil)

get '/evaluate_given_false', a: 'y'
expect(JSON.parse(last_response.body)).to eq('a' => 'y', 'b' => { 'd' => nil }, 'f' => nil)
end
end

context 'nested given parameter' do
before do
[true, false].each do |evaluate_given|
subject.params do
optional :a, values: %w[x y]
given a: ->(a) { a == 'x' } do
optional :b, type: Hash do
optional :c
given :c do
optional :g
optional :e, type: Hash do
optional :h
end
end
end
end
given a: ->(a) { a == 'y' } do
optional :b, type: Hash do
optional :d
given :d do
optional :f
optional :e, type: Hash do
optional :i
end
end
end
end
end
subject.get("/evaluate_given_#{evaluate_given}") { declared(params, evaluate_given: evaluate_given).to_json }
end
end

it 'evaluate_given_true' do
get '/evaluate_given_true', a: 'x'
expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => { 'd' => nil, 'f' => nil, 'e' => { 'i' => nil } })

get '/evaluate_given_true', a: 'x', b: { c: 'c' }
expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => { 'd' => nil, 'f' => nil, 'e' => { 'i' => nil } })

get '/evaluate_given_true', a: 'y'
expect(JSON.parse(last_response.body)).to eq('a' => 'y', 'b' => { 'd' => nil, 'f' => nil, 'e' => { 'i' => nil } })

get '/evaluate_given_true', a: 'y', b: { d: 'd' }
expect(JSON.parse(last_response.body)).to eq('a' => 'y', 'b' => { 'd' => 'd', 'f' => nil, 'e' => { 'i' => nil } })
end

it 'evaluate_given_false' do
get '/evaluate_given_false', a: 'x'
expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => { 'c' => nil })

get '/evaluate_given_false', a: 'x', b: { c: 'c' }
expect(JSON.parse(last_response.body)).to eq('a' => 'x', 'b' => { 'c' => 'c', 'g' => nil, 'e' => { 'h' => nil } })

get '/evaluate_given_false', a: 'y'
expect(JSON.parse(last_response.body)).to eq('a' => 'y', 'b' => { 'd' => nil })

get '/evaluate_given_false', a: 'y', b: { d: 'd' }
expect(JSON.parse(last_response.body)).to eq('a' => 'y', 'b' => { 'd' => 'd', 'f' => nil, 'e' => { 'i' => nil } })
end
end
end
end

context 'default value in given block' do
Expand Down
Loading