Refactor versioner middleware: simplify base class and improve consistency

CHANGELOG.md

@@ -11,6 +11,9 @@
 * [#2582](https://github.com/ruby-grape/grape/pull/2582): Fix leaky slash when normalizing - [@ericproulx](https://github.com/ericproulx).
 * [#2583](https://github.com/ruby-grape/grape/pull/2583): Optimize api parameter documentation and memory usage - [@ericproulx](https://github.com/ericproulx).
 * [#2589](https://github.com/ruby-grape/grape/pull/2589): Replace `send` by `__send__` in codebase - [@ericproulx](https://github.com/ericproulx).
+* [#2598](https://github.com/ruby-grape/grape/pull/2598): Refactor settings DSL to use explicit methods instead of dynamic generation - [@ericproulx](https://github.com/ericproulx).
+* [#2599](https://github.com/ruby-grape/grape/pull/2599): Simplify settings DSL get_or_set method and optimize logger implementation - [@ericproulx](https://github.com/ericproulx).
+* [#2600](https://github.com/ruby-grape/grape/pull/2600): Refactor versioner middleware: simplify base class and improve consistency - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

lib/grape/api/instance.rb

@@ -232,22 +232,19 @@ def collect_route_config_per_pattern(all_routes)
         end
       end
 
+      ROOT_PREFIX_VERSIONING_KEY = %i[version version_options root_prefix].freeze
+      private_constant :ROOT_PREFIX_VERSIONING_KEY
+
       # Allows definition of endpoints that ignore the versioning configuration
       # used by the rest of your API.
       def without_root_prefix_and_versioning
-        old_version = self.class.namespace_inheritable(:version)
-        old_version_options = self.class.namespace_inheritable(:version_options)
-        old_root_prefix = self.class.namespace_inheritable(:root_prefix)
-
-        self.class.namespace_inheritable_to_nil(:version)
-        self.class.namespace_inheritable_to_nil(:version_options)
-        self.class.namespace_inheritable_to_nil(:root_prefix)
-
+        inheritable_setting = self.class.inheritable_setting
+        deleted_values = inheritable_setting.namespace_inheritable.delete(*ROOT_PREFIX_VERSIONING_KEY)
         yield
-
-        self.class.namespace_inheritable(:version, old_version)
-        self.class.namespace_inheritable(:version_options, old_version_options)
-        self.class.namespace_inheritable(:root_prefix, old_root_prefix)
+      ensure
+        ROOT_PREFIX_VERSIONING_KEY.each_with_index do |key, index|
+          inheritable_setting.namespace_inheritable[key] = deleted_values[index]
+        end
       end
     end
   end

lib/grape/dsl/logger.rb

@@ -7,10 +7,11 @@ module Logger
       # method will create a new one, logging to stdout.
       # @param logger [Object] the new logger to use
       def logger(logger = nil)
+        global_settings = inheritable_setting.global
         if logger
-          global_setting(:logger, logger)
+          global_settings[:logger] = logger
         else
-          global_setting(:logger) || global_setting(:logger, ::Logger.new($stdout))
+          global_settings[:logger] || global_settings[:logger] = ::Logger.new($stdout)
         end
       end
     end

lib/grape/dsl/settings.rb

@@ -26,64 +26,32 @@ def inheritable_setting
         @inheritable_setting ||= Grape::Util::InheritableSetting.new.tap { |new_settings| new_settings.inherit_from top_level_setting }
       end
 
-      # @param type [Symbol]
-      # @param key [Symbol]
-      def unset(type, key)
-        setting = inheritable_setting.__send__(type)
-        setting.delete key
+      def namespace_inheritable(key, value = nil)
+        get_or_set(inheritable_setting.namespace_inheritable, key, value)
       end
 
-      # @param type [Symbol]
-      # @param key [Symbol]
-      # @param value [Object] will be stored if the value is currently empty
-      # @return either the old value, if it wasn't nil, or the given value
-      def get_or_set(type, key, value)
-        setting = inheritable_setting.__send__(type)
-        if value.nil?
-          setting[key]
-        else
-          setting[key] = value
-        end
+      def namespace_stackable(key, value = nil)
+        get_or_set(inheritable_setting.namespace_stackable, key, value)
       end
 
-      # defines the following methods:
-      # - namespace_inheritable
-      # - namespace_stackable
-
-      %i[namespace_inheritable namespace_stackable].each do |method_name|
-        define_method method_name do |key, value = nil|
-          get_or_set method_name, key, value
-        end
+      def global_setting(key, value = nil)
+        get_or_set(inheritable_setting.global, key, value)
       end
 
-      def unset_namespace_stackable(*keys)
-        keys.each do |key|
-          unset :namespace_stackable, key
-        end
+      def route_setting(key, value = nil)
+        get_or_set(inheritable_setting.route, key, value)
       end
 
-      # defines the following methods:
-      # - global_setting
-      # - route_setting
-      # - namespace_setting
-
-      %i[global route namespace].each do |method_name|
-        define_method :"#{method_name}_setting" do |key, value = nil|
-          get_or_set method_name, key, value
-        end
-      end
-
-      # @param key [Symbol]
-      def namespace_inheritable_to_nil(key)
-        inheritable_setting.namespace_inheritable[key] = nil
+      def namespace_setting(key, value = nil)
+        get_or_set(inheritable_setting.namespace, key, value)
       end
 
       def namespace_reverse_stackable(key, value = nil)
-        get_or_set :namespace_reverse_stackable, key, value
+        get_or_set(inheritable_setting.namespace_reverse_stackable, key, value)
       end
 
       def namespace_stackable_with_hash(key)
-        settings = get_or_set :namespace_stackable, key, nil
+        settings = namespace_stackable(key)
         return if settings.blank?
 
         settings.each_with_object({}) { |value, result| result.deep_merge!(value) }
@@ -107,6 +75,12 @@ def within_namespace
 
         result
       end
+
+      def get_or_set(setting, key, value)
+        return setting[key] if value.nil?
+
+        setting[key] = value
+      end
     end
   end
 end

lib/grape/dsl/validations.rb

@@ -22,7 +22,7 @@ module Validations
       #      # whatever
       #    end
       def reset_validations!
-        unset_namespace_stackable :declared_params, :params, :validations
+        inheritable_setting.namespace_stackable.delete(:declared_params, :params, :validations)
       end
 
       # Opens a root-level ParamsScope, defining parameter coercions and

lib/grape/middleware/versioner/accept_version_header.rb

@@ -19,7 +19,7 @@ module Versioner
       class AcceptVersionHeader < Base
         def before
           potential_version = env['HTTP_ACCEPT_VERSION'].try(:scrub)
-          not_acceptable!('Accept-Version header must be set.') if strict? && potential_version.blank?
+          not_acceptable!('Accept-Version header must be set.') if strict && potential_version.blank?
 
           return if potential_version.blank?
 

lib/grape/middleware/versioner/base.rb

@@ -6,67 +6,49 @@ module Versioner
       class Base < Grape::Middleware::Base
         DEFAULT_OPTIONS = {
           pattern: /.*/i,
+          prefix: nil,
+          mount_path: nil,
           version_options: {
             strict: false,
             cascade: true,
-            parameter: 'apiver'
+            parameter: 'apiver',
+            vendor: nil
           }.freeze
         }.freeze
 
-        def self.inherited(klass)
-          super
-          Versioner.register(klass)
-        end
-
-        def versions
-          options[:versions]
-        end
-
-        def prefix
-          options[:prefix]
-        end
-
-        def mount_path
-          options[:mount_path]
-        end
+        CASCADE_PASS_HEADER = { 'X-Cascade' => 'pass' }.freeze
 
-        def pattern
-          options[:pattern]
+        DEFAULT_OPTIONS.each_key do |key|
+          define_method key do
+            options[key]
+          end
         end
 
-        def version_options
-          options[:version_options]
+        DEFAULT_OPTIONS[:version_options].each_key do |key|
+          define_method key do
+            options[:version_options][key]
+          end
         end
 
-        def strict?
-          version_options[:strict]
-        end
-
-        # By default those errors contain an `X-Cascade` header set to `pass`, which allows nesting and stacking
-        # of routes (see Grape::Router) for more information). To prevent
-        # this behavior, and not add the `X-Cascade` header, one can set the `:cascade` option to `false`.
-        def cascade?
-          version_options[:cascade]
-        end
-
-        def parameter_key
-          version_options[:parameter]
+        def self.inherited(klass)
+          super
+          Versioner.register(klass)
         end
 
-        def vendor
-          version_options[:vendor]
-        end
+        attr_reader :error_headers, :versions
 
-        def error_headers
-          cascade? ? { 'X-Cascade' => 'pass' } : {}
+        def initialize(app, **options)
+          super
+          @error_headers = cascade ? CASCADE_PASS_HEADER : {}
+          @versions = options[:versions]&.map(&:to_s) # making sure versions are strings to ease potential match
         end
 
         def potential_version_match?(potential_version)
-          versions.blank? || versions.any? { |v| v.to_s == potential_version }
+          versions.blank? || versions.include?(potential_version)
         end
 
         def version_not_found!
-          throw :error, status: 404, message: '404 API Version Not Found', headers: { 'X-Cascade' => 'pass' }
+          throw :error, status: 404, message: '404 API Version Not Found', headers: CASCADE_PASS_HEADER
         end
       end
     end

lib/grape/middleware/versioner/header.rb

@@ -53,7 +53,7 @@ def accept_header
         end
 
         def strict_header_checks!
-          return unless strict?
+          return unless strict
 
           accept_header_check!
           version_and_vendor_check!

lib/grape/middleware/versioner/param.rb

@@ -20,11 +20,11 @@ module Versioner
       #   env['api.version'] => 'v1'
       class Param < Base
         def before
-          potential_version = query_params[parameter_key]
+          potential_version = query_params[parameter]
           return if potential_version.blank?
 
           version_not_found! unless potential_version_match?(potential_version)
-          env[Grape::Env::API_VERSION] = env[Rack::RACK_REQUEST_QUERY_HASH].delete(parameter_key)
+          env[Grape::Env::API_VERSION] = env[Rack::RACK_REQUEST_QUERY_HASH].delete(parameter)
         end
       end
     end

lib/grape/util/base_inheritable.rb

@@ -14,8 +14,11 @@ def initialize(inherited_values = nil)
         @new_values = {}
       end
 
-      def delete(key)
-        new_values.delete key
+      def delete(*keys)
+        keys.map do |key|
+          # since delete returns the deleted value, seems natural to `map` the result
+          new_values.delete key
+        end
       end
 
       def initialize_copy(other)

spec/grape/dsl/logger_spec.rb

@@ -4,17 +4,7 @@
   let(:dummy_logger) do
     Class.new do
       extend Grape::DSL::Logger
-      def self.global_setting(key, value = nil)
-        if value
-          global_setting_hash[key] = value
-        else
-          global_setting_hash[key]
-        end
-      end
-
-      def self.global_setting_hash
-        @global_setting_hash ||= {}
-      end
+      extend Grape::DSL::Settings
     end
   end