Merge pull request #1961 from ruby-grape/revert-1953-chore/micto-optimization-3

dm1try · web-flow · commit 9feefdff9490 · 2020-01-10T00:07:59.000+03:00
Revert "delete a reversible stackable values class"
diff --git 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
@@ -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
@@ -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
@@ -96,18 +96,21 @@ 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_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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/api_spec.rb b/spec/grape/api_spec.rb
@@ -1942,6 +1942,26 @@ def foo
       expect { get '/unrescued' }.to raise_error(RuntimeError, 'beefcake')
     end
 
+    it 'mimics default ruby "rescue" handler' do
+      # The exception is matched to the rescue starting at the top, and matches only once
+
+      subject.rescue_from ArgumentError do |e|
+        error!(e, 402)
+      end
+      subject.rescue_from StandardError do |e|
+        error!(e, 401)
+      end
+
+      subject.get('/child_of_standard_error') { raise ArgumentError }
+      subject.get('/standard_error') { raise StandardError }
+
+      get '/child_of_standard_error'
+      expect(last_response.status).to eql 402
+
+      get '/standard_error'
+      expect(last_response.status).to eql 401
+    end
+
     context 'CustomError subclass of Grape::Exceptions::Base' do
       before do
         module ApiSpec
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_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
@@ -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
