Allow tests to pass on both Rack 2 and Rack 3 (#477)

Fix a few other Rack 2/3 incompatibilities that we found here after merging all the other Rack 3 support changes:

- Stop replacing the session object with a plain symbolized hash. This was causing various errors from deep inside Rack. Instead, _wrap_ the session object in a new `Hanami::Action::Request::Session`, which delegates to the session and handles the translation of symbolized key access.
- Ensure `Rack::Response` sets an appropriate content length after we run our own `Response#body=` writer.
- Replace `Rack::File` (which has been removed) with `Rack::Files`. These are functionally identical, it's just a new name.
- Handle more header downcasing.
- Handle some response status strings that subtly differ between Rack 2/3.
- Update tests for new minimum session secret length.

Author: timriley

hanami-controller.gemspec

@@ -20,7 +20,7 @@ Gem::Specification.new do |spec|
   spec.metadata["rubygems_mfa_required"] = "true"
   spec.required_ruby_version = ">= 3.1"
 
-  spec.add_dependency "rack", ">= 2.0"
+  spec.add_dependency "rack", ">= 2.1"
   spec.add_dependency "hanami-utils", "~> 2.2"
   spec.add_dependency "dry-configurable", "~> 1.0", "< 2"
   spec.add_dependency "dry-core", "~> 1.0"

lib/hanami/action.rb

@@ -7,6 +7,7 @@
 require "hanami/utils/string"
 require "rack"
 require "rack/utils"
+require "hanami/action/rack_utils"
 require "zeitwerk"
 
 require_relative "action/constants"
@@ -561,7 +562,7 @@ def _run_after_callbacks(*args)
     #
     #   # Both Content-Type and X-No-Pass are removed because they're not allowed
     def keep_response_header?(header)
-      ENTITY_HEADERS.include?(header)
+      ENTITY_HEADERS.include?(header.downcase)
     end
 
     # @since 2.0.0

lib/hanami/action/constants.rb

@@ -50,14 +50,14 @@ class Action
     # @see http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.2.5
     # @see http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html
     ENTITY_HEADERS = {
-      "Allow" => true,
-      "Content-Encoding" => true,
-      "Content-Language" => true,
-      "Content-Location" => true,
-      "Content-MD5" => true,
-      "Content-Range" => true,
-      "Expires" => true,
-      "Last-Modified" => true,
+      "allow" => true,
+      "content-encoding" => true,
+      "content-language" => true,
+      "content-location" => true,
+      "content-md5" => true,
+      "content-range" => true,
+      "expires" => true,
+      "last-modified" => true,
       "extension-header" => true
     }.freeze
 

lib/hanami/action/params.rb

@@ -362,13 +362,7 @@ def _extract_params
       # @since 0.7.0
       # @api private
       def _router_params(fallback = {})
-        env.fetch(ROUTER_PARAMS) do
-          if session = fallback.delete(Action::RACK_SESSION)
-            fallback[Action::RACK_SESSION] = Utils::Hash.deep_symbolize(session)
-          end
-
-          fallback
-        end
+        env.fetch(ROUTER_PARAMS, fallback)
       end
     end
   end

lib/hanami/action/rack/file.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require "rack/file"
+require "rack/files"
 
 module Hanami
   class Action
@@ -21,7 +21,7 @@ class File
         # @since 0.4.3
         # @api private
         def initialize(path, root)
-          @file = ::Rack::File.new(root.to_s)
+          @file = ::Rack::Files.new(root.to_s)
           @path = path.to_s
         end
 

lib/hanami/action/rack_utils.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Hanami
+  class Action
+    # @since 2.2.0
+    # @api private
+    def self.rack_3?
+      defined?(::Rack::Headers)
+    end
+  end
+end

lib/hanami/action/request.rb

@@ -56,7 +56,7 @@ def session_enabled?
 
       # Returns the session for the request.
       #
-      # @return [Hash] the session object
+      # @return [Hanami::Request::Session] the session object
       #
       # @raise [MissingSessionError] if the session is not enabled
       #
@@ -70,7 +70,7 @@ def session
           raise Hanami::Action::MissingSessionError.new("Hanami::Action::Request#session")
         end
 
-        super
+        @session ||= Session.new(super)
       end
 
       # Returns the flash for the request.

lib/hanami/action/request/session.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+
+require "forwardable"
+
+module Hanami
+  class Action
+    class Request < ::Rack::Request
+      # Wrapper for Rack-provided sessions, allowing access using symbol keys.
+      #
+      # @since 2.3.0
+      # @api public
+      class Session
+        extend Forwardable
+
+        def_delegators \
+          :@session,
+          :clear,
+          :delete,
+          :empty?,
+          :size,
+          :length,
+          :each,
+          :to_h,
+          :inspect,
+          :keys,
+          :values
+
+        def initialize(session)
+          @session = session
+        end
+
+        def [](key)
+          @session[key.to_s]
+        end
+
+        def []=(key, value)
+          @session[key.to_s] = value
+        end
+
+        def key?(key)
+          @session.key?(key.to_s)
+        end
+
+        alias_method :has_key?, :key?
+        alias_method :include?, :key?
+
+        def ==(other)
+          Utils::Hash.deep_symbolize(@session) == Utils::Hash.deep_symbolize(other)
+        end
+
+        private
+
+        # Provides a fallback for any methods not handled by the def_delegators.
+        def method_missing(method_name, *args, &block)
+          if @session.respond_to?(method_name)
+            @session.send(method_name, *args, &block)
+          else
+            super
+          end
+        end
+
+        def respond_to_missing?(method_name, include_private = false)
+          @session.respond_to?(method_name, include_private) || super
+        end
+      end
+    end
+  end
+end

lib/hanami/action/response.rb

@@ -71,12 +71,15 @@ def initialize(request:, config:, content_type: nil, env: {}, headers: {}, view_
       # @api public
       def body=(str)
         @length = 0
-        @body   = EMPTY_BODY.dup
+        @body = EMPTY_BODY.dup
+
+        return if str.nil? || str == EMPTY_BODY
 
         if str.is_a?(::Rack::Files::BaseIterator)
           @body = str
+          buffered_body! # Ensure appropriate content-length is set
         else
-          write(str) unless str.nil? || str == EMPTY_BODY
+          write(str)
         end
       end
 

spec/integration/hanami/controller/head_spec.rb

@@ -20,8 +20,13 @@ def response
 
     expect(response.status).to be(200)
     expect(response.body.to_s).to be_empty
-    expect(headers).to include(["Content-Type", "application/octet-stream; charset=utf-8"])
-    expect(headers).to include(%w[Content-Length 5])
+    expect(headers).to include([rack_header("Content-Type"), "application/octet-stream; charset=utf-8"])
+
+    # In Rack 2, the Content-Length header is inferred from the body, even for HEAD requests.
+    # In Rack 3, the Content-Length header is no longer automatically set.
+    content_length = Hanami::Action.rack_3? ? "0" : "5"
+
+    expect(headers).to include([rack_header("Content-Length"), content_length])
   end
 
   it "allows to bypass restriction on custom headers" do
@@ -31,11 +36,11 @@ def response
     expect(response.body).to   eq("")
 
     headers = response.headers.to_a
-    expect(headers).to include(["Last-Modified", "Fri, 27 Nov 2015 13:32:36 GMT"])
-    expect(headers).to include(%w[X-Rate-Limit 4000])
+    expect(headers).to include([rack_header("Last-Modified"), "Fri, 27 Nov 2015 13:32:36 GMT"])
+    expect(headers).to include([rack_header("X-Rate-Limit"), "4000"])
 
-    expect(headers).to_not include(%w[X-No-Pass true])
-    expect(headers).to_not include(["Content-Type", "application/octet-stream; charset=utf-8"])
+    expect(headers).to_not include([rack_header("X-No-Pass"), "true"])
+    expect(headers).to_not include([rack_header("Content-Type"), "application/octet-stream; charset=utf-8"])
   end
 
   HTTP_TEST_STATUSES_WITHOUT_BODY.each do |code|
@@ -46,93 +51,93 @@ def response
 
           expect(response.status).to           be(code)
           expect(response.body).to             eq("")
-          expect(response.headers.to_a).to_not include(%w[X-Frame-Options DENY])
+          expect(response.headers.to_a).to_not include([rack_header("X-Frame-Options"), "DENY"])
         end
 
         it "doesn't send Content-Length header" do
           get "/code/#{code}"
 
           expect(response.status).to      be(code)
-          expect(response.headers).to_not have_key("Content-Length")
+          expect(response.headers).to_not have_key(rack_header("Content-Length"))
         end
       else
         it "does send body and default headers" do
           get "/code/#{code}"
 
           expect(response.status).to           be(code)
           expect(response.body).to_not         be_empty
-          expect(response.headers.to_a).to_not include(%w[X-Frame-Options DENY])
+          expect(response.headers.to_a).to_not include([rack_header("X-Frame-Options"), "DENY"])
         end
 
         it "does send Content-Length header" do
           get "/code/#{code}"
 
           expect(response.status).to      be(code)
-          expect(response.headers).to     have_key("Content-Length")
+          expect(response.headers).to     have_key(rack_header("Content-Length"))
         end
       end
 
       it "sends Allow header" do
         get "/code/#{code}"
 
-        expect(response.status).to           be(code)
-        expect(response.headers["Allow"]).to eq("GET, HEAD")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Allow")]).to eq("GET, HEAD")
       end
 
       it "sends Content-Encoding header" do
         get "/code/#{code}"
 
-        expect(response.status).to                      be(code)
-        expect(response.headers["Content-Encoding"]).to eq("identity")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Content-Encoding")]).to eq("identity")
       end
 
       it "sends Content-Language header" do
         get "/code/#{code}"
 
-        expect(response.status).to                      be(code)
-        expect(response.headers["Content-Language"]).to eq("en")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Content-Language")]).to eq("en")
       end
 
       it "doesn't send Content-Length header" do
         get "/code/#{code}"
 
         expect(response.status).to be(code)
-        expect(response.headers.keys).to_not include("Content-Length")
+        expect(response.headers.keys).to_not include(rack_header("Content-Length"))
       end
 
       it "doesn't send Content-Type header" do
         get "/code/#{code}"
 
         expect(response.status).to be(code)
-        expect(response.headers.keys).to_not include("Content-Type")
+        expect(response.headers.keys).to_not include(rack_header("Content-Type"))
       end
 
       it "sends Content-Location header" do
         get "/code/#{code}"
 
-        expect(response.status).to                      be(code)
-        expect(response.headers["Content-Location"]).to eq("relativeURI")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Content-Location")]).to eq("relativeURI")
       end
 
       it "sends Content-MD5 header" do
         get "/code/#{code}"
 
-        expect(response.status).to                 be(code)
-        expect(response.headers["Content-MD5"]).to eq("c13367945d5d4c91047b3b50234aa7ab")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Content-MD5")]).to eq("c13367945d5d4c91047b3b50234aa7ab")
       end
 
       it "sends Expires header" do
         get "/code/#{code}"
 
-        expect(response.status).to             be(code)
-        expect(response.headers["Expires"]).to eq("Thu, 01 Dec 1994 16:00:00 GMT")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Expires")]).to eq("Thu, 01 Dec 1994 16:00:00 GMT")
       end
 
       it "sends Last-Modified header" do
         get "/code/#{code}"
 
-        expect(response.status).to                   be(code)
-        expect(response.headers["Last-Modified"]).to eq("Wed, 21 Jan 2015 11:32:10 GMT")
+        expect(response.status).to be(code)
+        expect(response.headers[rack_header("Last-Modified")]).to eq("Wed, 21 Jan 2015 11:32:10 GMT")
       end
     end
   end