diff --git a/CHANGELOG.md b/CHANGELOG.md index f4e383979..f4cfc8899 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * [#2782](https://github.com/ruby-grape/grape/pull/2782): Remove the dead `format` keyword from `Grape::Router::Pattern` - [@ericproulx](https://github.com/ericproulx). * [#2783](https://github.com/ruby-grape/grape/pull/2783): Read a route's `version`, `anchor` and `requirements` from its pattern instead of storing them again on the route - [@ericproulx](https://github.com/ericproulx). * [#2784](https://github.com/ruby-grape/grape/pull/2784): Move HEAD route creation into `Grape::Router::Route#to_head` - [@ericproulx](https://github.com/ericproulx). +* [#2785](https://github.com/ruby-grape/grape/pull/2785): Make route `params` a first-class endpoint input and split `Grape::Router::Route#params` into `#params` (declared definitions) and `#params_for(input)` (extracted values) - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index 9e0a070c8..c323388a5 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -60,6 +60,21 @@ The `:format` member was removed from the endpoint's internal `Options`. Nothing `Grape::Router::Pattern#initialize` no longer accepts `format:` either. It was never given a non-`nil` value — a route's `:format` capture comes from the path suffix built by `Grape::Path`, not from the pattern — so the keyword was dead. `Grape::Router::Pattern.new(..., format: …)` now raises `unknown keyword: :format`. +#### `Grape::Router::Route#params` no longer takes an argument + +`Route#params` used to do two jobs depending on its argument: `route.params(input)` extracted param values from a matched request path, while `route.params` (no argument) returned the route's declared param definitions. These are now separate methods: + +```ruby +route.params # declared param definitions, keyed by name (unchanged) +route.params_for(input) # values extracted from a matched path (was route.params(input)) +``` + +The no-argument form is unchanged — grape-swagger and other documentation consumers keep using `route.params`. Only the value-extraction form moved, and it is internal to the router; if you called `route.params(input)` directly, switch to `route.params_for(input)`. + +#### `params` is a first-class endpoint input, no longer in `route.options` + +A route's declared params were previously carried inside the `route_options` bag and reachable as `route.options[:params]`. They are now composed into their own endpoint input (`Grape::Endpoint::Options` gains a `:params` member and `Grape::Endpoint.new` a `params:` keyword) and exposed only through `route.params`. `route.options[:params]` now returns `nil`. Nothing in Grape or grape-swagger read it that way — grape-swagger uses the `route.params` method — so this only affects code that reached into the options Hash for params directly. + ### Upgrading to >= 3.3 #### Minimum required Ruby is now 3.3 diff --git a/lib/grape/dsl/declared.rb b/lib/grape/dsl/declared.rb index 8dbdd7f2b..907dcf90f 100644 --- a/lib/grape/dsl/declared.rb +++ b/lib/grape/dsl/declared.rb @@ -26,7 +26,7 @@ def declared(passed_params, include_parent_namespaces: true, include_missing: tr handler = DeclaredParamsHandler.new(include_missing:, evaluate_given:, stringify:, contract_key_map:) declared_params = include_parent_namespaces ? inheritable_setting.route[:declared_params] : (inheritable_setting.namespace_stackable[:declared_params].last || []) renamed_params = inheritable_setting.route[:renamed_params] || {} - route_params = options.dig(:route_options, :params) || {} # options = endpoint's option + route_params = config.params handler.call(passed_params, declared_params, route_params, renamed_params) end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 4b34b8290..ff5a14482 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -179,10 +179,12 @@ def mount(mounts, *opts) # end def route(methods, paths = ['/'], route_options = {}, &) http_methods = methods == :any ? '*' : methods - endpoint_params = inheritable_setting.namespace_stackable_with_hash(:params) || {} - endpoint_description = inheritable_setting.route[:description] - all_route_options = { params: endpoint_params } - all_route_options.deep_merge!(endpoint_description) if endpoint_description + endpoint_description = inheritable_setting.route[:description] || {} + + # +params+ travels as its own endpoint input; the route-options bag keeps + # the description's other keys (+success+, +tags+, …) plus explicit options. + params = prepare_params(endpoint_description[:params]) + all_route_options = endpoint_description.except(:params) all_route_options.deep_merge!(route_options) if route_options.present? new_endpoint = Grape::Endpoint.new( @@ -190,6 +192,7 @@ def route(methods, paths = ['/'], route_options = {}, &) http_methods:, path: paths, api: self, + params:, route_options: all_route_options, & ) @@ -259,6 +262,15 @@ def versions private + # Compose a route's params: the declared params (+params do … end+) deep-merged + # with any documented alongside +desc ..., params:+ (+description_params+). + def prepare_params(description_params) + endpoint_params = inheritable_setting.namespace_stackable_with_hash(:params) || {} + return endpoint_params if description_params.blank? + + endpoint_params.deep_merge(description_params) + end + # Remove all defined routes. def reset_routes! endpoints.each(&:reset_routes!) diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 482378ae6..2e2fe7c72 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -54,13 +54,15 @@ def block_to_unbound_method(block) # {#api}. # @param app [#call, nil] the Rack app or Grape API mounted at this # endpoint; +nil+ for a plain block endpoint. Exposed as {#mounted_app}. + # @param params [Hash] the declared params for this endpoint, keyed by name. + # Kept out of +route_options+ and read via +config.params+. # @param options [Hash] attributes of this endpoint, normalized into a # +Grape::Endpoint::Options+ value object. # @option options route_options [Hash] # @note This happens at the time of API definition, so in this context the # endpoint does not know if it will be mounted under a different endpoint. # @yield a block defining what your API should do when this endpoint is hit - def initialize(new_settings, http_methods:, path:, api:, app: nil, **options, &block) + def initialize(new_settings, http_methods:, path:, api:, app: nil, params: {}, **options, &block) self.inheritable_setting = new_settings.point_in_time_copy # now +namespace_stackable(:declared_params)+ contains all params defined for @@ -73,7 +75,7 @@ def initialize(new_settings, http_methods:, path:, api:, app: nil, **options, &b inheritable_setting.namespace_inheritable[:default_error_status] ||= 500 @options = options - @config = Options.new(http_methods:, path:, api:, app:, **options) + @config = Options.new(http_methods:, path:, api:, app:, params:, **options) # +:app+ is still surfaced on the public options Hash for backwards # compatibility (e.g. grape-swagger); prefer the +mounted_app+ reader. @options[:app] = app if app @@ -282,6 +284,7 @@ def compile! def to_routes route_options = config.route_options + params = config.params path_settings = prepare_default_path_settings forward_match = config.app && !config.app.respond_to?(:inheritable_setting) version = prepare_version(inheritable_setting.namespace_inheritable[:version]) @@ -297,11 +300,11 @@ def to_routes origin: prepared_path.origin, suffix: prepared_path.suffix, anchor:, - params: route_options[:params], + params:, version:, requirements: ) - Grape::Router::Route.new(self, method, pattern, route_options, forward_match:, namespace:, prefix:, settings:) + Grape::Router::Route.new(self, method, pattern, route_options, forward_match:, params:, namespace:, prefix:, settings:) end end end diff --git a/lib/grape/endpoint/options.rb b/lib/grape/endpoint/options.rb index b4201f02b..7d1ee58c4 100644 --- a/lib/grape/endpoint/options.rb +++ b/lib/grape/endpoint/options.rb @@ -6,8 +6,8 @@ class Endpoint # +Grape::Endpoint.new+. Internal to {Grape::Endpoint}, which builds it # from the +**options+ Hash in #initialize so the public +options+ reader # stays a plain Hash for downstream gems (e.g. grape-swagger). - Options = Data.define(:path, :http_methods, :api, :route_options, :app) do - def initialize(path:, http_methods:, api:, route_options: {}, app: nil) + Options = Data.define(:path, :http_methods, :api, :route_options, :app, :params) do + def initialize(path:, http_methods:, api:, route_options: {}, app: nil, params: {}) path = Array(path) path << '/' if path.empty? http_methods = Array(http_methods) diff --git a/lib/grape/router.rb b/lib/grape/router.rb index baa0a1da3..9f51f22f1 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -122,7 +122,7 @@ def halt?(response) end def process_route(route, input, env, include_allow_header: false) - route_params = route.params(input) + route_params = route.params_for(input) env[Grape::Env::GRAPE_ROUTING_ARGS] ||= { route_info: route } env[Grape::Env::GRAPE_ROUTING_ARGS].merge!(route_params) if route_params.present? env[Grape::Env::GRAPE_ALLOWED_METHODS] = route.allow_header if include_allow_header diff --git a/lib/grape/router/route.rb b/lib/grape/router/route.rb index e2ca0c7e1..7afa40bc1 100644 --- a/lib/grape/router/route.rb +++ b/lib/grape/router/route.rb @@ -12,11 +12,12 @@ class Route < BaseRoute def_delegators :@app, :call - def initialize(endpoint, method, pattern, options, forward_match:, **route_attributes) + def initialize(endpoint, method, pattern, options, forward_match:, params: {}, **route_attributes) super(pattern, options, **route_attributes) @app = endpoint @request_method = upcase_method(method) @match_function = forward_match ? FORWARD_MATCH_METHOD : NON_FORWARD_MATCH_METHOD + @declared_params = params end def to_head @@ -36,9 +37,15 @@ def match?(input) @match_function.call(input, pattern) end - def params(input = nil) - return params_without_input if input.blank? + # The route's declared params keyed by name — path captures plus any + # declared body/query params, as their definitions. Used for documentation + # (e.g. grape-swagger), not for extracting request values. + def params + @params ||= pattern.captures_default.merge(@declared_params) + end + # Extract param values from a matched request path. Used by the router. + def params_for(input) parsed = pattern.params(input) return unless parsed @@ -53,10 +60,6 @@ def convert_to_head_request! private - def params_without_input - @params_without_input ||= pattern.captures_default.merge(options[:params]) - end - def upcase_method(method) method_s = method.to_s Grape::HTTP_SUPPORTED_METHODS.detect { |m| m.casecmp(method_s).zero? } || method_s.upcase diff --git a/spec/grape/dsl/routing_spec.rb b/spec/grape/dsl/routing_spec.rb index 05d27398f..50ecb1b7e 100644 --- a/spec/grape/dsl/routing_spec.rb +++ b/spec/grape/dsl/routing_spec.rb @@ -158,7 +158,8 @@ class << self expect(endpoint_options[:http_methods]).to eq :get expect(endpoint_options[:path]).to eq '/foo' expect(endpoint_options[:api]).to eq subject - expect(endpoint_options[:route_options]).to eq(foo: 'bar', fiz: 'baz', params: { nuz: 'naz' }) + expect(endpoint_options[:params]).to eq(nuz: 'naz') + expect(endpoint_options[:route_options]).to eq(foo: 'bar', fiz: 'baz') end.and_yield subject.route(:get, '/foo', { foo: 'bar' }, &proc {}) diff --git a/spec/grape/router/route_spec.rb b/spec/grape/router/route_spec.rb index b032a7da3..1ee892806 100644 --- a/spec/grape/router/route_spec.rb +++ b/spec/grape/router/route_spec.rb @@ -64,6 +64,42 @@ end end + describe 'params' do + let(:pattern) do + Grape::Router::Pattern.new(origin: '/users/:id', suffix: '', anchor: true, + params: { 'id' => { type: 'Integer' } }, version: nil, requirements: {}) + end + + describe '#params (declared definitions, no input)' do + subject(:route) do + described_class.new(endpoint, :get, pattern, options, forward_match: false, + params: { 'id' => { required: true, type: 'Integer' } }) + end + + it 'merges the declared definitions over the path capture defaults' do + expect(route.params).to eq('id' => { required: true, type: 'Integer' }) + end + + it 'reads the params keyword, not the options Hash' do + route = described_class.new(endpoint, :get, pattern, { params: { 'ignored' => {} } }, + forward_match: false, params: { 'id' => { required: true } }) + expect(route.params).to eq('id' => { required: true }) + end + end + + describe '#params_for (extracted values, with input)' do + subject(:route) { described_class.new(endpoint, :get, pattern, options, forward_match: false) } + + it 'extracts param values from a matched input path' do + expect(route.params_for('/users/7')).to eq(id: '7') + end + + it 'returns nil when the input does not match' do + expect(route.params_for('/nope')).to be_nil + end + end + end + describe '#match?' do subject { instance.match?(input) }