diff --git a/.rubocop.yml b/.rubocop.yml index ab16e2e16..144f1cbca 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -49,6 +49,7 @@ Metrics/CyclomaticComplexity: Max: 15 Metrics/ParameterLists: + CountKeywordArgs: false MaxOptionalParameters: 4 Metrics/MethodLength: diff --git a/CHANGELOG.md b/CHANGELOG.md index cf51ed8f9..ce60c801f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,11 @@ #### Features +* [#2629](https://github.com/ruby-grape/grape/pull/2629): Refactor Router Architecture - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes - + * Your contribution here. ### 3.0.1 (2025-11-24) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 8fb764eb0..7fbd142a4 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -177,7 +177,7 @@ def cascade? def add_head_not_allowed_methods_and_options_methods # The paths we collected are prepared (cf. Path#prepare), so they # contain already versioning information when using path versioning. - all_routes = self.class.endpoints.map(&:routes).flatten + all_routes = self.class.endpoints.flat_map(&:routes) # Disable versioning so adding a route won't prepend versioning # informations again. @@ -186,23 +186,21 @@ def add_head_not_allowed_methods_and_options_methods def collect_route_config_per_pattern(all_routes) routes_by_regexp = all_routes.group_by(&:pattern_regexp) + namespace_inheritable = self.class.inheritable_setting.namespace_inheritable # Build the configuration based on the first endpoint and the collection of methods supported. routes_by_regexp.each_value do |routes| - last_route = routes.last # Most of the configuration is taken from the last endpoint next if routes.any? { |route| route.request_method == '*' } - namespace_inheritable = self.class.inheritable_setting.namespace_inheritable + last_route = routes.last # Most of the configuration is taken from the last endpoint allowed_methods = routes.map(&:request_method) allowed_methods |= [Rack::HEAD] if !namespace_inheritable[:do_not_route_head] && allowed_methods.include?(Rack::GET) allow_header = namespace_inheritable[:do_not_route_options] ? allowed_methods : [Rack::OPTIONS] | allowed_methods last_route.app.options[:options_route_enabled] = true unless namespace_inheritable[:do_not_route_options] || allowed_methods.include?(Rack::OPTIONS) - @router.associate_routes(last_route.pattern, { - endpoint: last_route.app, - allow_header: allow_header - }) + greedy_route = Grape::Router::GreedyRoute.new(last_route.pattern, endpoint: last_route.app, allow_header: allow_header) + @router.associate_routes(greedy_route) end end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index b877bcbfa..27837b54d 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -149,14 +149,14 @@ def route(methods, paths = ['/'], route_options = {}, &block) all_route_options.deep_merge!(endpoint_description) if endpoint_description all_route_options.deep_merge!(route_options) if route_options&.any? - endpoint_options = { + new_endpoint = Grape::Endpoint.new( + inheritable_setting, method: method, path: paths, for: self, - route_options: all_route_options - } - - new_endpoint = Grape::Endpoint.new(inheritable_setting, endpoint_options, &block) + route_options: all_route_options, + &block + ) endpoints << new_endpoint unless endpoints.any? { |e| e.equals?(new_endpoint) } inheritable_setting.route_end @@ -217,7 +217,7 @@ def reset_endpoints! # # @param param [Symbol] The name of the parameter you wish to declare. # @option options [Regexp] You may supply a regular expression that the declared parameter must meet. - def route_param(param, options = {}, &block) + def route_param(param, **options, &block) options = options.dup options[:requirements] = { diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 2bf1d1c21..19394f27b 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -46,10 +46,7 @@ def run_before_each(endpoint) # @note This happens at the time of API definition, so in this context the # endpoint does not know if it will be mounted under a different endpoint. # @yield a block defining what your API should do when this endpoint is hit - def initialize(new_settings, options = {}, &block) - require_option(options, :path) - require_option(options, :method) - + def initialize(new_settings, **options, &block) self.inheritable_setting = new_settings.point_in_time_copy # now +namespace_stackable(:declared_params)+ contains all params defined for @@ -67,7 +64,6 @@ def initialize(new_settings, options = {}, &block) @options[:path] << '/' if options[:path].empty? @options[:method] = Array(options[:method]) - @options[:route_options] ||= {} @lazy_initialize_lock = Mutex.new @lazy_initialized = nil @@ -88,10 +84,6 @@ def inherit_settings(namespace_stackable) endpoints&.each { |e| e.inherit_settings(namespace_stackable) } end - def require_option(options, key) - raise Grape::Exceptions::MissingOption.new(key) unless options.key?(key) - end - def routes @routes ||= endpoints&.collect(&:routes)&.flatten || to_routes end @@ -117,53 +109,6 @@ def mount_in(router) end end - def to_routes - default_route_options = prepare_default_route_attributes - - map_routes do |method, raw_path| - prepared_path = Path.new(raw_path, namespace, prepare_default_path_settings) - params = options[:route_options].present? ? options[:route_options].merge(default_route_options) : default_route_options - route = Grape::Router::Route.new(method, prepared_path.origin, prepared_path.suffix, params) - route.apply(self) - end.flatten - end - - def prepare_routes_requirements - {}.merge!(*inheritable_setting.namespace_stackable[:namespace].map(&:requirements)).tap do |requirements| - endpoint_requirements = options.dig(:route_options, :requirements) - requirements.merge!(endpoint_requirements) if endpoint_requirements - end - end - - def prepare_default_route_attributes - { - namespace: namespace, - version: prepare_version, - requirements: prepare_routes_requirements, - prefix: inheritable_setting.namespace_inheritable[:root_prefix], - anchor: options[:route_options].fetch(:anchor, true), - settings: inheritable_setting.route.except(:declared_params, :saved_validations), - forward_match: options[:forward_match] - } - end - - def prepare_version - version = inheritable_setting.namespace_inheritable[:version] - return if version.blank? - - version.length == 1 ? version.first : version - end - - def map_routes - options[:method].map { |method| options[:path].map { |path| yield method, path } } - end - - def prepare_default_path_settings - namespace_stackable_hash = inheritable_setting.namespace_stackable.to_hash - namespace_inheritable_hash = inheritable_setting.namespace_inheritable.to_hash - namespace_stackable_hash.merge!(namespace_inheritable_hash) - end - def namespace @namespace ||= Namespace.joined_space_path(inheritable_setting.namespace_stackable[:namespace]) end @@ -311,6 +256,59 @@ def options? private + def to_routes + route_options = options[:route_options] + default_route_options = prepare_default_route_attributes(route_options) + complete_route_options = route_options.merge(default_route_options) + path_settings = prepare_default_path_settings + + options[:method].flat_map do |method| + options[:path].map do |path| + prepared_path = Path.new(path, default_route_options[:namespace], path_settings) + pattern = Grape::Router::Pattern.new( + origin: prepared_path.origin, + suffix: prepared_path.suffix, + anchor: default_route_options[:anchor], + params: route_options[:params], + format: options[:format], + version: default_route_options[:version], + requirements: default_route_options[:requirements] + ) + Grape::Router::Route.new(self, method, pattern, complete_route_options) + end + end + end + + def prepare_default_route_attributes(route_options) + { + namespace: namespace, + version: prepare_version(inheritable_setting.namespace_inheritable[:version]), + requirements: prepare_routes_requirements(route_options[:requirements]), + prefix: inheritable_setting.namespace_inheritable[:root_prefix], + anchor: route_options.fetch(:anchor, true), + settings: inheritable_setting.route.except(:declared_params, :saved_validations), + forward_match: options[:forward_match] + } + end + + def prepare_default_path_settings + namespace_stackable_hash = inheritable_setting.namespace_stackable.to_hash + namespace_inheritable_hash = inheritable_setting.namespace_inheritable.to_hash + namespace_stackable_hash.merge!(namespace_inheritable_hash) + end + + def prepare_routes_requirements(route_options_requirements) + namespace_requirements = inheritable_setting.namespace_stackable[:namespace].filter_map(&:requirements) + namespace_requirements << route_options_requirements if route_options_requirements.present? + namespace_requirements.reduce({}, :merge) + end + + def prepare_version(namespace_inheritable_version) + return if namespace_inheritable_version.blank? + + namespace_inheritable_version.length == 1 ? namespace_inheritable_version.first : namespace_inheritable_version + end + def build_stack stack = Grape::Middleware::Stack.new diff --git a/lib/grape/router.rb b/lib/grape/router.rb index 53e61360c..0a81773b1 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -57,11 +57,9 @@ def append(route) map[route.request_method] << route end - def associate_routes(pattern, options) - Grape::Router::GreedyRoute.new(pattern, options).then do |greedy_route| - @neutral_regexes << greedy_route.to_regexp(@neutral_map.length) - @neutral_map << greedy_route - end + def associate_routes(greedy_route) + @neutral_regexes << greedy_route.to_regexp(@neutral_map.length) + @neutral_map << greedy_route end def call(env) @@ -91,7 +89,7 @@ def identity(env) def rotation(env, exact_route = nil) response = nil - input, method = *extract_input_and_method(env) + input, method = extract_input_and_method(env) map[method].each do |route| next if exact_route == route next unless route.match?(input) @@ -103,7 +101,7 @@ def rotation(env, exact_route = nil) end def transaction(env) - input, method = *extract_input_and_method(env) + input, method = extract_input_and_method(env) # using a Proc is important since `return` will exit the enclosing function cascade_or_return_response = proc do |response| @@ -126,7 +124,7 @@ def transaction(env) route = match?(input, '*') - return last_neighbor_route.options[:endpoint].call(env) if last_neighbor_route && last_response_cascade && route + return last_neighbor_route.call(env) if last_neighbor_route && last_response_cascade && route last_response_cascade = cascade_or_return_response.call(process_route(route, env)) if route @@ -142,7 +140,8 @@ def process_route(route, env) def make_routing_args(default_args, route, input) args = default_args || { route_info: route } - args.merge(route.params(input)) + route_params = route.params(input) + route_params ? args.merge(route_params) : args end def extract_input_and_method(env) @@ -171,12 +170,12 @@ def greedy_match?(input) def call_with_allow_headers(env, route) prepare_env_from_route(env, route) - env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.options[:allow_header] - route.options[:endpoint].call(env) + env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.allow_header + route.call(env) end def prepare_env_from_route(env, route) - input, = *extract_input_and_method(env) + input, = extract_input_and_method(env) env[Grape::Env::GRAPE_ROUTING_ARGS] = make_routing_args(env[Grape::Env::GRAPE_ROUTING_ARGS], route, input) end diff --git a/lib/grape/router/base_route.rb b/lib/grape/router/base_route.rb index 86439e908..b6f8980ef 100644 --- a/lib/grape/router/base_route.rb +++ b/lib/grape/router/base_route.rb @@ -3,22 +3,31 @@ module Grape class Router class BaseRoute + extend Forwardable + delegate_missing_to :@options - attr_reader :index, :pattern, :options + attr_reader :index, :options, :pattern + + def_delegators :@pattern, :path, :origin + def_delegators :@options, :description, :version, :requirements, :prefix, :anchor, :settings, :forward_match, *Grape::Util::ApiDescription::DSL_METHODS - def initialize(options) + def initialize(pattern, options = {}) + @pattern = pattern @options = options.is_a?(ActiveSupport::OrderedOptions) ? options : ActiveSupport::OrderedOptions.new.update(options) end - alias attributes options + # see https://github.com/ruby-grape/grape/issues/1348 + def namespace + @options[:namespace] + end def regexp_capture_index CaptureIndexCache[index] end def pattern_regexp - pattern.to_regexp + @pattern.to_regexp end def to_regexp(index) diff --git a/lib/grape/router/greedy_route.rb b/lib/grape/router/greedy_route.rb index c2fbcf8e8..f1d424bb2 100644 --- a/lib/grape/router/greedy_route.rb +++ b/lib/grape/router/greedy_route.rb @@ -6,14 +6,20 @@ module Grape class Router class GreedyRoute < BaseRoute - def initialize(pattern, options) - @pattern = pattern - super(options) + extend Forwardable + + def_delegators :@endpoint, :call + + attr_reader :endpoint, :allow_header + + def initialize(pattern, endpoint:, allow_header:) + super(pattern) + @endpoint = endpoint + @allow_header = allow_header end - # Grape::Router:Route defines params as a function def params(_input = nil) - options[:params] || {} + nil end end end diff --git a/lib/grape/router/pattern.rb b/lib/grape/router/pattern.rb index 4529a9271..0bcfa9059 100644 --- a/lib/grape/router/pattern.rb +++ b/lib/grape/router/pattern.rb @@ -13,10 +13,10 @@ class Pattern def_delegators :to_regexp, :=== alias match? === - def initialize(origin, suffix, options) + def initialize(origin:, suffix:, anchor:, params:, format:, version:, requirements:) @origin = origin - @path = build_path(origin, options[:anchor], suffix) - @pattern = build_pattern(@path, options[:params], options[:format], options[:version], options[:requirements]) + @path = PatternCache[[build_path_from_pattern(@origin, anchor), suffix]] + @pattern = Mustermann::Grape.new(@path, uri_decode: true, params: params, capture: extract_capture(format, version, requirements)) @to_regexp = @pattern.to_regexp end @@ -28,24 +28,10 @@ def captures_default private - def build_pattern(path, params, format, version, requirements) - Mustermann::Grape.new( - path, - uri_decode: true, - params: params, - capture: extract_capture(format, version, requirements) - ) - end - - def build_path(pattern, anchor, suffix) - PatternCache[[build_path_from_pattern(pattern, anchor), suffix]] - end - def extract_capture(format, version, requirements) - capture = {}.tap do |h| - h[:format] = map_str(format) if format.present? - h[:version] = map_str(version) if version.present? - end + capture = {} + capture[:format] = map_str(format) if format.present? + capture[:version] = map_str(version) if version.present? return capture if requirements.blank? diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index 64105be9c..9e1a5c2a2 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -3,20 +3,16 @@ module Grape class Router class Route < BaseRoute - extend Forwardable - FORWARD_MATCH_METHOD = ->(input, pattern) { input.start_with?(pattern.origin) } NON_FORWARD_MATCH_METHOD = ->(input, pattern) { pattern.match?(input) } - attr_reader :app, :request_method - - def_delegators :pattern, :path, :origin + attr_reader :app, :request_method, :index - def initialize(method, origin, path, options) + def initialize(endpoint, method, pattern, options) + super(pattern, options) + @app = endpoint @request_method = upcase_method(method) - @pattern = Grape::Router::Pattern.new(origin, path, options) @match_function = options[:forward_match] ? FORWARD_MATCH_METHOD : NON_FORWARD_MATCH_METHOD - super(options) end def convert_to_head_request! @@ -42,7 +38,7 @@ def params(input = nil) return params_without_input if input.blank? parsed = pattern.params(input) - return {} unless parsed + return unless parsed parsed.compact.symbolize_keys end @@ -50,7 +46,7 @@ def params(input = nil) private def params_without_input - @params_without_input ||= pattern.captures_default.merge(attributes.params) + @params_without_input ||= pattern.captures_default.merge(options[:params]) end def upcase_method(method) diff --git a/lib/grape/util/api_description.rb b/lib/grape/util/api_description.rb index 6246f67d1..0b007c4d0 100644 --- a/lib/grape/util/api_description.rb +++ b/lib/grape/util/api_description.rb @@ -3,13 +3,7 @@ module Grape module Util class ApiDescription - def initialize(description, endpoint_configuration, &block) - @endpoint_configuration = endpoint_configuration - @attributes = { description: description } - instance_eval(&block) - end - - %i[ + DSL_METHODS = %i[ body_name consumes default @@ -27,7 +21,15 @@ def initialize(description, endpoint_configuration, &block) security summary tags - ].each do |attribute| + ].freeze + + def initialize(description, endpoint_configuration, &block) + @endpoint_configuration = endpoint_configuration + @attributes = { description: description } + instance_eval(&block) + end + + DSL_METHODS.each do |attribute| define_method attribute do |value| @attributes[attribute] = value end diff --git a/spec/grape/dsl/routing_spec.rb b/spec/grape/dsl/routing_spec.rb index a24b55178..d751ad913 100644 --- a/spec/grape/dsl/routing_spec.rb +++ b/spec/grape/dsl/routing_spec.rb @@ -258,19 +258,19 @@ class << self it 'calls #namespace with given params' do expect(subject).to receive(:namespace).with(':foo', {}).and_yield - subject.route_param('foo', {}, &proc {}) + subject.route_param('foo', &proc {}) end it 'nests requirements option under param name' do expect(subject).to receive(:namespace) do |_param, options| expect(options[:requirements][:foo]).to eq regex end - subject.route_param('foo', options, &proc {}) + subject.route_param('foo', **options, &proc {}) end it 'does not modify options parameter' do allow(subject).to receive(:namespace) - expect { subject.route_param('foo', options, &proc {}) } + expect { subject.route_param('foo', **options, &proc {}) } .not_to(change { options }) end end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 3f4d484ea..8ef338dd4 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -60,18 +60,6 @@ def app end end - describe '#initialize' do - it 'takes a settings stack, options, and a block' do - p = proc {} - expect do - described_class.new(Grape::Util::InheritableSetting.new, { - path: '/', - method: :get - }, &p) - end.not_to raise_error - end - end - it 'sets itself in the env upon call' do subject.get('/') { 'Hello world.' } get '/' @@ -610,6 +598,9 @@ def handle_argument_error end it 'accepts a code' do + subject.desc 'patate' do + http_codes [[401, 'Unauthorized']] + end subject.get('/hey') do error! 'Unauthorized.', 401 end @@ -1136,7 +1127,7 @@ def memoized end describe '#inspect' do - subject { described_class.new(settings, options).inspect } + subject { described_class.new(settings, **options).inspect } let(:options) do { diff --git a/spec/grape/router/greedy_route_spec.rb b/spec/grape/router/greedy_route_spec.rb index f93c013b4..39c82c87b 100644 --- a/spec/grape/router/greedy_route_spec.rb +++ b/spec/grape/router/greedy_route_spec.rb @@ -1,14 +1,15 @@ # frozen_string_literal: true RSpec.describe Grape::Router::GreedyRoute do - let(:instance) { described_class.new(pattern, options) } - let(:index) { 0 } + let(:instance) { described_class.new(pattern, endpoint: endpoint, allow_header: allow_header) } let(:pattern) { :pattern } - let(:params) do - { a_param: 1 }.freeze - end - let(:options) do - { params: params }.freeze + let(:endpoint) { instance_double(Grape::Endpoint) } + let(:allow_header) { false } + + describe 'inheritance' do + subject { instance } + + it { is_expected.to be_a(Grape::Router::BaseRoute) } end describe '#pattern' do @@ -17,21 +18,21 @@ it { is_expected.to eq(pattern) } end - describe '#options' do - subject { instance.options } + describe '#endpoint' do + subject { instance.endpoint } - it { is_expected.to eq(options) } + it { is_expected.to eq(endpoint) } end - describe '#params' do - subject { instance.params } + describe '#allow_header' do + subject { instance.allow_header } - it { is_expected.to eq(params) } + it { is_expected.to eq(allow_header) } end - describe '#attributes' do - subject { instance.attributes } + describe '#params' do + subject { instance.params } - it { is_expected.to eq(options) } + it { is_expected.to be_nil } end end