From e563b2e0654e39a0e47107bd7244ad1046dda271 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Fri, 18 Sep 2026 16:43:22 +0200 Subject: [PATCH] Lint every response of an API once under lint! `lint!` and Grape.config.lint put Rack::Lint in each endpoint's stack, so two kinds of response never met it: the 404 the router answers for a path nothing matches, which no endpoint builds, and a Rack app mounted with `mount`, which the router calls directly instead of through a stack. That is how a HEAD request for an unknown path answered with a body (#2958) while the suite, which lints every API, stayed green. The per-endpoint Rack::Lint is gone. A linted API instead wraps its router in a single Rack::Lint, which checks every response the API gives -- endpoints, errors, the router's own 404, mounted Rack apps -- exactly once, and after Rack::Head, so a HEAD response is checked with its body stripped. Keeping the endpoint Lints and adding one around the router would nest two, and nested Rack::Lints cannot survive a middleware that calls `to_ary` on the body -- the inner one closes it, the outer one then iterates it -- which Rack::ETag does in any Rails app mounting the API. `lint!` is therefore read from the top of the API that is served, as the README describes it ("at the API level"). Declared inside a namespace, or in an API mounted into another one, it no longer does anything; README and UPGRADING say so. The suite's own lint then flagged two fixtures: `API.call({})`, an env Rack::Lint rejects, and a mounted Rack app returning a frozen headers Hash, which Rack 3 forbids. The first now passes a real env; the second an unfrozen Hash the spec still checks is not written into. Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 1 + README.md | 2 +- UPGRADING.md | 16 +++++++++++ lib/grape/api/instance.rb | 14 +++++++++- lib/grape/endpoint.rb | 5 ---- spec/grape/api/instance_spec.rb | 6 ++--- spec/grape/api_spec.rb | 47 ++++++++++++++++++++++++++++++++- 7 files changed, 80 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c456f133..8144b21a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#2913](https://github.com/ruby-grape/grape/pull/2913): Bench ZJIT alongside YJIT in the version benchmark - [@ericproulx](https://github.com/ericproulx). * [#2941](https://github.com/ruby-grape/grape/pull/2941): Bench static, parameterized and many-route shapes in separate tables in the version throughput benchmark - [@ericproulx](https://github.com/ericproulx). * [#2951](https://github.com/ruby-grape/grape/pull/2951): Re-bench only `master` in the version throughput benchmark and carry released versions' results over - [@ericproulx](https://github.com/ericproulx). +* [#2960](https://github.com/ruby-grape/grape/pull/2960): Lint every response of an API once under `lint!`, including the router's 404 and mounted Rack apps, instead of inside each endpoint's stack - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/README.md b/README.md index b2976e3f8..1aee6e3f3 100644 --- a/README.md +++ b/README.md @@ -508,7 +508,7 @@ version 'v1', using: :param, parameter: 'v' ## Linting -You can check whether your API is in conformance with the [Rack's specification](https://github.com/rack/rack/blob/main/SPEC.rdoc) by calling `lint!` at the API level or through [configuration](#configuration). +You can check whether your API is in conformance with the [Rack's specification](https://github.com/rack/rack/blob/main/SPEC.rdoc) by calling `lint!` at the top level of the API you serve, or through [configuration](#configuration). Every response the API gives is then checked once with `Rack::Lint`, including the 404 for a path no route matches and the responses of Rack apps mounted with `mount`. A `lint!` inside a namespace, or in an API mounted into another one, has no effect. ```ruby class Api < Grape::API diff --git a/UPGRADING.md b/UPGRADING.md index 08d099080..793086930 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,22 @@ Upgrading Grape ### Upgrading to >= 4.1.0 +#### `lint!` checks every response once, from the API that is served + +`lint!` and `Grape.config.lint` used to put `Rack::Lint` in each endpoint's stack, which missed two kinds of response: the 404 the router answers for a path nothing matches, and those of a Rack app mounted with `mount`, which the router calls directly. A single `Rack::Lint` around the router now checks every response the API gives ([#2960](https://github.com/ruby-grape/grape/pull/2960)). Two things follow from that. + +`lint!` counts only at the top level of the API that is served. Declared inside a namespace, or in an API mounted into another one, it no longer does anything: declare it on the API you serve, or set `Grape.config.lint`. + +A test suite that lints its API may start raising `Rack::Lint::LintError` where a mounted Rack app breaks the Rack SPEC — returning a frozen headers Hash, for instance — or where a spec calls the API with an incomplete env: + +```ruby +# Before: passed under Grape.config.lint +MyAPI.call({}) + +# After +MyAPI.call(Rack::MockRequest.env_for('/')) +``` + #### The header versioner's `api.*` env values are frozen `version ..., using: :header` now answers the Accept headers most requests send from a table built once, so every request sending the same header is handed the same parsed media type ([#2936](https://github.com/ruby-grape/grape/pull/2936)). The strings it writes into the env — `api.type`, `api.subtype`, `api.vendor`, `api.version` and `api.format` — are therefore frozen, as is `Grape::Util::MediaType` itself. Code that altered one of them in place now raises `FrozenError`; build a new String instead: diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 22808657e..c6ff3a2be 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -111,12 +111,13 @@ def initialize @router.compile! @router.freeze + @app = lint? ? Rack::Lint.new(@router) : @router @cascade = resolve_cascade end # Handle a request. See Rack documentation for what `env` is. def call(env) - response = @router.call(env) + response = @app.call(env) return response if @cascade headers = response[1] @@ -187,6 +188,17 @@ def collect_route_config_per_pattern(all_routes) end end + # +lint!+ declared at the top of this API, or Grape.config.lint: one + # Rack::Lint around the router then checks every response the API gives + # -- an endpoint's, the 404 the router answers itself, a mounted Rack + # app's -- exactly once. Linting inside each endpoint's stack instead + # missed the last two, and wrapping a second Rack::Lint around those + # breaks a middleware that calls +to_ary+ on the body, as Rack::ETag does. + # Called once, from the constructor. + def lint? + self.class.inheritable_setting.lint? || Grape.config.lint + end + # Backs {#cascade?}; called once, from the constructor. def resolve_cascade setting = self.class.inheritable_setting diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 9d39d6038..fa18b420a 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -383,7 +383,6 @@ def build_stack format = inheritable_setting.format stack.use Rack::Head - stack.use Rack::Lint if lint? stack.use Grape::Middleware::Error, **error_middleware_options(format, content_types) stack.concat inheritable_setting.middleware @@ -456,9 +455,5 @@ def build_response_cookies Rack::Utils.set_cookie_header! header, name, cookie_value end end - - def lint? - inheritable_setting.lint? || Grape.config.lint - end end end diff --git a/spec/grape/api/instance_spec.rb b/spec/grape/api/instance_spec.rb index e7deff512..79e1cb4db 100644 --- a/spec/grape/api/instance_spec.rb +++ b/spec/grape/api/instance_spec.rb @@ -120,7 +120,7 @@ def app describe '#call' do context 'when cascade is false' do - let(:rack_headers) { { 'content-type' => 'text/plain', 'x-cascade' => 'pass' }.freeze } + let(:rack_headers) { { 'content-type' => 'text/plain', 'x-cascade' => 'pass' } } let(:root_api) do headers = rack_headers Class.new(Grape::API::Instance) do @@ -134,8 +134,8 @@ def app expect(last_response.headers).not_to have_key('x-cascade') end - # The mounted app owns the Hash it returned and is free to hand back a - # frozen or shared one, so removing X-Cascade has to happen on a copy. + # The mounted app owns the Hash it returned and is free to hand back one + # it shares, so removing X-Cascade has to happen on a copy. it 'does not write into the headers the mounted app returned' do get '/rack' expect(rack_headers).to eq('content-type' => 'text/plain', 'x-cascade' => 'pass') diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index adfede91c..850fb3f42 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -336,7 +336,7 @@ def first = raise('#first should not have been called') context 'it does not add to the app setup' do it 'calls the app' do expect(subject).not_to receive(:add_setup) - subject.call({}) + subject.call(Rack::MockRequest.env_for('/')) end end end @@ -5410,6 +5410,51 @@ def uniqe_id_route # Status must be an Integer >= 100 expect { get '/' }.to raise_error(Rack::Lint::LintError) end + + # A path nothing matches is answered by the router itself. Rack::Lint + # checks the request as well as the response, and a PATH_INFO without its + # leading slash is one the router still routes. + it 'raises a Rack::Lint error for a request to a path nothing matches' do + env = Rack::MockRequest.env_for('/') + env[Rack::PATH_INFO] = 'nothing' + expect { app.call(env) }.to raise_error(Rack::Lint::LintError) + end + + it 'answers HEAD to a path nothing matches' do + head '/nothing' + expect(last_response.status).to eq(404) + end + + context 'with a Rack app mounted' do + let(:app) do + Class.new(described_class) do + lint! + mount ->(_env) { [42, {}, ['mounted']] } => '/rack' + end + end + + # The router calls a mounted Rack app directly, through no endpoint's + # stack. + it 'raises a Rack::Lint error' do + expect { get '/rack' }.to raise_error(Rack::Lint::LintError) + end + end + + context 'with a Grape API mounted' do + let(:app) do + mounted = Class.new(described_class) do + get('/bad') { status 42 } + end + Class.new(described_class) do + lint! + mount mounted => '/mounted' + end + end + + it 'raises a Rack::Lint error for an endpoint of the mounted API' do + expect { get '/mounted/bad' }.to raise_error(Rack::Lint::LintError) + end + end end describe '.cascade' do