delete a reversible stackable values class (#1953)

dnesteryuk · dblock · commit 9f786adbffa2 · 2020-01-05T23:22:03.000-05:00
* 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.
diff --git a/CHANGELOG.md b/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).
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -140,7 +140,6 @@ 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
@@ -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)
diff --git a/lib/grape/dsl/settings.rb b/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
diff --git a/lib/grape/endpoint.rb b/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
diff --git 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, :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
diff --git a/lib/grape/util/reverse_stackable_values.rb b/lib/grape/util/reverse_stackable_values.rb
diff --git a/lib/grape/util/stackable_values.rb b/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
 
diff --git a/lib/grape/validations/attributes_iterator.rb b/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
diff --git a/lib/grape/validations/params_scope.rb b/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,
diff --git a/lib/grape/validations/types/build_coercer.rb b/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,            {})
diff --git a/spec/grape/api/instance_spec.rb b/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
diff --git 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_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
diff --git a/spec/grape/util/inheritable_setting_spec.rb b/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
diff --git a/spec/grape/util/reverse_stackable_values_spec.rb b/spec/grape/util/reverse_stackable_values_spec.rb
diff --git a/spec/grape/util/stackable_values_spec.rb b/spec/grape/util/stackable_values_spec.rb
