From c50b651efeb45776a76218f6a1a6dbd0d51fd310 Mon Sep 17 00:00:00 2001 From: "Daniel Doubrovkine (dB.) @dblockdotorg" Date: Thu, 9 Jan 2020 15:27:13 -0500 Subject: [PATCH] Revert "delete a reversible stackable values class (#1953)" This reverts commit 9f786adbffa25574916e5d5504dacb4f5e79c6bb. --- CHANGELOG.md | 1 - lib/grape.rb | 1 + lib/grape/dsl/request_response.rb | 2 +- lib/grape/dsl/settings.rb | 15 +- lib/grape/endpoint.rb | 20 ++- lib/grape/util/inheritable_setting.rb | 8 +- lib/grape/util/reverse_stackable_values.rb | 18 +++ lib/grape/util/stackable_values.rb | 1 - lib/grape/validations/attributes_iterator.rb | 3 +- lib/grape/validations/params_scope.rb | 10 +- lib/grape/validations/types/build_coercer.rb | 6 +- spec/grape/api/instance_spec.rb | 6 - spec/grape/dsl/request_response_spec.rb | 10 +- spec/grape/util/inheritable_setting_spec.rb | 23 +++ .../util/reverse_stackable_values_spec.rb | 133 ++++++++++++++++++ spec/grape/util/stackable_values_spec.rb | 2 +- 16 files changed, 219 insertions(+), 40 deletions(-) create mode 100644 lib/grape/util/reverse_stackable_values.rb create mode 100644 spec/grape/util/reverse_stackable_values_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 47151226b1..e3527613cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,6 @@ #### Features * Your contribution here. -* [#1953](https://github.com/ruby-grape/grape/pull/1953): Delete a reversible stackable values class - [@dnesteryuk](https://github.com/dnesteryuk). * [#1949](https://github.com/ruby-grape/grape/pull/1949): Add support for Ruby 2.7 - [@nbulaj](https://github.com/nbulaj). * [#1948](https://github.com/ruby-grape/grape/pull/1948): Relax `dry-types` dependency version - [@nbulaj](https://github.com/nbulaj). * [#1944](https://github.com/ruby-grape/grape/pull/1944): Reduces `attribute_translator` string allocations - [@ericproulx](https://github.com/ericproulx). diff --git a/lib/grape.rb b/lib/grape.rb index 8d34594819..388fb54bdd 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -140,6 +140,7 @@ module Util eager_autoload do autoload :InheritableValues autoload :StackableValues + autoload :ReverseStackableValues autoload :InheritableSetting autoload :StrictHashConfiguration autoload :Registrable diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index cab80eea96..cbb1db9123 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -127,7 +127,7 @@ def rescue_from(*args, &block) :base_only_rescue_handlers end - namespace_stackable handler_type, Hash[args.map { |arg| [arg, handler] }] + namespace_reverse_stackable handler_type, Hash[args.map { |arg| [arg, handler] }] end namespace_stackable(:rescue_options, options) diff --git a/lib/grape/dsl/settings.rb b/lib/grape/dsl/settings.rb index 086b238d7c..eebd12750d 100644 --- a/lib/grape/dsl/settings.rb +++ b/lib/grape/dsl/settings.rb @@ -96,6 +96,10 @@ def namespace_stackable(key, value = nil) get_or_set :namespace_stackable, key, value end + def namespace_reverse_stackable(key, value = nil) + get_or_set :namespace_reverse_stackable, key, value + end + def namespace_stackable_with_hash(key) settings = get_or_set :namespace_stackable, key, nil return if settings.blank? @@ -103,11 +107,10 @@ def namespace_stackable_with_hash(key) end def namespace_reverse_stackable_with_hash(key) - settings = get_or_set :namespace_stackable, key, nil + settings = get_or_set :namespace_reverse_stackable, key, nil return if settings.blank? - result = {} - settings.reverse_each do |setting| + settings.each do |setting| setting.each do |field, value| result[field] ||= value end @@ -168,11 +171,7 @@ def within_namespace(&_block) # the superclass's :inheritable_setting. def build_top_level_setting Grape::Util::InheritableSetting.new.tap do |setting| - # Doesn't try to inherit settings from +Grape::API::Instance+ which also responds to - # +inheritable_setting+, however, it doesn't contain any user-defined settings. - # Otherwise, it would lead to an extra instance of +Grape::Util::InheritableSetting+ - # in the chain for every endpoint. - if defined?(superclass) && superclass.respond_to?(:inheritable_setting) && superclass != Grape::API::Instance + if defined?(superclass) && superclass.respond_to?(:inheritable_setting) && superclass != Grape::API setting.inherit_from superclass.inheritable_setting end end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 6b51a622f3..3fb4178749 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -386,8 +386,24 @@ def run_filters(filters, type = :other) extend post_extension if post_extension end - %i[befores before_validations after_validations afters finallies].each do |meth_id| - define_method(meth_id) { namespace_stackable(meth_id) } + def befores + namespace_stackable(:befores) || [] + end + + def before_validations + namespace_stackable(:before_validations) || [] + end + + def after_validations + namespace_stackable(:after_validations) || [] + end + + def afters + namespace_stackable(:afters) || [] + end + + def finallies + namespace_stackable(:finallies) || [] end def validations diff --git a/lib/grape/util/inheritable_setting.rb b/lib/grape/util/inheritable_setting.rb index befc6bd685..2a6024bca1 100644 --- a/lib/grape/util/inheritable_setting.rb +++ b/lib/grape/util/inheritable_setting.rb @@ -6,7 +6,7 @@ module Util # and inheritable values (see InheritableValues and StackableValues). class InheritableSetting attr_accessor :route, :api_class, :namespace - attr_accessor :namespace_inheritable, :namespace_stackable + attr_accessor :namespace_inheritable, :namespace_stackable, :namespace_reverse_stackable attr_accessor :parent, :point_in_time_copies # Retrieve global settings. @@ -31,6 +31,7 @@ def initialize # used with a mount, or should every API::Class be a separate namespace by default? self.namespace_inheritable = InheritableValues.new self.namespace_stackable = StackableValues.new + self.namespace_reverse_stackable = ReverseStackableValues.new self.point_in_time_copies = [] @@ -53,6 +54,7 @@ def inherit_from(parent) namespace_inheritable.inherited_values = parent.namespace_inheritable namespace_stackable.inherited_values = parent.namespace_stackable + namespace_reverse_stackable.inherited_values = parent.namespace_reverse_stackable self.route = parent.route.merge(route) point_in_time_copies.map { |cloned_one| cloned_one.inherit_from parent } @@ -70,6 +72,7 @@ def point_in_time_copy new_setting.namespace = namespace.clone new_setting.namespace_inheritable = namespace_inheritable.clone new_setting.namespace_stackable = namespace_stackable.clone + new_setting.namespace_reverse_stackable = namespace_reverse_stackable.clone new_setting.route = route.clone new_setting.api_class = api_class @@ -90,7 +93,8 @@ def to_hash route: route.clone, namespace: namespace.to_hash, namespace_inheritable: namespace_inheritable.to_hash, - namespace_stackable: namespace_stackable.to_hash + namespace_stackable: namespace_stackable.to_hash, + namespace_reverse_stackable: namespace_reverse_stackable.to_hash } end end diff --git a/lib/grape/util/reverse_stackable_values.rb b/lib/grape/util/reverse_stackable_values.rb new file mode 100644 index 0000000000..d8e8f160c7 --- /dev/null +++ b/lib/grape/util/reverse_stackable_values.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require_relative 'stackable_values' + +module Grape + module Util + class ReverseStackableValues < StackableValues + protected + + def concat_values(inherited_value, new_value) + [].tap do |value| + value.concat(new_value) + value.concat(inherited_value) + end + end + end + end +end diff --git a/lib/grape/util/stackable_values.rb b/lib/grape/util/stackable_values.rb index 79d3f37771..324aa19dbc 100644 --- a/lib/grape/util/stackable_values.rb +++ b/lib/grape/util/stackable_values.rb @@ -13,7 +13,6 @@ def initialize(*_args) @frozen_values = {} end - # Even if there is no value, an empty array will be returned. def [](name) return @frozen_values[name] if @frozen_values.key? name diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index bfa69cfc60..6c53d469a0 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -11,7 +11,6 @@ def initialize(validator, scope, params) @scope = scope @attrs = validator.attrs @original_params = scope.params(params) - @array_given = @original_params.is_a?(Array) @params = Array.wrap(@original_params) end @@ -31,7 +30,7 @@ def do_each(params_to_process, parent_indicies = [], &block) end if @scope.type == Array - next unless @array_given # do not validate content of array if it isn't array + next unless @original_params.is_a?(Array) # do not validate content of array if it isn't array # fill current and parent scopes with correct array indicies parent_scope = @scope.parent diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index c8b1527c0b..0cf8d2ef77 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -45,10 +45,8 @@ def configuration # @return [Boolean] whether or not this entire scope needs to be # validated def should_validate?(parameters) - scoped_params = params(parameters) - - return false if @optional && (scoped_params.blank? || all_element_blank?(scoped_params)) - return false unless meets_dependency?(scoped_params, parameters) + return false if @optional && (params(parameters).blank? || all_element_blank?(parameters)) + return false unless meets_dependency?(params(parameters), parameters) return true if parent.nil? parent.should_validate?(parameters) end @@ -448,8 +446,8 @@ def options_key?(type, key, validations) validations[type].respond_to?(:key?) && validations[type].key?(key) && !validations[type][key].nil? end - def all_element_blank?(scoped_params) - scoped_params.respond_to?(:all?) && scoped_params.all?(&:blank?) + def all_element_blank?(parameters) + params(parameters).respond_to?(:all?) && params(parameters).all?(&:blank?) end # Validators don't have access to each other and they don't need, however, diff --git a/lib/grape/validations/types/build_coercer.rb b/lib/grape/validations/types/build_coercer.rb index fc07ad4b4e..fc07621211 100644 --- a/lib/grape/validations/types/build_coercer.rb +++ b/lib/grape/validations/types/build_coercer.rb @@ -83,11 +83,7 @@ def self.cache_instance(type, method, strict, &_block) end def self.cache_key(type, method, strict) - [type, method, strict].each_with_object(+'') do |val, memo| - next if val.nil? - - memo << '_' << val.to_s - end + [type, method, strict].compact.map(&:to_s).join('_') end instance_variable_set(:@__cache, {}) diff --git a/spec/grape/api/instance_spec.rb b/spec/grape/api/instance_spec.rb index fa5c949e5b..6de77c51e1 100644 --- a/spec/grape/api/instance_spec.rb +++ b/spec/grape/api/instance_spec.rb @@ -45,10 +45,4 @@ def app expect(last_response.body).to eq 'success' end end - - context 'top level setting' do - it 'does not inherit settings from the superclass (Grape::API::Instance)' do - expect(an_instance.top_level_setting.parent).to be_nil - end - end end diff --git a/spec/grape/dsl/request_response_spec.rb b/spec/grape/dsl/request_response_spec.rb index 515e48cb0b..00d9e3a6a8 100644 --- a/spec/grape/dsl/request_response_spec.rb +++ b/spec/grape/dsl/request_response_spec.rb @@ -161,34 +161,34 @@ def self.imbue(key, value) describe 'list of exceptions is passed' do it 'sets hash of exceptions as rescue handlers' do - expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => nil) + expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => nil) expect(subject).to receive(:namespace_stackable).with(:rescue_options, {}) subject.rescue_from StandardError end it 'rescues only base handlers if rescue_subclasses: false option is passed' do - expect(subject).to receive(:namespace_stackable).with(:base_only_rescue_handlers, StandardError => nil) + expect(subject).to receive(:namespace_reverse_stackable).with(:base_only_rescue_handlers, StandardError => nil) expect(subject).to receive(:namespace_stackable).with(:rescue_options, rescue_subclasses: false) subject.rescue_from StandardError, rescue_subclasses: false end it 'sets given proc as rescue handler for each key in hash' do rescue_handler_proc = proc {} - expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc) + expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc) expect(subject).to receive(:namespace_stackable).with(:rescue_options, {}) subject.rescue_from StandardError, rescue_handler_proc end it 'sets given block as rescue handler for each key in hash' do rescue_handler_proc = proc {} - expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc) + expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc) expect(subject).to receive(:namespace_stackable).with(:rescue_options, {}) subject.rescue_from StandardError, &rescue_handler_proc end it 'sets a rescue handler declared through :with option for each key in hash' do with_block = -> { 'hello' } - expect(subject).to receive(:namespace_stackable).with(:rescue_handlers, StandardError => an_instance_of(Proc)) + expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => an_instance_of(Proc)) expect(subject).to receive(:namespace_stackable).with(:rescue_options, {}) subject.rescue_from StandardError, with: with_block end diff --git a/spec/grape/util/inheritable_setting_spec.rb b/spec/grape/util/inheritable_setting_spec.rb index 6b3b44ff0e..0e0b672e75 100644 --- a/spec/grape/util/inheritable_setting_spec.rb +++ b/spec/grape/util/inheritable_setting_spec.rb @@ -14,6 +14,7 @@ module Util settings.namespace[:namespace_thing] = :namespace_foo_bar settings.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar settings.namespace_stackable[:namespace_stackable_thing] = :namespace_stackable_foo_bar + settings.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :namespace_reverse_stackable_foo_bar settings.route[:route_thing] = :route_foo_bar end end @@ -23,6 +24,7 @@ module Util settings.namespace[:namespace_thing] = :namespace_foo_bar_other settings.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar_other settings.namespace_stackable[:namespace_stackable_thing] = :namespace_stackable_foo_bar_other + settings.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :namespace_reverse_stackable_foo_bar_other settings.route[:route_thing] = :route_foo_bar_other end end @@ -120,6 +122,16 @@ module Util end end + describe '#namespace_reverse_stackable' do + it 'works with reverse stackable values' do + expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar] + + subject.inherit_from other_parent + + expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar_other] + end + end + describe '#route' do it 'sets a value until the next route' do subject.route[:some_thing] = :foo_bar @@ -186,6 +198,14 @@ module Util expect(cloned_obj.namespace_stackable[:namespace_stackable_thing]).to eq [:namespace_stackable_foo_bar] end + it 'decouples namespace reverse stackable values' do + expect(cloned_obj.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar] + + subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :other_thing + expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq %i[other_thing namespace_reverse_stackable_foo_bar] + expect(cloned_obj.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar] + end + it 'decouples route values' do expect(cloned_obj.route[:route_thing]).to eq :route_foo_bar @@ -204,6 +224,7 @@ module Util subject.namespace[:namespace_thing] = :namespace_foo_bar subject.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar subject.namespace_stackable[:namespace_stackable_thing] = [:namespace_stackable_foo_bar] + subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = [:namespace_reverse_stackable_foo_bar] subject.route[:route_thing] = :route_foo_bar expect(subject.to_hash).to include(global: { global_thing: :global_foo_bar }) @@ -212,6 +233,8 @@ module Util namespace_inheritable_thing: :namespace_inheritable_foo_bar }) expect(subject.to_hash).to include(namespace_stackable: { namespace_stackable_thing: [:namespace_stackable_foo_bar, [:namespace_stackable_foo_bar]] }) + expect(subject.to_hash).to include(namespace_reverse_stackable: + { namespace_reverse_stackable_thing: [[:namespace_reverse_stackable_foo_bar], :namespace_reverse_stackable_foo_bar] }) expect(subject.to_hash).to include(route: { route_thing: :route_foo_bar }) end end diff --git a/spec/grape/util/reverse_stackable_values_spec.rb b/spec/grape/util/reverse_stackable_values_spec.rb new file mode 100644 index 0000000000..c5ffbd293a --- /dev/null +++ b/spec/grape/util/reverse_stackable_values_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +require 'spec_helper' +module Grape + module Util + describe ReverseStackableValues do + let(:parent) { described_class.new } + subject { described_class.new(parent) } + + describe '#keys' do + it 'returns all keys' do + subject[:some_thing] = :foo_bar + subject[:some_thing_else] = :foo_bar + expect(subject.keys).to eq %i[some_thing some_thing_else].sort + end + + it 'returns merged keys with parent' do + parent[:some_thing] = :foo + parent[:some_thing_else] = :foo + + subject[:some_thing] = :foo_bar + subject[:some_thing_more] = :foo_bar + + expect(subject.keys).to eq %i[some_thing some_thing_else some_thing_more].sort + end + end + + describe '#delete' do + it 'deletes a key' do + subject[:some_thing] = :new_foo_bar + subject.delete :some_thing + expect(subject[:some_thing]).to eq [] + end + + it 'does not delete parent values' do + parent[:some_thing] = :foo + subject[:some_thing] = :new_foo_bar + subject.delete :some_thing + expect(subject[:some_thing]).to eq [:foo] + end + end + + describe '#[]' do + it 'returns an array of values' do + subject[:some_thing] = :foo + expect(subject[:some_thing]).to eq [:foo] + end + + it 'returns parent value when no value is set' do + parent[:some_thing] = :foo + expect(subject[:some_thing]).to eq [:foo] + end + + it 'combines parent and actual values (actual first)' do + parent[:some_thing] = :foo + subject[:some_thing] = :foo_bar + expect(subject[:some_thing]).to eq %i[foo_bar foo] + end + + it 'parent values are not changed' do + parent[:some_thing] = :foo + subject[:some_thing] = :foo_bar + expect(parent[:some_thing]).to eq [:foo] + end + end + + describe '#[]=' do + it 'sets a value' do + subject[:some_thing] = :foo + expect(subject[:some_thing]).to eq [:foo] + end + + it 'pushes further values' do + subject[:some_thing] = :foo + subject[:some_thing] = :bar + expect(subject[:some_thing]).to eq %i[foo bar] + end + + it 'can handle array values' do + subject[:some_thing] = :foo + subject[:some_thing] = %i[bar more] + expect(subject[:some_thing]).to eq [:foo, %i[bar more]] + + parent[:some_thing_else] = %i[foo bar] + subject[:some_thing_else] = %i[some bar foo] + + expect(subject[:some_thing_else]).to eq [%i[some bar foo], %i[foo bar]] + end + end + + describe '#to_hash' do + it 'returns a Hash representation' do + parent[:some_thing] = :foo + subject[:some_thing] = %i[bar more] + subject[:some_thing_more] = :foo_bar + expect(subject.to_hash).to eq( + some_thing: [%i[bar more], :foo], + some_thing_more: [:foo_bar] + ) + end + end + + describe '#clone' do + let(:obj_cloned) { subject.clone } + it 'copies all values' do + parent = described_class.new + child = described_class.new parent + grandchild = described_class.new child + + parent[:some_thing] = :foo + child[:some_thing] = %i[bar more] + grandchild[:some_thing] = :grand_foo_bar + grandchild[:some_thing_more] = :foo_bar + + expect(grandchild.clone.to_hash).to eq( + some_thing: [:grand_foo_bar, %i[bar more], :foo], + some_thing_more: [:foo_bar] + ) + end + + context 'complex (i.e. not primitive) data types (ex. middleware, please see bug #930)' do + let(:middleware) { double } + + before { subject[:middleware] = middleware } + + it 'copies values; does not duplicate them' do + expect(obj_cloned[:middleware]).to eq [middleware] + end + end + end + end + end +end diff --git a/spec/grape/util/stackable_values_spec.rb b/spec/grape/util/stackable_values_spec.rb index c7defdf75a..07e6e4a982 100644 --- a/spec/grape/util/stackable_values_spec.rb +++ b/spec/grape/util/stackable_values_spec.rb @@ -8,7 +8,7 @@ module Util subject { StackableValues.new(parent) } describe '#keys' do - it 'returns all keys' do + it 'returns all key' do subject[:some_thing] = :foo_bar subject[:some_thing_else] = :foo_bar expect(subject.keys).to eq %i[some_thing some_thing_else].sort