From 5dc4e5bf6a11c198c814882df216fe4cbc67bed5 Mon Sep 17 00:00:00 2001 From: Tim Connor Date: Fri, 18 Sep 2020 15:21:46 +1200 Subject: [PATCH] Ensure complete declared params structure is present --- CHANGELOG.md | 1 + README.md | 53 ++++++++++++++++-- UPGRADING.md | 47 ++++++++++++++-- lib/grape/dsl/inside_route.rb | 59 ++++++++------------ spec/grape/endpoint/declared_spec.rb | 81 +++++++++++++++++++--------- 5 files changed, 171 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08995a1a47..1c8f0f8005 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ #### Fixes * Your contribution here. +* [#2103](https://github.com/ruby-grape/grape/pull/2103): Ensure complete declared params structure is present - [@tlconnor](https://github.com/tlconnor). * [#2099](https://github.com/ruby-grape/grape/pull/2099): Added truffleruby to Travis-CI - [@gogainda](https://github.com/gogainda). * [#2089](https://github.com/ruby-grape/grape/pull/2089): Specify order of mounting Grape with Rack::Cascade in README - [@jonmchan](https://github.com/jonmchan). * [#2083](https://github.com/ruby-grape/grape/pull/2083): Set `Cache-Control` header only for streamed responses - [@stanhu](https://github.com/stanhu). diff --git a/README.md b/README.md index c3a5ce73b7..e1ca5c4172 100644 --- a/README.md +++ b/README.md @@ -353,7 +353,7 @@ use Rack::Session::Cookie run Rack::Cascade.new [Web, API] ``` -Note that order of loading apps using `Rack::Cascade` matters. The grape application must be last if you want to raise custom 404 errors from grape (such as `error!('Not Found',404)`). If the grape application is not last and returns 404 or 405 response, [cascade utilizes that as a signal to try the next app](https://www.rubydoc.info/gems/rack/Rack/Cascade). This may lead to undesirable behavior showing the [wrong 404 page from the wrong app](https://github.com/ruby-grape/grape/issues/1515). +Note that order of loading apps using `Rack::Cascade` matters. The grape application must be last if you want to raise custom 404 errors from grape (such as `error!('Not Found',404)`). If the grape application is not last and returns 404 or 405 response, [cascade utilizes that as a signal to try the next app](https://www.rubydoc.info/gems/rack/Rack/Cascade). This may lead to undesirable behavior showing the [wrong 404 page from the wrong app](https://github.com/ruby-grape/grape/issues/1515). ### Rails @@ -787,7 +787,12 @@ Available parameter builders are `Grape::Extensions::Hash::ParamBuilder`, `Grape ### Declared -Grape allows you to access only the parameters that have been declared by your `params` block. It filters out the params that have been passed, but are not allowed. Consider the following API endpoint: +Grape allows you to access only the parameters that have been declared by your `params` block. It will: + + * Filter out the params that have been passed, but are not allowed. + * Include any optional params that are declared but not passed. + +Consider the following API endpoint: ````ruby format :json @@ -820,9 +825,9 @@ Once we add parameters requirements, grape will start returning only the declare format :json params do - requires :user, type: Hash do - requires :first_name, type: String - requires :last_name, type: String + optional :user, type: Hash do + optional :first_name, type: String + optional :last_name, type: String end end @@ -850,6 +855,44 @@ curl -X POST -H "Content-Type: application/json" localhost:9292/users/signup -d } ```` +Missing params that are declared as type `Hash` or `Array` will be included. + +````ruby +format :json + +params do + optional :user, type: Hash do + optional :first_name, type: String + optional :last_name, type: String + end + optional :widgets, type: Array +end + +post 'users/signup' do + { 'declared_params' => declared(params) } +end +```` + +**Request** + +````bash +curl -X POST -H "Content-Type: application/json" localhost:9292/users/signup -d '{}' +```` + +**Response** + +````json +{ + "declared_params": { + "user": { + "first_name": null, + "last_name": null + }, + "widgets": [] + } +} +```` + The returned hash is an `ActiveSupport::HashWithIndifferentAccess`. The `#declared` method is not available to `before` filters, as those are evaluated prior to parameter coercion. diff --git a/UPGRADING.md b/UPGRADING.md index 591fa51add..86f9bc7fa9 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,45 @@ Upgrading Grape =============== +### Upgrading to >= 1.4.1 + +Prior to 1.3.3, the `declared` helper would always return the complete params structure if `include_missing=true` was set. In 1.3.3 the behavior was changed, so a missing Hash with or without nested parameters would always resolve to `{}`. + +In 1.4.1 this behavior is reverted, so the whole params structure will always be available via `declared`, regardless of whether any params are passed. + +The following rules now apply to the `declared` helper when params are missing and `include_missing=true`: + +* Hash params with children will resolve to a Hash with keys for each declared child. +* Hash params with no children will resolve to `{}`. +* Set params will resolve to `Set.new`. +* Array params will resolve to `[]`. +* All other params will resolve to `nil`. + +#### Example + +```ruby +class Api < Grape::API + params do + optional :outer, type: Hash do + optional :inner, type: Hash do + optional :value, type: String + end + end + end + get 'example' do + declared(params, include_missing: true) + end +end +``` + +``` +get '/example' +# 1.4.0 = {} +# 1.4.1 = {outer: {inner: {value:null}}} +``` + +For more information see [#2103](https://github.com/ruby-grape/grape/pull/2103). + ### Upgrading to >= 1.4.0 #### Reworking stream and file and un-deprecating stream like-objects @@ -28,17 +67,17 @@ class API < Grape::API end ``` -Or use `stream` to stream other kinds of content. In the following example a streamer class +Or use `stream` to stream other kinds of content. In the following example a streamer class streams paginated data from a database. ```ruby -class MyObject +class MyObject attr_accessor :result def initialize(query) @result = query end - + def each yield '[' # Do paginated DB fetches and return each page formatted @@ -47,7 +86,7 @@ class MyObject yield process_records(records, first) first = false end - yield ']' + yield ']' end def process_records(records, first) diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index dc8f9f05cd..1eb0c3084e 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -58,7 +58,7 @@ def declared_hash(passed_params, options, declared_params, params_nested_path) 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(passed_children_params, params_nested_path_dup) do + 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 end @@ -70,57 +70,44 @@ def declared_hash(passed_params, options, declared_params, params_nested_path) next unless options[:include_missing] || passed_params.key?(declared_param) || (param_renaming && passed_params.key?(param_renaming)) - if param_renaming - memo[optioned_param_key(param_renaming, options)] = passed_params[param_renaming] - else - memo[optioned_param_key(declared_param, options)] = passed_params[declared_param] + memo_key = optioned_param_key(param_renaming || declared_param, options) + passed_param = passed_params[param_renaming || declared_param] + + params_nested_path_dup = params_nested_path.dup + params_nested_path_dup << declared_param.to_s + + memo[memo_key] = handle_passed_param(params_nested_path_dup) do + passed_param end end end end - def handle_passed_param(passed_children_params, params_nested_path, &_block) - if should_be_empty_hash?(passed_children_params, params_nested_path) + def handle_passed_param(params_nested_path, has_passed_children = false, &_block) + return yield if has_passed_children + + key = params_nested_path[0] + key += '[' + params_nested_path[1..-1].join('][') + ']' if params_nested_path.size > 1 + + route_options_params = options[:route_options][:params] || {} + type = route_options_params.dig(key, :type) + has_children = route_options_params.keys.any? { |k| k != key && k.start_with?(key) } + + if type == 'Hash' && !has_children {} - elsif should_be_empty_array?(passed_children_params, params_nested_path) + elsif type == 'Array' || type&.start_with?('[') [] + elsif type == 'Set' || type&.start_with?('# 1 - key - end - def optioned_declared_params(**options) declared_params = if options[:include_parent_namespaces] # Declared params including parent namespaces diff --git a/spec/grape/endpoint/declared_spec.rb b/spec/grape/endpoint/declared_spec.rb index 73d145a8eb..100e0707c3 100644 --- a/spec/grape/endpoint/declared_spec.rb +++ b/spec/grape/endpoint/declared_spec.rb @@ -28,10 +28,20 @@ def app optional :nested_arr, type: Array do optional :eighth end + optional :empty_arr, type: Array + optional :empty_typed_arr, type: Array[String] + optional :empty_hash, type: Hash + optional :empty_set, type: Set + optional :empty_typed_set, type: Set[String] end optional :arr, type: Array do optional :nineth end + optional :empty_arr, type: Array + optional :empty_typed_arr, type: Array[String] + optional :empty_hash, type: Hash + optional :empty_set, type: Set + optional :empty_typed_set, type: Set[String] end end @@ -103,7 +113,7 @@ def app end get '/declared?first=present' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body).keys.size).to eq(5) + expect(JSON.parse(last_response.body).keys.size).to eq(10) end it 'has a optional param with default value all the time' do @@ -122,7 +132,7 @@ def app get '/declared?first=present&nested[fourth]=1' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body)['nested'].keys.size).to eq 4 + expect(JSON.parse(last_response.body)['nested'].keys.size).to eq 9 end it 'builds nested params when given array' do @@ -145,45 +155,66 @@ def app expect(JSON.parse(last_response.body)['nested'].size).to eq 2 end - context 'sets nested objects when the param is missing' do - it 'to be a hash when include_missing is true' do - subject.get '/declared' do - declared(params, include_missing: true) - end + context 'when the param is missing and include_missing=false' do + before do + subject.get('/declared') { declared(params, include_missing: false) } + end + it 'sets nested objects to be nil' do get '/declared?first=present' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body)['nested']).to eq({}) + expect(JSON.parse(last_response.body)['nested']).to be_nil end + end - it 'to be an array when include_missing is true' do - subject.get '/declared' do - declared(params, include_missing: true) - end + context 'when the param is missing and include_missing=true' do + before do + subject.get('/declared') { declared(params, include_missing: true) } + end + it 'sets objects with type=Hash to be a hash' do get '/declared?first=present' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body)['arr']).to be_a(Array) - end - it 'to be an array when nested and include_missing is true' do - subject.get '/declared' do - declared(params, include_missing: true) - end + body = JSON.parse(last_response.body) + expect(body['empty_hash']).to eq({}) + expect(body['nested']).to be_a(Hash) + expect(body['nested']['empty_hash']).to eq({}) + expect(body['nested']['nested_two']).to be_a(Hash) + end - get '/declared?first=present&nested[fourth]=1' + it 'sets objects with type=Set to be a set' do + get '/declared?first=present' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body)['nested']['nested_arr']).to be_a(Array) + + body = JSON.parse(last_response.body) + expect(body['empty_set']).to eq('#') + expect(body['empty_typed_set']).to eq('#') + expect(body['nested']['empty_set']).to eq('#') + expect(body['nested']['empty_typed_set']).to eq('#') end - it 'to be nil when include_missing is false' do - subject.get '/declared' do - declared(params, include_missing: false) - end + it 'sets objects with type=Array to be an array' do + get '/declared?first=present' + expect(last_response.status).to eq(200) + + body = JSON.parse(last_response.body) + expect(body['empty_arr']).to eq([]) + expect(body['empty_typed_arr']).to eq([]) + expect(body['arr']).to eq([]) + expect(body['nested']['empty_arr']).to eq([]) + expect(body['nested']['empty_typed_arr']).to eq([]) + expect(body['nested']['nested_arr']).to eq([]) + end + it 'includes all declared children when type=Hash' do get '/declared?first=present' expect(last_response.status).to eq(200) - expect(JSON.parse(last_response.body)['nested']).to be_nil + + body = JSON.parse(last_response.body) + expect(body['nested'].keys).to eq(%w[fourth fifth nested_two nested_arr empty_arr empty_typed_arr empty_hash empty_set empty_typed_set]) + expect(body['nested']['nested_two'].keys).to eq(%w[sixth nested_three]) + expect(body['nested']['nested_two']['nested_three'].keys).to eq(%w[seventh]) end end