Fix Rack::Lint

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2024-04-17 16:26:06 UTC using RuboCop version 1.63.2.
+# on 2024-05-10 16:10:58 UTC using RuboCop version 1.63.2.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -102,6 +102,14 @@ RSpec/DescribeClass:
     - '**/spec/views/**/*'
     - 'spec/grape/named_api_spec.rb'
 
+# Offense count: 2
+# This cop supports unsafe autocorrection (--autocorrect-all).
+# Configuration parameters: SkipBlocks, EnforcedStyle, OnlyStaticConstants.
+# SupportedStyles: described_class, explicit
+RSpec/DescribedClass:
+  Exclude:
+    - 'spec/grape/middleware/exception_spec.rb'
+
 # Offense count: 3
 # This cop supports safe autocorrection (--autocorrect).
 # Configuration parameters: CustomTransform, IgnoredWords, DisallowedExamples.
@@ -203,7 +211,7 @@ RSpec/ScatteredSetup:
   Exclude:
     - 'spec/grape/util/inheritable_setting_spec.rb'
 
-# Offense count: 8
+# Offense count: 5
 RSpec/StubbedMock:
   Exclude:
     - 'spec/grape/dsl/inside_route_spec.rb'
@@ -259,12 +267,6 @@ Style/CombinableLoops:
 Style/FormatStringToken:
   EnforcedStyle: template
 
-# Offense count: 1
-# This cop supports unsafe autocorrection (--autocorrect-all).
-Style/MapIntoArray:
-  Exclude:
-    - 'spec/support/chunks.rb'
-
 # Offense count: 12
 # Configuration parameters: AllowedMethods.
 # AllowedMethods: respond_to_missing?

CHANGELOG.md

