From 8a73c7feefa50295a54663dbc22b32de07c504c0 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 26 Oct 2024 17:14:02 +0200 Subject: [PATCH 1/5] Grape::Path is now exposing only path and suffix Last parameter is now double splatted --- lib/grape/endpoint.rb | 2 +- lib/grape/path.rb | 95 ++++++++++------------- spec/grape/path_spec.rb | 166 ++-------------------------------------- 3 files changed, 47 insertions(+), 216 deletions(-) diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 5fdb03567..94cf1eb90 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -208,7 +208,7 @@ def prepare_path(path) namespace_stackable_hash = inheritable_setting.namespace_stackable.to_hash namespace_inheritable_hash = inheritable_setting.namespace_inheritable.to_hash path_settings = namespace_stackable_hash.merge!(namespace_inheritable_hash) - Path.new(path, namespace, path_settings) + Path.new(path, namespace, **path_settings) end def namespace diff --git a/lib/grape/path.rb b/lib/grape/path.rb index fd0577893..13eec8cd0 100644 --- a/lib/grape/path.rb +++ b/lib/grape/path.rb @@ -3,65 +3,66 @@ module Grape # Represents a path to an endpoint. class Path - attr_reader :raw_path, :namespace, :settings + DEFAULT_FORMAT_SEGMENT = '(/.:format)' + NO_VERSIONING_WITH_VALID_PATH_FORMAT_SEGMENT = '(.:format)' + VERSION_SEGMENT = ':version' - def initialize(raw_path, namespace, settings) - @raw_path = raw_path - @namespace = namespace - @settings = settings - end + attr_reader :path, :suffix - def mount_path - settings[:mount_path] + def initialize(raw_path, raw_namespace, **settings) + @path = PartsCache[build_parts(raw_path, raw_namespace, settings)] + @suffix = build_suffix(raw_path, raw_namespace, settings) end - def root_prefix - settings[:root_prefix] + def to_s + "#{path}#{suffix}" end - def uses_specific_format? - return false unless settings.key?(:format) && settings.key?(:content_types) + private - settings[:format] && Array(settings[:content_types]).size == 1 + def build_suffix(raw_path, raw_namespace, settings) + if uses_specific_format?(settings) + "(.#{settings[:format]})" + elsif !uses_path_versioning?(settings) || (valid_part?(raw_namespace) || valid_part?(raw_path)) + NO_VERSIONING_WITH_VALID_PATH_FORMAT_SEGMENT + else + DEFAULT_FORMAT_SEGMENT + end end - def uses_path_versioning? - return false unless settings.key?(:version) && settings[:version_options]&.key?(:using) - - settings[:version] && settings[:version_options][:using] == :path + def build_parts(raw_path, raw_namespace, settings) + [].tap do |parts| + add_part(parts, settings[:mount_path]) + add_part(parts, settings[:root_prefix]) + parts << VERSION_SEGMENT if uses_path_versioning?(settings) + add_part(parts, raw_namespace) + add_part(parts, raw_path) + end end - def namespace? - namespace&.match?(/^\S/) && not_slash?(namespace) + def add_part(parts, value) + parts << value if value && not_slash?(value) end - def path? - raw_path&.match?(/^\S/) && not_slash?(raw_path) + def not_slash?(value) + value != '/' end - def suffix - if uses_specific_format? - "(.#{settings[:format]})" - elsif !uses_path_versioning? || (namespace? || path?) - '(.:format)' - else - '(/.:format)' - end - end + def uses_specific_format?(settings) + return false unless settings.key?(:format) && settings.key?(:content_types) - def path - PartsCache[parts] + settings[:format] && Array(settings[:content_types]).size == 1 end - def path_with_suffix - "#{path}#{suffix}" - end + def uses_path_versioning?(settings) + return false unless settings.key?(:version) && settings[:version_options]&.key?(:using) - def to_s - path_with_suffix + settings[:version] && settings[:version_options][:using] == :path end - private + def valid_part?(part) + part&.match?(/^\S/) && not_slash?(part) + end class PartsCache < Grape::Util::Cache def initialize @@ -71,23 +72,5 @@ def initialize end end end - - def parts - [].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 add_part(parts, value) - parts << value if value && not_slash?(value) - end - - def not_slash?(value) - value != '/' - end end end diff --git a/spec/grape/path_spec.rb b/spec/grape/path_spec.rb index 2a6dd2fce..2f5da8911 100644 --- a/spec/grape/path_spec.rb +++ b/spec/grape/path_spec.rb @@ -1,130 +1,6 @@ # frozen_string_literal: true describe Grape::Path do - describe '#initialize' do - it 'remembers the path' do - path = described_class.new('/:id', anything, anything) - expect(path.raw_path).to eql('/:id') - end - - it 'remembers the namespace' do - path = described_class.new(anything, '/users', anything) - expect(path.namespace).to eql('/users') - end - - it 'remebers the settings' do - path = described_class.new(anything, anything, foo: 'bar') - expect(path.settings).to eql(foo: 'bar') - end - end - - describe '#mount_path' do - it 'is nil when no mount path setting exists' do - path = described_class.new(anything, anything, {}) - expect(path.mount_path).to be_nil - end - - it 'is nil when the mount path is nil' do - path = described_class.new(anything, anything, mount_path: nil) - expect(path.mount_path).to be_nil - end - - it 'splits the mount path' do - path = described_class.new(anything, anything, mount_path: %w[foo bar]) - expect(path.mount_path).to eql(%w[foo bar]) - end - end - - describe '#root_prefix' do - it 'is nil when no root prefix setting exists' do - path = described_class.new(anything, anything, {}) - expect(path.root_prefix).to be_nil - end - - it 'is nil when the mount path is nil' do - path = described_class.new(anything, anything, root_prefix: nil) - expect(path.root_prefix).to be_nil - end - - it 'splits the mount path' do - path = described_class.new(anything, anything, root_prefix: 'hello/world') - expect(path.root_prefix).to eql('hello/world') - end - end - - describe '#uses_path_versioning?' do - it 'is false when the version setting is nil' do - path = described_class.new(anything, anything, version: nil) - expect(path.uses_path_versioning?).to be false - end - - it 'is false when the version option is header' do - path = described_class.new( - anything, - anything, - version: 'v1', - version_options: { using: :header } - ) - - expect(path.uses_path_versioning?).to be false - end - - it 'is true when the version option is path' do - path = described_class.new( - anything, - anything, - version: 'v1', - version_options: { using: :path } - ) - - expect(path.uses_path_versioning?).to be true - end - end - - describe '#namespace?' do - it 'is false when the namespace is nil' do - path = described_class.new(anything, nil, anything) - expect(path).not_to be_namespace - end - - it 'is false when the namespace starts with whitespace' do - path = described_class.new(anything, ' /foo', anything) - expect(path).not_to be_namespace - end - - it 'is false when the namespace is the root path' do - path = described_class.new(anything, '/', anything) - expect(path.namespace?).to be false - end - - it 'is true otherwise' do - path = described_class.new(anything, '/world', anything) - expect(path.namespace?).to be true - end - end - - describe '#path?' do - it 'is false when the path is nil' do - path = described_class.new(nil, anything, anything) - expect(path).not_to be_path - end - - it 'is false when the path starts with whitespace' do - path = described_class.new(' /foo', anything, anything) - expect(path).not_to be_path - end - - it 'is false when the path is the root path' do - path = described_class.new('/', anything, anything) - expect(path.path?).to be false - end - - it 'is true otherwise' do - path = described_class.new('/hello', anything, anything) - expect(path.path?).to be true - end - end - describe '#path' do context 'mount_path' do it 'is not included when it is nil' do @@ -133,14 +9,14 @@ end it 'is included when it is not nil' do - path = described_class.new(nil, nil, {}) + path = described_class.new(nil, nil) expect(path.path).to eql('/') end end context 'root_prefix' do it 'is not included when it is nil' do - path = described_class.new(nil, nil, {}) + path = described_class.new(nil, nil) expect(path.path).to eql('/') end @@ -182,61 +58,33 @@ describe '#suffix' do context 'when using a specific format' do it 'accepts specified format' do - path = described_class.new(nil, nil, {}) - allow(path).to receive_messages(uses_specific_format?: true, settings: { format: :json }) - + path = described_class.new(nil, nil, format: 'json', content_types: 'application/json') expect(path.suffix).to eql('(.json)') end end context 'when path versioning is used' do it "includes a '/'" do - path = described_class.new(nil, nil, {}) - allow(path).to receive_messages(uses_specific_format?: false, uses_path_versioning?: true) - + path = described_class.new(nil, nil, version: :v1, version_options: { using: :path }) expect(path.suffix).to eql('(/.:format)') end end context 'when path versioning is not used' do it "does not include a '/' when the path has a namespace" do - path = described_class.new(nil, 'namespace', {}) - allow(path).to receive_messages(uses_specific_format?: false, uses_path_versioning?: true) - + path = described_class.new(nil, 'namespace') expect(path.suffix).to eql('(.:format)') end it "does not include a '/' when the path has a path" do - path = described_class.new('/path', nil, {}) - allow(path).to receive_messages(uses_specific_format?: false, uses_path_versioning?: true) - + path = described_class.new('/path', nil, version: :v1, version_options: { using: :path }) expect(path.suffix).to eql('(.:format)') end it "includes a '/' otherwise" do - path = described_class.new(nil, nil, {}) - allow(path).to receive_messages(uses_specific_format?: false, uses_path_versioning?: true) - + path = described_class.new(nil, nil, version: :v1, version_options: { using: :path }) expect(path.suffix).to eql('(/.:format)') end end end - - describe '#path_with_suffix' do - it 'combines the path and suffix' do - path = described_class.new(nil, nil, {}) - allow(path).to receive_messages(path: '/the/path', suffix: 'suffix') - - expect(path.path_with_suffix).to eql('/the/pathsuffix') - end - - context 'when using a specific format' do - it 'might have a suffix with specified format' do - path = described_class.new(nil, nil, {}) - allow(path).to receive_messages(path: '/the/path', uses_specific_format?: true, settings: { format: :json }) - - expect(path.path_with_suffix).to eql('/the/path(.json)') - end - end - end end From feb56bec49472ee329c2b4416007ff06a501c532 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 26 Oct 2024 19:23:55 +0200 Subject: [PATCH 2/5] Remove double splat operator --- lib/grape/endpoint.rb | 2 +- lib/grape/path.rb | 2 +- spec/grape/path_spec.rb | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 94cf1eb90..5fdb03567 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -208,7 +208,7 @@ def prepare_path(path) namespace_stackable_hash = inheritable_setting.namespace_stackable.to_hash namespace_inheritable_hash = inheritable_setting.namespace_inheritable.to_hash path_settings = namespace_stackable_hash.merge!(namespace_inheritable_hash) - Path.new(path, namespace, **path_settings) + Path.new(path, namespace, path_settings) end def namespace diff --git a/lib/grape/path.rb b/lib/grape/path.rb index 13eec8cd0..72f50aefb 100644 --- a/lib/grape/path.rb +++ b/lib/grape/path.rb @@ -9,7 +9,7 @@ class Path attr_reader :path, :suffix - def initialize(raw_path, raw_namespace, **settings) + def initialize(raw_path, raw_namespace, settings) @path = PartsCache[build_parts(raw_path, raw_namespace, settings)] @suffix = build_suffix(raw_path, raw_namespace, settings) end diff --git a/spec/grape/path_spec.rb b/spec/grape/path_spec.rb index 2f5da8911..8753ac911 100644 --- a/spec/grape/path_spec.rb +++ b/spec/grape/path_spec.rb @@ -9,14 +9,14 @@ end it 'is included when it is not nil' do - path = described_class.new(nil, nil) + path = described_class.new(nil, nil, {}) expect(path.path).to eql('/') end end context 'root_prefix' do it 'is not included when it is nil' do - path = described_class.new(nil, nil) + path = described_class.new(nil, nil, {}) expect(path.path).to eql('/') end @@ -72,7 +72,7 @@ context 'when path versioning is not used' do it "does not include a '/' when the path has a namespace" do - path = described_class.new(nil, 'namespace') + path = described_class.new(nil, 'namespace', {}) expect(path.suffix).to eql('(.:format)') end From b3c5fd7a0841ec68fb755f2be17bfb97cf29eaa5 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 24 Nov 2024 20:49:21 -0500 Subject: [PATCH 3/5] Optimize default path settings Use const static function for route match? --- benchmark/compile_many_routes.rb | 9 +++---- lib/grape/endpoint.rb | 18 ++++++------- lib/grape/path.rb | 6 ++--- lib/grape/router/pattern.rb | 43 ++++++++++++++++++-------------- lib/grape/router/route.rb | 12 ++++++--- spec/grape/path_spec.rb | 14 +++++------ 6 files changed, 55 insertions(+), 47 deletions(-) diff --git a/benchmark/compile_many_routes.rb b/benchmark/compile_many_routes.rb index 1b273302f..acd98c5c8 100644 --- a/benchmark/compile_many_routes.rb +++ b/benchmark/compile_many_routes.rb @@ -3,6 +3,7 @@ $LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib')) require 'grape' require 'benchmark/ips' +require 'memory_profiler' class API < Grape::API prefix :api @@ -15,8 +16,6 @@ class API < Grape::API 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: 'optimize_path.txt') diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 6bd72c23c..4fb0594d9 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -160,12 +160,13 @@ def mount_in(router) end def to_routes - route_options = prepare_default_route_attributes - map_routes do |method, path| - path = prepare_path(path) - route_options[:suffix] = path.suffix - params = options[:route_options].merge(route_options) - route = Grape::Router::Route.new(method, path.path, params) + default_route_options = prepare_default_route_attributes + default_path_settings = prepare_default_path_settings + + map_routes do |method, raw_path| + prepared_path = Path.new(raw_path, namespace, 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 @@ -200,11 +201,10 @@ def map_routes options[:method].map { |method| options[:path].map { |path| yield method, path } } end - def prepare_path(path) + def prepare_default_path_settings namespace_stackable_hash = inheritable_setting.namespace_stackable.to_hash namespace_inheritable_hash = inheritable_setting.namespace_inheritable.to_hash - path_settings = namespace_stackable_hash.merge!(namespace_inheritable_hash) - Path.new(path, namespace, path_settings) + namespace_stackable_hash.merge!(namespace_inheritable_hash) end def namespace diff --git a/lib/grape/path.rb b/lib/grape/path.rb index 72f50aefb..b4e2570de 100644 --- a/lib/grape/path.rb +++ b/lib/grape/path.rb @@ -7,15 +7,15 @@ class Path NO_VERSIONING_WITH_VALID_PATH_FORMAT_SEGMENT = '(.:format)' VERSION_SEGMENT = ':version' - attr_reader :path, :suffix + attr_reader :origin, :suffix def initialize(raw_path, raw_namespace, settings) - @path = PartsCache[build_parts(raw_path, raw_namespace, settings)] + @origin = PartsCache[build_parts(raw_path, raw_namespace, settings)] @suffix = build_suffix(raw_path, raw_namespace, settings) end def to_s - "#{path}#{suffix}" + "#{origin}#{suffix}" end private diff --git a/lib/grape/router/pattern.rb b/lib/grape/router/pattern.rb index 5761b1ea1..4529a9271 100644 --- a/lib/grape/router/pattern.rb +++ b/lib/grape/router/pattern.rb @@ -9,14 +9,14 @@ class Pattern attr_reader :origin, :path, :pattern, :to_regexp - def_delegators :pattern, :named_captures, :params + def_delegators :pattern, :params def_delegators :to_regexp, :=== alias match? === - def initialize(pattern, options) - @origin = pattern - @path = build_path(pattern, options) - @pattern = build_pattern(@path, options) + def initialize(origin, suffix, options) + @origin = origin + @path = build_path(origin, options[:anchor], suffix) + @pattern = build_pattern(@path, options[:params], options[:format], options[:version], options[:requirements]) @to_regexp = @pattern.to_regexp end @@ -28,33 +28,34 @@ def captures_default private - def build_pattern(path, options) + def build_pattern(path, params, format, version, requirements) Mustermann::Grape.new( path, uri_decode: true, - params: options[:params], - capture: extract_capture(options) + params: params, + capture: extract_capture(format, version, requirements) ) end - def build_path(pattern, options) - PatternCache[[build_path_from_pattern(pattern, options), options[:suffix]]] + def build_path(pattern, anchor, suffix) + PatternCache[[build_path_from_pattern(pattern, anchor), suffix]] end - def extract_capture(options) - sliced_options = options - .slice(:format, :version) - .delete_if { |_k, v| v.blank? } - .transform_values { |v| Array.wrap(v).map(&:to_s) } - return sliced_options if options[:requirements].blank? + 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 + + return capture if requirements.blank? - options[:requirements].merge(sliced_options) + requirements.merge(capture) end - def build_path_from_pattern(pattern, options) + def build_path_from_pattern(pattern, anchor) if pattern.end_with?('*path') pattern.dup.insert(pattern.rindex('/') + 1, '?') - elsif options[:anchor] + elsif anchor pattern elsif pattern.end_with?('/') "#{pattern}?*path" @@ -63,6 +64,10 @@ def build_path_from_pattern(pattern, options) end end + def map_str(value) + Array.wrap(value).map(&:to_s) + end + class PatternCache < Grape::Util::Cache def initialize super diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index 244b0a407..48599610c 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -5,13 +5,17 @@ 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 - def initialize(method, pattern, options) + def initialize(method, origin, path, options) @request_method = upcase_method(method) - @pattern = Grape::Router::Pattern.new(pattern, options) + @pattern = Grape::Router::Pattern.new(origin, path, options) + @match_function = options[:forward_match] ? FORWARD_MATCH_METHOD : NON_FORWARD_MATCH_METHOD super(options) end @@ -31,7 +35,7 @@ def apply(app) def match?(input) return false if input.blank? - options[:forward_match] ? input.start_with?(pattern.origin) : pattern.match?(input) + @match_function.call(input, pattern) end def params(input = nil) @@ -46,7 +50,7 @@ def params(input = nil) private def params_without_input - pattern.captures_default.merge(attributes.params) + @params_without_input ||= pattern.captures_default.merge(attributes.params) end def upcase_method(method) diff --git a/spec/grape/path_spec.rb b/spec/grape/path_spec.rb index 8753ac911..9b9b2ed26 100644 --- a/spec/grape/path_spec.rb +++ b/spec/grape/path_spec.rb @@ -1,23 +1,23 @@ # frozen_string_literal: true describe Grape::Path do - describe '#path' do + describe '#origin' do context 'mount_path' do it 'is not included when it is nil' do path = described_class.new(nil, nil, mount_path: '/foo/bar') - expect(path.path).to eql '/foo/bar' + expect(path.origin).to eql '/foo/bar' end it 'is included when it is not nil' do path = described_class.new(nil, nil, {}) - expect(path.path).to eql('/') + expect(path.origin).to eql('/') end end context 'root_prefix' do it 'is not included when it is nil' do path = described_class.new(nil, nil, {}) - expect(path.path).to eql('/') + expect(path.origin).to eql('/') end it 'is included after the mount path' do @@ -28,7 +28,7 @@ root_prefix: '/hello' ) - expect(path.path).to eql('/foo/hello') + expect(path.origin).to eql('/foo/hello') end end @@ -40,7 +40,7 @@ root_prefix: '/hello' ) - expect(path.path).to eql('/foo/hello/namespace') + expect(path.origin).to eql('/foo/hello/namespace') end it 'uses the raw path after the namespace' do @@ -51,7 +51,7 @@ root_prefix: '/hello' ) - expect(path.path).to eql('/foo/hello/namespace/raw_path') + expect(path.origin).to eql('/foo/hello/namespace/raw_path') end end From 1e048a6f6e200f3f924dcb1f91362b321eae7d11 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sun, 24 Nov 2024 21:04:08 -0500 Subject: [PATCH 4/5] Revert compile_many_routes.rb --- benchmark/compile_many_routes.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/benchmark/compile_many_routes.rb b/benchmark/compile_many_routes.rb index acd98c5c8..1b273302f 100644 --- a/benchmark/compile_many_routes.rb +++ b/benchmark/compile_many_routes.rb @@ -3,7 +3,6 @@ $LOAD_PATH.unshift(File.join(File.dirname(__FILE__), '..', 'lib')) require 'grape' require 'benchmark/ips' -require 'memory_profiler' class API < Grape::API prefix :api @@ -16,6 +15,8 @@ class API < Grape::API end end -MemoryProfiler.report(allow_files: 'grape') do - API.compile! -end.pretty_print(to_file: 'optimize_path.txt') +Benchmark.ips do |ips| + ips.report('Compiling 2000 routes') do + API.compile! + end +end From de439f4da63f7c7e431a1f71a2fcd77eae037976 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 30 Nov 2024 09:48:39 -0500 Subject: [PATCH 5/5] Add CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a15293fb..d758f3a77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [#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). +* [#2513](https://github.com/ruby-grape/grape/pull/2513): Optimize Grape::Path - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes