delete a reversible stackable values class

CHANGELOG.md

@@ -3,6 +3,7 @@
 #### 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).

lib/grape.rb

@@ -140,7 +140,6 @@ module Util
     eager_autoload do
       autoload :InheritableValues
       autoload :StackableValues
-      autoload :ReverseStackableValues
       autoload :InheritableSetting
       autoload :StrictHashConfiguration
       autoload :Registrable

lib/grape/dsl/request_response.rb

@@ -127,7 +127,7 @@ def rescue_from(*args, &block)
                 :base_only_rescue_handlers
               end
 
-            namespace_reverse_stackable handler_type, Hash[args.map { |arg| [arg, handler] }]
+            namespace_stackable handler_type, Hash[args.map { |arg| [arg, handler] }]
           end
 
           namespace_stackable(:rescue_options, options)

lib/grape/dsl/settings.rb

@@ -96,21 +96,18 @@ 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?
         settings.each_with_object({}) { |value, result| result.deep_merge!(value) }
       end
 
       def namespace_reverse_stackable_with_hash(key)
-        settings = get_or_set :namespace_reverse_stackable, key, nil
+        settings = get_or_set :namespace_stackable, key, nil
         return if settings.blank?
+
         result = {}
-        settings.each do |setting|
+        settings.reverse_each do |setting|
           setting.each do |field, value|
             result[field] ||= value
           end
@@ -171,7 +168,11 @@ def within_namespace(&_block)
       # the superclass's :inheritable_setting.
       def build_top_level_setting
         Grape::Util::InheritableSetting.new.tap do |setting|
-          if defined?(superclass) && superclass.respond_to?(:inheritable_setting) && superclass != Grape::API
+          # 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
             setting.inherit_from superclass.inheritable_setting
           end
         end

lib/grape/endpoint.rb

@@ -386,24 +386,8 @@ def run_filters(filters, type = :other)
       extend post_extension if post_extension
     end
 
-    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) || []
+    %i[befores before_validations after_validations afters finallies].each do |meth_id|
+      define_method(meth_id) { namespace_stackable(meth_id) }
     end
 
     def validations

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, :namespace_reverse_stackable
+      attr_accessor :namespace_inheritable, :namespace_stackable
       attr_accessor :parent, :point_in_time_copies
 
       # Retrieve global settings.
@@ -31,7 +31,6 @@ 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 = []
 
@@ -54,7 +53,6 @@ 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 }
@@ -72,7 +70,6 @@ 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
 
@@ -93,8 +90,7 @@ def to_hash
           route: route.clone,
           namespace: namespace.to_hash,
           namespace_inheritable: namespace_inheritable.to_hash,
-          namespace_stackable: namespace_stackable.to_hash,
-          namespace_reverse_stackable: namespace_reverse_stackable.to_hash
+          namespace_stackable: namespace_stackable.to_hash
         }
       end
     end

lib/grape/util/stackable_values.rb

@@ -13,6 +13,7 @@ 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
 

lib/grape/validations/attributes_iterator.rb

@@ -11,6 +11,7 @@ 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
 
@@ -30,7 +31,7 @@ def do_each(params_to_process, parent_indicies = [], &block)
           end
 
           if @scope.type == Array
-            next unless @original_params.is_a?(Array) # do not validate content of array if it isn't array
+            next unless @array_given # do not validate content of array if it isn't array
 
             # fill current and parent scopes with correct array indicies
             parent_scope = @scope.parent

lib/grape/validations/params_scope.rb

@@ -45,8 +45,10 @@ def configuration
       # @return [Boolean] whether or not this entire scope needs to be
       #   validated
       def should_validate?(parameters)
-        return false if @optional && (params(parameters).blank? || all_element_blank?(parameters))
-        return false unless meets_dependency?(params(parameters), 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 true if parent.nil?
         parent.should_validate?(parameters)
       end
@@ -446,8 +448,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?(parameters)
-        params(parameters).respond_to?(:all?) && params(parameters).all?(&:blank?)
+      def all_element_blank?(scoped_params)
+        scoped_params.respond_to?(:all?) && scoped_params.all?(&:blank?)
       end
 
       # Validators don't have access to each other and they don't need, however,

lib/grape/validations/types/build_coercer.rb

@@ -83,7 +83,11 @@ def self.cache_instance(type, method, strict, &_block)
       end
 
       def self.cache_key(type, method, strict)
-        [type, method, strict].compact.map(&:to_s).join('_')
+        [type, method, strict].each_with_object(+'') do |val, memo|
+          next if val.nil?
+
+          memo << '_' << val.to_s
+        end
       end
 
       instance_variable_set(:@__cache,            {})

spec/grape/api/instance_spec.rb

@@ -45,4 +45,10 @@ 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

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_reverse_stackable).with(:rescue_handlers, StandardError => nil)
+            expect(subject).to receive(:namespace_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_reverse_stackable).with(:base_only_rescue_handlers, StandardError => nil)
+            expect(subject).to receive(:namespace_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_reverse_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc)
+            expect(subject).to receive(:namespace_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_reverse_stackable).with(:rescue_handlers, StandardError => rescue_handler_proc)
+            expect(subject).to receive(:namespace_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_reverse_stackable).with(:rescue_handlers, StandardError => an_instance_of(Proc))
+            expect(subject).to receive(:namespace_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

spec/grape/util/inheritable_setting_spec.rb

@@ -14,7 +14,6 @@ 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
@@ -24,7 +23,6 @@ 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
@@ -122,16 +120,6 @@ 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
@@ -198,14 +186,6 @@ 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
 
@@ -224,7 +204,6 @@ 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 })
@@ -233,8 +212,6 @@ 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