@@ -28,6 +28,7 @@
 * [#2435](https://github.com/ruby-grape/grape/pull/2435): Use rack constants - [@ericproulx](https://github.com/ericproulx).
 * [#2436](https://github.com/ruby-grape/grape/pull/2436): Update coverallsapp github-action - [@ericproulx](https://github.com/ericproulx).
 * [#2434](https://github.com/ruby-grape/grape/pull/2434): Implement nested `with` support in parameter dsl - [@numbata](https://github.com/numbata).
+* [#2438](https://github.com/ruby-grape/grape/pull/2438): Fix some Rack::Lint - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

lib/grape/error_formatter/txt.rb

@@ -10,16 +10,17 @@ def call(message, backtrace, options = {}, env = nil, original_exception = nil)
           message = present(message, env)
 
           result = message.is_a?(Hash) ? ::Grape::Json.dump(message) : message
-          rescue_options = options[:rescue_options] || {}
-          if rescue_options[:backtrace] && backtrace && !backtrace.empty?
-            result += "\r\n backtrace:"
-            result += backtrace.join("\r\n ")
-          end
-          if rescue_options[:original_exception] && original_exception
-            result += "\r\n original exception:"
-            result += "\r\n #{original_exception.inspect}"
-          end
-          result
+          Array.wrap(result).tap do |final_result|
+            rescue_options = options[:rescue_options] || {}
+            if rescue_options[:backtrace] && backtrace.present?
+              final_result << 'backtrace:'
+              final_result.concat(backtrace)
+            end
+            if rescue_options[:original_exception] && original_exception
+              final_result << 'original exception:'
+              final_result << original_exception.inspect
+            end
+          end.join("\r\n ")
         end
       end
     end

lib/grape/middleware/formatter.rb

@@ -25,7 +25,7 @@ def after
         status, headers, bodies = *@app_response
 
         if Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.include?(status)
-          @app_response
+          [status, headers, []]
         else
           build_formatted_response(status, headers, bodies)
         end
@@ -83,12 +83,12 @@ def read_body_input
 
         return unless (input = env[Rack::RACK_INPUT])
 
-        input.rewind
+        rewind_input input
         body = env[Grape::Env::API_REQUEST_INPUT] = input.read
         begin
           read_rack_input(body) if body && !body.empty?
         ensure
-          input.rewind
+          rewind_input input
         end
       end
 
@@ -173,6 +173,10 @@ def mime_array
               .sort_by { |_, quality_preference| -(quality_preference ? quality_preference.to_f : 1.0) }
               .flat_map { |mime, _| [mime, mime.sub(vendor_prefix_pattern, '')] }
       end
+
+      def rewind_input(input)
+        input.rewind if input.respond_to?(:rewind)
+      end
     end
   end
 end

lib/grape/router.rb

@@ -88,26 +88,33 @@ def rotation(env, exact_route = nil)
 
     def transaction(env)
       input, method = *extract_input_and_method(env)
-      response = yield(input, method)
 
-      return response if response && !(cascade = cascade?(response))
+      # using a Proc is important since `return` will exit the enclosing function
+      cascade_or_return_response = proc do |response|
+        if response
+          cascade?(response).tap do |cascade|
+            return response unless cascade
 
+            # we need to close the body if possible before dismissing
+            response[2].close if response[2].respond_to?(:close)
+          end
+        end
+      end
+
+      last_response_cascade = cascade_or_return_response.call(yield(input, method))
       last_neighbor_route = greedy_match?(input)
 
       # If last_neighbor_route exists and request method is OPTIONS,
       # return response by using #call_with_allow_headers.
-      return call_with_allow_headers(env, last_neighbor_route) if last_neighbor_route && method == Rack::OPTIONS && !cascade
+      return call_with_allow_headers(env, last_neighbor_route) if last_neighbor_route && method == Rack::OPTIONS && !last_response_cascade
 
       route = match?(input, '*')
 
-      return last_neighbor_route.endpoint.call(env) if last_neighbor_route && cascade && route
+      return last_neighbor_route.endpoint.call(env) if last_neighbor_route && last_response_cascade && route
 
-      if route
-        response = process_route(route, env)
-        return response if response && !(cascade = cascade?(response))
-      end
+      last_response_cascade = cascade_or_return_response.call(process_route(route, env)) if route
 
-      return call_with_allow_headers(env, last_neighbor_route) if !cascade && last_neighbor_route
+      return call_with_allow_headers(env, last_neighbor_route) if !last_response_cascade && last_neighbor_route
 
       nil
     end

spec/grape/api/custom_validations_spec.rb

@@ -33,7 +33,7 @@ def validate_param!(attr_name, params)
         end
       end
     end
-    let(:app) { Rack::Builder.new(subject) }
+    let(:app) { subject }
 
     before { stub_const('Grape::Validations::Validators::DefaultLengthValidator', default_length_validator) }
 
@@ -75,7 +75,7 @@ def validate(request)
         end
       end
     end
-    let(:app) { Rack::Builder.new(subject) }
+    let(:app) { subject }
 
     before { stub_const('Grape::Validations::Validators::InBodyValidator', in_body_validator) }
 
@@ -111,7 +111,7 @@ def validate_param!(attr_name, _params)
         end
       end
     end
-    let(:app) { Rack::Builder.new(subject) }
+    let(:app) { subject }
 
     before { stub_const('Grape::Validations::Validators::WithMessageKeyValidator', message_key_validator) }
 
@@ -156,7 +156,7 @@ def access_header
       end
     end
 
-    let(:app) { Rack::Builder.new(subject) }
+    let(:app) { subject }
     let(:x_access_token_header) { 'x-access-token' }
 
     before { stub_const('Grape::Validations::Validators::AdminValidator', admin_validator) }

spec/grape/api_spec.rb

@@ -460,7 +460,7 @@ def to_txt
               subject.send(verb) do
                 env[Grape::Env::API_REQUEST_INPUT]
               end
-              send verb, '/', ::Grape::Json.dump(object), 'CONTENT_TYPE' => 'application/json', Grape::Http::Headers::HTTP_TRANSFER_ENCODING => 'chunked', 'CONTENT_LENGTH' => nil
+              send verb, '/', ::Grape::Json.dump(object), 'CONTENT_TYPE' => 'application/json', Grape::Http::Headers::HTTP_TRANSFER_ENCODING => 'chunked'
               expect(last_response.status).to eq(verb == :post ? 201 : 200)
               expect(last_response.body).to eql ::Grape::Json.dump(object).to_json
             end
@@ -1999,32 +1999,49 @@ def custom_error!(name)
     end
 
     context 'with multiple apis' do
-      let(:a) { Class.new(described_class) }
-      let(:b) { Class.new(described_class) }
+      let(:a) do
+        Class.new(described_class) do
+          namespace :a do
+            helpers do
+              def foo
+                error!('foo', 401)
+              end
+            end
 
-      before do
-        a.helpers do
-          def foo
-            error!('foo', 401)
+            rescue_from(:all) { foo }
+
+            get { raise 'boo' }
           end
         end
-        a.rescue_from(:all) { foo }
-        a.get { raise 'boo' }
-        b.helpers do
-          def foo
-            error!('bar', 401)
+      end
+      let(:b) do
+        Class.new(described_class) do
+          namespace :b do
+            helpers do
+              def foo
+                error!('bar', 401)
+              end
+            end
+
+            rescue_from(:all) { foo }
+
+            get { raise 'boo' }
           end
         end
-        b.rescue_from(:all) { foo }
-        b.get { raise 'boo' }
       end
 
-      it 'avoids polluting global namespace' do
-        env = Rack::MockRequest.env_for('/')
+      before do
+        subject.mount a
+        subject.mount b
+      end
 
-        expect(read_chunks(a.call(env)[2])).to eq(['foo'])
-        expect(read_chunks(b.call(env)[2])).to eq(['bar'])
-        expect(read_chunks(a.call(env)[2])).to eq(['foo'])
+      it 'avoids polluting global namespace' do
+        get '/a'
+        expect(last_response.body).to eq('foo')
+        get '/b'
+        expect(last_response.body).to eq('bar')
+        get '/a'
+        expect(last_response.body).to eq('foo')
       end
     end
 
@@ -3817,14 +3834,14 @@ def my_method
       it 'raised :error from middleware' do
         middleware = Class.new(Grape::Middleware::Base) do
           def before
-            throw :error, message: 'Unauthorized', status: 42
+            throw :error, message: 'Unauthorized', status: 500
           end
         end
         subject.use middleware
         subject.get do
         end
         get '/'
-        expect(last_response.status).to eq(42)
+        expect(last_response).to be_server_error
         expect(last_response.body).to eq({ error: 'Unauthorized' }.to_json)
       end
     end
@@ -3923,14 +3940,14 @@ def serializable_hash
       it 'raised :error from middleware' do
         middleware = Class.new(Grape::Middleware::Base) do
           def before
-            throw :error, message: 'Unauthorized', status: 42
+            throw :error, message: 'Unauthorized', status: 500
           end
         end
         subject.use middleware
         subject.get do
         end
         get '/'
-        expect(last_response.status).to eq(42)
+        expect(last_response.status).to eq(500)
         expect(last_response.body).to eq <<~XML
           <?xml version="1.0" encoding="UTF-8"?>
           <error>
@@ -4372,7 +4389,7 @@ def uniqe_id_route
     let(:app) do
       Class.new(described_class) do
         rescue_from :all do
-          rack_response('deprecated', 500)
+          rack_response('deprecated', 500, 'Content-Type' => 'text/plain')
         end
 
         get 'test' do

spec/grape/endpoint_spec.rb

@@ -153,14 +153,6 @@ def app
       x_grape_client_header = 'x-grape-client'
       expect(JSON.parse(last_response.body)[x_grape_client_header]).to eq('1')
     end
-
-    it 'includes headers passed as symbols' do
-      env = Rack::MockRequest.env_for('/headers')
-      env[:HTTP_SYMBOL_HEADER] = 'Goliath passes symbols'
-      body = read_chunks(subject.call(env)[2]).join
-      symbol_header = 'symbol-header'
-      expect(JSON.parse(body)[symbol_header]).to eq('Goliath passes symbols')
-    end
   end
 
   describe '#cookies' do
@@ -662,7 +654,7 @@ def app
       subject.post('/hey') do
         redirect '/ha'
       end
-      post '/hey', {}, 'HTTP_VERSION' => 'HTTP/1.1'
+      post '/hey', {}, 'HTTP_VERSION' => 'HTTP/1.1', 'SERVER_PROTOCOL' => 'HTTP/1.1'
       expect(last_response.status).to eq 303
       expect(last_response.location).to eq '/ha'
       expect(last_response.body).to eq 'An alternate resource is located at /ha.'
@@ -842,33 +834,33 @@ def memoized
   context 'anchoring' do
     describe 'delete 204' do
       it 'allows for the anchoring option with a delete method' do
-        subject.send(:delete, '/example', anchor: true) {}
-        send(:delete, '/example/and/some/more')
-        expect(last_response.status).to be 404
+        subject.delete('/example', anchor: true)
+        delete '/example/and/some/more'
+        expect(last_response).to be_not_found
       end
 
       it 'anchors paths by default for the delete method' do
-        subject.send(:delete, '/example') {}
-        send(:delete, '/example/and/some/more')
-        expect(last_response.status).to be 404
+        subject.delete '/example'
+        delete '/example/and/some/more'
+        expect(last_response).to be_not_found
       end
 
       it 'responds to /example/and/some/more for the non-anchored delete method' do
-        subject.send(:delete, '/example', anchor: false) {}
-        send(:delete, '/example/and/some/more')
-        expect(last_response.status).to be 204
+        subject.delete '/example', anchor: false
+        delete '/example/and/some/more'
+        expect(last_response).to be_no_content
         expect(last_response.body).to be_empty
       end
     end
 
     describe 'delete 200, with response body' do
       it 'responds to /example/and/some/more for the non-anchored delete method' do
-        subject.send(:delete, '/example', anchor: false) do
+        subject.delete('/example', anchor: false) do
           status 200
           body 'deleted'
         end
-        send(:delete, '/example/and/some/more')
-        expect(last_response.status).to be 200
+        delete '/example/and/some/more'
+        expect(last_response).to be_successful
         expect(last_response.body).not_to be_empty
       end
     end