Re: [PATCH 2/2] rack 3.x compatibility

[Eric Wong <bofh@yhbt.net>]

Stefan Sundin <git@stefansundin.com> wrote:
> Rack::Utils::HeaderHash will be removed in rack 3.1 so these changes mostly
> address that. The initializer in Rack::Headers inherits from Hash, so switching
> to the ::[] class method to achieve the same result.

OK.  Fwiw, I don't do much Ruby nowadays and will be keeping my
legacy projects Rack 2 for the forseeable future.

Patch 1 for the LGPL wording change is fine and I was able to
apply your patches without problems.

> The test test_broken_header_response was removed as Rack::Headers now throws
> an error in that case. I don't know if that necessarily is a good test case?

It's not a great test case, but I prefer we test malformed
responses in some way (more inline..)

> Since the rackup functionality was split out to its own gem, the test case
> test_lint_error_wrapper now only works with rack 3.x.

OK (more inline...)

> Other than that exception, all tests are passing with rack 1.6.13, 2.2.8,
> and 3.0.8.

Good to know.

> --- a/clogger.gemspec
> +++ b/clogger.gemspec
> @@ -16,8 +16,9 @@ Gem::Specification.new do |s|
>    s.test_files = %w(test/test_clogger.rb test/test_clogger_to_path.rb)
>  
>    # HeaderHash wasn't case-insensitive in old versions
> -  s.add_dependency(%q<rack>, ['>= 1.0', '< 3.0'])
> +  s.add_dependency(%q<rack>, ['>= 1.0', '< 4.0'])
>    s.add_development_dependency('test-unit', '~> 3.0')
> +  s.add_development_dependency('rackup', '~> 2.0')

I think rackup can be optional and we can rescue NameError
(below) if missing.

>  static VALUE g_HTTP_X_FORWARDED_FOR;
> @@ -628,7 +628,7 @@ static void append_response(struct clogger *c, VALUE key)
>  {
>  	VALUE v;
>  
> -	assert(rb_obj_is_kind_of(c->headers, cHeaderHash) && "not HeaderHash");
> +	assert(rb_obj_is_kind_of(c->headers, cRackHeaders) && "not Rack::Headers");

Please wrap at <80 columns (I need gigantic fonts to see) Thanks.

> +  RackHeaders = if Object.const_defined?("Rack::Headers")
> +    # Rack >= 3.0
> +    Rack::Headers
> +  else
> +    # Rack < 3.0
> +    Rack::Utils::HeaderHash
> +  end
> +
>    def initialize(app, opts = {})
>      # trigger autoload to avoid thread-safety issues later on
> -    Rack::Utils::HeaderHash
> +    RackHeaders

Minor: the explicit autoload trigger is no longer needed since the
above if/else already triggered it.

<snip>

> @@ -399,12 +408,12 @@ class TestClogger < Test::Unit::TestCase
>    end
>  
>    # rack allows repeated headers with "\n":
> -  # { 'Set-Cookie' => "a\nb" } =>
> -  #   Set-Cookie: a
> -  #   Set-Cookie: b
> +  # { 'set-cookie' => "a\nb" } =>
> +  #   set-cookie: a
> +  #   set-cookie: b
>    def test_escape_header_newlines
>      str = StringIO.new
> -    app = lambda { |env| [302, { 'Set-Cookie' => "a\nb" }, [] ] }
> +    app = lambda { |env| [302, { 'set-cookie' => "a\nb" }, [] ] }

Important:

Rack 3 uses arrays instead of "\n" to denote multi-values.
So I think the test (and clogger itself) need to to account
for array values (but continue to support Rack 2 "\n"-delimited
multi-values)

> -  def test_broken_header_response
> -    str = StringIO.new
> -    app = lambda { |env| [302, [ %w(a) ], []] }
> -    cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie')
> -    assert_nothing_raised { cl.call(@req) }
> -  end

It's fine if the broken header raises w/ Rack 3.
I mainly want to make sure a malformed response doesn't
trigger a segfault or memory corruption on our end.
So whether it raises or silently ignores the invalid response is
fine.

> @@ -877,14 +879,15 @@ class TestClogger < Test::Unit::TestCase
>    end
>  
>    def test_lint_error_wrapper
> -    require 'rack/lobster'
> +    # This test only works on Rack >= 3.0
> +    require 'rackup/lobster'

I prefer we rescue NameError here and skip tests if rackup or
lobster is missing.  It's not important to run the odd tests if
we can save some bandwidth and storage.

I also noticed some problems with Ruby 3.3, will post followups
in a bit.

Thanks.