Skip to content

Commit 2f8732a

Browse files
authored
feat: Expand version support for http.rb v6 (#82)
This commit adds support for http.rb v6 while maintaining backward compatibility with v4 and v5. The http.rb v6 release changed from accepting an options hash to requiring keyword arguments for the HTTP::Client constructor and related methods. Key changes: - Update gemspec to allow http.rb versions up to 7.0.0 - Add VALID_HTTP_CLIENT_OPTIONS constant to filter options passed to HTTP::Client, preventing errors from unsupported options - Convert all HTTP client option keys from strings to symbols - Update HTTP::Client initialization and method calls to use keyword arguments (**options) instead of hash arguments - Add EOFError handling for improved connection cleanup in v6 - Add comprehensive test coverage for options filtering The filtering mechanism ensures that only valid HTTP client options are passed through, which is particularly important for maintaining backward compatibility when custom http_client_options are provided.
1 parent 37bb294 commit 2f8732a

3 files changed

Lines changed: 129 additions & 14 deletions

File tree

ld-eventsource.gemspec

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,5 +28,5 @@ Gem::Specification.new do |spec|
2828
spec.add_development_dependency 'webrick', '~> 1.7'
2929

3030
spec.add_runtime_dependency 'concurrent-ruby', '~> 1.0'
31-
spec.add_runtime_dependency 'http', '>= 4.4.1', '< 6.0.0'
31+
spec.add_runtime_dependency 'http', '>= 4.4.1', '< 7.0.0'
3232
end

lib/ld-eventsource/client.rb

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,15 @@ class Client
5252
# The default HTTP method for requests.
5353
DEFAULT_HTTP_METHOD = "GET"
5454

55+
# TODO(breaking): Remove this filtering once we have updated to the next major breaking version.
56+
# HTTP v6 requires keyword arguments instead of an options hash, so we filter to only known valid
57+
# arguments to avoid passing unsupported options.
58+
VALID_HTTP_CLIENT_OPTIONS = %i[
59+
base_uri body encoding features follow form headers json keep_alive_timeout
60+
nodelay params persistent proxy response retriable socket_class ssl_context
61+
ssl ssl_socket_class timeout_class timeout_options
62+
].freeze
63+
5564
#
5665
# Creates a new SSE client.
5766
#
@@ -118,15 +127,15 @@ def initialize(uri,
118127
@retry_enabled = retry_enabled
119128

120129
@headers = headers.clone
121-
@connect_timeout = connect_timeout
122-
@read_timeout = read_timeout
130+
@connect_timeout = connect_timeout&.to_f
131+
@read_timeout = read_timeout&.to_f
123132
@method = method.to_s.upcase
124133
@payload = payload
125134
@logger = logger || default_logger
126135

127136
base_http_client_options = {}
128137
if socket_factory
129-
base_http_client_options["socket_class"] = socket_factory
138+
base_http_client_options[:socket_class] = socket_factory
130139
end
131140

132141
if proxy
@@ -139,22 +148,29 @@ def initialize(uri,
139148
end
140149

141150
if @proxy
142-
base_http_client_options["proxy"] = {
151+
base_http_client_options[:proxy] = {
143152
:proxy_address => @proxy.host,
144153
:proxy_port => @proxy.port,
145154
}
146-
base_http_client_options["proxy"][:proxy_username] = @proxy.user unless @proxy.user.nil?
147-
base_http_client_options["proxy"][:proxy_password] = @proxy.password unless @proxy.password.nil?
155+
base_http_client_options[:proxy][:proxy_username] = @proxy.user unless @proxy.user.nil?
156+
base_http_client_options[:proxy][:proxy_password] = @proxy.password unless @proxy.password.nil?
148157
end
149158

150159
options = http_client_options.is_a?(Hash) ? base_http_client_options.merge(http_client_options) : base_http_client_options
160+
options = options.transform_keys(&:to_sym)
161+
options = options.select do |key, _|
162+
included = VALID_HTTP_CLIENT_OPTIONS.include?(key)
163+
@logger.warn { "Ignoring unsupported HTTP client option: #{key}" } unless included
164+
included
165+
end
166+
167+
timeout_options = {}
168+
timeout_options[:connect] = @connect_timeout if @connect_timeout
169+
timeout_options[:read] = @read_timeout if @read_timeout
151170

152-
@http_client = HTTP::Client.new(options)
171+
@http_client = HTTP::Client.new(**options)
153172
.follow
154-
.timeout({
155-
read: read_timeout,
156-
connect: connect_timeout,
157-
})
173+
@http_client = @http_client.timeout(timeout_options) unless timeout_options.empty?
158174
@cxn = nil
159175
@lock = Mutex.new
160176

@@ -342,7 +358,7 @@ def connect
342358
begin
343359
uri = build_uri_with_query_params
344360
@logger.info { "Connecting to event stream at #{uri}" }
345-
cxn = @http_client.request(@method, uri, build_opts)
361+
cxn = @http_client.request(@method, uri, **build_opts)
346362
headers = cxn.headers
347363
if cxn.status.code == 200
348364
content_type = cxn.content_type.mime_type
@@ -390,8 +406,10 @@ def read_stream(cxn)
390406
rescue HTTP::TimeoutError
391407
# For historical reasons, we rethrow this as our own type
392408
raise Errors::ReadTimeoutError.new(@read_timeout)
409+
rescue EOFError
410+
break
393411
end
394-
break if data.nil?
412+
break if data.nil? # keep for v5 compat
395413
gen.yield data
396414
end
397415
end

spec/client_spec.rb

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -908,6 +908,103 @@ def test_object.to_s
908908
end
909909
end
910910

911+
describe "http_client_options filtering" do
912+
it "filters out unsupported options" do
913+
with_server do |server|
914+
server.setup_response("/") do |req,res|
915+
send_stream_content(res, "", keep_open: true)
916+
end
917+
918+
client = subject.new(server.base_uri,
919+
http_client_options: {
920+
"socket_class" => "MySocket",
921+
"ssl" => { verify_mode: 0 },
922+
"not_a_real_option" => "should be removed",
923+
"another_fake" => 123,
924+
})
925+
926+
http_client = client.instance_variable_get(:@http_client)
927+
options = http_client.default_options
928+
929+
expect(options.socket_class).to eq("MySocket")
930+
expect(options.ssl).to eq({ verify_mode: 0 })
931+
932+
client.close
933+
end
934+
end
935+
936+
it "filters out unsupported options provided as symbols" do
937+
with_server do |server|
938+
server.setup_response("/") do |req,res|
939+
send_stream_content(res, "", keep_open: true)
940+
end
941+
942+
client = subject.new(server.base_uri,
943+
http_client_options: {
944+
socket_class: "MySocket",
945+
not_a_real_option: "should be removed",
946+
})
947+
948+
http_client = client.instance_variable_get(:@http_client)
949+
options = http_client.default_options
950+
951+
expect(options.socket_class).to eq("MySocket")
952+
953+
client.close
954+
end
955+
end
956+
957+
it "does not raise when only unsupported options are provided" do
958+
with_server do |server|
959+
server.setup_response("/") do |req,res|
960+
send_stream_content(res, "", keep_open: true)
961+
end
962+
963+
client = nil
964+
expect {
965+
client = subject.new(server.base_uri,
966+
http_client_options: {
967+
"totally_fake" => true,
968+
"also_fake" => "yes",
969+
})
970+
}.not_to raise_error
971+
972+
client.close
973+
end
974+
end
975+
976+
it "preserves all valid options" do
977+
with_server do |server|
978+
server.setup_response("/") do |req,res|
979+
send_stream_content(res, "", keep_open: true)
980+
end
981+
982+
socket_factory = double("SocketFactory")
983+
ssl_socket_factory = double("SSLSocketFactory")
984+
985+
client = subject.new(server.base_uri,
986+
http_client_options: {
987+
socket_class: socket_factory,
988+
ssl_socket_class: ssl_socket_factory,
989+
nodelay: true,
990+
keep_alive_timeout: 30,
991+
ssl: { verify_mode: 0 },
992+
})
993+
994+
http_client = client.instance_variable_get(:@http_client)
995+
options = http_client.default_options
996+
997+
expect(options.socket_class).to eq(socket_factory)
998+
expect(options.ssl_socket_class).to eq(ssl_socket_factory)
999+
expect(options.nodelay).to eq(true)
1000+
expect(options.keep_alive_timeout).to eq(30)
1001+
expect(options.ssl).to eq({ verify_mode: 0 })
1002+
1003+
client.close
1004+
end
1005+
end
1006+
end
1007+
9111008
describe "retry parameter" do
9121009
it "defaults to true (retries enabled)" do
9131010
events_body = simple_event_1_text

0 commit comments

Comments
 (0)