delete a reversible stackable values class (#1953)

* delete a reversible stackable values class

The reversible stackable values object was initialized for every endpoint,
however, it is only needed for keeping rescue handlers. The idea is
simple, handlers defined "closer" to an endpoint have higher priority.
That test https://github.com/ruby-grape/grape/blob/master/spec/grape/api_spec.rb#L3215-L3232
demonstrates how it works.

In our project rescue handlers are defined at the top level, so almost every
endpoint keeps the unused object.

The mentioned behavior is easy to achieve with the stackable values object and
the `reverse_each` method. Thus, endpoints keeps less objects and
have less code to be maintained.

Besides that, there are a few other simple performance optimizations.

Author: dnesteryuk

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/reverse_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/util/stackable_values.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/attributes_iterator.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,