diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 44b3ed850d..74dd51f3c9 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2024-05-10 16:10:58 UTC using RuboCop version 1.63.2. +# on 2024-05-20 14:55:33 UTC using RuboCop version 1.63.2. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -40,16 +40,6 @@ Lint/EmptyClass: Exclude: - 'lib/grape/dsl/parameters.rb' -# Offense count: 5 -# Configuration parameters: AllowedParentClasses. -Lint/MissingSuper: - Exclude: - - 'lib/grape/api/instance.rb' - - 'lib/grape/exceptions/validation_array_errors.rb' - - 'lib/grape/namespace.rb' - - 'lib/grape/path.rb' - - 'lib/grape/router/pattern.rb' - # Offense count: 1 # Configuration parameters: CountComments, Max, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: diff --git a/CHANGELOG.md b/CHANGELOG.md index 60f7a77ea9..9b409b9131 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ * [#2414](https://github.com/ruby-grape/grape/pull/2414): Fix Rack::Lint missing content-type - [@ericproulx](https://github.com/ericproulx). * [#2378](https://github.com/ruby-grape/grape/pull/2378): Do not overwrite `route_param` with a regular one if they share same name - [@arg](https://github.com/arg). * [#2444](https://github.com/ruby-grape/grape/pull/2444): Replace method_missing in endpoint - [@ericproulx](https://github.com/ericproulx). +* [#2441](https://github.com/ruby-grape/grape/pull/2441): Optimize memory alloc and retained - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 2.0.0 (2023/11/11) diff --git a/lib/grape.rb b/lib/grape.rb index 2c99d377a8..963e37364f 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -16,6 +16,7 @@ require 'active_support/core_ext/hash/keys' require 'active_support/core_ext/hash/reverse_merge' require 'active_support/core_ext/hash/slice' +require 'active_support/core_ext/module/delegation' require 'active_support/core_ext/object/blank' require 'active_support/core_ext/object/deep_dup' require 'active_support/core_ext/object/duplicable' @@ -23,6 +24,7 @@ require 'active_support/core_ext/string/exclude' require 'active_support/deprecation' require 'active_support/inflector' +require 'active_support/ordered_options' require 'active_support/notifications' require 'English' diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 8047b4e256..a17c8e63f0 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -125,6 +125,7 @@ def evaluate_as_instance_with_configuration(block, lazy: false) end def inherited(subclass) + super subclass.reset! subclass.logger = logger.clone end @@ -220,7 +221,7 @@ def add_head_not_allowed_methods_and_options_methods def collect_route_config_per_pattern all_routes = self.class.endpoints.map(&:routes).flatten - routes_by_regexp = all_routes.group_by { |route| route.pattern.to_regexp } + 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| diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index a422c34d04..812ddd1d08 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -30,7 +30,7 @@ def version(*args, &block) if args.any? options = args.extract_options! options = options.reverse_merge(using: :path) - requested_versions = args.flatten + requested_versions = args.flatten.map(&:to_s) raise Grape::Exceptions::MissingVendorOption.new if options[:using] == :header && !options.key?(:vendor) @@ -54,7 +54,7 @@ def version(*args, &block) # Define a root URL prefix for your entire API. def prefix(prefix = nil) - namespace_inheritable(:root_prefix, prefix) + namespace_inheritable(:root_prefix, prefix&.to_s) end # Create a scope without affecting the URL. diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index b04f47e1cf..7fc83fc437 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -190,10 +190,11 @@ def prepare_default_route_attributes end def prepare_version - version = namespace_inheritable(:version) || [] + version = namespace_inheritable(:version) + return unless version return if version.empty? - version.length == 1 ? version.first.to_s : version + version.length == 1 ? version.first : version end def merge_route_options(**default) @@ -206,7 +207,7 @@ def map_routes def prepare_path(path) path_settings = inheritable_setting.to_hash[:namespace_stackable].merge(inheritable_setting.to_hash[:namespace_inheritable]) - Path.prepare(path, namespace, path_settings) + Path.new(path, namespace, path_settings) end def namespace diff --git a/lib/grape/exceptions/validation_array_errors.rb b/lib/grape/exceptions/validation_array_errors.rb index d596c88770..d7815b1f6a 100644 --- a/lib/grape/exceptions/validation_array_errors.rb +++ b/lib/grape/exceptions/validation_array_errors.rb @@ -6,6 +6,7 @@ class ValidationArrayErrors < Base attr_reader :errors def initialize(errors) + super() @errors = errors end end diff --git a/lib/grape/namespace.rb b/lib/grape/namespace.rb index 8375e3a561..537e7ff663 100644 --- a/lib/grape/namespace.rb +++ b/lib/grape/namespace.rb @@ -31,13 +31,14 @@ def self.joined_space(settings) # Join the namespaces from a list of settings to create a path prefix. # @param settings [Array] list of Grape::Util::InheritableSettings. def self.joined_space_path(settings) - Grape::Router.normalize_path(JoinedSpaceCache[joined_space(settings)]) + JoinedSpaceCache[joined_space(settings)] end class JoinedSpaceCache < Grape::Util::Cache def initialize + super @cache = Hash.new do |h, joined_space| - h[joined_space] = -joined_space.join('/') + h[joined_space] = Grape::Router.normalize_path(joined_space.join('/')) end end end diff --git a/lib/grape/path.rb b/lib/grape/path.rb index e6fa540e7e..fd05778930 100644 --- a/lib/grape/path.rb +++ b/lib/grape/path.rb @@ -3,10 +3,6 @@ module Grape # Represents a path to an endpoint. class Path - def self.prepare(raw_path, namespace, settings) - Path.new(raw_path, namespace, settings) - end - attr_reader :raw_path, :namespace, :settings def initialize(raw_path, namespace, settings) @@ -20,31 +16,27 @@ def mount_path end def root_prefix - split_setting(:root_prefix) + settings[:root_prefix] end def uses_specific_format? - if settings.key?(:format) && settings.key?(:content_types) - settings[:format] && Array(settings[:content_types]).size == 1 - else - false - end + return false unless settings.key?(:format) && settings.key?(:content_types) + + settings[:format] && Array(settings[:content_types]).size == 1 end def uses_path_versioning? - if settings.key?(:version) && settings[:version_options] && settings[:version_options].key?(:using) - settings[:version] && settings[:version_options][:using] == :path - else - false - end + return false unless settings.key?(:version) && settings[:version_options]&.key?(:using) + + settings[:version] && settings[:version_options][:using] == :path end def namespace? - namespace&.match?(/^\S/) && namespace != '/' + namespace&.match?(/^\S/) && not_slash?(namespace) end def path? - raw_path&.match?(/^\S/) && raw_path != '/' + raw_path&.match?(/^\S/) && not_slash?(raw_path) end def suffix @@ -58,7 +50,7 @@ def suffix end def path - Grape::Router.normalize_path(PartsCache[parts]) + PartsCache[parts] end def path_with_suffix @@ -73,24 +65,29 @@ def to_s class PartsCache < Grape::Util::Cache def initialize + super @cache = Hash.new do |h, parts| - h[parts] = -parts.join('/') + h[parts] = Grape::Router.normalize_path(parts.join('/')) end end end def parts - parts = [mount_path, root_prefix].compact - parts << ':version' if uses_path_versioning? - parts << namespace.to_s - parts << raw_path.to_s - parts.flatten.reject { |part| part == '/' } + [].tap do |parts| + add_part(parts, mount_path) + add_part(parts, root_prefix) + parts << ':version' if uses_path_versioning? + add_part(parts, namespace) + add_part(parts, raw_path) + end end - def split_setting(key) - return if settings[key].nil? + def add_part(parts, value) + parts << value if value && not_slash?(value) + end - settings[key].to_s.split('/') + def not_slash?(value) + value != '/' end end end diff --git a/lib/grape/router.rb b/lib/grape/router.rb index ba02073957..e1adf63bb0 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -12,10 +12,6 @@ def self.normalize_path(path) path end - def self.supported_methods - @supported_methods ||= Grape::Http::Headers::SUPPORTED_METHODS + ['*'] - end - def initialize @neutral_map = [] @neutral_regexes = [] @@ -28,13 +24,12 @@ def compile! @union = Regexp.union(@neutral_regexes) @neutral_regexes = nil - self.class.supported_methods.each do |method| + (Grape::Http::Headers::SUPPORTED_METHODS + ['*']).each do |method| + next unless map.key?(method) + routes = map[method] - @optimized_map[method] = routes.map.with_index do |route, index| - route.index = index - Regexp.new("(?<_#{index}>#{route.pattern.to_regexp})") - end - @optimized_map[method] = Regexp.union(@optimized_map[method]) + optimized_map = routes.map.with_index { |route, index| route.to_regexp(index) } + @optimized_map[method] = Regexp.union(optimized_map) end @compiled = true end @@ -44,8 +39,10 @@ def append(route) end def associate_routes(pattern, **options) - @neutral_regexes << Regexp.new("(?<_#{@neutral_map.length}>)#{pattern.to_regexp}") - @neutral_map << Grape::Router::GreedyRoute.new(pattern: pattern, index: @neutral_map.length, **options) + Grape::Router::GreedyRoute.new(pattern: pattern, **options).then do |greedy_route| + @neutral_regexes << greedy_route.to_regexp(@neutral_map.length) + @neutral_map << greedy_route + end end def call(env) @@ -145,11 +142,11 @@ def default_response end def match?(input, method) - @optimized_map[method].match(input) { |m| @map[method].detect { |route| m["_#{route.index}"] } } + @optimized_map[method].match(input) { |m| @map[method].detect { |route| m[route.regexp_capture_index] } } end def greedy_match?(input) - @union.match(input) { |m| @neutral_map.detect { |route| m["_#{route.index}"] } } + @union.match(input) { |m| @neutral_map.detect { |route| m[route.regexp_capture_index] } } end def call_with_allow_headers(env, route) diff --git a/lib/grape/router/attribute_translator.rb b/lib/grape/router/attribute_translator.rb deleted file mode 100644 index 2197e0efe9..0000000000 --- a/lib/grape/router/attribute_translator.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -module Grape - class Router - # this could be an OpenStruct, but doesn't work in Ruby 2.3.0, see https://bugs.ruby-lang.org/issues/12251 - # fixed >= 3.0 - class AttributeTranslator - ROUTE_ATTRIBUTES = (%i[ - allow_header - anchor - endpoint - format - forward_match - namespace - not_allowed_method - prefix - request_method - requirements - settings - suffix - version - ] | Grape::DSL::Desc::ROUTE_ATTRIBUTES).freeze - - def initialize(**attributes) - @attributes = attributes - end - - ROUTE_ATTRIBUTES.each do |attr| - define_method attr do - @attributes[attr] - end - - define_method(:"#{attr}=") do |val| - @attributes[attr] = val - end - end - - def to_h - @attributes - end - - def method_missing(method_name, *args) - if setter?(method_name) - @attributes[method_name.to_s.chomp('=').to_sym] = args.first - else - @attributes[method_name] - end - end - - def respond_to_missing?(method_name, _include_private = false) - return true if setter?(method_name) - - @attributes.key?(method_name) - end - - private - - def setter?(method_name) - method_name.end_with?('=') - end - end - end -end diff --git a/lib/grape/router/base_route.rb b/lib/grape/router/base_route.rb new file mode 100644 index 0000000000..9d19c87209 --- /dev/null +++ b/lib/grape/router/base_route.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Grape + class Router + class BaseRoute + delegate_missing_to :@options + + attr_reader :index, :pattern, :options + + def initialize(**options) + @options = ActiveSupport::OrderedOptions.new.update(options) + end + + alias attributes options + + def regexp_capture_index + CaptureIndexCache[index] + end + + def pattern_regexp + pattern.to_regexp + end + + def to_regexp(index) + @index = index + Regexp.new("(?<#{regexp_capture_index}>#{pattern_regexp})") + end + + class CaptureIndexCache < Grape::Util::Cache + def initialize + super + @cache = Hash.new do |h, index| + h[index] = "_#{index}" + end + end + end + end + end +end diff --git a/lib/grape/router/greedy_route.rb b/lib/grape/router/greedy_route.rb index 787c1265b6..a999c1b906 100644 --- a/lib/grape/router/greedy_route.rb +++ b/lib/grape/router/greedy_route.rb @@ -5,24 +5,15 @@ module Grape class Router - class GreedyRoute - extend Forwardable - - attr_reader :index, :pattern, :options, :attributes - - # params must be handled in this class to avoid method redefined warning - delegate Grape::Router::AttributeTranslator::ROUTE_ATTRIBUTES - [:params] => :@attributes - - def initialize(index:, pattern:, **options) - @index = index + class GreedyRoute < BaseRoute + def initialize(pattern:, **options) @pattern = pattern - @options = options - @attributes = Grape::Router::AttributeTranslator.new(**options) + super(**options) end # Grape::Router:Route defines params as a function def params(_input = nil) - @attributes.params || {} + options[:params] || {} end end end diff --git a/lib/grape/router/pattern.rb b/lib/grape/router/pattern.rb index 0f646bea90..7b5b5276f0 100644 --- a/lib/grape/router/pattern.rb +++ b/lib/grape/router/pattern.rb @@ -3,9 +3,12 @@ module Grape class Router class Pattern - attr_reader :origin, :path, :pattern, :to_regexp, :captures_default - extend Forwardable + + DEFAULT_CAPTURES = %w[format version].freeze + + attr_reader :origin, :path, :pattern, :to_regexp + def_delegators :pattern, :named_captures, :params def_delegators :to_regexp, :=== alias match? === @@ -15,7 +18,12 @@ def initialize(pattern, **options) @path = build_path(pattern, anchor: options[:anchor], suffix: options[:suffix]) @pattern = build_pattern(@path, options) @to_regexp = @pattern.to_regexp - @captures_default = regex_captures_default(@to_regexp) + end + + def captures_default + to_regexp.names + .delete_if { |n| DEFAULT_CAPTURES.include?(n) } + .to_h { |k| [k, ''] } end private @@ -43,11 +51,6 @@ def extract_capture(**options) options[:requirements].merge(sliced_options) end - def regex_captures_default(regex) - names = regex.names - %w[format version] # remove default format and version - names.to_h { |k| [k, ''] } - end - def build_path_from_pattern(pattern, anchor: false) if pattern.end_with?('*path') pattern.dup.insert(pattern.rindex('/') + 1, '?') @@ -62,6 +65,7 @@ def build_path_from_pattern(pattern, anchor: false) class PatternCache < Grape::Util::Cache def initialize + super @cache = Hash.new do |h, (pattern, suffix)| h[[pattern, suffix]] = -"#{pattern}#{suffix}" end diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index 2ee3bb89fb..335ecb712f 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -2,20 +2,17 @@ module Grape class Router - class Route + class Route < BaseRoute extend Forwardable - attr_reader :app, :pattern, :options, :attributes - attr_accessor :index + attr_reader :app, :request_method def_delegators :pattern, :path, :origin - # params must be handled in this class to avoid method redefined warning - delegate Grape::Router::AttributeTranslator::ROUTE_ATTRIBUTES - [:params] => :attributes def initialize(method, pattern, **options) - @options = options + @request_method = upcase_method(method) @pattern = Grape::Router::Pattern.new(pattern, **options) - @attributes = Grape::Router::AttributeTranslator.new(**options, request_method: upcase_method(method)) + super(**options) end def exec(env) @@ -30,7 +27,7 @@ def apply(app) def match?(input) return false if input.blank? - attributes.forward_match ? input.start_with?(pattern.origin) : pattern.match?(input) + options[:forward_match] ? input.start_with?(pattern.origin) : pattern.match?(input) end def params(input = nil) @@ -50,7 +47,7 @@ def params_without_input def upcase_method(method) method_s = method.to_s - Grape::Http::Headers.find_supported_method(method_s) || method_s.upcase + Grape::Http::Headers::SUPPORTED_METHODS.detect { |m| m.casecmp(method_s).zero? } || method_s.upcase end end end diff --git a/lib/grape/util/base_inheritable.rb b/lib/grape/util/base_inheritable.rb index 5db6e3455d..cc68ababfe 100644 --- a/lib/grape/util/base_inheritable.rb +++ b/lib/grape/util/base_inheritable.rb @@ -26,10 +26,10 @@ def initialize_copy(other) def keys if new_values.any? - combined = inherited_values.keys - combined.concat(new_values.keys) - combined.uniq! - combined + inherited_values.keys.tap do |combined| + combined.concat(new_values.keys) + combined.uniq! + end else inherited_values.keys end diff --git a/lib/grape/util/reverse_stackable_values.rb b/lib/grape/util/reverse_stackable_values.rb index 3b008a926c..43da1ead53 100644 --- a/lib/grape/util/reverse_stackable_values.rb +++ b/lib/grape/util/reverse_stackable_values.rb @@ -8,10 +8,7 @@ class ReverseStackableValues < StackableValues def concat_values(inherited_value, new_value) return inherited_value unless new_value - [].tap do |value| - value.concat(new_value) - value.concat(inherited_value) - end + new_value + inherited_value end end end diff --git a/lib/grape/util/stackable_values.rb b/lib/grape/util/stackable_values.rb index c19e2f5828..64336182c0 100644 --- a/lib/grape/util/stackable_values.rb +++ b/lib/grape/util/stackable_values.rb @@ -29,10 +29,7 @@ def to_hash def concat_values(inherited_value, new_value) return inherited_value unless new_value - [].tap do |value| - value.concat(inherited_value) - value.concat(new_value) - end + inherited_value + new_value end end end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 9e1e28b84f..403b0ef09b 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -209,11 +209,11 @@ def push_renamed_param(path, new_name) def require_required_and_optional_fields(context, opts) if context == :all - optional_fields = Array(opts[:except]) - required_fields = opts[:using].keys - optional_fields + optional_fields = Array.wrap(opts[:except]) + required_fields = opts[:using].keys.delete_if { |f| optional_fields.include?(f) } else # context == :none - required_fields = Array(opts[:except]) - optional_fields = opts[:using].keys - required_fields + required_fields = Array.wrap(opts[:except]) + optional_fields = opts[:using].keys.delete_if { |f| required_fields.include?(f) } end required_fields.each do |field| field_opts = opts[:using][field] @@ -229,7 +229,10 @@ def require_required_and_optional_fields(context, opts) def require_optional_fields(context, opts) optional_fields = opts[:using].keys - optional_fields -= Array(opts[:except]) unless context == :all + unless context == :all + except_fields = Array.wrap(opts[:except]) + optional_fields.delete_if { |f| except_fields.include?(f) } + end optional_fields.each do |field| field_opts = opts[:using][field] optional(field, field_opts) if field_opts diff --git a/spec/grape/path_spec.rb b/spec/grape/path_spec.rb index a222a2c400..afad438df4 100644 --- a/spec/grape/path_spec.rb +++ b/spec/grape/path_spec.rb @@ -49,7 +49,7 @@ module Grape it 'splits the mount path' do path = described_class.new(anything, anything, root_prefix: 'hello/world') - expect(path.root_prefix).to eql(%w[hello world]) + expect(path.root_prefix).to eql('hello/world') end end diff --git a/spec/grape/router/attribute_translator_spec.rb b/spec/grape/router/attribute_translator_spec.rb deleted file mode 100644 index 54e22dd64b..0000000000 --- a/spec/grape/router/attribute_translator_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true - -describe Grape::Router::AttributeTranslator do - described_class::ROUTE_ATTRIBUTES.each do |attribute| - describe "##{attribute}" do - it "returns value from #{attribute} key if present" do - translator = described_class.new(attribute => 'value') - expect(translator.public_send(attribute)).to eq('value') - end - - it "returns nil from #{attribute} key if missing" do - translator = described_class.new - expect(translator.public_send(attribute)).to be_nil - end - end - - describe "##{attribute}=" do - it "sets value for #{attribute}", :aggregate_failures do - translator = described_class.new(attribute => 'value') - expect do - translator.public_send(:"#{attribute}=", 'new_value') - end.to change(translator, attribute).from('value').to('new_value') - end - end - end -end diff --git a/spec/grape/router/greedy_route_spec.rb b/spec/grape/router/greedy_route_spec.rb index add108ac50..29e966280e 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(index: index, pattern: pattern, **options) } + let(:instance) { described_class.new(pattern: pattern, **options) } let(:index) { 0 } let(:pattern) { :pattern } let(:params) do @@ -11,12 +11,6 @@ { params: params }.freeze end - describe '#index' do - subject { instance.index } - - it { is_expected.to eq(index) } - end - describe '#pattern' do subject { instance.pattern } @@ -38,6 +32,6 @@ describe '#attributes' do subject { instance.attributes } - it { is_expected.to be_a(Grape::Router::AttributeTranslator) } + it { is_expected.to eq(options) } end end