From 7862598aa551ac353c2785fbc5914e61eb8aabb3 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 2 Nov 2024 14:27:12 +0100 Subject: [PATCH 01/11] Remove double splat operators when not needed including specs Grape::Validations::Validators::Base last arguments is now just a simple param Grape::Exceptions::Validation and Grape::Exceptions::ValidationErrors does not end with **args. Grape::Request has a named param build_params_with: nil instead of **options Remove @namespace_description in routing.rb --- benchmark/compile_many_routes.rb | 19 ++++---- lib/grape/api.rb | 2 +- lib/grape/api/instance.rb | 16 +++---- lib/grape/dsl/inside_route.rb | 15 +++--- lib/grape/dsl/parameters.rb | 4 +- lib/grape/dsl/routing.rb | 17 ++----- lib/grape/endpoint.rb | 13 ++--- lib/grape/exceptions/base.rb | 48 +++++++------------ lib/grape/exceptions/validation.rb | 9 ++-- lib/grape/exceptions/validation_errors.rb | 4 +- lib/grape/namespace.rb | 2 +- lib/grape/request.rb | 4 +- lib/grape/router.rb | 4 +- lib/grape/router/base_route.rb | 2 +- lib/grape/router/greedy_route.rb | 4 +- lib/grape/router/pattern.rb | 16 +++---- lib/grape/router/route.rb | 6 +-- lib/grape/validations/params_scope.rb | 14 ++---- lib/grape/validations/validator_factory.rb | 4 +- lib/grape/validations/validators/base.rb | 9 ++-- .../validators/coerce_validator.rb | 2 +- .../validators/default_validator.rb | 2 +- .../validators/except_values_validator.rb | 2 +- .../validators/length_validator.rb | 2 +- .../validators/values_validator.rb | 2 +- spec/grape/dsl/parameters_spec.rb | 2 +- spec/grape/dsl/routing_spec.rb | 2 +- spec/grape/middleware/error_spec.rb | 2 +- .../versioner/accept_version_header_spec.rb | 2 +- .../grape/middleware/versioner/header_spec.rb | 2 +- spec/grape/middleware/versioner/param_spec.rb | 2 +- spec/grape/middleware/versioner/path_spec.rb | 2 +- spec/grape/router/greedy_route_spec.rb | 2 +- spec/integration/grape_entity/entity_spec.rb | 2 +- spec/integration/hashie/hashie_spec.rb | 4 +- spec/shared/versioning_examples.rb | 40 ++++++++-------- spec/support/versioned_helpers.rb | 10 ++-- 37 files changed, 133 insertions(+), 161 deletions(-) diff --git a/benchmark/compile_many_routes.rb b/benchmark/compile_many_routes.rb index 1b273302ff..b120c03b58 100644 --- a/benchmark/compile_many_routes.rb +++ b/benchmark/compile_many_routes.rb @@ -3,20 +3,23 @@ $LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib')) require 'grape' require 'benchmark/ips' +require 'memory_profiler' class API < Grape::API prefix :api version 'v1', using: :path - 2000.times do |index| - get "/test#{index}/" do - 'hello' + namespace :test do + 2000.times do |index| + namespace index do + get do + 'hello' + end + end end end end -Benchmark.ips do |ips| - ips.report('Compiling 2000 routes') do - API.compile! - end -end +MemoryProfiler.report(allow_files: 'grape') do + API.compile! +end.pretty_print(to_file: 'without_double_splat.txt') diff --git a/lib/grape/api.rb b/lib/grape/api.rb index eaa351c9bf..2a3435aa38 100644 --- a/lib/grape/api.rb +++ b/lib/grape/api.rb @@ -91,7 +91,7 @@ def const_missing(*args) # For instance, a description could be done using: `desc configuration[:description]` if it may vary # depending on where the endpoint is mounted. Use with care, if you find yourself using configuration # too much, you may actually want to provide a new API rather than remount it. - def mount_instance(**opts) + def mount_instance(opts = {}) instance = Class.new(@base_parent) instance.configuration = Grape::Util::EndpointConfiguration.new(opts[:configuration] || {}) instance.base = self diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index ce290df7cc..c21a847d4a 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -202,18 +202,16 @@ def add_head_not_allowed_methods_and_options_methods without_root_prefix do without_versioning do versioned_route_configs.each do |config| - next if config[:options][:matching_wildchar] + next if config.dig(:options, :matching_wildchar) allowed_methods = config[:methods].dup - allowed_methods |= [Rack::HEAD] if !self.class.namespace_inheritable(:do_not_route_head) && allowed_methods.include?(Rack::GET) - allow_header = (self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Rack::OPTIONS] | allowed_methods) config[:endpoint].options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS) - - attributes = config.merge(allowed_methods: allowed_methods, allow_header: allow_header) - generate_not_allowed_method(config[:pattern], **attributes) + config[:allowed_methods] = allowed_methods + config[:allow_header] = allow_header + generate_not_allowed_method(config[:pattern], config) end end end @@ -240,15 +238,15 @@ def collect_route_config_per_pattern # Generate a route that returns an HTTP 405 response for a user defined # path on methods not specified - def generate_not_allowed_method(pattern, allowed_methods: [], **attributes) + def generate_not_allowed_method(pattern, options) supported_methods = if self.class.namespace_inheritable(:do_not_route_options) Grape::Http::Headers::SUPPORTED_METHODS else Grape::Http::Headers::SUPPORTED_METHODS_WITHOUT_OPTIONS end - not_allowed_methods = supported_methods - allowed_methods - @router.associate_routes(pattern, not_allowed_methods: not_allowed_methods, **attributes) + options[:not_allowed_methods] = supported_methods - options[:allowed_methods] + @router.associate_routes(pattern, options) end # Allows definition of endpoints that ignore the versioning configuration diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index a7efcd4905..c88f7867a9 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -26,8 +26,8 @@ def self.post_filter_methods(type) # has completed module PostBeforeFilter def declared(passed_params, options = {}, declared_params = nil, params_nested_path = []) - options = options.reverse_merge(include_missing: true, include_parent_namespaces: true, evaluate_given: false) - declared_params ||= optioned_declared_params(**options) + options.reverse_merge!(include_missing: true, include_parent_namespaces: true, evaluate_given: false) + declared_params ||= optioned_declared_params(options[:include_parent_namespaces]) res = if passed_params.is_a?(Array) declared_array(passed_params, options, declared_params, params_nested_path) @@ -120,8 +120,8 @@ def optioned_param_key(declared_param, options) options[:stringify] ? declared_param.to_s : declared_param.to_sym end - def optioned_declared_params(**options) - declared_params = if options[:include_parent_namespaces] + def optioned_declared_params(include_parent_namespaces) + declared_params = if include_parent_namespaces # Declared params including parent namespaces route_setting(:declared_params) else @@ -199,10 +199,9 @@ def rack_response(message, status = 200, headers = { Rack::CONTENT_TYPE => conte # Redirect to a new url. # # @param url [String] The url to be redirect. - # @param options [Hash] The options used when redirect. - # :permanent, default false. - # :body, default a short message including the URL. - def redirect(url, permanent: false, body: nil, **_options) + # @param permanent [Boolean] default false. + # @param body default a short message including the URL. + def redirect(url, permanent: false, body: nil) body_message = body if permanent status 301 diff --git a/lib/grape/dsl/parameters.rb b/lib/grape/dsl/parameters.rb index 821da5d793..48f53bcb95 100644 --- a/lib/grape/dsl/parameters.rb +++ b/lib/grape/dsl/parameters.rb @@ -136,7 +136,7 @@ def requires(*attrs, &block) require_required_and_optional_fields(attrs.first, opts) else validate_attributes(attrs, opts, &block) - block ? new_scope(orig_attrs, &block) : push_declared_params(attrs, **opts.slice(:as)) + block ? new_scope(orig_attrs, &block) : push_declared_params(attrs, opts.slice(:as)) end end @@ -162,7 +162,7 @@ def optional(*attrs, &block) else validate_attributes(attrs, opts, &block) - block ? new_scope(orig_attrs, true, &block) : push_declared_params(attrs, **opts.slice(:as)) + block ? new_scope(orig_attrs, true, &block) : push_declared_params(attrs, opts.slice(:as)) end end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 812ddd1d08..8145726ada 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -175,19 +175,12 @@ def route(methods, paths = ['/'], route_options = {}, &block) # end # end def namespace(space = nil, options = {}, &block) - @namespace_description = nil unless instance_variable_defined?(:@namespace_description) && @namespace_description - - if space || block - within_namespace do - previous_namespace_description = @namespace_description - @namespace_description = (@namespace_description || {}).deep_merge(namespace_setting(:description) || {}) - nest(block) do - namespace_stackable(:namespace, Namespace.new(space, **options)) if space - end - @namespace_description = previous_namespace_description + return Namespace.joined_space_path(namespace_stackable(:namespace)) unless space || block + + within_namespace do + nest(block) do + namespace_stackable(:namespace, Namespace.new(space, options)) if space end - else - Namespace.joined_space_path(namespace_stackable(:namespace)) end end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 5fdb03567a..72ead141f1 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -153,7 +153,7 @@ def mount_in(router) methods = [route.request_method] methods << Rack::HEAD if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET methods.each do |method| - route = Grape::Router::Route.new(method, route.origin, **route.attributes.to_h) unless route.request_method == method + route = Grape::Router::Route.new(method, route.origin, route.attributes.to_h) unless route.request_method == method router.append(route.apply(self)) end end @@ -164,8 +164,9 @@ def to_routes route_options = prepare_default_route_attributes map_routes do |method, path| path = prepare_path(path) - params = merge_route_options(**route_options.merge(suffix: path.suffix)) - route = Router::Route.new(method, path.path, **params) + route_options[:suffix] = path.suffix + params = options[:route_options].merge(route_options) + route = Router::Route.new(method, path.path, params) route.apply(self) end.flatten end @@ -196,10 +197,6 @@ def prepare_version version.length == 1 ? version.first : version end - def merge_route_options(**default) - options[:route_options].clone.merge!(**default) - end - def map_routes options[:method].map { |method| options[:path].map { |path| yield method, path } } end @@ -411,7 +408,7 @@ def validations return enum_for(:validations) unless block_given? route_setting(:saved_validations)&.each do |saved_validation| - yield Grape::Validations::ValidatorFactory.create_validator(**saved_validation) + yield Grape::Validations::ValidatorFactory.create_validator(saved_validation) end end diff --git a/lib/grape/exceptions/base.rb b/lib/grape/exceptions/base.rb index e262646c99..27ef78b456 100644 --- a/lib/grape/exceptions/base.rb +++ b/lib/grape/exceptions/base.rb @@ -9,7 +9,7 @@ class Base < StandardError attr_reader :status, :headers - def initialize(status: nil, message: nil, headers: nil, **_options) + def initialize(status: nil, message: nil, headers: nil) super(message) @status = status @@ -26,42 +26,32 @@ def [](index) # if BASE_ATTRIBUTES_KEY.key respond to a string message, then short_message is returned # if BASE_ATTRIBUTES_KEY.key respond to a Hash, means it may have problem , summary and resolution def compose_message(key, **attributes) - short_message = translate_message(key, **attributes) - if short_message.is_a? Hash - @problem = problem(key, **attributes) - @summary = summary(key, **attributes) - @resolution = resolution(key, **attributes) - [['Problem', @problem], ['Summary', @summary], ['Resolution', @resolution]].each_with_object(+'') do |detail_array, message| - message << "\n#{detail_array[0]}:\n #{detail_array[1]}" unless detail_array[1].blank? - message - end - else - short_message - end - end + short_message = translate_message(key, attributes) + return short_message unless short_message.is_a?(Hash) - def problem(key, **attributes) - translate_message(:"#{key}.problem", **attributes) + each_steps(key, attributes).with_object(+'') do |detail_array, message| + message << "\n#{detail_array[0]}:\n #{detail_array[1]}" unless detail_array[1].blank? + end end - def summary(key, **attributes) - translate_message(:"#{key}.summary", **attributes) - end + def each_steps(key, attributes) + return enum_for(:each_steps, key, attributes) unless block_given? - def resolution(key, **attributes) - translate_message(:"#{key}.resolution", **attributes) + yield 'Problem', translate_message(:"#{key}.problem", attributes) + yield 'Summary', translate_message(:"#{key}.summary", attributes) + yield 'Resolution', translate_message(:"#{key}.resolution", attributes) end - def translate_attributes(keys, **options) + def translate_attributes(keys, options = {}) keys.map do |key| - translate("#{BASE_ATTRIBUTES_KEY}.#{key}", default: key, **options) + translate("#{BASE_ATTRIBUTES_KEY}.#{key}", options.merge(default: key.to_s)) end.join(', ') end - def translate_message(key, **options) + def translate_message(key, options = {}) case key when Symbol - translate("#{BASE_MESSAGES_KEY}.#{key}", default: '', **options) + translate("#{BASE_MESSAGES_KEY}.#{key}", options.merge(default: '')) when Proc key.call else @@ -69,14 +59,12 @@ def translate_message(key, **options) end end - def translate(key, **options) - options = options.dup - options[:default] &&= options[:default].to_s + def translate(key, options) message = ::I18n.translate(key, **options) - message.presence || fallback_message(key, **options) + message.presence || fallback_message(key, options) end - def fallback_message(key, **options) + def fallback_message(key, options) if ::I18n.enforce_available_locales && ::I18n.available_locales.exclude?(FALLBACK_LOCALE) key else diff --git a/lib/grape/exceptions/validation.rb b/lib/grape/exceptions/validation.rb index 8d368d2776..0a9e9c5dd3 100644 --- a/lib/grape/exceptions/validation.rb +++ b/lib/grape/exceptions/validation.rb @@ -2,16 +2,17 @@ module Grape module Exceptions - class Validation < Grape::Exceptions::Base + class Validation < Base attr_accessor :params, :message_key - def initialize(params:, message: nil, **args) + def initialize(params:, message: nil, status: nil, headers: nil) @params = params if message @message_key = message if message.is_a?(Symbol) - args[:message] = translate_message(message) + message = translate_message(message) end - super(**args) + + super(status: status, message: message, headers: headers) end # Remove all the unnecessary stuff from Grape::Exceptions::Base like status diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index b8a843b1a5..8859d579b1 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -2,7 +2,7 @@ module Grape module Exceptions - class ValidationErrors < Grape::Exceptions::Base + class ValidationErrors < Base ERRORS_FORMAT_KEY = 'grape.errors.format' DEFAULT_ERRORS_FORMAT = '%s %s' @@ -10,7 +10,7 @@ class ValidationErrors < Grape::Exceptions::Base attr_reader :errors - def initialize(errors: [], headers: {}, **_options) + def initialize(errors: [], headers: {}) @errors = errors.group_by(&:params) super(message: full_messages.join(', '), status: 400, headers: headers) end diff --git a/lib/grape/namespace.rb b/lib/grape/namespace.rb index 537e7ff663..bdaa3a53f7 100644 --- a/lib/grape/namespace.rb +++ b/lib/grape/namespace.rb @@ -12,7 +12,7 @@ class Namespace # @option options :requirements [Hash] param-regex pairs, all of which must # be met by a request's params for all endpoints in this namespace, or # validation will fail and return a 422. - def initialize(space, **options) + def initialize(space, options) @space = space.to_s @options = options end diff --git a/lib/grape/request.rb b/lib/grape/request.rb index 7093ab95dd..b2a62053ea 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -6,8 +6,8 @@ class Request < Rack::Request alias rack_params params - def initialize(env, **options) - extend options[:build_params_with] || Grape.config.param_builder + def initialize(env, build_params_with: nil) + extend build_params_with || Grape.config.param_builder super(env) end diff --git a/lib/grape/router.rb b/lib/grape/router.rb index 61722011af..368297eb12 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -38,8 +38,8 @@ def append(route) map[route.request_method] << route end - def associate_routes(pattern, **options) - Grape::Router::GreedyRoute.new(pattern: pattern, **options).then do |greedy_route| + 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 diff --git a/lib/grape/router/base_route.rb b/lib/grape/router/base_route.rb index 9d19c87209..28d0f50b18 100644 --- a/lib/grape/router/base_route.rb +++ b/lib/grape/router/base_route.rb @@ -7,7 +7,7 @@ class BaseRoute attr_reader :index, :pattern, :options - def initialize(**options) + def initialize(options) @options = ActiveSupport::OrderedOptions.new.update(options) end diff --git a/lib/grape/router/greedy_route.rb b/lib/grape/router/greedy_route.rb index a999c1b906..c2fbcf8e87 100644 --- a/lib/grape/router/greedy_route.rb +++ b/lib/grape/router/greedy_route.rb @@ -6,9 +6,9 @@ module Grape class Router class GreedyRoute < BaseRoute - def initialize(pattern:, **options) + def initialize(pattern, options) @pattern = pattern - super(**options) + super(options) end # Grape::Router:Route defines params as a function diff --git a/lib/grape/router/pattern.rb b/lib/grape/router/pattern.rb index 7b5b5276f0..5761b1ea1e 100644 --- a/lib/grape/router/pattern.rb +++ b/lib/grape/router/pattern.rb @@ -13,9 +13,9 @@ class Pattern def_delegators :to_regexp, :=== alias match? === - def initialize(pattern, **options) + def initialize(pattern, options) @origin = pattern - @path = build_path(pattern, anchor: options[:anchor], suffix: options[:suffix]) + @path = build_path(pattern, options) @pattern = build_pattern(@path, options) @to_regexp = @pattern.to_regexp end @@ -33,15 +33,15 @@ def build_pattern(path, options) path, uri_decode: true, params: options[:params], - capture: extract_capture(**options) + capture: extract_capture(options) ) end - def build_path(pattern, anchor: false, suffix: nil) - PatternCache[[build_path_from_pattern(pattern, anchor: anchor), suffix]] + def build_path(pattern, options) + PatternCache[[build_path_from_pattern(pattern, options), options[:suffix]]] end - def extract_capture(**options) + def extract_capture(options) sliced_options = options .slice(:format, :version) .delete_if { |_k, v| v.blank? } @@ -51,10 +51,10 @@ def extract_capture(**options) options[:requirements].merge(sliced_options) end - def build_path_from_pattern(pattern, anchor: false) + def build_path_from_pattern(pattern, options) if pattern.end_with?('*path') pattern.dup.insert(pattern.rindex('/') + 1, '?') - elsif anchor + elsif options[:anchor] pattern elsif pattern.end_with?('/') "#{pattern}?*path" diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index 335ecb712f..207a806b84 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -9,10 +9,10 @@ class Route < BaseRoute def_delegators :pattern, :path, :origin - def initialize(method, pattern, **options) + def initialize(method, pattern, options) @request_method = upcase_method(method) - @pattern = Grape::Router::Pattern.new(pattern, **options) - super(**options) + @pattern = Grape::Router::Pattern.new(pattern, options) + super(options) end def exec(env) diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index b3c4268f61..36c6ed4d3c 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -174,16 +174,12 @@ def reset_index # Adds a parameter declaration to our list of validations. # @param attrs [Array] (see Grape::DSL::Parameters#requires) - def push_declared_params(attrs, **opts) - opts = opts.merge(declared_params_scope: self) unless opts.key?(:declared_params_scope) - if lateral? - @parent.push_declared_params(attrs, **opts) - else - push_renamed_param(full_path + [attrs.first], opts[:as]) \ - if opts && opts[:as] + def push_declared_params(attrs, opts = {}) + opts[:declared_params_scope] = self unless opts.key?(:declared_params_scope) + return @parent.push_declared_params(attrs, opts) if lateral? - @declared_params.concat(attrs.map { |attr| ::Grape::Validations::ParamsScope::Attr.new(attr, opts[:declared_params_scope]) }) - end + push_renamed_param(full_path + [attrs.first], opts[:as]) if opts[:as] + @declared_params.concat(attrs.map { |attr| ::Grape::Validations::ParamsScope::Attr.new(attr, opts[:declared_params_scope]) }) end # Get the full path of the parameter scope in the hierarchy. diff --git a/lib/grape/validations/validator_factory.rb b/lib/grape/validations/validator_factory.rb index 444fa0421e..0e2022d3af 100644 --- a/lib/grape/validations/validator_factory.rb +++ b/lib/grape/validations/validator_factory.rb @@ -3,12 +3,12 @@ module Grape module Validations class ValidatorFactory - def self.create_validator(**options) + def self.create_validator(options) options[:validator_class].new(options[:attributes], options[:options], options[:required], options[:params_scope], - **options[:opts]) + options[:opts]) end end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 3dd49fd797..ee2dc4a87e 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -13,15 +13,14 @@ class Base # @param options [Object] implementation-dependent Validator options # @param required [Boolean] attribute(s) are required or optional # @param scope [ParamsScope] parent scope for this Validator - # @param opts [Array] additional validation options - def initialize(attrs, options, required, scope, *opts) + # @param opts [Hash] additional validation options + def initialize(attrs, options, required, scope, opts) @attrs = Array(attrs) @option = options @required = required @scope = scope - opts = opts.any? ? opts.shift : {} - @fail_fast = opts.fetch(:fail_fast, false) - @allow_blank = opts.fetch(:allow_blank, false) + @fail_fast = opts[:fail_fast] + @allow_blank = opts[:allow_blank] end # Validates a given request. diff --git a/lib/grape/validations/validators/coerce_validator.rb b/lib/grape/validations/validators/coerce_validator.rb index 979ad47c67..eaf7c40697 100644 --- a/lib/grape/validations/validators/coerce_validator.rb +++ b/lib/grape/validations/validators/coerce_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class CoerceValidator < Base - def initialize(attrs, options, required, scope, **opts) + def initialize(attrs, options, required, scope, opts) super @converter = if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) diff --git a/lib/grape/validations/validators/default_validator.rb b/lib/grape/validations/validators/default_validator.rb index 9a59e5da15..d4a6fdf99f 100644 --- a/lib/grape/validations/validators/default_validator.rb +++ b/lib/grape/validations/validators/default_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class DefaultValidator < Base - def initialize(attrs, options, required, scope, **opts) + def initialize(attrs, options, required, scope, opts) @default = options super end diff --git a/lib/grape/validations/validators/except_values_validator.rb b/lib/grape/validations/validators/except_values_validator.rb index b125c12cfb..298eb0ab91 100644 --- a/lib/grape/validations/validators/except_values_validator.rb +++ b/lib/grape/validations/validators/except_values_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class ExceptValuesValidator < Base - def initialize(attrs, options, required, scope, **opts) + def initialize(attrs, options, required, scope, opts) @except = options.is_a?(Hash) ? options[:value] : options super end diff --git a/lib/grape/validations/validators/length_validator.rb b/lib/grape/validations/validators/length_validator.rb index f844f047ec..c84b4c0962 100644 --- a/lib/grape/validations/validators/length_validator.rb +++ b/lib/grape/validations/validators/length_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class LengthValidator < Base - def initialize(attrs, options, required, scope, **opts) + def initialize(attrs, options, required, scope, opts) @min = options[:min] @max = options[:max] @is = options[:is] diff --git a/lib/grape/validations/validators/values_validator.rb b/lib/grape/validations/validators/values_validator.rb index cc2a641721..30cdeee7b2 100644 --- a/lib/grape/validations/validators/values_validator.rb +++ b/lib/grape/validations/validators/values_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class ValuesValidator < Base - def initialize(attrs, options, required, scope, **opts) + def initialize(attrs, options, required, scope, opts) @values = options.is_a?(Hash) ? options[:value] : options super end diff --git a/spec/grape/dsl/parameters_spec.rb b/spec/grape/dsl/parameters_spec.rb index 28ff1cce0b..8c7c1e96cb 100644 --- a/spec/grape/dsl/parameters_spec.rb +++ b/spec/grape/dsl/parameters_spec.rb @@ -20,7 +20,7 @@ def validate_attributes_reader @validate_attributes end - def push_declared_params(args, **_opts) + def push_declared_params(args, _opts) @push_declared_params = args end diff --git a/spec/grape/dsl/routing_spec.rb b/spec/grape/dsl/routing_spec.rb index 617980bb29..1b54ba913a 100644 --- a/spec/grape/dsl/routing_spec.rb +++ b/spec/grape/dsl/routing_spec.rb @@ -179,7 +179,7 @@ it 'creates a new namespace with given name and options' do expect(subject).to receive(:within_namespace).and_yield expect(subject).to receive(:nest).and_yield - expect(Grape::Namespace).to receive(:new).with(:foo, foo: 'bar').and_return(new_namespace) + expect(Grape::Namespace).to receive(:new).with(:foo, { foo: 'bar' }).and_return(new_namespace) expect(subject).to receive(:namespace_stackable).with(:namespace, new_namespace) subject.namespace :foo, foo: 'bar', &proc {} diff --git a/spec/grape/middleware/error_spec.rb b/spec/grape/middleware/error_spec.rb index 88eb88725e..cc8a735d4b 100644 --- a/spec/grape/middleware/error_spec.rb +++ b/spec/grape/middleware/error_spec.rb @@ -29,7 +29,7 @@ def call(_env) context = self Rack::Builder.app do use Spec::Support::EndpointFaker - use Grape::Middleware::Error, **opts # rubocop:disable RSpec/DescribedClass + use Grape::Middleware::Error, opts # rubocop:disable RSpec/DescribedClass run context.err_app end end diff --git a/spec/grape/middleware/versioner/accept_version_header_spec.rb b/spec/grape/middleware/versioner/accept_version_header_spec.rb index 4f44384980..6bcf7b14ad 100644 --- a/spec/grape/middleware/versioner/accept_version_header_spec.rb +++ b/spec/grape/middleware/versioner/accept_version_header_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe Grape::Middleware::Versioner::AcceptVersionHeader do - subject { described_class.new(app, **(@options || {})) } + subject { described_class.new(app, @options) } let(:app) { ->(env) { [200, env, env] } } diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index 1d686aae33..12fd837e00 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/spec/grape/middleware/versioner/header_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe Grape::Middleware::Versioner::Header do - subject { described_class.new(app, **(@options || {})) } + subject { described_class.new(app, @options) } let(:app) { ->(env) { [200, env, env] } } diff --git a/spec/grape/middleware/versioner/param_spec.rb b/spec/grape/middleware/versioner/param_spec.rb index 00099dfc91..4e6bb4aa72 100644 --- a/spec/grape/middleware/versioner/param_spec.rb +++ b/spec/grape/middleware/versioner/param_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe Grape::Middleware::Versioner::Param do - subject { described_class.new(app, **options) } + subject { described_class.new(app, options) } let(:app) { ->(env) { [200, env, env[Grape::Env::API_VERSION]] } } let(:options) { {} } diff --git a/spec/grape/middleware/versioner/path_spec.rb b/spec/grape/middleware/versioner/path_spec.rb index 79aff53761..4593eca034 100644 --- a/spec/grape/middleware/versioner/path_spec.rb +++ b/spec/grape/middleware/versioner/path_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true describe Grape::Middleware::Versioner::Path do - subject { described_class.new(app, **options) } + subject { described_class.new(app, options) } let(:app) { ->(env) { [200, env, env[Grape::Env::API_VERSION]] } } let(:options) { {} } diff --git a/spec/grape/router/greedy_route_spec.rb b/spec/grape/router/greedy_route_spec.rb index 29e966280e..f93c013b4b 100644 --- a/spec/grape/router/greedy_route_spec.rb +++ b/spec/grape/router/greedy_route_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe Grape::Router::GreedyRoute do - let(:instance) { described_class.new(pattern: pattern, **options) } + let(:instance) { described_class.new(pattern, options) } let(:index) { 0 } let(:pattern) { :pattern } let(:params) do diff --git a/spec/integration/grape_entity/entity_spec.rb b/spec/integration/grape_entity/entity_spec.rb index cdac039a6e..eaea823823 100644 --- a/spec/integration/grape_entity/entity_spec.rb +++ b/spec/integration/grape_entity/entity_spec.rb @@ -348,7 +348,7 @@ def call(_env) opts = options Rack::Builder.app do use Spec::Support::EndpointFaker - use Grape::Middleware::Error, **opts + use Grape::Middleware::Error, opts run ErrApp end end diff --git a/spec/integration/hashie/hashie_spec.rb b/spec/integration/hashie/hashie_spec.rb index 73c97ce1e1..d90b771265 100644 --- a/spec/integration/hashie/hashie_spec.rb +++ b/spec/integration/hashie/hashie_spec.rb @@ -164,11 +164,9 @@ end describe 'when the build_params_with is set to Hashie' do - subject(:request_params) { Grape::Request.new(env, **opts).params } + subject(:request_params) { Grape::Request.new(env, build_params_with: Grape::Extensions::Hashie::Mash::ParamBuilder).params } context 'when the API includes a specific param builder' do - let(:opts) { { build_params_with: Grape::Extensions::Hashie::Mash::ParamBuilder } } - it { is_expected.to be_a(Hashie::Mash) } end end diff --git a/spec/shared/versioning_examples.rb b/spec/shared/versioning_examples.rb index 0215a7c440..9f42eeda18 100644 --- a/spec/shared/versioning_examples.rb +++ b/spec/shared/versioning_examples.rb @@ -7,7 +7,7 @@ subject.get :hello do "Version: #{request.env[Grape::Env::API_VERSION]}" end - versioned_get '/hello', 'v1', **macro_options + versioned_get '/hello', 'v1', macro_options expect(last_response.body).to eql 'Version: v1' end @@ -18,7 +18,7 @@ subject.get :hello do "Version: #{request.env[Grape::Env::API_VERSION]}" end - versioned_get '/hello', 'v1', **macro_options.merge(prefix: 'api') + versioned_get '/hello', 'v1', macro_options.merge(prefix: 'api') expect(last_response.body).to eql 'Version: v1' end @@ -34,14 +34,14 @@ end end - versioned_get '/awesome', 'v1', **macro_options + versioned_get '/awesome', 'v1', macro_options expect(last_response.status).to be 404 - versioned_get '/awesome', 'v2', **macro_options + versioned_get '/awesome', 'v2', macro_options expect(last_response.status).to be 200 - versioned_get '/legacy', 'v1', **macro_options + versioned_get '/legacy', 'v1', macro_options expect(last_response.status).to be 200 - versioned_get '/legacy', 'v2', **macro_options + versioned_get '/legacy', 'v2', macro_options expect(last_response.status).to be 404 end @@ -51,11 +51,11 @@ 'I exist' end - versioned_get '/awesome', 'v1', **macro_options + versioned_get '/awesome', 'v1', macro_options expect(last_response.status).to be 200 - versioned_get '/awesome', 'v2', **macro_options + versioned_get '/awesome', 'v2', macro_options expect(last_response.status).to be 200 - versioned_get '/awesome', 'v3', **macro_options + versioned_get '/awesome', 'v3', macro_options expect(last_response.status).to be 404 end @@ -74,10 +74,10 @@ end end - versioned_get '/version', 'v2', **macro_options + versioned_get '/version', 'v2', macro_options expect(last_response.status).to eq(200) expect(last_response.body).to eq('v2') - versioned_get '/version', 'v1', **macro_options + versioned_get '/version', 'v1', macro_options expect(last_response.status).to eq(200) expect(last_response.body).to eq('version v1') end @@ -98,11 +98,11 @@ end end - versioned_get '/version', 'v1', **macro_options.merge(prefix: subject.prefix) + versioned_get '/version', 'v1', macro_options.merge(prefix: subject.prefix) expect(last_response.status).to eq(200) expect(last_response.body).to eq('version v1') - versioned_get '/version', 'v2', **macro_options.merge(prefix: subject.prefix) + versioned_get '/version', 'v2', macro_options.merge(prefix: subject.prefix) expect(last_response.status).to eq(200) expect(last_response.body).to eq('v2') end @@ -133,11 +133,11 @@ end end - versioned_get '/version', 'v1', **macro_options.merge(prefix: subject.prefix) + versioned_get '/version', 'v1', macro_options.merge(prefix: subject.prefix) expect(last_response.status).to eq(200) expect(last_response.body).to eq('v1-version') - versioned_get '/version', 'v2', **macro_options.merge(prefix: subject.prefix) + versioned_get '/version', 'v2', macro_options.merge(prefix: subject.prefix) expect(last_response.status).to eq(200) expect(last_response.body).to eq('v2-version') end @@ -150,7 +150,7 @@ subject.get :api_version_with_version_param do params[:version] end - versioned_get '/api_version_with_version_param?version=1', 'v1', **macro_options + versioned_get '/api_version_with_version_param?version=1', 'v1', macro_options expect(last_response.body).to eql '1' end @@ -186,13 +186,13 @@ context 'v1' do it 'finds endpoint' do - versioned_get '/version', 'v1', **macro_options + versioned_get '/version', 'v1', macro_options expect(last_response.status).to eq(200) expect(last_response.body).to eq('v1') end it 'finds catch all' do - versioned_get '/whatever', 'v1', **macro_options + versioned_get '/whatever', 'v1', macro_options expect(last_response.status).to eq(200) expect(last_response.body).to end_with 'whatever' end @@ -200,13 +200,13 @@ context 'v2' do it 'finds endpoint' do - versioned_get '/version', 'v2', **macro_options + versioned_get '/version', 'v2', macro_options expect(last_response.status).to eq(200) expect(last_response.body).to eq('v2') end it 'finds catch all' do - versioned_get '/whatever', 'v2', **macro_options + versioned_get '/whatever', 'v2', macro_options expect(last_response.status).to eq(200) expect(last_response.body).to end_with 'whatever' end diff --git a/spec/support/versioned_helpers.rb b/spec/support/versioned_helpers.rb index 75e56e7d18..c4e841249e 100644 --- a/spec/support/versioned_helpers.rb +++ b/spec/support/versioned_helpers.rb @@ -6,7 +6,7 @@ module Support module Helpers # Returns the path with options[:version] prefixed if options[:using] is :path. # Returns normal path otherwise. - def versioned_path(**options) + def versioned_path(options) case options[:using] when :path File.join('/', options[:prefix] || '', options[:version], options[:path]) @@ -17,7 +17,7 @@ def versioned_path(**options) end end - def versioned_headers(**options) + def versioned_headers(options) case options[:using] when :path, :param {} @@ -37,9 +37,9 @@ def versioned_headers(**options) end end - def versioned_get(path, version_name, **version_options) - path = versioned_path(**version_options.merge(version: version_name, path: path)) - headers = versioned_headers(**version_options.merge(version: version_name)) + def versioned_get(path, version_name, version_options) + path = versioned_path(version_options.merge(version: version_name, path: path)) + headers = versioned_headers(version_options.merge(version: version_name)) params = {} params = { version_options[:parameter] => version_name } if version_options[:using] == :param get path, params, headers From d4406747f9bf4d04724f535e4e56b8a0cb5f2a6e Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 2 Nov 2024 14:28:19 +0100 Subject: [PATCH 02/11] Revert compile_many_routes.rb --- benchmark/compile_many_routes.rb | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/benchmark/compile_many_routes.rb b/benchmark/compile_many_routes.rb index b120c03b58..1b273302ff 100644 --- a/benchmark/compile_many_routes.rb +++ b/benchmark/compile_many_routes.rb @@ -3,23 +3,20 @@ $LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib')) require 'grape' require 'benchmark/ips' -require 'memory_profiler' class API < Grape::API prefix :api version 'v1', using: :path - namespace :test do - 2000.times do |index| - namespace index do - get do - 'hello' - end - end + 2000.times do |index| + get "/test#{index}/" do + 'hello' end end end -MemoryProfiler.report(allow_files: 'grape') do - API.compile! -end.pretty_print(to_file: 'without_double_splat.txt') +Benchmark.ips do |ips| + ips.report('Compiling 2000 routes') do + API.compile! + end +end From d0da869405fd72db695de72c0c653cc2d4d596b9 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 2 Nov 2024 14:54:01 +0100 Subject: [PATCH 03/11] Optimize Head Route --- lib/grape/endpoint.rb | 20 +++++++++----------- lib/grape/router/base_route.rb | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 72ead141f1..6ae541f426 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -145,16 +145,14 @@ def reset_routes! end def mount_in(router) - if endpoints - endpoints.each { |e| e.mount_in(router) } - else - reset_routes! - routes.each do |route| - methods = [route.request_method] - methods << Rack::HEAD if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET - methods.each do |method| - route = Grape::Router::Route.new(method, route.origin, route.attributes.to_h) unless route.request_method == method - router.append(route.apply(self)) + return endpoints.each { |e| e.mount_in(router) } if endpoints + + reset_routes! + routes.each do |route| + router.append(route.apply(self)) + if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET + Grape::Router::Route.new(Rack::HEAD, route.origin, route.attributes).then do |head_route| + router.append(head_route.apply(self)) end end end @@ -166,7 +164,7 @@ def to_routes path = prepare_path(path) route_options[:suffix] = path.suffix params = options[:route_options].merge(route_options) - route = Router::Route.new(method, path.path, params) + route = Grape::Router::Route.new(method, path.path, params) route.apply(self) end.flatten end diff --git a/lib/grape/router/base_route.rb b/lib/grape/router/base_route.rb index 28d0f50b18..86439e908f 100644 --- a/lib/grape/router/base_route.rb +++ b/lib/grape/router/base_route.rb @@ -8,7 +8,7 @@ class BaseRoute attr_reader :index, :pattern, :options def initialize(options) - @options = ActiveSupport::OrderedOptions.new.update(options) + @options = options.is_a?(ActiveSupport::OrderedOptions) ? options : ActiveSupport::OrderedOptions.new.update(options) end alias attributes options From 6a20afbedb96dab19e7fe699a4b936dd611a0ddf Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 2 Nov 2024 15:11:24 +0100 Subject: [PATCH 04/11] Use dup instead of new for Head Route --- lib/grape/endpoint.rb | 3 ++- lib/grape/router/route.rb | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 6ae541f426..52ff538d69 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -151,7 +151,8 @@ def mount_in(router) routes.each do |route| router.append(route.apply(self)) if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET - Grape::Router::Route.new(Rack::HEAD, route.origin, route.attributes).then do |head_route| + route.dup.then do |head_route| + head_route.convert_to_head_request! router.append(head_route.apply(self)) end end diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index 207a806b84..244b0a4070 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -15,6 +15,10 @@ def initialize(method, pattern, options) super(options) end + def convert_to_head_request! + @request_method = Rack::HEAD + end + def exec(env) @app.call(env) end From 42d93d40e422f562f5c2acc8c8352a74726187a5 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 12:24:11 +0100 Subject: [PATCH 05/11] Use opts = {} for validator instead of just opts rubocop --- lib/grape/endpoint.rb | 116 +++++++++--------- lib/grape/validations/validators/base.rb | 2 +- .../validators/coerce_validator.rb | 2 +- .../validators/default_validator.rb | 2 +- .../validators/except_values_validator.rb | 2 +- .../validators/length_validator.rb | 2 +- .../validators/values_validator.rb | 2 +- 7 files changed, 64 insertions(+), 64 deletions(-) diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 52ff538d69..ce64678c71 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -150,11 +150,11 @@ def mount_in(router) reset_routes! routes.each do |route| router.append(route.apply(self)) - if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET - route.dup.then do |head_route| - head_route.convert_to_head_request! - router.append(head_route.apply(self)) - end + next unless !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET + + route.dup.then do |head_route| + head_route.convert_to_head_request! + router.append(head_route.apply(self)) end end end @@ -283,59 +283,6 @@ def run end end - def build_stack(helpers) - stack = Grape::Middleware::Stack.new - - content_types = namespace_stackable_with_hash(:content_types) - format = namespace_inheritable(:format) - - stack.use Rack::Head - stack.use Class.new(Grape::Middleware::Error), - helpers: helpers, - format: format, - content_types: content_types, - default_status: namespace_inheritable(:default_error_status), - rescue_all: namespace_inheritable(:rescue_all), - rescue_grape_exceptions: namespace_inheritable(:rescue_grape_exceptions), - default_error_formatter: namespace_inheritable(:default_error_formatter), - error_formatters: namespace_stackable_with_hash(:error_formatters), - rescue_options: namespace_stackable_with_hash(:rescue_options), - rescue_handlers: namespace_reverse_stackable_with_hash(:rescue_handlers), - base_only_rescue_handlers: namespace_stackable_with_hash(:base_only_rescue_handlers), - all_rescue_handler: namespace_inheritable(:all_rescue_handler), - grape_exceptions_rescue_handler: namespace_inheritable(:grape_exceptions_rescue_handler) - - stack.concat namespace_stackable(:middleware) - - if namespace_inheritable(:version).present? - stack.use Grape::Middleware::Versioner.using(namespace_inheritable(:version_options)[:using]), - versions: namespace_inheritable(:version).flatten, - version_options: namespace_inheritable(:version_options), - prefix: namespace_inheritable(:root_prefix), - mount_path: namespace_stackable(:mount_path).first - end - - stack.use Grape::Middleware::Formatter, - format: format, - default_format: namespace_inheritable(:default_format) || :txt, - content_types: content_types, - formatters: namespace_stackable_with_hash(:formatters), - parsers: namespace_stackable_with_hash(:parsers) - - builder = stack.build - builder.run ->(env) { env[Grape::Env::API_ENDPOINT].run } - builder.to_app - end - - def build_helpers - helpers = namespace_stackable(:helpers) - return if helpers.empty? - - Module.new { helpers.each { |mod_to_include| include mod_to_include } } - end - - private :build_stack, :build_helpers - def execute @block&.call(self) end @@ -415,5 +362,58 @@ def options? options[:options_route_enabled] && env[Rack::REQUEST_METHOD] == Rack::OPTIONS end + + private + + def build_stack(helpers) + stack = Grape::Middleware::Stack.new + + content_types = namespace_stackable_with_hash(:content_types) + format = namespace_inheritable(:format) + + stack.use Rack::Head + stack.use Class.new(Grape::Middleware::Error), + helpers: helpers, + format: format, + content_types: content_types, + default_status: namespace_inheritable(:default_error_status), + rescue_all: namespace_inheritable(:rescue_all), + rescue_grape_exceptions: namespace_inheritable(:rescue_grape_exceptions), + default_error_formatter: namespace_inheritable(:default_error_formatter), + error_formatters: namespace_stackable_with_hash(:error_formatters), + rescue_options: namespace_stackable_with_hash(:rescue_options), + rescue_handlers: namespace_reverse_stackable_with_hash(:rescue_handlers), + base_only_rescue_handlers: namespace_stackable_with_hash(:base_only_rescue_handlers), + all_rescue_handler: namespace_inheritable(:all_rescue_handler), + grape_exceptions_rescue_handler: namespace_inheritable(:grape_exceptions_rescue_handler) + + stack.concat namespace_stackable(:middleware) + + if namespace_inheritable(:version).present? + stack.use Grape::Middleware::Versioner.using(namespace_inheritable(:version_options)[:using]), + versions: namespace_inheritable(:version).flatten, + version_options: namespace_inheritable(:version_options), + prefix: namespace_inheritable(:root_prefix), + mount_path: namespace_stackable(:mount_path).first + end + + stack.use Grape::Middleware::Formatter, + format: format, + default_format: namespace_inheritable(:default_format) || :txt, + content_types: content_types, + formatters: namespace_stackable_with_hash(:formatters), + parsers: namespace_stackable_with_hash(:parsers) + + builder = stack.build + builder.run ->(env) { env[Grape::Env::API_ENDPOINT].run } + builder.to_app + end + + def build_helpers + helpers = namespace_stackable(:helpers) + return if helpers.empty? + + Module.new { helpers.each { |mod_to_include| include mod_to_include } } + end end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index ee2dc4a87e..870e68d159 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -14,7 +14,7 @@ class Base # @param required [Boolean] attribute(s) are required or optional # @param scope [ParamsScope] parent scope for this Validator # @param opts [Hash] additional validation options - def initialize(attrs, options, required, scope, opts) + def initialize(attrs, options, required, scope, opts = {}) @attrs = Array(attrs) @option = options @required = required diff --git a/lib/grape/validations/validators/coerce_validator.rb b/lib/grape/validations/validators/coerce_validator.rb index eaf7c40697..4e08aa8786 100644 --- a/lib/grape/validations/validators/coerce_validator.rb +++ b/lib/grape/validations/validators/coerce_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class CoerceValidator < Base - def initialize(attrs, options, required, scope, opts) + def initialize(attrs, options, required, scope, opts = {}) super @converter = if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) diff --git a/lib/grape/validations/validators/default_validator.rb b/lib/grape/validations/validators/default_validator.rb index d4a6fdf99f..eba8c77302 100644 --- a/lib/grape/validations/validators/default_validator.rb +++ b/lib/grape/validations/validators/default_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class DefaultValidator < Base - def initialize(attrs, options, required, scope, opts) + def initialize(attrs, options, required, scope, opts = {}) @default = options super end diff --git a/lib/grape/validations/validators/except_values_validator.rb b/lib/grape/validations/validators/except_values_validator.rb index 298eb0ab91..33305d198c 100644 --- a/lib/grape/validations/validators/except_values_validator.rb +++ b/lib/grape/validations/validators/except_values_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class ExceptValuesValidator < Base - def initialize(attrs, options, required, scope, opts) + def initialize(attrs, options, required, scope, opts = {}) @except = options.is_a?(Hash) ? options[:value] : options super end diff --git a/lib/grape/validations/validators/length_validator.rb b/lib/grape/validations/validators/length_validator.rb index c84b4c0962..9583365614 100644 --- a/lib/grape/validations/validators/length_validator.rb +++ b/lib/grape/validations/validators/length_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class LengthValidator < Base - def initialize(attrs, options, required, scope, opts) + def initialize(attrs, options, required, scope, opts = {}) @min = options[:min] @max = options[:max] @is = options[:is] diff --git a/lib/grape/validations/validators/values_validator.rb b/lib/grape/validations/validators/values_validator.rb index 30cdeee7b2..18509c95c1 100644 --- a/lib/grape/validations/validators/values_validator.rb +++ b/lib/grape/validations/validators/values_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class ValuesValidator < Base - def initialize(attrs, options, required, scope, opts) + def initialize(attrs, options, required, scope, opts = {}) @values = options.is_a?(Hash) ? options[:value] : options super end From db7c156010b8dc178fa3fec92a4165daff4c9b7c Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 14:08:32 +0100 Subject: [PATCH 06/11] Optimize add_head_not_allowed_methods_and_options_methods Renamed to add_head_and_options_methods --- lib/grape/api/instance.rb | 88 ++++++++++++--------------------------- lib/grape/router.rb | 2 +- 2 files changed, 28 insertions(+), 62 deletions(-) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index c21a847d4a..8547747683 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -150,7 +150,7 @@ def inherit_settings(other_settings) # this API into a usable form. def initialize @router = Router.new - add_head_not_allowed_methods_and_options_methods + add_head_and_options_methods self.class.endpoints.each do |endpoint| endpoint.mount_in(@router) end @@ -190,90 +190,56 @@ def cascade? private # For every resource add a 'OPTIONS' route that returns an HTTP 204 response - # with a list of HTTP methods that can be called. Also add a route that - # will return an HTTP 405 response for any HTTP method that the resource - # cannot handle. - def add_head_not_allowed_methods_and_options_methods - versioned_route_configs = collect_route_config_per_pattern + # with a list of HTTP methods that can be called. + def add_head_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 + # Disable versioning so adding a route won't prepend versioning # informations again. - without_root_prefix do - without_versioning do - versioned_route_configs.each do |config| - next if config.dig(:options, :matching_wildchar) - - allowed_methods = config[:methods].dup - allowed_methods |= [Rack::HEAD] if !self.class.namespace_inheritable(:do_not_route_head) && allowed_methods.include?(Rack::GET) - allow_header = (self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Rack::OPTIONS] | allowed_methods) - - config[:endpoint].options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS) - config[:allowed_methods] = allowed_methods - config[:allow_header] = allow_header - generate_not_allowed_method(config[:pattern], config) - end - end - end + without_root_prefix_and_versioning { collect_route_config_per_pattern(all_routes) } end - def collect_route_config_per_pattern - all_routes = self.class.endpoints.map(&:routes).flatten + def collect_route_config_per_pattern(all_routes) routes_by_regexp = all_routes.group_by(&:pattern_regexp) # Build the configuration based on the first endpoint and the collection of methods supported. - routes_by_regexp.values.map do |routes| - last_route = routes.last # Most of the configuration is taken from the last endpoint - matching_wildchar = routes.any? { |route| route.request_method == '*' } - { - options: { matching_wildchar: matching_wildchar }, - pattern: last_route.pattern, - requirements: last_route.requirements, - path: last_route.origin, - endpoint: last_route.app, - methods: matching_wildchar ? Grape::Http::Headers::SUPPORTED_METHODS : routes.map(&:request_method) - } - end - end + 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 == '*' } - # Generate a route that returns an HTTP 405 response for a user defined - # path on methods not specified - def generate_not_allowed_method(pattern, options) - supported_methods = - if self.class.namespace_inheritable(:do_not_route_options) - Grape::Http::Headers::SUPPORTED_METHODS - else - Grape::Http::Headers::SUPPORTED_METHODS_WITHOUT_OPTIONS - end - options[:not_allowed_methods] = supported_methods - options[:allowed_methods] - @router.associate_routes(pattern, options) + allowed_methods = routes.map(&:request_method) + allowed_methods |= [Rack::HEAD] if !self.class.namespace_inheritable(:do_not_route_head) && allowed_methods.include?(Rack::GET) + + allow_header = self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Rack::OPTIONS] | allowed_methods + last_route.app.options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS) + + @router.associate_routes(last_route.pattern, { + requirements: last_route.options[:requirements], + path: last_route.origin, + endpoint: last_route.app, + allow_header: allow_header + }) + end end # Allows definition of endpoints that ignore the versioning configuration # used by the rest of your API. - def without_versioning(&_block) + 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) yield self.class.namespace_inheritable(:version, old_version) self.class.namespace_inheritable(:version_options, old_version_options) - end - - # Allows definition of endpoints that ignore the root prefix used by the - # rest of your API. - def without_root_prefix(&_block) - old_prefix = self.class.namespace_inheritable(:root_prefix) - - self.class.namespace_inheritable_to_nil(:root_prefix) - - yield - - self.class.namespace_inheritable(:root_prefix, old_prefix) + self.class.namespace_inheritable(:root_prefix, old_root_prefix) end end end diff --git a/lib/grape/router.rb b/lib/grape/router.rb index 368297eb12..f62c0df9c4 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -152,7 +152,7 @@ def greedy_match?(input) def call_with_allow_headers(env, route) prepare_env_from_route(env, route) - env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.allow_header.join(', ').freeze + env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.options[:allow_header].join(', ').freeze route.endpoint.call(env) end From ccbe23cf23c22fa16bbc822a982d3cdfd1c69d53 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 14:41:37 +0100 Subject: [PATCH 07/11] Remove requirements and path from greedy route (not used) allow header is joined --- lib/grape/api/instance.rb | 6 ++---- lib/grape/router.rb | 6 +++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 8547747683..1abac60872 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -216,10 +216,8 @@ def collect_route_config_per_pattern(all_routes) last_route.app.options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS) @router.associate_routes(last_route.pattern, { - requirements: last_route.options[:requirements], - path: last_route.origin, - endpoint: last_route.app, - allow_header: allow_header + endpoint: last_route.app, + allow_header: allow_header.join(', ').freeze }) end end diff --git a/lib/grape/router.rb b/lib/grape/router.rb index f62c0df9c4..6889b42135 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -107,7 +107,7 @@ def transaction(env) route = match?(input, '*') - return last_neighbor_route.endpoint.call(env) if last_neighbor_route && last_response_cascade && route + return last_neighbor_route.options[:endpoint].call(env) if last_neighbor_route && last_response_cascade && route last_response_cascade = cascade_or_return_response.call(process_route(route, env)) if route @@ -152,8 +152,8 @@ 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].join(', ').freeze - route.endpoint.call(env) + env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.options[:allow_header] + route.options[:endpoint].call(env) end def prepare_env_from_route(env, route) From d8a224c98101576b78dd2f32ea9e7cbe993a4ef4 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 14:55:25 +0100 Subject: [PATCH 08/11] Remove {} for opts since its internal. --- lib/grape/validations/validators/base.rb | 2 +- lib/grape/validations/validators/coerce_validator.rb | 2 +- lib/grape/validations/validators/except_values_validator.rb | 2 +- lib/grape/validations/validators/length_validator.rb | 2 +- lib/grape/validations/validators/values_validator.rb | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 870e68d159..ee2dc4a87e 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -14,7 +14,7 @@ class Base # @param required [Boolean] attribute(s) are required or optional # @param scope [ParamsScope] parent scope for this Validator # @param opts [Hash] additional validation options - def initialize(attrs, options, required, scope, opts = {}) + def initialize(attrs, options, required, scope, opts) @attrs = Array(attrs) @option = options @required = required diff --git a/lib/grape/validations/validators/coerce_validator.rb b/lib/grape/validations/validators/coerce_validator.rb index 4e08aa8786..eaf7c40697 100644 --- a/lib/grape/validations/validators/coerce_validator.rb +++ b/lib/grape/validations/validators/coerce_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class CoerceValidator < Base - def initialize(attrs, options, required, scope, opts = {}) + def initialize(attrs, options, required, scope, opts) super @converter = if type.is_a?(Grape::Validations::Types::VariantCollectionCoercer) diff --git a/lib/grape/validations/validators/except_values_validator.rb b/lib/grape/validations/validators/except_values_validator.rb index 33305d198c..298eb0ab91 100644 --- a/lib/grape/validations/validators/except_values_validator.rb +++ b/lib/grape/validations/validators/except_values_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class ExceptValuesValidator < Base - def initialize(attrs, options, required, scope, opts = {}) + def initialize(attrs, options, required, scope, opts) @except = options.is_a?(Hash) ? options[:value] : options super end diff --git a/lib/grape/validations/validators/length_validator.rb b/lib/grape/validations/validators/length_validator.rb index 9583365614..c84b4c0962 100644 --- a/lib/grape/validations/validators/length_validator.rb +++ b/lib/grape/validations/validators/length_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class LengthValidator < Base - def initialize(attrs, options, required, scope, opts = {}) + def initialize(attrs, options, required, scope, opts) @min = options[:min] @max = options[:max] @is = options[:is] diff --git a/lib/grape/validations/validators/values_validator.rb b/lib/grape/validations/validators/values_validator.rb index 18509c95c1..30cdeee7b2 100644 --- a/lib/grape/validations/validators/values_validator.rb +++ b/lib/grape/validations/validators/values_validator.rb @@ -4,7 +4,7 @@ module Grape module Validations module Validators class ValuesValidator < Base - def initialize(attrs, options, required, scope, opts = {}) + def initialize(attrs, options, required, scope, opts) @values = options.is_a?(Hash) ? options[:value] : options super end From f3e08bfec350a2f2204285d05a737b8f237533a8 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 14:56:50 +0100 Subject: [PATCH 09/11] Fix rubocop --- lib/grape/api/instance.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 1abac60872..f35a0b46a3 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -216,7 +216,7 @@ def collect_route_config_per_pattern(all_routes) last_route.app.options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS) @router.associate_routes(last_route.pattern, { - endpoint: last_route.app, + endpoint: last_route.app, allow_header: allow_header.join(', ').freeze }) end From bb7db5f03a5f34ed6bc1f8d5ef6c58f4f2f26e22 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 16:04:38 +0100 Subject: [PATCH 10/11] Remove frozen allow_header. Now Dynamic --- lib/grape/api/instance.rb | 10 ++++++---- lib/grape/endpoint.rb | 5 +++-- lib/grape/middleware/versioner/header.rb | 10 +++------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index f35a0b46a3..c6a608713a 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -150,7 +150,7 @@ def inherit_settings(other_settings) # this API into a usable form. def initialize @router = Router.new - add_head_and_options_methods + add_head_not_allowed_methods_and_options_methods self.class.endpoints.each do |endpoint| endpoint.mount_in(@router) end @@ -190,8 +190,10 @@ def cascade? private # For every resource add a 'OPTIONS' route that returns an HTTP 204 response - # with a list of HTTP methods that can be called. - def add_head_and_options_methods + # with a list of HTTP methods that can be called. Also add a route that + # will return an HTTP 405 response for any HTTP method that the resource + # cannot handle. + 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 @@ -217,7 +219,7 @@ def collect_route_config_per_pattern(all_routes) @router.associate_routes(last_route.pattern, { endpoint: last_route.app, - allow_header: allow_header.join(', ').freeze + allow_header: allow_header }) end end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index ce64678c71..6bd72c23c9 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -255,9 +255,10 @@ def run run_filters befores, :before if (allowed_methods = env[Grape::Env::GRAPE_ALLOWED_METHODS]) - raise Grape::Exceptions::MethodNotAllowed.new(header.merge('Allow' => allowed_methods)) unless options? + allow_header_value = allowed_methods.join(', ') + raise Grape::Exceptions::MethodNotAllowed.new(header.merge('Allow' => allow_header_value)) unless options? - header Grape::Http::Headers::ALLOW, allowed_methods + header Grape::Http::Headers::ALLOW, allow_header_value response_object = '' status 204 else diff --git a/lib/grape/middleware/versioner/header.rb b/lib/grape/middleware/versioner/header.rb index 619549304f..11cbfc7bcb 100644 --- a/lib/grape/middleware/versioner/header.rb +++ b/lib/grape/middleware/versioner/header.rb @@ -46,14 +46,10 @@ def match_best_quality_media_type! if media_type yield media_type else - fail!(allowed_methods) + fail! end end - def allowed_methods - env[Grape::Env::GRAPE_ALLOWED_METHODS] - end - def accept_header env[Grape::Http::Headers::HTTP_ACCEPT] end @@ -93,8 +89,8 @@ def invalid_version_header!(message) raise Grape::Exceptions::InvalidVersionHeader.new(message, error_headers) end - def fail!(grape_allowed_methods) - return grape_allowed_methods if grape_allowed_methods.present? + def fail! + return if env[Grape::Env::GRAPE_ALLOWED_METHODS].present? media_types = q_values_mime_types.map { |mime_type| Grape::Util::MediaType.parse(mime_type) } vendor_not_found!(media_types) || version_not_found!(media_types) From 8a183a76d55220c5940ffcef5e7c390d35a10234 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 11 Nov 2024 16:16:41 +0100 Subject: [PATCH 11/11] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ff1642b1..0a15293fb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#2500](https://github.com/ruby-grape/grape/pull/2500): Remove deprecated `file` method - [@ericproulx](https://github.com/ericproulx). * [#2501](https://github.com/ruby-grape/grape/pull/2501): Remove deprecated `except` and `proc` options in values validator - [@ericproulx](https://github.com/ericproulx). * [#2502](https://github.com/ruby-grape/grape/pull/2502): Remove deprecation `options` in `desc` - [@ericproulx](https://github.com/ericproulx). +* [#2512](https://github.com/ruby-grape/grape/pull/2512): Optimize hash alloc - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes