Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't add content-length header. #347

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ioquatix
Copy link
Member

@last_response.finish modifies the response, and in Rack 3.1 it can add content-length which in some cases is unexpected.

Rails is trying to test “streaming” - #each without #to_ary - and it’s tricky.

      def process_request(uri, env)
        env['HTTP_COOKIE'] ||= cookie_jar.for(uri)
        @last_request = Rack::Request.new(env)
        status, headers, body = @app.call(env).to_a

        @last_response = MockResponse.new(status, headers, body, env['rack.errors'].flush)
        close_body(body)
        cookie_jar.merge(last_response.headers['set-cookie'], uri)
        @after_request.each(&:call)
        @last_response.finish. # <-------------------- Adds 'content-length' header

        yield @last_response if block_given?

        @last_response
      end

The above code comes from Rack::Test::Session. The problem is, it adds content-length header which was not present in the original response. It’s reasonable to do that if you are dealing with an actual response… But rails is interested in what was returned by the application - not some normalised “response”.

Maybe the change to Rack::Response#finish was problematic: rack/rack#2149

test.rb:

#!/usr/bin/env ruby

$LOAD_PATH.unshift File.expand_path('lib', __dir__)
require 'rack'

response = Rack::MockResponse.new(200, {}, 'Hello, World!')

pp response
response.finish
pp response

3-0-stable:

#<Rack::MockResponse:0x000000011bc7af08
 @block=nil,
 @body=["Hello, World!"],
 @buffered=true,
 @cookies={},
 @errors="",
 @headers={},
 @length=13,
 @original_headers={},
 @status=200,
 @writer=#<Method: Rack::MockResponse(Rack::Response::Helpers)#append(chunk) /Users/samuel/Developer/rack/rack/lib/rack/response.rb:353>>
#<Rack::MockResponse:0x000000011bc7af08
 @block=nil,
 @body=["Hello, World!"],
 @buffered=true,
 @cookies={},
 @errors="",
 @headers={},
 @length=13,
 @original_headers={},
 @status=200,
 @writer=#<Method: Rack::MockResponse(Rack::Response::Helpers)#append(chunk) /Users/samuel/Developer/rack/rack/lib/rack/response.rb:353>>

3-1-stable:

#<Rack::MockResponse:0x000000011dab97f8
 @block=nil,
 @body=["Hello, World!"],
 @buffered=true,
 @cookies={},
 @errors="",
 @headers={},
 @length=13,
 @original_headers={},
 @status=200,
 @writer=#<Method: Rack::MockResponse(Rack::Response::Helpers)#append(chunk) /Users/samuel/Developer/rack/rack/lib/rack/response.rb:353>>
#<Rack::MockResponse:0x000000011dab97f8
 @block=nil,
 @body=["Hello, World!"],
 @buffered=true,
 @cookies={},
 @errors="",
 @headers={"content-length"=>"13"},
 @length=13,
 @original_headers={},
 @status=200,
 @writer=#<Method: Rack::MockResponse(Rack::Response::Helpers)#append(chunk) /Users/samuel/Developer/rack/rack/lib/rack/response.rb:353>>

I actually don’t think the latter behaviour is bad - if we know the content-length we can add it… and we do in that case. So, maybe rack-test should not invoke #finish?

@ioquatix
Copy link
Member Author

It's interesting to see a lot of tests fail. Rack::Response can add content-length if you write to it... I wonder if that's what's happening here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant