From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS15169 209.85.128.0/17 X-Spam-Status: No, score=-3.1 required=3.0 tests=AWL,BAYES_00, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from mail-pj1-f44.google.com (mail-pj1-f44.google.com [209.85.216.44]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 1C4831F405 for ; Sun, 28 Jan 2024 01:28:35 +0000 (UTC) Received: by mail-pj1-f44.google.com with SMTP id 98e67ed59e1d1-290483f8c7bso1272521a91.3 for ; Sat, 27 Jan 2024 17:28:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706405314; x=1707010114; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=diGWaO8CoX+0kA4szkIRVvfYl7FrsulU/nqzTiOyBXs=; b=DhWa6UMXjxRtuUn/lQpYkNR+3t+nwtipzEolcl36REX/EhxxYZ9eLq0WUbX+aUStBm TNAytwmX+8Gpq7DTPohbAVT3v1ftaPluui0semNcjHw02WkNjhFderDxxftPy7leTkfN WG1BwBgAVN6vO+zABIVsZvmYOC8ZsXY10125QOCfyDRy0a0wxu+IAj6v/EFTuHaYX840 B+Gm0cyt21uC80raolIU+lhool4b0TdX0A2r+QcMTLFNogU0ClBIDhCywWyO9LgPu9O6 4JMfrKRXf6PsXroXneWqXpHr6T01cfVDlyhYa1ya+/+I7gjQdSzNBBH6xHfJOC+1D2o2 p6ng== X-Gm-Message-State: AOJu0YyxDuTzklD4m9Nbs+865/2SbVWeJNZt6uVoJawzMwhEdPvfTOVF svbycqlRMjqe4b1QiOfzajuGa9VC7EGgEyPxn66fULNmRqLAMt4dVWGhpapSyVk= X-Google-Smtp-Source: AGHT+IENV5xIanTRy4sMt4JQt6Zvteda8qG60EZ4Y+VRxcGg11CW7Ma5K6KdDpY9CkxPXeBwDIRs2Q== X-Received: by 2002:a17:90a:ec0e:b0:28c:8eaa:e5e3 with SMTP id l14-20020a17090aec0e00b0028c8eaae5e3mr1478037pjy.17.1706405314369; Sat, 27 Jan 2024 17:28:34 -0800 (PST) Received: from spot.lan ([2601:602:a300:5ffb:858:76dd:2bb6:ee4f]) by smtp.gmail.com with ESMTPSA id h22-20020a170902f7d600b001d731095eabsm2955061plw.244.2024.01.27.17.28.33 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Sat, 27 Jan 2024 17:28:34 -0800 (PST) From: Stefan Sundin To: bofh@yhbt.net Cc: clogger-public@yhbt.net, Stefan Sundin Subject: [PATCH v2 2/2] rack 3.x compatibility Date: Sat, 27 Jan 2024 17:23:00 -0800 Message-ID: <20240128012812.76997-3-git@stefansundin.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240128012812.76997-1-git@stefansundin.com> References: <20240123124730.M353391@dcvr> <20240128012812.76997-1-git@stefansundin.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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. --- clogger.gemspec | 3 +- ext/clogger_ext/clogger.c | 28 +++++++++++------ lib/clogger/pure.rb | 14 +++++++-- test/test_clogger.rb | 61 +++++++++++++++++++++++++----------- test/test_clogger_to_path.rb | 14 +++++---- 5 files changed, 83 insertions(+), 37 deletions(-) diff --git a/clogger.gemspec b/clogger.gemspec index ab479c5..7a08dc5 100644 --- 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, ['>= 1.0', '< 3.0']) + s.add_dependency(%q, ['>= 1.0', '< 4.0']) s.add_development_dependency('test-unit', '~> 3.0') + s.add_development_dependency('rackup', '~> 2.0') s.extensions = %w(ext/clogger_ext/extconf.rb) s.licenses = %w(LGPL-2.1-or-later) end diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c index 23aea39..1993ae8 100644 --- a/ext/clogger_ext/clogger.c +++ b/ext/clogger_ext/clogger.c @@ -136,7 +136,7 @@ static ID to_path_id; static ID respond_to_id; static VALUE cClogger; static VALUE mFormat; -static VALUE cHeaderHash; +static VALUE cRackHeaders; /* common hash lookup keys */ static VALUE g_HTTP_X_FORWARDED_FOR; @@ -172,6 +172,10 @@ static VALUE byte_xs(VALUE obj) { static const char esc[] = "0123456789ABCDEF"; unsigned char *new_ptr; + if (rb_obj_is_kind_of(obj, rb_cArray)) { + // Rack 3 + obj = rb_ary_join(obj, rb_str_new2("\n")); + } VALUE from = rb_obj_as_string(obj); const unsigned char *ptr = (const unsigned char *)RSTRING_PTR(from); long len = RSTRING_LEN(from); @@ -628,7 +632,8 @@ 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"); v = rb_funcall(c->headers, sq_brace_id, 1, key); v = NIL_P(v) ? g_dash : byte_xs(v); @@ -900,10 +905,9 @@ static VALUE ccall(struct clogger *c, VALUE env) rv = rb_ary_dup(rv); if (c->need_resp && - ! rb_obj_is_kind_of(c->headers, cHeaderHash)) { - c->headers = rb_funcall(cHeaderHash, new_id, 1, - c->headers); - rb_ary_store(rv, 1, c->headers); + ! rb_obj_is_kind_of(c->headers, cRackHeaders)) { + c->headers = rb_funcall(cRackHeaders, sq_brace_id, + 1, c->headers); } } else { VALUE tmp = rb_inspect(rv); @@ -1114,9 +1118,15 @@ void Init_clogger_ext(void) CONST_GLOBAL_STR2(rack_request_cookie_hash, "rack.request.cookie_hash"); tmp = rb_const_get(rb_cObject, rb_intern("Rack")); - tmp = rb_const_get(tmp, rb_intern("Utils")); - cHeaderHash = rb_const_get(tmp, rb_intern("HeaderHash")); - rb_ary_push(mark_ary, cHeaderHash); + if (rb_const_defined(tmp, rb_intern("Headers"))) { + // Rack >= 3.0 + cRackHeaders = rb_const_get(tmp, rb_intern("Headers")); + } else { + // Rack < 3.0 + tmp = rb_const_get(tmp, rb_intern("Utils")); + cRackHeaders = rb_const_get(tmp, rb_intern("HeaderHash")); + } + rb_ary_push(mark_ary, cRackHeaders); rb_obj_freeze(mark_ary); } diff --git a/lib/clogger/pure.rb b/lib/clogger/pure.rb index 4b38e90..156f11e 100644 --- a/lib/clogger/pure.rb +++ b/lib/clogger/pure.rb @@ -8,10 +8,15 @@ class Clogger attr_accessor :env, :status, :headers, :body attr_writer :body_bytes_sent, :start - def initialize(app, opts = {}) - # trigger autoload to avoid thread-safety issues later on + RackHeaders = if Object.const_defined?("Rack::Headers") + # Rack >= 3.0 + Rack::Headers + else + # Rack < 3.0 Rack::Utils::HeaderHash + end + def initialize(app, opts = {}) @app = app @logger = opts[:logger] path = opts[:path] @@ -35,7 +40,7 @@ class Clogger raise TypeError, "app response not a 3 element Array: #{resp.inspect}" end status, headers, body = resp - headers = Rack::Utils::HeaderHash.new(headers) if @need_resp + headers = RackHeaders[headers] if @need_resp if @wrap_body @reentrant = env['rack.multithread'] if @reentrant.nil? wbody = @reentrant ? self.dup : self @@ -91,6 +96,9 @@ private def byte_xs(s) s = s.dup + if s.is_a?(Array) + s = s.join("\n") + end s.force_encoding(Encoding::BINARY) if defined?(Encoding::BINARY) s.gsub!(/(['"\x00-\x1f\x7f-\xff])/) do |x| "\\x#{$1.unpack('H2').first.upcase}" diff --git a/test/test_clogger.rb b/test/test_clogger.rb index 1dee652..5646025 100644 --- a/test/test_clogger.rb +++ b/test/test_clogger.rb @@ -10,6 +10,16 @@ require "rack" require "clogger" +IS_RACK3 = Gem::Version.new(Rack.release) >= Gem::Version.new('3.0.0') + +RackHeaders = if Object.const_defined?("Rack::Headers") + # Rack >= 3.0 + Rack::Headers +else + # Rack < 3.0 + Rack::Utils::HeaderHash +end + # used to test subclasses class FooString < String end @@ -22,6 +32,7 @@ class TestClogger < Test::Unit::TestCase @nginx_fmt = "%d/%b/%Y:%H:%M:%S %z" @req = { "REQUEST_METHOD" => "GET", + "SERVER_PROTOCOL" => "HTTP/1.0", "HTTP_VERSION" => "HTTP/1.0", "HTTP_USER_AGENT" => 'echo and socat \o/', "PATH_INFO" => "/hello", @@ -85,13 +96,13 @@ class TestClogger < Test::Unit::TestCase def test_clen_stringio start = DateTime.now - 1 str = StringIO.new - app = lambda { |env| [ 301, {'Content-Length' => '5'}, ['abcde'] ] } + app = lambda { |env| [ 301, {'content-length' => '5'}, ['abcde'] ] } format = Common.dup assert format.gsub!(/response_length/, 'sent_http_content_length') cl = Clogger.new(app, :logger => str, :format => format) status, headers, body = cl.call(@req) assert_equal(301, status) - assert_equal({'Content-Length' => '5'}, headers) + assert_equal({'content-length' => '5'}, headers) body.each { |part| assert_equal('abcde', part) } str = str.string r = %r{\Ahome - - \[[^\]]+\] "GET /hello\?goodbye=true HTTP/1.0" 301 5\n\z} @@ -253,7 +264,7 @@ class TestClogger < Test::Unit::TestCase def test_rack_1_0 start = DateTime.now - 1 str = StringIO.new - app = lambda { |env| [ 200, {'Content-Length'=>'0'}, %w(a b c)] } + app = lambda { |env| [ 200, {'content-length'=>'0'}, %w(a b c)] } cl = Clogger.new(app, :logger => str, :format => Rack_1_0) status, headers, body = cl.call(@req) tmp = [] @@ -344,7 +355,7 @@ class TestClogger < Test::Unit::TestCase def test_combined start = DateTime.now - 1 str = StringIO.new - app = lambda { |env| [ 200, {'Content-Length'=>'3'}, %w(a b c)] } + app = lambda { |env| [ 200, {'content-length'=>'3'}, %w(a b c)] } cl = Clogger.new(app, :logger => str, :format => Combined) status, headers, body = cl.call(@req) tmp = [] @@ -365,7 +376,7 @@ class TestClogger < Test::Unit::TestCase def test_rack_errors_fallback err = StringIO.new - app = lambda { |env| [ 200, {'Content-Length'=>'3'}, %w(a b c)] } + app = lambda { |env| [ 200, {'content-length'=>'3'}, %w(a b c)] } cl = Clogger.new(app, :format => '$pid') req = @req.merge('rack.errors' => err) status, headers, body = cl.call(req) @@ -374,7 +385,7 @@ class TestClogger < Test::Unit::TestCase def test_body_close s_body = StringIO.new(%w(a b c).join("\n")) - app = lambda { |env| [ 200, {'Content-Length'=>'5'}, s_body] } + app = lambda { |env| [ 200, {'content-length'=>'5'}, s_body] } cl = Clogger.new(app, :logger => [], :format => '$pid') status, headers, body = cl.call(@req) assert ! s_body.closed? @@ -384,7 +395,7 @@ class TestClogger < Test::Unit::TestCase def test_escape str = StringIO.new - app = lambda { |env| [ 200, {'Content-Length'=>'5'}, [] ] } + app = lambda { |env| [ 200, {'content-length'=>'5'}, [] ] } cl = Clogger.new(app, :logger => str, :format => '$http_user_agent "$request"') @@ -398,13 +409,21 @@ class TestClogger < Test::Unit::TestCase assert_equal expect, str.string end - # rack allows repeated headers with "\n": - # { '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" }, [] ] } + # rack >= 3 allows repeated headers with array: + # { 'set-cookie' => ["a","b"] } => + # set-cookie: a + # set-cookie: b + # rack < 3 allows repeated headers with "\n": + # { 'set-cookie' => "a\nb" } => + # set-cookie: a + # set-cookie: b + def test_multiheader + str = StringIO.new + if IS_RACK3 then + app = lambda { |env| [302, { 'set-cookie' => ["a","b"] }, [] ] } + else + app = lambda { |env| [302, { 'set-cookie' => "a\nb" }, [] ] } + end cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie') cl.call(@req) assert_equal "a\\x0Ab\n", str.string @@ -463,12 +482,16 @@ class TestClogger < Test::Unit::TestCase 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) } + if IS_RACK3 then + assert_raise(ArgumentError) { cl.call(@req) } + else + assert_nothing_raised { cl.call(@req) } + end end def test_subclass_hash str = StringIO.new - req = Rack::Utils::HeaderHash.new(@req) + req = RackHeaders[@req] app = lambda { |env| [302, [ %w(a) ], []] } cl = Clogger.new(app, :logger => str, :format => Rack_1_0) assert_nothing_raised { cl.call(req).last.each {}.close } @@ -877,19 +900,21 @@ class TestClogger < Test::Unit::TestCase end def test_lint_error_wrapper - require 'rack/lobster' + require 'rackup/lobster' @req["SERVER_NAME"] = "FOO" @req["SERVER_PORT"] = "666" @req["rack.version"] = [1,1] @req["rack.multithread"] = true @req["rack.multiprocess"] = true @req["rack.run_once"] = false - app = Rack::ContentLength.new(Rack::ContentType.new(Rack::Lobster.new)) + app = Rack::ContentLength.new(Rack::ContentType.new(Rackup::Lobster.new)) cl = Clogger.new(app, :format => :Combined) @req["rack.errors"] = err = StringIO.new status, headers, body = r = Rack::Lint.new(cl).call(@req) body.each { |x| assert_kind_of String, x.to_str } body.close # might raise here assert_match(%r{GET /hello}, err.string) + rescue LoadError, Gem::ConflictError + # This test only works on Rack >= 3.0 end end diff --git a/test/test_clogger_to_path.rb b/test/test_clogger_to_path.rb index f74b991..4cc1738 100644 --- a/test/test_clogger_to_path.rb +++ b/test/test_clogger_to_path.rb @@ -3,6 +3,7 @@ $stderr.sync = $stdout.sync = true require "test/unit" require "date" require "stringio" +require "tempfile" require "rack" require "clogger" @@ -26,6 +27,7 @@ class TestCloggerToPath < Test::Unit::TestCase @req = { "REQUEST_METHOD" => "GET", "HTTP_VERSION" => "HTTP/1.0", + "SERVER_PROTOCOL" => "HTTP/1.0", "HTTP_USER_AGENT" => 'echo and socat \o/', "PATH_INFO" => "/", "QUERY_STRING" => "", @@ -50,8 +52,8 @@ class TestCloggerToPath < Test::Unit::TestCase app = Rack::Builder.new do tmp.syswrite(' ' * 365) h = { - 'Content-Length' => '0', - 'Content-Type' => 'text/plain', + 'content-length' => '0', + 'content-type' => 'text/plain', } use Clogger, :logger => logger, @@ -82,8 +84,8 @@ class TestCloggerToPath < Test::Unit::TestCase app = Rack::Builder.new do tmp.syswrite(' ' * 365) h = { - 'Content-Length' => '0', - 'Content-Type' => 'text/plain', + 'content-length' => '0', + 'content-type' => 'text/plain', } use Clogger, :logger => logger, @@ -106,8 +108,8 @@ class TestCloggerToPath < Test::Unit::TestCase logger = StringIO.new app = Rack::Builder.new do h = { - 'Content-Length' => '3', - 'Content-Type' => 'text/plain', + 'content-length' => '3', + 'content-type' => 'text/plain', } use Clogger, :logger => logger, -- 2.43.0