Skip to content

Conversation

Watson1978
Copy link
Contributor

@Watson1978 Watson1978 commented May 17, 2025

Which issue(s) this PR fixes:
Fixes #

What this PR does / why we need it:
At Ruby 3.5, CGI.parse will be removed.
https://bugs.ruby-lang.org/issues/21258

Therefore, the test fails where CGI.parse is used.

[master][~/src/fluentd]$ ruby -v -I"lib:test" test/plugin_helper/http_server/test_request.rb -v
ruby 3.5.0dev (2025-05-17T00:42:12Z master aa0f689bf4) +PRISM [x86_64-linux]
/home/watson/.rbenv/versions/3.5-dev/lib/ruby/gems/3.5.0+1/gems/async-pool-0.10.3/lib/async/pool/controller.rb:331: warning: assigned but unused variable - error
/home/watson/.rbenv/versions/3.5-dev/lib/ruby/gems/3.5.0+1/gems/traces-0.15.2/lib/traces/config.rb:44: warning: assigned but unused variable - error
/home/watson/src/fluentd/lib/fluent/plugin_helper/http_server/request.rb:17: warning: CGI library is removed from Ruby 3.5. Please use cgi/escape instead for CGI.escape and CGI.unescape features.
If you need to use the full features of CGI library, Please install cgi gem.
/home/watson/src/fluentd/lib/fluent/plugin_helper.rb:46: warning: method redefined; discarding old inherited
/home/watson/src/fluentd/lib/fluent/plugin_helper.rb:46: warning: previous definition of inherited was here
Loaded suite test/plugin_helper/http_server/test_request
Started
HttpHelperRequestTest:
  test_request:                                                                                                                 E
================================================================================================================================================
Error: test_request(HttpHelperRequestTest): NoMethodError: undefined method 'parse' for class CGI
/home/watson/src/fluentd/lib/fluent/plugin_helper/http_server/request.rb:38:in 'Fluent::PluginHelper::HttpServer::Request#query'
test/plugin_helper/http_server/test_request.rb:14:in 'HttpHelperRequestTest#test_request'
     11:
     12:     assert_equal('/path', request.path)
     13:     assert_equal('foo=42', request.query_string)
  => 14:     assert_equal({'foo'=>['42']}, request.query)
     15:     assert_equal('text/html', request.headers['content-type'])
     16:     assert_equal(['gzip'], request.headers['content-encoding'])
     17:   end
================================================================================================================================================
: (0.005199)

Finished in 0.005372395 seconds.
------------------------------------------------------------------------------------------------------------------------------------------------
1 tests, 2 assertions, 0 failures, 1 errors, 0 pendings, 0 omissions, 0 notifications
0% passed
------------------------------------------------------------------------------------------------------------------------------------------------
186.14 tests/s, 372.27 assertions/s

This patch will replace CGI.parse with URI.decode_www_form

Docs Changes:
Not needed.

Release Note:

  • in_monitor_agent: stop using CGI.parse due to support Ruby 3.5
  • HTTP server plugin helper: stop using CGI.parse due to support Ruby 3.5

Signed-off-by: Shizuo Fujita <fujita@clear-code.com>
@Watson1978 Watson1978 added CI Test/CI issues and removed CI Test/CI issues labels May 17, 2025
@Watson1978 Watson1978 changed the title http_server: use URI.decode_www_form instead of CGI.parse http_server: use URI.decode_www_form instead of CGI.parse due to support Ruby 3.5 May 17, 2025
@Watson1978 Watson1978 changed the title http_server: use URI.decode_www_form instead of CGI.parse due to support Ruby 3.5 in_monitor_agent: stop to use CGI.parse due to support Ruby 3.5 May 18, 2025
Signed-off-by: Shizuo Fujita <fujita@clear-code.com>
@Watson1978 Watson1978 marked this pull request as ready for review May 18, 2025 07:46
@daipom daipom added this to the v1.19.0 milestone May 19, 2025
@daipom daipom self-requested a review May 19, 2025 02:41
@daipom
Copy link
Contributor

daipom commented May 19, 2025

Note: About compatibility of Request#query of Http Server Plugin Helper.

This fix maintains the compatibility. (Thanks!)

On the other hand, if we could abandon the compatibility, then it would be simpler and more useful.

def query
  @query_string && Hash[URI.decode_www_form(@query_string)]
end

Before: "a=foo&b=bar" => {"a"=>["foo"], "b"=>["bar"]}
After: "a=foo&b=bar" => {"a"=>"foo", "b"=>"bar"}

This would be more natural as a parsed result.
In addition, we can remove the annoying code of in_monitor_agent that is taking values by using Hash.new { |_, _| [] } and .first.

As a plugin helper, it would be very important to maintain compatibility, but the current specification seems inconvenient, so it might be good if it could be improved (maybe in the future).

Signed-off-by: Shizuo Fujita <fujita@clear-code.com>
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@daipom daipom changed the title in_monitor_agent: stop to use CGI.parse due to support Ruby 3.5 Stop using CGI.parse due to support Ruby 3.5 May 19, 2025
@daipom daipom merged commit 2f8cce7 into fluent:master May 19, 2025
9 of 13 checks passed
@Watson1978 Watson1978 deleted the cgi-parse branch May 19, 2025 05:21
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.

2 participants