diff --git a/CHANGELOG.md b/CHANGELOG.md index 28768be28..f2a5b6ce5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ * [#2615](https://github.com/ruby-grape/grape/pull/2615): Remove manual toc and tod danger check - [@alexanderadam](https://github.com/alexanderadam). * [#2612](https://github.com/ruby-grape/grape/pull/2612): Avoid multiple mount pollution - [@alexanderadam](https://github.com/alexanderadam). * [#2617](https://github.com/ruby-grape/grape/pull/2617): Migrate from `ActiveSupport::Configurable` to `Dry::Configurable` - [@ericproulx](https://github.com/ericproulx). +* [#2618](https://github.com/ruby-grape/grape/pull/2618): Modernize argument delegation for Ruby 3+ compatibility - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index 5f13d7bc3..e84952924 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,18 @@ Upgrading Grape ### Upgrading to >= 3.0.0 +#### Ruby 3+ Argument Delegation Modernization + +Grape has been modernized to use Ruby 3+'s preferred argument delegation patterns. This change replaces `args.extract_options!` with explicit `**kwargs` parameters throughout the codebase. + +- All DSL methods now use explicit keyword arguments (`**kwargs`) instead of extracting options from mixed argument lists +- Method signatures are now more explicit and follow Ruby 3+ best practices +- The `active_support/core_ext/array/extract_options` dependency has been removed + +This is a modernization effort that improves code quality while maintaining full backward compatibility. + +See [#2618](https://github.com/ruby-grape/grape/pull/2618) for more information. + #### Configuration API Migration from ActiveSupport::Configurable to Dry::Configurable Grape has migrated from `ActiveSupport::Configurable` to `Dry::Configurable` for its configuration system since its [deprecated](https://github.com/rails/rails/blob/1cdd190a25e483b65f1f25bbd0f13a25d696b461/activesupport/lib/active_support/configurable.rb#L3-L7). diff --git a/lib/grape.rb b/lib/grape.rb index aac1e48a5..0015d67eb 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -6,11 +6,11 @@ require 'active_support/version' require 'active_support/isolated_execution_state' require 'active_support/core_ext/array/conversions' -require 'active_support/core_ext/array/extract_options' require 'active_support/core_ext/array/wrap' require 'active_support/core_ext/enumerable' require 'active_support/core_ext/hash/conversions' require 'active_support/core_ext/hash/deep_merge' +require 'active_support/core_ext/hash/deep_transform_values' require 'active_support/core_ext/hash/except' require 'active_support/core_ext/hash/indifferent_access' require 'active_support/core_ext/hash/keys' diff --git a/lib/grape/api.rb b/lib/grape/api.rb index d384635be..17a998861 100644 --- a/lib/grape/api.rb +++ b/lib/grape/api.rb @@ -30,15 +30,6 @@ class << self # NOTE: This will only be called on an API directly mounted on RACK def_delegators :base_instance, :new, :configuration, :call, :compile! - # When inherited, will create a list of all instances (times the API was mounted) - # It will listen to the setup required to mount that endpoint, and replicate it on any new instance - def inherited(api) - super - - api.initial_setup(self == Grape::API ? Grape::API::Instance : @base_instance) - api.override_all_methods! - end - # Initialize the instance variables on the remountable class, and the base_instance # an instance that will be used to create the set up but will not be mounted def initial_setup(base_instance_parent) @@ -51,8 +42,8 @@ def initial_setup(base_instance_parent) # Redefines all methods so that are forwarded to add_setup and be recorded def override_all_methods! (base_instance.methods - Class.methods - NON_OVERRIDABLE).each do |method_override| - define_singleton_method(method_override) do |*args, &block| - add_setup(method: method_override, args: args, block: block) + define_singleton_method(method_override) do |*args, **kwargs, &block| + add_setup(method: method_override, args: args, kwargs: kwargs, block: block) end end end @@ -86,6 +77,15 @@ def mount_instance(configuration: nil) private + # When inherited, will create a list of all instances (times the API was mounted) + # It will listen to the setup required to mount that endpoint, and replicate it on any new instance + def inherited(api) + super + + api.initial_setup(self == Grape::API ? Grape::API::Instance : @base_instance) + api.override_all_methods! + end + # Replays the set up to produce an API as defined in this class, can be called # on classes that inherit from Grape::API def replay_setup_on(instance) @@ -95,7 +95,7 @@ def replay_setup_on(instance) end # Adds a new stage to the set up require to get a Grape::API up and running - def add_setup(step) + def add_setup(**step) @setup << step last_response = nil @instances.each do |instance| @@ -119,12 +119,13 @@ def refresh_mount_step end end - def replay_step_on(instance, method:, args:, block:) - return if skip_immediate_run?(instance, args) + def replay_step_on(instance, method:, args:, kwargs:, block:) + return if skip_immediate_run?(instance, args, kwargs) eval_args = evaluate_arguments(instance.configuration, *args) - response = instance.__send__(method, *eval_args, &block) - if skip_immediate_run?(instance, [response]) + eval_kwargs = kwargs.deep_transform_values { |v| evaluate_arguments(instance.configuration, v).first } + response = instance.__send__(method, *eval_args, **eval_kwargs, &block) + if skip_immediate_run?(instance, [response], kwargs) response else evaluate_arguments(instance.configuration, response).first @@ -132,9 +133,9 @@ def replay_step_on(instance, method:, args:, block:) end # Skips steps that contain arguments to be lazily executed (on re-mount time) - def skip_immediate_run?(instance, args) + def skip_immediate_run?(instance, args, kwargs) instance.base_instance? && - (any_lazy?(args) || args.any? { |arg| arg.is_a?(Hash) && any_lazy?(arg.values) }) + (any_lazy?(args) || args.any? { |arg| arg.is_a?(Hash) && any_lazy?(arg.values) } || any_lazy?(kwargs.values)) end def any_lazy?(args) diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index da091ced3..be79d45b5 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -61,11 +61,6 @@ def compile @instance ||= new # rubocop:disable Naming/MemoizedInstanceVariableName end - # Wipe the compiled API so we can recompile after changes were made. - def change! - @instance = nil - end - # This is the interface point between Rack and Grape; it accepts a request # from Rack and ultimately returns an array of three values: the status, # the headers, and the body. See [the rack specification] @@ -101,12 +96,6 @@ def recognize_path(path) protected - def inherited(subclass) - super - subclass.reset! - subclass.logger logger.clone - end - def inherit_settings(other_settings) top_level_setting.inherit_from other_settings.point_in_time_copy @@ -119,6 +108,19 @@ def inherit_settings(other_settings) reset_routes! end + + # Wipe the compiled API so we can recompile after changes were made. + def change! + @instance = nil + end + + private + + def inherited(subclass) + super + subclass.reset! + subclass.logger logger.clone + end end attr_reader :router diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index 1e0fd27e0..2a8e8a512 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -337,8 +337,7 @@ def stream(value = nil) # with: API::Entities::User, # admin: current_user.admin? # end - def present(*args) - options = args.count > 1 ? args.extract_options! : {} + def present(*args, **options) key, object = if args.count == 2 && args.first.is_a?(Symbol) args else diff --git a/lib/grape/dsl/parameters.rb b/lib/grape/dsl/parameters.rb index b4e6154da..0041d850d 100644 --- a/lib/grape/dsl/parameters.rb +++ b/lib/grape/dsl/parameters.rb @@ -53,9 +53,8 @@ def build_with(build_with) # Collection.page(params[:page]).per(params[:per_page]) # end # end - def use(*names) + def use(*names, **options) named_params = @api.inheritable_setting.namespace_stackable_with_hash(:named_params) || {} - options = names.extract_options! names.each do |name| params_block = named_params.fetch(name) do raise "Params :#{name} not found!" @@ -123,10 +122,7 @@ def use(*names) # requires :name, type: String # end # end - def requires(*attrs, &block) - orig_attrs = attrs.clone - - opts = attrs.extract_options!.clone + def requires(*attrs, **opts, &block) opts[:presence] = { value: true, message: opts[:message] } opts = @group.deep_merge(opts) if instance_variable_defined?(:@group) && @group @@ -134,7 +130,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(attrs, opts, &block) : push_declared_params(attrs, opts.slice(:as)) end end @@ -142,10 +138,7 @@ def requires(*attrs, &block) # endpoint. # @param (see #requires) # @option (see #requires) - def optional(*attrs, &block) - orig_attrs = attrs.clone - - opts = attrs.extract_options!.clone + def optional(*attrs, **opts, &block) type = opts[:type] opts = @group.deep_merge(opts) if instance_variable_defined?(:@group) && @group @@ -160,7 +153,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(attrs, opts, true, &block) : push_declared_params(attrs, opts.slice(:as)) end end diff --git a/lib/grape/dsl/request_response.rb b/lib/grape/dsl/request_response.rb index 117ee4179..8956db2f3 100644 --- a/lib/grape/dsl/request_response.rb +++ b/lib/grape/dsl/request_response.rb @@ -93,14 +93,13 @@ def default_error_status(new_status = nil) # @option options [Boolean] :rescue_subclasses Also rescue subclasses of exception classes # @param [Proc] handler Execution proc to handle the given exception as an # alternative to passing a block. - def rescue_from(*args, &block) + def rescue_from(*args, **options, &block) if args.last.is_a?(Proc) handler = args.pop elsif block handler = block end - options = args.extract_options! raise ArgumentError, 'both :with option and block cannot be passed' if block && options.key?(:with) handler ||= extract_with(options) diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 9017eb23e..b877bcbfa 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -22,9 +22,8 @@ module Routing # end # end # - def version(*args, &block) + def version(*args, **options, &block) if args.any? - options = args.extract_options! options = options.reverse_merge(using: :path) requested_versions = args.flatten.map(&:to_s) @@ -165,8 +164,7 @@ def route(methods, paths = ['/'], route_options = {}, &block) end Grape::HTTP_SUPPORTED_METHODS.each do |supported_method| - define_method supported_method.downcase do |*args, &block| - options = args.extract_options! + define_method supported_method.downcase do |*args, **options, &block| paths = args.first || ['/'] route(supported_method, paths, options, &block) end diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index f2cf223c6..913a5846d 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -58,6 +58,8 @@ def format_structured_message(_structured_message) raise NotImplementedError end + private + def inherited(klass) super ErrorFormatter.register(klass) diff --git a/lib/grape/params_builder/base.rb b/lib/grape/params_builder/base.rb index 49f583d74..57523fe87 100644 --- a/lib/grape/params_builder/base.rb +++ b/lib/grape/params_builder/base.rb @@ -8,6 +8,8 @@ def call(_params) raise NotImplementedError end + private + def inherited(klass) super ParamsBuilder.register(klass) diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index 78478bafc..7f4ba7931 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -218,22 +218,25 @@ def push_renamed_param(path, new_name) end def require_required_and_optional_fields(context, opts) + except_fields = Array.wrap(opts[:except]) + using_fields = opts[:using].keys.delete_if { |f| except_fields.include?(f) } + if context == :all - optional_fields = Array.wrap(opts[:except]) - required_fields = opts[:using].keys.delete_if { |f| optional_fields.include?(f) } + optional_fields = except_fields + required_fields = using_fields else # context == :none - required_fields = Array.wrap(opts[:except]) - optional_fields = opts[:using].keys.delete_if { |f| required_fields.include?(f) } + required_fields = except_fields + optional_fields = using_fields end required_fields.each do |field| field_opts = opts[:using][field] raise ArgumentError, "required field not exist: #{field}" unless field_opts - requires(field, field_opts) + requires(field, **field_opts) end optional_fields.each do |field| field_opts = opts[:using][field] - optional(field, field_opts) if field_opts + optional(field, **field_opts) if field_opts end end @@ -245,7 +248,7 @@ def require_optional_fields(context, opts) end optional_fields.each do |field| field_opts = opts[:using][field] - optional(field, field_opts) if field_opts + optional(field, **field_opts) if field_opts end end @@ -262,9 +265,9 @@ def validate_attributes(attrs, opts, &block) # @param optional [Boolean] whether the parameter this are nested under # is optional or not (and hence, whether this block's params will be). # @yield parameter scope - def new_scope(attrs, optional = false, &block) + def new_scope(attrs, opts, optional = false, &block) # if required params are grouped and no type or unsupported type is provided, raise an error - type = attrs[1] ? attrs[1][:type] : nil + type = opts[:type] if attrs.first && !optional raise Grape::Exceptions::MissingGroupType if type.nil? raise Grape::Exceptions::UnsupportedGroupType unless Grape::Validations::Types.group?(type) @@ -273,7 +276,7 @@ def new_scope(attrs, optional = false, &block) self.class.new( api: @api, element: attrs.first, - element_renamed: attrs[1][:as], + element_renamed: opts[:as], parent: self, optional: optional, type: type || Array, diff --git a/spec/grape/dsl/parameters_spec.rb b/spec/grape/dsl/parameters_spec.rb index 065811a82..cd1795b39 100644 --- a/spec/grape/dsl/parameters_spec.rb +++ b/spec/grape/dsl/parameters_spec.rb @@ -36,7 +36,7 @@ def validates_reader @validates end - def new_scope(args, _, &block) + def new_scope(args, _opts, _, &block) nested_scope = self.class.new nested_scope.new_group_scope(args, &block) nested_scope @@ -71,7 +71,7 @@ def extract_message_option(attrs) subject.api = Class.new { include Grape::DSL::Settings }.new subject.api.inheritable_setting.namespace_stackable[:named_params] = named_params expect(subject).to receive(:instance_exec).with(options).and_yield - subject.use :params_group, options + subject.use :params_group, **options end it 'raises error when non-existent named param is called' do diff --git a/spec/grape/dsl/routing_spec.rb b/spec/grape/dsl/routing_spec.rb index 7ae1f02eb..a24b55178 100644 --- a/spec/grape/dsl/routing_spec.rb +++ b/spec/grape/dsl/routing_spec.rb @@ -145,49 +145,49 @@ class << self describe '.get' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::GET, path, options) - subject.get path, options, &proc + subject.get path, **options, &proc end end describe '.post' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::POST, path, options) - subject.post path, options, &proc + subject.post path, **options, &proc end end describe '.put' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::PUT, path, options) - subject.put path, options, &proc + subject.put path, **options, &proc end end describe '.head' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::HEAD, path, options) - subject.head path, options, &proc + subject.head path, **options, &proc end end describe '.delete' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::DELETE, path, options) - subject.delete path, options, &proc + subject.delete path, **options, &proc end end describe '.options' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::OPTIONS, path, options) - subject.options path, options, &proc + subject.options path, **options, &proc end end describe '.patch' do it 'delegates to .route' do expect(subject).to receive(:route).with(Rack::PATCH, path, options) - subject.patch path, options, &proc + subject.patch path, **options, &proc end end diff --git a/spec/grape/validations/validators/except_values_validator_spec.rb b/spec/grape/validations/validators/except_values_validator_spec.rb index d72d0092d..11b15ccff 100644 --- a/spec/grape/validations/validators/except_values_validator_spec.rb +++ b/spec/grape/validations/validators/except_values_validator_spec.rb @@ -165,8 +165,8 @@ Class.new(Grape::API) do default_format :json params do - requires :type, param_def[:requires] if param_def.key? :requires - optional :type, param_def[:optional] if param_def.key? :optional + requires :type, **param_def[:requires] if param_def.key? :requires + optional :type, **param_def[:optional] if param_def.key? :optional end get path do { type: params[:type] } diff --git a/spec/integration/grape_entity/entity_spec.rb b/spec/integration/grape_entity/entity_spec.rb index cdac039a6..96af0899e 100644 --- a/spec/integration/grape_entity/entity_spec.rb +++ b/spec/integration/grape_entity/entity_spec.rb @@ -20,7 +20,7 @@ def first it 'sets the object as the body if no options are provided' do inner_body = nil subject.get '/example' do - present(abc: 'def') + present({ abc: 'def' }) inner_body = body end get '/example' diff --git a/spec/shared/versioning_examples.rb b/spec/shared/versioning_examples.rb index 9f42eeda1..60e622e44 100644 --- a/spec/shared/versioning_examples.rb +++ b/spec/shared/versioning_examples.rb @@ -3,7 +3,7 @@ shared_examples_for 'versioning' do it 'sets the API version' do subject.format :txt - subject.version 'v1', macro_options + subject.version 'v1', **macro_options subject.get :hello do "Version: #{request.env[Grape::Env::API_VERSION]}" end @@ -14,7 +14,7 @@ it 'adds the prefix before the API version' do subject.format :txt subject.prefix 'api' - subject.version 'v1', macro_options + subject.version 'v1', **macro_options subject.get :hello do "Version: #{request.env[Grape::Env::API_VERSION]}" end @@ -23,12 +23,12 @@ end it 'is able to specify version as a nesting' do - subject.version 'v2', macro_options + subject.version 'v2', **macro_options subject.get '/awesome' do 'Radical' end - subject.version 'v1', macro_options do + subject.version 'v1', **macro_options do get '/legacy' do 'Totally' end @@ -46,7 +46,7 @@ end it 'is able to specify multiple versions' do - subject.version 'v1', 'v2', macro_options + subject.version 'v1', 'v2', **macro_options subject.get 'awesome' do 'I exist' end @@ -63,12 +63,12 @@ context 'without a prefix' do it 'allows the same endpoint to be implemented' do subject.format :txt - subject.version 'v2', macro_options + subject.version 'v2', **macro_options subject.get 'version' do request.env[Grape::Env::API_VERSION] end - subject.version 'v1', macro_options do + subject.version 'v1', **macro_options do get 'version' do "version #{request.env[Grape::Env::API_VERSION]}" end @@ -87,12 +87,12 @@ it 'allows the same endpoint to be implemented' do subject.format :txt subject.prefix 'api' - subject.version 'v2', macro_options + subject.version 'v2', **macro_options subject.get 'version' do request.env[Grape::Env::API_VERSION] end - subject.version 'v1', macro_options do + subject.version 'v1', **macro_options do get 'version' do "version #{request.env[Grape::Env::API_VERSION]}" end @@ -113,7 +113,7 @@ it 'calls before block that is defined within the version block' do subject.format :txt subject.prefix 'api' - subject.version 'v2', macro_options do + subject.version 'v2', **macro_options do before do @output ||= 'v2-' end @@ -123,7 +123,7 @@ end end - subject.version 'v1', macro_options do + subject.version 'v1', **macro_options do before do @output ||= 'v1-' end @@ -145,7 +145,7 @@ it 'does not overwrite version parameter with API version' do subject.format :txt - subject.version 'v1', macro_options + subject.version 'v1', **macro_options subject.params { requires :version } subject.get :api_version_with_version_param do params[:version] @@ -158,7 +158,7 @@ let(:options) { macro_options } let(:v1) do klass = Class.new(Grape::API) - klass.version 'v1', options + klass.version 'v1', **options klass.get 'version' do 'v1' end @@ -166,7 +166,7 @@ end let(:v2) do klass = Class.new(Grape::API) - klass.version 'v2', options + klass.version 'v2', **options klass.get 'version' do 'v2' end