Refactor of request params parsing process (#947)

* wip

* Form params parser. First step

* Cleanup mess a bit

* Fix specs

* Let rubocop be happy

* Fixes after rubocop autocorrection

* Cleanup a little

* Fix spec

* Add requst param parser regestry

* Refactor param parsers

* Fix rubocop offenses

Author: numbata

lib/grape-swagger.rb

@@ -10,22 +10,27 @@
 
 require 'grape-swagger/doc_methods'
 require 'grape-swagger/model_parsers'
+require 'grape-swagger/request_param_parser_registry'
 
 module GrapeSwagger
   class << self
     def model_parsers
       @model_parsers ||= GrapeSwagger::ModelParsers.new
     end
+
+    def request_param_parsers
+      @request_param_parsers ||= GrapeSwagger::RequestParamParserRegistry.new
+    end
   end
   autoload :Rake, 'grape-swagger/rake/oapi_tasks'
 
   # Copied from https://github.com/ruby-grape/grape/blob/v2.2.0/lib/grape/formatter.rb
   FORMATTER_DEFAULTS = {
+    xml: Grape::Formatter::Xml,
+    serializable_hash: Grape::Formatter::SerializableHash,
     json: Grape::Formatter::Json,
     jsonapi: Grape::Formatter::Json,
-    serializable_hash: Grape::Formatter::SerializableHash,
-    txt: Grape::Formatter::Txt,
-    xml: Grape::Formatter::Xml
+    txt: Grape::Formatter::Txt
   }.freeze
 
   # Copied from https://github.com/ruby-grape/grape/blob/v2.2.0/lib/grape/content_types.rb

lib/grape-swagger/doc_methods.rb

@@ -11,7 +11,6 @@
 require 'grape-swagger/doc_methods/tag_name_description'
 require 'grape-swagger/doc_methods/parse_params'
 require 'grape-swagger/doc_methods/move_params'
-require 'grape-swagger/doc_methods/headers'
 require 'grape-swagger/doc_methods/build_model_definition'
 require 'grape-swagger/doc_methods/version'
 

lib/grape-swagger/doc_methods/format_data.rb

@@ -7,7 +7,7 @@ class << self
         def to_format(parameters)
           parameters.reject { |parameter| parameter[:in] == 'body' }.each do |b|
             related_parameters = parameters.select do |p|
-              p[:name] != b[:name] && p[:name].to_s.start_with?("#{b[:name].to_s.gsub(/\[\]\z/, '')}[")
+              p[:name] != b[:name] && p[:name].start_with?("#{b[:name].to_s.gsub(/\[\]\z/, '')}[")
             end
             parameters.reject! { |p| p[:name] == b[:name] } if move_down(b, related_parameters)
           end

lib/grape-swagger/doc_methods/headers.rb

@@ -163,7 +163,7 @@ def object_type
         end
 
         def prepare_nested_names(property, params)
-          params.each { |x| x[:name] = x[:name].sub(property, '').sub('[', '').sub(']', '') }
+          params.each { |x| x[:name] = x[:name].sub(property.to_s, '').sub('[', '').sub(']', '') }
         end
 
         def unify!(params)

lib/grape-swagger/doc_methods/move_params.rb

@@ -2,7 +2,9 @@
 
 require 'active_support'
 require 'active_support/core_ext/string/inflections'
-require 'grape-swagger/endpoint/params_parser'
+require_relative 'request_param_parsers/headers'
+require_relative 'request_param_parsers/route'
+require_relative 'request_param_parsers/body'
 
 module Grape
   class Endpoint # rubocop:disable Metrics/ClassLength
@@ -12,9 +14,7 @@ def content_types_for(target_class)
       if content_types.empty?
         formats       = [target_class.format, target_class.default_format].compact.uniq
         formats       = GrapeSwagger::FORMATTER_DEFAULTS.keys if formats.empty?
-        content_types = GrapeSwagger::CONTENT_TYPE_DEFAULTS.select do |content_type, _mime_type| # rubocop:disable Style/HashSlice
-          formats.include? content_type
-        end.values
+        content_types = formats.filter_map { |f| GrapeSwagger::CONTENT_TYPE_DEFAULTS[f] }
       end
 
       content_types.uniq
@@ -383,45 +383,15 @@ def build_file_response(memo)
     end
 
     def build_request_params(route, settings)
-      required = merge_params(route)
-      required = GrapeSwagger::DocMethods::Headers.parse(route) + required unless route.headers.nil?
-
-      default_type(required)
-
-      request_params = GrapeSwagger::Endpoint::ParamsParser.parse_request_params(required, settings, self)
-
-      request_params.empty? ? required : request_params
-    end
-
-    def merge_params(route)
-      path_params = get_path_params(route.app&.inheritable_setting&.namespace_stackable)
-      param_keys = route.params.keys
-
-      # Merge path params options into route params
-      route_params = route.params
-      route_params.each_key do |key|
-        path = path_params[key] || {}
-        params = route_params[key]
-        params = {} unless params.is_a? Hash
-        route_params[key] = path.merge(params)
-      end
-
-      route_params.delete_if { |key| key.is_a?(String) && param_keys.include?(key.to_sym) }.to_a
-    end
-
-    # Iterates over namespaces recursively
-    # to build a hash of path params with options, including type
-    def get_path_params(stackable_values)
-      params = {}
-      return param unless stackable_values
-      return params unless stackable_values.is_a? Grape::Util::StackableValues
-
-      stackable_values&.new_values&.dig(:namespace)&.each do |namespace| # rubocop:disable Style/SafeNavigationChainLength
-        space = namespace.space.to_s.gsub(':', '')
-        params[space] = namespace.options || {}
+      GrapeSwagger.request_param_parsers.each_with_object({}) do |parser_klass, accum|
+        params = parser_klass.parse(
+          route,
+          accum,
+          settings,
+          self
+        )
+        accum.merge!(params.stringify_keys)
       end
-      inherited_params = get_path_params(stackable_values.inherited_values)
-      inherited_params.merge(params)
     end
 
     def default_type(params)

lib/grape-swagger/endpoint.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+require_relative 'request_param_parsers/headers'
+require_relative 'request_param_parsers/route'
+require_relative 'request_param_parsers/body'
+
+module GrapeSwagger
+  class RequestParamParserRegistry
+    DEFAULT_PARSERS = [
+      GrapeSwagger::RequestParamParsers::Headers,
+      GrapeSwagger::RequestParamParsers::Route,
+      GrapeSwagger::RequestParamParsers::Body
+    ].freeze
+
+    include Enumerable
+
+    def initialize
+      @parsers = DEFAULT_PARSERS.dup
+    end
+
+    def register(klass)
+      remove_parser(klass)
+      @parsers << klass
+    end
+
+    def insert_before(before_klass, klass)
+      remove_parser(klass)
+      insert_at = @parsers.index(before_klass) || @parsers.size
+      @parsers.insert(insert_at, klass)
+    end
+
+    def insert_after(after_klass, klass)
+      remove_parser(klass)
+      insert_at = @parsers.index(after_klass)
+      @parsers.insert(insert_at ? insert_at + 1 : @parsers.size, klass)
+    end
+
+    def each(&)
+      @parsers.each(&)
+    end
+
+    private
+
+    def remove_parser(klass)
+      @parsers.reject! { |k| k == klass }
+    end
+  end
+end

lib/grape-swagger/request_param_parser_registry.rb

@@ -1,27 +1,26 @@
 # frozen_string_literal: true
 
 module GrapeSwagger
-  module Endpoint
-    class ParamsParser
-      attr_reader :params, :settings, :endpoint
+  module RequestParamParsers
+    class Body
+      attr_reader :route, :params, :settings, :endpoint
 
-      def self.parse_request_params(params, settings, endpoint)
-        new(params, settings, endpoint).parse_request_params
+      def self.parse(route, params, settings, endpoint)
+        new(route, params, settings, endpoint).parse
       end
 
-      def initialize(params, settings, endpoint)
+      def initialize(_route, params, settings, endpoint)
         @params = params
         @settings = settings
         @endpoint = endpoint
       end
 
-      def parse_request_params
+      def parse
         public_params.each_with_object({}) do |(name, options), memo|
           name = name.to_s
-          param_type = options[:type]
-          param_type = param_type.to_s unless param_type.nil?
+          param_type = options[:type]&.to_s
 
-          if param_type_is_array?(param_type)
+          if array_param?(param_type)
             options[:is_array] = true
             name += '[]' if array_use_braces?
           end
@@ -36,8 +35,8 @@ def array_use_braces?
         @array_use_braces ||= settings[:array_use_braces] && !includes_body_param?
       end
 
-      def param_type_is_array?(param_type)
-        return false unless param_type
+      def array_param?(param_type)
+        return false if param_type.nil?
         return true if param_type == 'Array'
 
         param_types = param_type.match(/\[(.*)\]$/)
@@ -48,11 +47,10 @@ def param_type_is_array?(param_type)
       end
 
       def public_params
-        params.select { |param| public_parameter?(param) }
+        params.select { |_key, param| public_parameter?(param) }
       end
 
-      def public_parameter?(param)
-        param_options = param.last
+      def public_parameter?(param_options)
         return true unless param_options.key?(:documentation) && !param_options[:required]
 
         param_hidden = param_options[:documentation].fetch(:hidden, false)

lib/grape-swagger/endpoint/params_parser.rb renamed to lib/grape-swagger/request_param_parsers/body.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module GrapeSwagger
+  module RequestParamParsers
+    class Headers
+      attr_reader :route
+
+      def self.parse(route, params, settings, endpoint)
+        new(route, params, settings, endpoint).parse
+      end
+
+      def initialize(route, _params, _settings, _endpoint)
+        @route = route
+      end
+
+      def parse
+        return {} unless route.headers
+
+        route.headers.each_with_object({}) do |(name, definition), accum|
+          # Extract the description from any key type (string or symbol)
+          description = definition[:description] || definition['description']
+          doc = { desc: description, in: 'header' }
+
+          header_attrs = definition.symbolize_keys.except(:description, 'description')
+          header_attrs[:type] = definition[:type].titleize if definition[:type]
+          header_attrs[:documentation] = doc
+
+          accum[name] = header_attrs
+        end
+      end
+    end
+  end
+end

lib/grape-swagger/request_param_parsers/headers.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+module GrapeSwagger
+  module RequestParamParsers
+    class Route
+      DEFAULT_PARAM_TYPE = { required: true, type: 'Integer' }.freeze
+
+      attr_reader :route
+
+      def self.parse(route, params, settings, endpoint)
+        new(route, params, settings, endpoint).parse
+      end
+
+      def initialize(route, _params, _settings, _endpoint)
+        @route = route
+      end
+
+      def parse
+        stackable_values = route.app&.inheritable_setting&.namespace_stackable
+
+        path_params = build_path_params(stackable_values)
+
+        fulfill_params(path_params)
+      end
+
+      private
+
+      def build_path_params(stackable_values)
+        params = {}
+
+        while stackable_values.is_a?(Grape::Util::StackableValues)
+          params.merge!(fetch_inherited_params(stackable_values))
+          stackable_values = stackable_values.inherited_values
+        end
+
+        params
+      end
+
+      def fetch_inherited_params(stackable_values)
+        return {} unless stackable_values.new_values
+
+        namespaces = stackable_values.new_values[:namespace] || []
+
+        namespaces.each_with_object({}) do |namespace, params|
+          space = namespace.space.to_s.gsub(':', '')
+          params[space] = namespace.options || {}
+        end
+      end
+
+      def fulfill_params(path_params)
+        # Merge path params options into route params
+        route.params.each_with_object({}) do |(param, definition), accum|
+          # The route.params hash includes both parametrized params (with a string as a key)
+          # and well-defined params from body/query (with a symbol as a key).
+          # We avoid overriding well-defined params with parametrized ones.
+          key = param.is_a?(String) ? param.to_sym : param
+          next if param.is_a?(String) && accum.key?(key)
+
+          defined_options = definition.is_a?(Hash) ? definition : {}
+          value = (path_params[param] || {}).merge(defined_options)
+          accum[key] = value.empty? ? DEFAULT_PARAM_TYPE : value
+        end
+      end
+    end
+  end
+end

lib/grape-swagger/request_param_parsers/route.rb

@@ -47,9 +47,9 @@
   end
 
   describe 'parse_request_params' do
-    let(:subject) { GrapeSwagger::Endpoint::ParamsParser }
+    let(:subject) { GrapeSwagger::RequestParamParsers::Body }
     before do
-      subject.send(:parse_request_params, params, {}, nil)
+      subject.parse(nil, params, {}, nil)
     end
 
     context 'when params do not contain an array' do