From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <bofh@yhbt.net>
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net
X-Spam-Level: 
X-Spam-ASN:  
X-Spam-Status: No, score=-4.1 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
	T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no
	version=3.4.6
Received: from localhost (dcvr.yhbt.net [127.0.0.1])
	by dcvr.yhbt.net (Postfix) with ESMTP id 7866A1F405;
	Tue, 23 Jan 2024 12:47:30 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=yhbt.net;
	s=selector1; t=1706014050;
	bh=KWYcYNV0c33UfC7ytUbL7mxcI/GEPRQUjswOv025f5w=;
	h=Date:From:To:Cc:Subject:References:In-Reply-To:From;
	b=tGlmCQ2aGb6NhAeJn4rLt+RsvvrrLlvfhU+hOEniPAjuHPGCQ/sOVkzbKw/IcCDXB
	 ga2PRajW9lfO33hiYAoyujJaKE/0YTre3PdyJshZvMp8knEtL53qpfA7VMFU4RwesP
	 Gxr4ABP9A1nMD0A79rFW+0ENPHNB3Dc7XnoQXoMY=
Date: Tue, 23 Jan 2024 12:47:30 +0000
From: Eric Wong <bofh@yhbt.net>
To: Stefan Sundin <git@stefansundin.com>
Cc: clogger-public@yhbt.net
Subject: Re: [PATCH 2/2] rack 3.x compatibility
Message-ID: <20240123124730.M353391@dcvr>
References: <20240123031126.70784-1-git@stefansundin.com>
 <20240123031126.70784-3-git@stefansundin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240123031126.70784-3-git@stefansundin.com>
List-Id: <clogger-public.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.