Refactor DSL::Routing#version: guard clause, explicit kwargs, value object

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

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`.

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

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<String, Symbol>] 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.<vendor>-<version>+); 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

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

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

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
 

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)

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
 

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'],

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,15 +57,15 @@
   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
 
   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

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,21 +153,21 @@
   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
 
   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