[DRAFT] Generalize middleware options to per-class Data value objects#2718
Draft
ericproulx wants to merge 8 commits into
Draft
[DRAFT] Generalize middleware options to per-class Data value objects#2718ericproulx wants to merge 8 commits into
ericproulx wants to merge 8 commits into
Conversation
Three changes that touch the same method: 1. Lift the body out of `if args.any? ... end` into a `return @versions&.last if args.empty?` guard. 2. Replace `**options` with explicit kwargs (`using:`, `cascade:`, `parameter:`, `strict:`, `vendor:`) and bake the defaults into the signature. The per-call `reverse_merge(using: :path)` is gone. 3. Introduce `Grape::DSL::VersionOptions` (built on `Data.define`) as the canonical representation of resolved version options. Threaded end-to-end: - `#version` builds and stores a `VersionOptions` on `inheritable_setting.namespace_inheritable[:version_options]`. - `Path#uses_path_versioning?`, `Endpoint#build_stack`, and `API::Instance#cascade?` read fields via accessors instead of `[:key]`. - The versioner middleware (`Versioner::Base`) now takes a `VersionOptions` directly, not a Hash. `Forwardable.def_delegators` forwards `cascade`/`parameter`/`strict`/`vendor` to it; the four ivars and per-call assignments are dropped. `DEFAULT_OPTIONS` stores a `VersionOptions.new(...)` so direct-mount callers still get safe defaults. Public `.version` DSL surface unchanged — still accepts kwargs the same way; the value object is an internal-only representation. `version_options` is read for exactly these five keys across the codebase: `:using` (`endpoint.rb`, `path.rb`), and `:cascade` / `:parameter` / `:strict` / `:vendor` (`Versioner::Base`, plus `:cascade` in `API::Instance#cascade?`). Any other kwarg passed to `.version` was previously swallowed by the splat with no effect. Spec fixtures updated: - `spec/shared/versioning_examples.rb` (14 sites): `**macro_options` -> `**macro_options.except(:format)`. The `:format` key stays in the macro hash so the test helper (`versioned_helpers.rb`) can still build the `HTTP_ACCEPT` header. - `spec/grape/exceptions/invalid_accept_header_spec.rb` (8 sites): dropped `format: :json` from direct `.version` calls. - `spec/grape/dsl/routing_spec.rb`: assertion now expects a `VersionOptions` instance with the full default set. - `spec/grape/path_spec.rb`: literal `version_options: { using: :path }` hashes replaced with `VersionOptions.new(...)` constructors. - `spec/grape/middleware/versioner/{header,accept_version_header,param}_spec.rb`: fixtures construct `VersionOptions` instances; mid-test `@options[:version_options][:strict] = X` assignments rewritten as `@options[:version_options] = @options[:version_options].with(strict: X)` (Data instances are immutable). No production behaviour change beyond `.version` raising `ArgumentError` for genuinely unknown kwargs (previously silently ignored). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The error_formatter contract previously took five positional arguments — `(message, backtrace, options, env, original_exception)` — which made it impossible to thread additional context through to custom formatters without breaking every existing override. Switch to keyword arguments and add two fields that have been asked for in #2527: def call(message:, backtrace:, options:, env:, status:, headers:, original_exception:) `status:` makes JSON:API-style error bodies straightforward (the spec embeds the HTTP status code in the response). `headers:` lets formatters react to the response content-type or trace the marker headers set by `error!`. Both were previously only reachable via `env[Grape::Env::API_ENDPOINT].status` and friends. This is a contract break — existing custom formatters that re-declare `call` with the positional signature will need to migrate. See UPGRADING.md for the before/after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Danger ReportNo issues found. |
…ntTypes `Middleware::Base#content_types` / `#mime_types` / `#content_type_for` / `#content_type` / `#content_types_indifferent_access` are only consumed by middleware that include `PrecomputedContentTypes` — namely `Formatter`, `Error`, and `Versioner::Base`. The other concrete middlewares (`Filter`, `Auth::Base`) never call them. Move the five methods into `PrecomputedContentTypes`. `Base` keeps the ivar plumbing (`@options`, `@env`, `@app`), `Rack::Request` wrapper, header merge, and `query_params`; the content-type concern lives with the mixin that already takes responsibility for warming its caches. This narrows `Base` to the storage / lifecycle layer and unblocks future work to give per-middleware `Options` value objects without `Base` reaching back into the option hash. No behaviour change — every middleware that called those helpers already included `PrecomputedContentTypes`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Demonstration / discussion PR. Right now every middleware that wants
typed accessors over its options has to hand-write the same boilerplate:
DEFAULT_OPTIONS = { foo: nil, bar: nil, ... }.freeze
attr_reader :foo, :bar, ...
def initialize(app, **options)
super
@foo = @options[:foo]
@bar = @options[:bar]
...
end
Since `@options` was already frozen by design (Middleware::Base#initialize
post-PR #2696), the natural next step is to replace the Hash with a
per-subclass `Options = Data.define(...)` and let `Forwardable` cover the
accessor wiring.
Mechanism added in this draft:
- `Grape::Middleware::OptionsCompat` — a small mixin Options classes
include to keep the legacy `options[:key]` idiom working (notably for
`Middleware::Base#content_types` and `#content_type`). Unknown keys
return `nil` to match Hash semantics.
- `Middleware::Base#initialize` detects `self.class::Options` and routes
kwargs through `Options.new(**options)`. Subclasses that still rely on
`DEFAULT_OPTIONS` Hash + deep_merge keep working unchanged.
Demonstrated on `Middleware::Formatter`:
- Replaces 5-line DEFAULT_OPTIONS Hash + 4-line `attr_reader` list +
6-line initialize body with:
Options = Data.define(:content_types, :default_format, :format,
:formatters, :parsers) do
include Grape::Middleware::OptionsCompat
def initialize(content_types: nil, default_format: :txt, format: nil,
formatters: nil, parsers: nil)
super
end
end
def_delegators :options, :default_format, :format, :formatters, :parsers
- Defaults move from the freeze'd Hash to `#initialize` signature.
- Immutability is implicit (Data instances).
Behaviour change: passing an unknown kwarg to a middleware whose `Options`
class doesn't declare it now raises `ArgumentError` instead of being
silently swallowed by `**options`. One formatter spec was passing
`rescue_options:` (dead weight; Formatter doesn't read it) — dropped.
If this direction is acceptable, follow-ups would convert
`Middleware::Error`, `Versioner::Base`, etc., each shedding the same
boilerplate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Building on the merged refactor/version-guard-clause (VersionOptions
Data) and feature/error-formatter-kwargs-2527 (RescueOptions Data),
convert the remaining two middlewares that include
`PrecomputedContentTypes` to per-class `Options = Data.define(...)`:
- Middleware::Error: 14-field Options replacing the 14-line
DEFAULT_OPTIONS Hash + the 12-line attr_reader / ivar-set initialize
body. `rescue_options:` defaults to `Grape::DSL::RescueOptions.new`;
initialize coerces an explicit nil (from
`Endpoint#error_middleware_options` when no `rescue_from` was called)
to the default. `Forwardable.def_delegators :options, ...` covers
every accessor; the existing `def_delegator :rescue_options, :backtrace,
:include_backtrace` (and `:original_exception`) carry through unchanged.
- Middleware::Versioner::Base: 7-field Options (adds `content_types:` /
`format:` so the mixin's accessor reads land cleanly). `version_options:`
defaults to `Grape::DSL::VersionOptions.new`. The four
`def_delegators :version_options, :cascade, :parameter, :strict, :vendor`
stay; `mount_path` / `pattern` / `prefix` / `version_options` are now
delegated via `def_delegators :options, ...`.
With every PrecomputedContentTypes consumer now using an Options Data
class, switch the mixin to accessor reads:
options.content_types # was options[:content_types]
options.format # was options[:format]
…and delete `Grape::Middleware::OptionsCompat` entirely.
Two supporting tweaks:
- `Middleware::Base#build_options` switches `const_defined?(:Options,
false)` → `const_defined?(:Options)` so Versioner subclasses
(`Path`, `Header`, `Param`, `AcceptVersionHeader`) inherit
`Versioner::Base::Options` without redeclaring it.
- `Middleware::Formatter#read_rack_input` switches `options[:parsers]`
to the existing `parsers` delegator.
`Filter` and `Auth::Base` / `Auth::*` remain on the legacy
`DEFAULT_OPTIONS` Hash path — `Base#build_options` keeps the fallback,
so they continue to work unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ericproulx
force-pushed
the
draft/middleware-options-data
branch
from
ba9b2e7 to
e707766
Compare
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
1 participant
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.
Discussion / direction-check
Following the per-feature
VersionOptions/RescueOptionswork in #2712 and #2716, every middleware that wants typed option accessors gets its ownOptions = Data.define(...)and usesForwardablefor the accessors. Since@optionsis already frozen by design (post-#2696), the Hash buys us nothing the Data class doesn't get for free, and the per-middleware DEFAULT_OPTIONS / attr_reader / ivar-set boilerplate evaporates.Still opening as draft to confirm the direction before this lands. This branch now contains #2712 and #2716 merged in (along with the prep-PR #2719 that moved content-type helpers out of
Base) plus the conversion of everyPrecomputedContentTypesconsumer.Mechanism
Middleware::Base#initializedetectsself.class::Options(ancestor search included, so subclasses inherit their parent's Options Data class without redeclaring) and routes the kwargs throughOptions.new(**options). Subclasses without anOptionsconstant still flow through theDEFAULT_OPTIONSHash +deep_mergepath (Filter,Auth::*) unchanged.PrecomputedContentTypes— already the only place inlib/that touchedoptions[:content_types]/options[:format]after #2719 — switches to accessor reads (options.content_types,options.format). The transitionalGrape::Middleware::OptionsCompatHash-like[]shim is deleted.Converted middlewares
Middleware::Formatter(5 fields).Middleware::Error(14 fields).rescue_options:defaults toGrape::DSL::RescueOptions.newand the initializer coerces an explicit nil (passed byEndpoint#error_middleware_optionswhen norescue_fromwas called) to the default.Middleware::Versioner::Base(7 fields). Addscontent_types:/format:so the mixin's accessor reads land cleanly.version_options:defaults toGrape::DSL::VersionOptions.new.FilterandAuth::Base/Auth::*are unchanged — they don't includePrecomputedContentTypesand don't benefit from typed accessors.What this drops
DEFAULT_OPTIONS+attr_reader+@ivar = @options[:key]boilerplate.Grape::Middleware::OptionsCompatshim.options[:key]lookup insidelib/grape/middleware/except for the unconvertedFilter/Auth::*(which still need the Hash path).Behaviour change
Passing an unknown kwarg to a middleware whose
Optionsclass doesn't declare it now raisesArgumentErrorinstead of being silently swallowed by**options. One formatter spec was passingrescue_options:(dead weight; Formatter doesn't actually read it) — dropped earlier in this draft.That stricter contract is exactly what made
version_options/rescue_optionscleaner in their respective PRs.Open questions
Middleware::Foo::Optionsreads well asFoo::Options.new(...). Alternative: top-levelGrape::Middleware::FooOptions. Preference?Filter/Auth::*conversion: leave them on the Hash path, or convert in a follow-up?Test plan
bundle exec rspec— 2308 examples, 0 failureslib/grape/middleware/🤖 Generated with Claude Code