Lint every response of an API once under lint!

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

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

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:

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

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

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')

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