diff --git a/CHANGELOG.md b/CHANGELOG.md index ec0e96fee..77b023bdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ * [#2717](https://github.com/ruby-grape/grape/pull/2717): Convert `Grape::Exceptions::ErrorResponse` to a `Data` value object - [@ericproulx](https://github.com/ericproulx). * [#2721](https://github.com/ruby-grape/grape/pull/2721): Use an internal `Grape::Validations::SharedOptions` value object in `Validators::Base` (public `opts` Hash contract unchanged) - [@ericproulx](https://github.com/ericproulx). * [#2719](https://github.com/ruby-grape/grape/pull/2719): Move content-type helpers from `Middleware::Base` into `PrecomputedContentTypes` - [@ericproulx](https://github.com/ericproulx). +* [#2716](https://github.com/ruby-grape/grape/pull/2716): Refactor `DSL::Routing#version`: guard clause, explicit kwargs in place of `**options`, and a `Grape::DSL::VersionOptions` value object stored internally - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/UPGRADING.md b/UPGRADING.md index 6b45573ac..57e34b658 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -74,6 +74,26 @@ The remaining middleware-options keys (`default_status`, `format`, `rescue_handl The change resolves [#2527](https://github.com/ruby-grape/grape/issues/2527): the HTTP `status` and the response `headers` are now part of the formatter contract, so JSON:API–style error bodies (which embed the status code) and header-aware formatters can be written without reaching into `env[Grape::Env::API_ENDPOINT]`. +#### `version` now takes explicit keyword arguments + +`version` previously accepted `**options` and silently ignored any keys it didn't use. It now declares its options explicitly: + +```ruby +def version(*args, using: :path, cascade: true, parameter: 'apiver', strict: false, vendor: nil, &block) +``` + +Passing an unrecognised keyword now raises `ArgumentError` instead of being swallowed. The most common offender is `format:` — it was never a `version` option (response format is set with `format`/`default_format`, and header-versioned requests carry the format in their `Accept` header), but the old splat let `version 'v1', using: :header, vendor: 'x', format: :json` through as a no-op. + +```ruby +# Before — `format:` silently ignored +version 'v1', using: :header, vendor: 'x', format: :json + +# After +version 'v1', using: :header, vendor: 'x' # set responses with `format :json` / `default_format :json` +``` + +Recognized keys are `using:`, `cascade:`, `parameter:`, `strict:`, `vendor:`. Calls using only those are unaffected. + #### `Grape::Middleware::Base#options` is now frozen `@options` is frozen at the end of `Grape::Middleware::Base#initialize` (after `merge_default_options`). The hash is initialized once and treated as immutable for the lifetime of the middleware. Custom middleware that mutates `options[...]` at runtime will now raise `FrozenError`. diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index c471b8766..e08d3199a 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -132,7 +132,7 @@ def call(env) def cascade? namespace_inheritable = self.class.inheritable_setting.namespace_inheritable return namespace_inheritable[:cascade] if namespace_inheritable.key?(:cascade) - return namespace_inheritable[:version_options][:cascade] if namespace_inheritable[:version_options]&.key?(:cascade) + return namespace_inheritable[:version_options].cascade if namespace_inheritable[:version_options] true end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 288efc43c..9badcf1f7 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -23,6 +23,12 @@ def cascade(value = nil) # Specify an API version. # + # Called without arguments, returns the most recently declared version + # (or +nil+). Called with one or more version strings, registers them + # and stores a {Grape::DSL::VersionOptions} value object on the + # inheritable settings; when given a block, the registration applies + # within a nested namespace. + # # @example API with legacy support. # class MyAPI < Grape::API # version 'v2' @@ -38,26 +44,41 @@ def cascade(value = nil) # end # end # - def version(*args, **options, &block) - if args.any? - options = options.reverse_merge(using: :path) - requested_versions = args.flatten.map(&:to_s) - - raise Grape::Exceptions::MissingVendorOption.new if options[:using] == :header && !options.key?(:vendor) - - @versions = versions | requested_versions - - if block - within_namespace do - inheritable_setting.namespace_inheritable[:version] = requested_versions - inheritable_setting.namespace_inheritable[:version_options] = options - - instance_eval(&block) - end - else + # @param args [Array] one or more version identifiers. + # @param using [Symbol] versioning strategy — one of +:path+ (default), + # +:header+, +:param+, or +:accept_version_header+. + # @param cascade [Boolean] forward to subsequent routes via the + # +X-Cascade+ header on version mismatch. Defaults to +true+. + # @param parameter [String] name of the query/body parameter that + # carries the version when +using: :param+. Defaults to +'apiver'+. + # @param strict [Boolean] reject requests that don't supply a usable + # version (header strategies). Defaults to +false+. + # @param vendor [String, nil] vendor segment for the +:header+ + # strategy (+application/vnd.-+); required when + # +using: :header+. + # @yield optional block to scope routes under this version. + # @return [String, nil] the most recently declared version. + # @raise [Grape::Exceptions::MissingVendorOption] when +using: :header+ + # is supplied without a +:vendor+. + def version(*args, using: :path, cascade: true, parameter: 'apiver', strict: false, vendor: nil, &block) + return @versions&.last if args.empty? + + raise Grape::Exceptions::MissingVendorOption.new if using == :header && vendor.nil? + + requested_versions = args.flatten.map(&:to_s) + options = VersionOptions.new(using:, cascade:, parameter:, strict:, vendor:) + + @versions = versions | requested_versions + + if block + within_namespace do inheritable_setting.namespace_inheritable[:version] = requested_versions inheritable_setting.namespace_inheritable[:version_options] = options + instance_eval(&block) end + else + inheritable_setting.namespace_inheritable[:version] = requested_versions + inheritable_setting.namespace_inheritable[:version_options] = options end @versions&.last diff --git a/lib/grape/dsl/version_options.rb b/lib/grape/dsl/version_options.rb new file mode 100644 index 000000000..0cd6c6598 --- /dev/null +++ b/lib/grape/dsl/version_options.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Grape + module DSL + # Immutable value object holding the resolved options from + # +Grape::DSL::Routing#version+. Stored on the inheritable settings as + # +namespace_inheritable[:version_options]+ and read by internal call + # sites (`Path`, `Endpoint`, `API::Instance#cascade?`, + # `Middleware::Versioner::Base`) via accessors. + # + # Defaults are duplicated on +#initialize+ here and on +#version+'s + # signature on purpose: keeping them on both sides means each entry point + # is self-documenting without needing to import a shared constant — the + # DSL signature shows what a user sees in the IDE, and the Data object + # has working defaults when constructed directly (middleware + # `DEFAULT_OPTIONS`, spec fixtures, etc.). The two must stay in lockstep. + VersionOptions = Data.define(:using, :cascade, :parameter, :strict, :vendor) do + def initialize(using: :path, cascade: true, parameter: 'apiver', strict: false, vendor: nil) + super + end + end + end +end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 2daa7d326..ec6038155 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -310,9 +310,10 @@ def build_stack stack.concat inheritable_setting.namespace_stackable[:middleware] if inheritable_setting.namespace_inheritable[:version].present? - stack.use Grape::Middleware::Versioner.using(inheritable_setting.namespace_inheritable[:version_options][:using]), + version_options = inheritable_setting.namespace_inheritable[:version_options] + stack.use Grape::Middleware::Versioner.using(version_options.using), versions: inheritable_setting.namespace_inheritable[:version].flatten, - version_options: inheritable_setting.namespace_inheritable[:version_options], + version_options:, prefix: inheritable_setting.namespace_inheritable[:root_prefix], mount_path: inheritable_setting.namespace_stackable[:mount_path].first end diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb index 54032e727..8b807e681 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -4,18 +4,14 @@ module Grape module Middleware module Versioner class Base < Grape::Middleware::Base + extend Forwardable include Grape::Middleware::PrecomputedContentTypes DEFAULT_OPTIONS = { mount_path: nil, pattern: /.*/i, prefix: nil, - version_options: { - cascade: true, - parameter: 'apiver', - strict: false, - vendor: nil - }.freeze + version_options: Grape::DSL::VersionOptions.new }.freeze CASCADE_PASS_HEADER = { 'X-Cascade' => 'pass' }.freeze @@ -25,21 +21,19 @@ def self.inherited(klass) Versioner.register(klass) end - attr_reader :available_media_types, :cascade, :error_headers, :mount_path, :parameter, - :pattern, :prefix, :strict, :vendor, :versions + attr_reader :available_media_types, :error_headers, :mount_path, :pattern, + :prefix, :version_options, :versions + + def_delegators :version_options, :cascade, :parameter, :strict, :vendor def initialize(app, **options) super - version_options = @options[:version_options] - @cascade = version_options[:cascade] + @version_options = @options[:version_options] @mount_path = @options[:mount_path] - @parameter = version_options[:parameter] @pattern = @options[:pattern] @prefix = @options[:prefix] - @strict = version_options[:strict] - @vendor = version_options[:vendor] @versions = @options[:versions]&.map(&:to_s) # making sure versions are strings to ease potential match - @error_headers = @cascade ? CASCADE_PASS_HEADER : {} + @error_headers = cascade ? CASCADE_PASS_HEADER : {} @available_media_types = build_available_media_types end diff --git a/lib/grape/path.rb b/lib/grape/path.rb index 516c9ef3c..27da1e46e 100644 --- a/lib/grape/path.rb +++ b/lib/grape/path.rb @@ -52,9 +52,9 @@ def uses_specific_format?(settings) end def uses_path_versioning?(settings) - return false unless settings.key?(:version) && settings[:version_options]&.key?(:using) + return false unless settings.key?(:version) && settings[:version_options] - settings[:version] && settings[:version_options][:using] == :path + settings[:version] && settings[:version_options].using == :path end def valid_part?(part) diff --git a/spec/grape/dsl/routing_spec.rb b/spec/grape/dsl/routing_spec.rb index a173b8418..415a02779 100644 --- a/spec/grape/dsl/routing_spec.rb +++ b/spec/grape/dsl/routing_spec.rb @@ -25,7 +25,7 @@ class << self version = 'v1' expect(subject.version(version)).to eq(version) expect(subject.inheritable_setting.namespace_inheritable[:version]).to eq([version]) - expect(subject.inheritable_setting.namespace_inheritable[:version_options]).to eq(using: :path) + expect(subject.inheritable_setting.namespace_inheritable[:version_options]).to eq(Grape::DSL::VersionOptions.new) end end diff --git a/spec/grape/exceptions/invalid_accept_header_spec.rb b/spec/grape/exceptions/invalid_accept_header_spec.rb index a74ced5d4..9aeaedd00 100644 --- a/spec/grape/exceptions/invalid_accept_header_spec.rb +++ b/spec/grape/exceptions/invalid_accept_header_spec.rb @@ -42,7 +42,7 @@ subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -77,7 +77,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.get '/beer' do 'beer received' end @@ -112,7 +112,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -152,7 +152,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: false + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: false subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], [404, 'Resource not found'], [406, 'API vendor or version not found'], @@ -192,7 +192,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -236,7 +236,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.get '/beer' do 'beer received' end @@ -271,7 +271,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.rescue_from :all do |e| error! 'message was processed', 400, e.headers end @@ -320,7 +320,7 @@ def app subject { Class.new(Grape::API) } before do - subject.version 'v99', using: :header, vendor: 'vendorname', format: :json, cascade: true + subject.version 'v99', using: :header, vendor: 'vendorname', cascade: true subject.desc 'Get beer' do failure [[400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], [404, 'Resource not found'], [406, 'API vendor or version not found'], diff --git a/spec/grape/middleware/versioner/accept_version_header_spec.rb b/spec/grape/middleware/versioner/accept_version_header_spec.rb index 3c8624dd0..8bef08e8c 100644 --- a/spec/grape/middleware/versioner/accept_version_header_spec.rb +++ b/spec/grape/middleware/versioner/accept_version_header_spec.rb @@ -7,9 +7,7 @@ before do @options = { - version_options: { - using: :accept_version_header - } + version_options: Grape::DSL::VersionOptions.new(using: :accept_version_header) } end @@ -59,7 +57,7 @@ end it 'succeeds if :strict is set to false' do - @options[:version_options][:strict] = false + @options[:version_options] = @options[:version_options].with(strict: false) expect(subject.call('HTTP_ACCEPT_VERSION' => '').first).to eq(200) expect(subject.call({}).first).to eq(200) end @@ -67,7 +65,7 @@ context 'when :strict is set' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true + @options[:version_options] = @options[:version_options].with(strict: true) end it 'fails with 406 Not Acceptable if header is not set' do @@ -94,8 +92,7 @@ context 'when :strict and cascade: false' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true - @options[:version_options][:cascade] = false + @options[:version_options] = @options[:version_options].with(strict: true, cascade: false) end it 'fails with 406 Not Acceptable if header is not set' do diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index d673e08b9..2ed015625 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/spec/grape/middleware/versioner/header_spec.rb @@ -7,10 +7,7 @@ before do @options = { - version_options: { - using: :header, - vendor: 'vendor' - } + version_options: Grape::DSL::VersionOptions.new(using: :header, vendor: 'vendor') } end @@ -156,13 +153,13 @@ end it 'succeeds if :strict is set to false' do - @options[:version_options][:strict] = false + @options[:version_options] = @options[:version_options].with(strict: false) expect(subject.call('HTTP_ACCEPT' => '').first).to eq(200) expect(subject.call({}).first).to eq(200) end it 'succeeds if :strict is set to false and given an invalid header' do - @options[:version_options][:strict] = false + @options[:version_options] = @options[:version_options].with(strict: false) expect(subject.call('HTTP_ACCEPT' => 'yaml').first).to eq(200) expect(subject.call({}).first).to eq(200) end @@ -170,7 +167,7 @@ context 'when :strict is set' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true + @options[:version_options] = @options[:version_options].with(strict: true) end it 'fails with 406 Not Acceptable if header is not set' do @@ -199,8 +196,7 @@ context 'when :strict and cascade: false' do before do @options[:versions] = ['v1'] - @options[:version_options][:strict] = true - @options[:version_options][:cascade] = false + @options[:version_options] = @options[:version_options].with(strict: true, cascade: false) end it 'fails with 406 Not Acceptable if header is not set' do diff --git a/spec/grape/middleware/versioner/param_spec.rb b/spec/grape/middleware/versioner/param_spec.rb index d772c610b..8f0fbab4b 100644 --- a/spec/grape/middleware/versioner/param_spec.rb +++ b/spec/grape/middleware/versioner/param_spec.rb @@ -24,7 +24,9 @@ end context 'with specified parameter name' do - let(:options) { { version_options: { parameter: 'v' } } } + let(:options) do + { version_options: Grape::DSL::VersionOptions.new(using: :param, parameter: 'v') } + end it 'sets the API version based on the custom parameter name' do env = Rack::MockRequest.env_for('/awesome', params: { 'v' => 'v1' }) @@ -55,7 +57,7 @@ let(:options) do { versions: ['v1'], - version_options: { using: :header } + version_options: Grape::DSL::VersionOptions.new(using: :header) } end diff --git a/spec/grape/path_spec.rb b/spec/grape/path_spec.rb index 9b9b2ed26..9b84756ba 100644 --- a/spec/grape/path_spec.rb +++ b/spec/grape/path_spec.rb @@ -65,7 +65,7 @@ context 'when path versioning is used' do it "includes a '/'" do - path = described_class.new(nil, nil, version: :v1, version_options: { using: :path }) + path = described_class.new(nil, nil, version: :v1, version_options: Grape::DSL::VersionOptions.new) expect(path.suffix).to eql('(/.:format)') end end @@ -77,12 +77,12 @@ end it "does not include a '/' when the path has a path" do - path = described_class.new('/path', nil, version: :v1, version_options: { using: :path }) + path = described_class.new('/path', nil, version: :v1, version_options: Grape::DSL::VersionOptions.new) expect(path.suffix).to eql('(.:format)') end it "includes a '/' otherwise" do - path = described_class.new(nil, nil, version: :v1, version_options: { using: :path }) + path = described_class.new(nil, nil, version: :v1, version_options: Grape::DSL::VersionOptions.new) expect(path.suffix).to eql('(/.:format)') end end diff --git a/spec/shared/versioning_examples.rb b/spec/shared/versioning_examples.rb index 60e622e44..f5c354064 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.except(:format) 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.except(:format) 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.except(:format) subject.get '/awesome' do 'Radical' end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) 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.except(:format) 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.except(:format) subject.get 'version' do request.env[Grape::Env::API_VERSION] end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) 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.except(:format) subject.get 'version' do request.env[Grape::Env::API_VERSION] end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) 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.except(:format) do before do @output ||= 'v2-' end @@ -123,7 +123,7 @@ end end - subject.version 'v1', **macro_options do + subject.version 'v1', **macro_options.except(:format) 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.except(:format) 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.except(:format) 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.except(:format) klass.get 'version' do 'v2' end