Merge pull request #2435 from ericproulx/use_rack_constant

Use Rack's Constants
ruby-grape · May 4, 2024 · 3ae7b7a · 3ae7b7a
2 parents 96c2a91 + e0cb8a9
commit 3ae7b7a
Show file tree

Hide file tree

Showing 44 changed files with 418 additions and 376 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -33,9 +33,6 @@ jobs:
           - ruby: '2.7'
             gemfile: gemfiles/multi_xml.gemfile
             specs: 'spec/integration/multi_xml'
-          - ruby: '2.7'
-            gemfile: gemfiles/rack_2_0.gemfile
-            specs: 'spec/integration/rack_2_0'
           - ruby: '2.7'
             gemfile: gemfiles/rack_3_0.gemfile
             specs: 'spec/integration/rack_3_0'

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@
 * [#2425](https://github.com/ruby-grape/grape/pull/2425): Replace `{}` with `Rack::Header` or `Rack::Utils::HeaderHash` - [@dhruvCW](https://github.com/dhruvCW).
 * [#2430](https://github.com/ruby-grape/grape/pull/2430): Isolate extensions within specific gemfile - [@ericproulx](https://github.com/ericproulx).
 * [#2431](https://github.com/ruby-grape/grape/pull/2431): Drop appraisals in favor of eval_gemfile - [@ericproulx](https://github.com/ericproulx).
+* [#2435](https://github.com/ruby-grape/grape/pull/2435): Use rack constants - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

diff --git a/benchmark/large_model.rb b/benchmark/large_model.rb
@@ -233,7 +233,7 @@ def self.vrp_request_schedule(this)
 puts Grape::VERSION
 
 options = {
-  method: 'POST',
+  method: Rack::POST,
   params: JSON.parse(File.read('benchmark/resource/vrp_example.json'))
 }
 

diff --git a/benchmark/nested_params.rb b/benchmark/nested_params.rb
@@ -21,7 +21,7 @@ class API < Grape::API
 end
 
 options = {
-  method: 'POST',
+  method: Rack::POST,
   params: {
     address: {
       street: 'Alexis Pl.',

diff --git a/benchmark/remounting.rb b/benchmark/remounting.rb
@@ -28,7 +28,7 @@ class CommentAPI < Grape::API
   mount VotingApi
 end
 
-env = Rack::MockRequest.env_for('/votes', method: 'GET')
+env = Rack::MockRequest.env_for('/votes', method: Rack::GET)
 
 Benchmark.memory do |api|
   calls = 1000

diff --git a/benchmark/simple.rb b/benchmark/simple.rb
@@ -13,7 +13,7 @@ class API < Grape::API
 end
 
 options = {
-  method: 'GET'
+  method: Rack::GET
 }
 
 env = Rack::MockRequest.env_for('/api/v1', options)

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -15,5 +15,3 @@ services:
     volumes:
       - .:/var/grape
       - gems:/usr/local/bundle
-    environment:
-      GEMFILE: multi_xml
diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb
@@ -160,9 +160,13 @@ def initialize
 
       # Handle a request. See Rack documentation for what `env` is.
       def call(env)
-        result = @router.call(env)
-        result[1].delete(Grape::Http::Headers::X_CASCADE) unless cascade?
-        result
+        status, headers, response = @router.call(env)
+        unless cascade?
+          headers = Grape::Util::Header.new.merge(headers)
+          headers.delete(Grape::Http::Headers::X_CASCADE)
+        end
+
+        [status, headers, response]
       end
 
       # Some requests may return a HTTP 404 error if grape cannot find a matching
@@ -201,11 +205,11 @@ def add_head_not_allowed_methods_and_options_methods
 
               allowed_methods = config[:methods].dup
 
-              allowed_methods |= [Grape::Http::Headers::HEAD] if !self.class.namespace_inheritable(:do_not_route_head) && allowed_methods.include?(Grape::Http::Headers::GET)
+              allowed_methods |= [Rack::HEAD] if !self.class.namespace_inheritable(:do_not_route_head) && allowed_methods.include?(Rack::GET)
 
-              allow_header = (self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Grape::Http::Headers::OPTIONS] | allowed_methods)
+              allow_header = (self.class.namespace_inheritable(:do_not_route_options) ? allowed_methods : [Rack::OPTIONS] | allowed_methods)
 
-              config[:endpoint].options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Grape::Http::Headers::OPTIONS)
+              config[:endpoint].options[:options_route_enabled] = true unless self.class.namespace_inheritable(:do_not_route_options) || allowed_methods.include?(Rack::OPTIONS)
 
               attributes = config.merge(allowed_methods: allowed_methods, allow_header: allow_header)
               generate_not_allowed_method(config[:pattern], **attributes)

diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb
@@ -200,7 +200,7 @@ def redirect(url, permanent: false, body: nil, **_options)
         if permanent
           status 301
           body_message ||= "This resource has been moved permanently to #{url}."
-        elsif env[Grape::Http::Headers::HTTP_VERSION] == 'HTTP/1.1' && request.request_method.to_s.upcase != Grape::Http::Headers::GET
+        elsif http_version == 'HTTP/1.1' && !request.get?
           status 303
           body_message ||= "An alternate resource is located at #{url}."
         else
@@ -226,10 +226,9 @@ def status(status = nil)
         when nil
           return @status if instance_variable_defined?(:@status) && @status
 
-          case request.request_method.to_s.upcase
-          when Grape::Http::Headers::POST
+          if request.post?
             201
-          when Grape::Http::Headers::DELETE
+          elsif request.delete?
             if instance_variable_defined?(:@body) && @body.present?
               200
             else
@@ -351,7 +350,7 @@ def stream(value = nil)
         return if value.nil? && @stream.nil?
 
         header Rack::CONTENT_LENGTH, nil
-        header Grape::Http::Headers::TRANSFER_ENCODING, nil
+        header Rack::TRANSFER_ENCODING, nil
         header Rack::CACHE_CONTROL, 'no-cache' # Skips ETag generation (reading the response up front)
         if value.is_a?(String)
           file_body = Grape::ServeStream::FileBody.new(value)
@@ -458,6 +457,10 @@ def entity_representation_for(entity_class, object, options)
         embeds[:version] = env[Grape::Env::API_VERSION] if env.key?(Grape::Env::API_VERSION)
         entity_class.represent(object, **embeds.merge(options))
       end
+
+      def http_version
+        env['HTTP_VERSION'] || env[Rack::SERVER_PROTOCOL]
+      end
     end
   end
 end
diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb
@@ -151,7 +151,7 @@ def mount_in(router)
         reset_routes!
         routes.each do |route|
           methods = [route.request_method]
-          methods << Grape::Http::Headers::HEAD if !namespace_inheritable(:do_not_route_head) && route.request_method == Grape::Http::Headers::GET
+          methods << Rack::HEAD if !namespace_inheritable(:do_not_route_head) && route.request_method == Rack::GET
           methods.each do |method|
             route = Grape::Router::Route.new(method, route.origin, **route.attributes.to_h) unless route.request_method == method
             router.append(route.apply(self))
@@ -401,7 +401,7 @@ def validations
 
     def options?
       options[:options_route_enabled] &&
-        env[Grape::Http::Headers::REQUEST_METHOD] == Grape::Http::Headers::OPTIONS
+        env[Rack::REQUEST_METHOD] == Rack::OPTIONS
     end
 
     def method_missing(name, *_args)

diff --git a/lib/grape/env.rb b/lib/grape/env.rb
@@ -11,11 +11,6 @@ module Env
     API_VENDOR = 'api.vendor'
     API_FORMAT = 'api.format'
 
-    RACK_INPUT = 'rack.input'
-    RACK_REQUEST_QUERY_HASH = 'rack.request.query_hash'
-    RACK_REQUEST_FORM_HASH = 'rack.request.form_hash'
-    RACK_REQUEST_FORM_INPUT = 'rack.request.form_input'
-
     GRAPE_REQUEST = 'grape.request'
     GRAPE_REQUEST_HEADERS = 'grape.request.headers'
     GRAPE_REQUEST_PARAMS = 'grape.request.params'

diff --git a/lib/grape/http/headers.rb b/lib/grape/http/headers.rb
@@ -3,44 +3,25 @@
 module Grape
   module Http
     module Headers
-      # https://github.com/rack/rack/blob/master/lib/rack.rb
-      HTTP_VERSION    = 'HTTP_VERSION'
-      PATH_INFO       = 'PATH_INFO'
-      REQUEST_METHOD  = 'REQUEST_METHOD'
-      QUERY_STRING    = 'QUERY_STRING'
-
-      def self.lowercase?
-        Rack::CONTENT_TYPE == 'content-type'
-      end
-
-      if lowercase?
-        ALLOW             = 'allow'
-        LOCATION          = 'location'
-        TRANSFER_ENCODING = 'transfer-encoding'
-        X_CASCADE         = 'x-cascade'
-      else
-        ALLOW             = 'Allow'
-        LOCATION          = 'Location'
-        TRANSFER_ENCODING = 'Transfer-Encoding'
-        X_CASCADE         = 'X-Cascade'
-      end
-
-      GET     = 'GET'
-      POST    = 'POST'
-      PUT     = 'PUT'
-      PATCH   = 'PATCH'
-      DELETE  = 'DELETE'
-      HEAD    = 'HEAD'
-      OPTIONS = 'OPTIONS'
-
-      SUPPORTED_METHODS = [GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS].freeze
-      SUPPORTED_METHODS_WITHOUT_OPTIONS = Grape::Util::Lazy::Object.new { [GET, POST, PUT, PATCH, DELETE, HEAD].freeze }
-
-      HTTP_ACCEPT_VERSION    = 'HTTP_ACCEPT_VERSION'
+      HTTP_ACCEPT_VERSION = 'HTTP_ACCEPT_VERSION'
+      HTTP_ACCEPT = 'HTTP_ACCEPT'
       HTTP_TRANSFER_ENCODING = 'HTTP_TRANSFER_ENCODING'
-      HTTP_ACCEPT            = 'HTTP_ACCEPT'
 
-      FORMAT                 = 'format'
+      ALLOW = 'Allow'
+      LOCATION = 'Location'
+      X_CASCADE = 'X-Cascade'
+
+      SUPPORTED_METHODS = [
+        Rack::GET,
+        Rack::POST,
+        Rack::PUT,
+        Rack::PATCH,
+        Rack::DELETE,
+        Rack::HEAD,
+        Rack::OPTIONS
+      ].freeze
+
+      SUPPORTED_METHODS_WITHOUT_OPTIONS = (SUPPORTED_METHODS - [Rack::OPTIONS]).freeze
 
       HTTP_HEADERS = Grape::Util::Lazy::Object.new do
         common_http_headers = %w[

diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb
@@ -40,7 +40,7 @@ def call!(env)
 
       def rack_response(status, headers, message)
         message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML
-        Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), headers)
+        Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), Grape::Util::Header.new.merge(headers))
       end
 
       def format_message(message, backtrace, original_exception = nil)

diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb
@@ -4,6 +4,7 @@ module Grape
   module Middleware
     class Formatter < Base
       CHUNKED = 'chunked'
+      FORMAT = 'format'
 
       def default_options
         {
@@ -80,7 +81,7 @@ def read_body_input
           !request.parseable_data? &&
           (request.content_length.to_i.positive? || request.env[Grape::Http::Headers::HTTP_TRANSFER_ENCODING] == CHUNKED)
 
-        return unless (input = env[Grape::Env::RACK_INPUT])
+        return unless (input = env[Rack::RACK_INPUT])
 
         input.rewind
         body = env[Grape::Env::API_REQUEST_INPUT] = input.read
@@ -101,12 +102,12 @@ def read_rack_input(body)
           begin
             body = (env[Grape::Env::API_REQUEST_BODY] = parser.call(body, env))
             if body.is_a?(Hash)
-              env[Grape::Env::RACK_REQUEST_FORM_HASH] = if env.key?(Grape::Env::RACK_REQUEST_FORM_HASH)
-                                                          env[Grape::Env::RACK_REQUEST_FORM_HASH].merge(body)
-                                                        else
-                                                          body
-                                                        end
-              env[Grape::Env::RACK_REQUEST_FORM_INPUT] = env[Grape::Env::RACK_INPUT]
+              env[Rack::RACK_REQUEST_FORM_HASH] = if env.key?(Rack::RACK_REQUEST_FORM_HASH)
+                                                    env[Rack::RACK_REQUEST_FORM_HASH].merge(body)
+                                                  else
+                                                    body
+                                                  end
+              env[Rack::RACK_REQUEST_FORM_INPUT] = env[Rack::RACK_INPUT]
             end
           rescue Grape::Exceptions::Base => e
             raise e
@@ -139,7 +140,7 @@ def format_from_extension
       end
 
       def format_from_params
-        fmt = Rack::Utils.parse_nested_query(env[Grape::Http::Headers::QUERY_STRING])[Grape::Http::Headers::FORMAT]
+        fmt = Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[FORMAT]
         # avoid symbol memory leak on an unknown format
         return fmt.to_sym if content_type_for(fmt)
 

diff --git a/lib/grape/middleware/globals.rb b/lib/grape/middleware/globals.rb
@@ -7,7 +7,7 @@ def before
         request = Grape::Request.new(@env, build_params_with: @options[:build_params_with])
         @env[Grape::Env::GRAPE_REQUEST] = request
         @env[Grape::Env::GRAPE_REQUEST_HEADERS] = request.headers
-        @env[Grape::Env::GRAPE_REQUEST_PARAMS] = request.params if @env[Grape::Env::RACK_INPUT]
+        @env[Grape::Env::GRAPE_REQUEST_PARAMS] = request.params if @env[Rack::RACK_INPUT]
       end
     end
   end

diff --git a/lib/grape/middleware/versioner/param.rb b/lib/grape/middleware/versioner/param.rb
@@ -28,12 +28,12 @@ def default_options
         end
 
         def before
-          potential_version = Rack::Utils.parse_nested_query(env[Grape::Http::Headers::QUERY_STRING])[paramkey]
+          potential_version = Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[paramkey]
           return if potential_version.nil?
 
           throw :error, status: 404, message: '404 API Version Not Found', headers: { Grape::Http::Headers::X_CASCADE => 'pass' } if options[:versions] && !options[:versions].find { |v| v.to_s == potential_version }
           env[Grape::Env::API_VERSION] = potential_version
-          env[Grape::Env::RACK_REQUEST_QUERY_HASH].delete(paramkey) if env.key? Grape::Env::RACK_REQUEST_QUERY_HASH
+          env[Rack::RACK_REQUEST_QUERY_HASH].delete(paramkey) if env.key? Rack::RACK_REQUEST_QUERY_HASH
         end
 
         private

diff --git a/lib/grape/middleware/versioner/path.rb b/lib/grape/middleware/versioner/path.rb
@@ -24,7 +24,7 @@ def default_options
         end
 
         def before
-          path = env[Grape::Http::Headers::PATH_INFO].dup
+          path = env[Rack::PATH_INFO].dup
           path.sub!(mount_path, '') if mounted_path?(path)
 
           if prefix && path.index(prefix) == 0 # rubocop:disable all

diff --git a/lib/grape/router.rb b/lib/grape/router.rb
@@ -96,7 +96,7 @@ def transaction(env)
 
       # 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 == Grape::Http::Headers::OPTIONS && !cascade
+      return call_with_allow_headers(env, last_neighbor_route) if last_neighbor_route && method == Rack::OPTIONS && !cascade
 
       route = match?(input, '*')
 
@@ -123,8 +123,8 @@ def make_routing_args(default_args, route, input)
     end
 
     def extract_input_and_method(env)
-      input = string_for(env[Grape::Http::Headers::PATH_INFO])
-      method = env[Grape::Http::Headers::REQUEST_METHOD]
+      input = string_for(env[Rack::PATH_INFO])
+      method = env[Rack::REQUEST_METHOD]
       [input, method]
     end
 

diff --git a/spec/grape/api/custom_validations_spec.rb b/spec/grape/api/custom_validations_spec.rb
@@ -71,7 +71,7 @@ def validate_param!(attr_name, params)
     let(:in_body_validator) do
       Class.new(Grape::Validations::Validators::PresenceValidator) do
         def validate(request)
-          validate!(request.env['api.request.body'])
+          validate!(request.env[Grape::Env::API_REQUEST_BODY])
         end
       end
     end

diff --git a/spec/grape/api/defines_boolean_in_params_spec.rb b/spec/grape/api/defines_boolean_in_params_spec.rb
@@ -24,7 +24,7 @@
     end
 
     context 'Params endpoint type' do
-      subject { app.new.router.map['POST'].first.options[:params]['message'][:type] }
+      subject { app.new.router.map[Rack::POST].first.options[:params]['message'][:type] }
 
       it 'params type is a boolean' do
         expect(subject).to eq 'Grape::API::Boolean'

diff --git a/spec/grape/api/patch_method_helpers_spec.rb b/spec/grape/api/patch_method_helpers_spec.rb
@@ -49,12 +49,12 @@ def app
 
   context 'patch' do
     it 'public' do
-      patch '/', {}, 'HTTP_ACCEPT' => 'application/vnd.grape-public-v1+json'
+      patch '/', {}, Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.grape-public-v1+json'
       expect(last_response.status).to eq 405
     end
 
     it 'private' do
-      patch '/', {}, 'HTTP_ACCEPT' => 'application/vnd.grape-private-v1+json'
+      patch '/', {}, Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.grape-private-v1+json'
       expect(last_response.status).to eq 405
     end
 
@@ -66,13 +66,13 @@ def app
 
   context 'default' do
     it 'public' do
-      get '/', {}, 'HTTP_ACCEPT' => 'application/vnd.grape-public-v1+json'
+      get '/', {}, Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.grape-public-v1+json'
       expect(last_response.status).to eq 200
       expect(last_response.body).to eq({ ok: 'public' }.to_json)
     end
 
     it 'private' do
-      get '/', {}, 'HTTP_ACCEPT' => 'application/vnd.grape-private-v1+json'
+      get '/', {}, Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.grape-private-v1+json'
       expect(last_response.status).to eq 200
       expect(last_response.body).to eq({ ok: 'private' }.to_json)
     end