Deprecate the positional options Hash in requires, optional and use - #2853
Open
ericproulx wants to merge 1 commit into
Open
Deprecate the positional options Hash in requires, optional and use#2853ericproulx wants to merge 1 commit into
ericproulx wants to merge 1 commit into
Conversation
#2618 replaced `attrs.extract_options!` with an explicit `**opts` parameter. The two are not equivalent: Ruby only converts a trailing Hash into keyword arguments when it is written without braces, so a braced Hash now stays in the splat and is taken for a parameter name. `optional :id, { type: Integer }` silently lost its coercion, `requires :id, { type: Integer }` made every request fail with `{type: Integer} is missing`, and `use :pg, { foo: 1 }` raised `Params :{foo: 1} not found!`. Restore the old behaviour behind a deprecation warning, following `merge_legacy_auth_options`. The trailing Hash is splatted back as keyword arguments and the method re-invoked, so Ruby redoes the keyword routing: merging into `**opts` in place would strand `:using` and `:except` there while their explicit keyword parameters stayed nil. Fixes #2851 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ericproulx
force-pushed
the
deprecate-positional-options-hash
branch
from
August 9, 2026 03:43
dd9afb0 to
5ee73d8
Compare
Danger ReportNo issues found. |
dblock
approved these changes
Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2851. Supersedes #2852.
The bug
#2618 replaced
attrs.extract_options!with an explicit**optsparameter inGrape::DSL::Parameters. As @OuYangJinTing pointed out in #2851, the two are not equivalent: Ruby only converts a trailingHashinto keyword arguments when it is written without braces, so a braced Hash stays in the splat — where these methods take it for a parameter name.UPGRADING.mdclaimed the change maintained "full backward compatibility". It did not:optional :id, { type: Integer }params[:id]is aStringrequires :id, { type: Integer }400s with{type: Integer} is missing, and:idloses its coercionuse :pg, { foo: 1 }RuntimeError: Params :{foo: 1} not found!The
optionalcase is the dangerous one. Nothing raises, nothing warns — a parameter that used to arrive coerced now arrives as a rawString, and the API keeps answering200.The fix
The old behaviour is restored behind a deprecation warning, following the
merge_legacy_auth_optionsprecedent already used forauth,http_basic,http_digestanddesc:Two private helpers in
lib/grape/dsl/parameters.rbdo the work:Why re-dispatch instead of merging into
**optsrequiresandoptionaldeclareusing:andexcept:as explicit keyword parameters. Merging the legacy Hash into**optsin place would leave:usingsitting inoptswhile theusinglocal stayednil, sorequires :all, { using: Entity.documentation }would quietly build a parameter literally named:allinstead of expanding the documentation Hash — one silent bug swapped for another.Splatting the Hash back at the call site and re-invoking the method lets Ruby redo the keyword routing, and it keeps working if either method gains another explicit keyword later. Keyword arguments win over the legacy Hash on conflict, matching
legacy_options.first.merge(options)inGrape::Middleware::Auth::DSL.Relationship to #2852
@cyphercodes' #2852 fixes the same issue for
use. This PR is a superset of it and I think it should land instead, for three reasons:requiresandoptionalare affected too, andoptionalfails silently — the worst of the three. Preserve positional options for named params #2852 leaves both untouched.using:/except:routing, described above, which a positional extraction insideusedoesn't need butrequiresandoptionaldo.#2852's guard also requires
options.empty?, so a mixed call likeuse :pg, { a: 1 }, b: 2silently drops{ a: 1 }; here the two are merged with the keywords taking precedence, asextract_options!effectively did.Thanks @cyphercodes for the fix and the regression example — the
usespec here is yours, extended.Scope
Deliberately left alone:
given(*attrs, &)must not get this check. It has no**, so keyword arguments legitimately arrive as a trailing Hash inattrs— that is whatfirst_hash_key_or_paramexists for. The check only applies to methods that actually declare a double splat.requires({ type: Integer })) does not warn; the guard isargs.size > 1. With nothing in front of it there is no parameter being declared either way, so there is no prior behaviour to preserve.Grape::DSL::Entity#presenthas the same latent problem —present(obj, { with: X })silently drops the options because theargs.count == 2 && args.first.is_a?(Symbol)destructuring fails. It is unrelated to named params and out of scope here; happy to open a follow-up.Docs
@param attrsdoc forrequiresstill advertised the trailing Hash ("The last key can be a hash, which specifies options for the parameters"). Updated to point at keyword arguments.UPGRADING.md— the >= 3.0.0 section that introduced this is amended: the "full backward compatibility" claim is now accurate again, with the deprecated form spelled out. No new entry, since nothing breaks.Test plan
spec/grape/dsl/parameters_spec.rb— warns and still works forrequires,optionalanduse; options of a positional Hash apply to every attribute;:usingroutes out of the Hash to the keyword parameter; the keyword form does not warn; a lone Hash does not warn.spec/grape/validations/params_scope_spec.rb— request-level regression proving the coercion that was silently lost is applied again.bundle exec rspec— 2558 examples, 0 failures.bundle exec rubocop— no offenses.🤖 Generated with Claude Code