From dd741b93b4841562866405654b7bce21a6b5ac21 Mon Sep 17 00:00:00 2001 From: Nicolas Klein Date: Thu, 22 Jun 2023 14:25:08 +0100 Subject: [PATCH 1/2] [ISSUE-2321] Updates documentation on re-mounted configuration for params (#2339) * [ISSUE-2321] Updates documentation on re-mounted configuration for params * Adds changelog * Extra space on changelog * More updates to changelog --- CHANGELOG.md | 1 + README.md | 6 +++--- spec/grape/api_remount_spec.rb | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23a01a2117..110e13f746 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ #### Fixes +* [#2339](https://github.com/ruby-grape/grape/pull/2339): Documentation and specs for remountable configuration in params - [@myxoh](https://github.com/myxoh). * [#2328](https://github.com/ruby-grape/grape/pull/2328): Don't cache Class.instance_methods - [@byroot](https://github.com/byroot). * [#2337](https://github.com/ruby-grape/grape/pull/2337): Fix: allow custom validators that do not end with _validator - [@ericproulx](https://github.com/ericproulx). * Your contribution here. diff --git a/README.md b/README.md index a334a72a88..b00d2c62b4 100644 --- a/README.md +++ b/README.md @@ -526,15 +526,15 @@ end ```ruby class BasicAPI < Grape::API desc 'Statuses index' do - params: mounted { configuration[:entity] || API::Entities::Status }.documentation + params: (configuration[:entity] || API::Entities::Status).documentation end params do - requires :all, using: mounted { configuration[:entity] || API::Entities::Status }.documentation + requires :all, using: (configuration[:entity] || API::Entities::Status).documentation end get '/statuses' do statuses = Status.all type = current_user.admin? ? :full : :default - present statuses, with: mounted { configuration[:entity] || API::Entities::Status }, type: type + present statuses, with: (configuration[:entity] || API::Entities::Status), type: type end end diff --git a/spec/grape/api_remount_spec.rb b/spec/grape/api_remount_spec.rb index 9fea1c3f04..40b5505157 100644 --- a/spec/grape/api_remount_spec.rb +++ b/spec/grape/api_remount_spec.rb @@ -147,6 +147,42 @@ def app end end + context 'when the params are configured via a configuration' do + subject(:a_remounted_api) do + Class.new(described_class) do + params do + requires configuration[:required_attr_name], type: String + end + + get(mounted { configuration[:endpoint] }) do + status 200 + end + end + end + + context 'when the configured param is my_attr' do + it 'requires the configured params' do + root_api.mount a_remounted_api, with: { + required_attr_name: 'my_attr', + endpoint: 'test' + } + get 'test?another_attr=1' + expect(last_response.status).to eq 400 + get 'test?my_attr=1' + expect(last_response.status).to eq 200 + + root_api.mount a_remounted_api, with: { + required_attr_name: 'another_attr', + endpoint: 'test_b' + } + get 'test_b?another_attr=1' + expect(last_response.status).to eq 200 + get 'test_b?my_attr=1' + expect(last_response.status).to eq 400 + end + end + end + context 'when executing a standard block within a `mounted` block with all dynamic params' do subject(:a_remounted_api) do Class.new(described_class) do From db7000b4ae1b09b5d31891fc91f1607d75419611 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Tue, 27 Jun 2023 01:48:40 +0200 Subject: [PATCH 2/2] Stop yielding skip value (#2341) * Not yielding when skipping Adjust specs * Remove begin in rescue blocks * CHANGELOG entry * Fix typo --- CHANGELOG.md | 1 + .../validations/multiple_attributes_iterator.rb | 2 +- .../validations/single_attribute_iterator.rb | 4 +++- lib/grape/validations/validators/base.rb | 11 ++++------- .../validators/multiple_params_base.rb | 12 ++++-------- .../multiple_attributes_iterator_spec.rb | 14 ++++++-------- .../single_attribute_iterator_spec.rb | 17 ++++++++--------- 7 files changed, 27 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 110e13f746..dc7d8f01ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * [#2331](https://github.com/ruby-grape/grape/pull/2331): Memory optimization when running validators - [@ericproulx](https://github.com/ericproulx). * [#2332](https://github.com/ruby-grape/grape/pull/2332): Use ActiveSupport configurable - [@ericproulx](https://github.com/ericproulx). * [#2333](https://github.com/ruby-grape/grape/pull/2333): Use custom messages in parameter validation with arity 1 - [@thedevjoao](https://github.com/TheDevJoao). +* [#2341](https://github.com/ruby-grape/grape/pull/2341): Stop yielding skip value - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/validations/multiple_attributes_iterator.rb b/lib/grape/validations/multiple_attributes_iterator.rb index d9ef7264b1..e5621bcaaf 100644 --- a/lib/grape/validations/multiple_attributes_iterator.rb +++ b/lib/grape/validations/multiple_attributes_iterator.rb @@ -6,7 +6,7 @@ class MultipleAttributesIterator < AttributesIterator private def yield_attributes(resource_params, _attrs) - yield resource_params, skip?(resource_params) + yield resource_params unless skip?(resource_params) end end end diff --git a/lib/grape/validations/single_attribute_iterator.rb b/lib/grape/validations/single_attribute_iterator.rb index 7fd3c3f479..218f4037b9 100644 --- a/lib/grape/validations/single_attribute_iterator.rb +++ b/lib/grape/validations/single_attribute_iterator.rb @@ -6,8 +6,10 @@ class SingleAttributeIterator < AttributesIterator private def yield_attributes(val, attrs) + return if skip?(val) + attrs.each do |attr_name| - yield val, attr_name, empty?(val), skip?(val) + yield val, attr_name, empty?(val) end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index cfa49155ef..ae0f0f48e5 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -46,16 +46,13 @@ def validate!(params) # there may be more than one error per field array_errors = [] - attributes.each do |val, attr_name, empty_val, skip_value| - next if skip_value + attributes.each do |val, attr_name, empty_val| next if !@scope.required? && empty_val next unless @scope.meets_dependency?(val, params) - begin - validate_param!(attr_name, val) if @required || (val.respond_to?(:key?) && val.key?(attr_name)) - rescue Grape::Exceptions::Validation => e - array_errors << e - end + validate_param!(attr_name, val) if @required || (val.respond_to?(:key?) && val.key?(attr_name)) + rescue Grape::Exceptions::Validation => e + array_errors << e end raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors.any? diff --git a/lib/grape/validations/validators/multiple_params_base.rb b/lib/grape/validations/validators/multiple_params_base.rb index c0b02ac50f..29df27720b 100644 --- a/lib/grape/validations/validators/multiple_params_base.rb +++ b/lib/grape/validations/validators/multiple_params_base.rb @@ -8,14 +8,10 @@ def validate!(params) attributes = MultipleAttributesIterator.new(self, @scope, params) array_errors = [] - attributes.each do |resource_params, skip_value| - next if skip_value - - begin - validate_params!(resource_params) - rescue Grape::Exceptions::Validation => e - array_errors << e - end + attributes.each do |resource_params| + validate_params!(resource_params) + rescue Grape::Exceptions::Validation => e + array_errors << e end raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors.any? diff --git a/spec/grape/validations/multiple_attributes_iterator_spec.rb b/spec/grape/validations/multiple_attributes_iterator_spec.rb index 5e63227850..14988b9811 100644 --- a/spec/grape/validations/multiple_attributes_iterator_spec.rb +++ b/spec/grape/validations/multiple_attributes_iterator_spec.rb @@ -12,8 +12,8 @@ { first: 'string', second: 'string' } end - it 'yields the whole params hash and the skipped flag without the list of attrs' do - expect { |b| iterator.each(&b) }.to yield_with_args(params, false) + it 'yields the whole params hash without the list of attrs' do + expect { |b| iterator.each(&b) }.to yield_with_args(params) end end @@ -23,17 +23,15 @@ end it 'yields each element of the array without the list of attrs' do - expect { |b| iterator.each(&b) }.to yield_successive_args([params[0], false], [params[1], false]) + expect { |b| iterator.each(&b) }.to yield_successive_args(params[0], params[1]) end end context 'when params is empty optional placeholder' do - let(:params) do - [Grape::DSL::Parameters::EmptyOptionalValue, { first: 'string2', second: 'string2' }] - end + let(:params) { [Grape::DSL::Parameters::EmptyOptionalValue] } - it 'yields each element of the array without the list of attrs' do - expect { |b| iterator.each(&b) }.to yield_successive_args([Grape::DSL::Parameters::EmptyOptionalValue, true], [params[1], false]) + it 'does not yield it' do + expect { |b| iterator.each(&b) }.to yield_successive_args end end end diff --git a/spec/grape/validations/single_attribute_iterator_spec.rb b/spec/grape/validations/single_attribute_iterator_spec.rb index 1962a8e319..2cde615e3a 100644 --- a/spec/grape/validations/single_attribute_iterator_spec.rb +++ b/spec/grape/validations/single_attribute_iterator_spec.rb @@ -14,7 +14,7 @@ it 'yields params and every single attribute from the list' do expect { |b| iterator.each(&b) } - .to yield_successive_args([params, :first, false, false], [params, :second, false, false]) + .to yield_successive_args([params, :first, false], [params, :second, false]) end end @@ -25,8 +25,8 @@ it 'yields every single attribute from the list for each of the array elements' do expect { |b| iterator.each(&b) }.to yield_successive_args( - [params[0], :first, false, false], [params[0], :second, false, false], - [params[1], :first, false, false], [params[1], :second, false, false] + [params[0], :first, false], [params[0], :second, false], + [params[1], :first, false], [params[1], :second, false] ) end @@ -35,9 +35,9 @@ it 'marks params with empty values' do expect { |b| iterator.each(&b) }.to yield_successive_args( - [params[0], :first, true, false], [params[0], :second, true, false], - [params[1], :first, true, false], [params[1], :second, true, false], - [params[2], :first, false, false], [params[2], :second, false, false] + [params[0], :first, true], [params[0], :second, true], + [params[1], :first, true], [params[1], :second, true], + [params[2], :first, false], [params[2], :second, false] ) end end @@ -45,10 +45,9 @@ context 'when missing optional value' do let(:params) { [Grape::DSL::Parameters::EmptyOptionalValue, 10] } - it 'marks params with skipped values' do + it 'does not yield skipped values' do expect { |b| iterator.each(&b) }.to yield_successive_args( - [params[0], :first, false, true], [params[0], :second, false, true], - [params[1], :first, false, false], [params[1], :second, false, false] + [params[1], :first, false], [params[1], :second, false] ) end end