Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure h2 Websocket requests include :scheme #60

Merged
merged 1 commit into from
Dec 9, 2023
Merged

Ensure h2 Websocket requests include :scheme #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/async/websocket/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,14 @@ def close
end
end

def connect(authority, path, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
def connect(authority, path, scheme: @delegate.scheme, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
Copy link
Member

Choose a reason for hiding this comment

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

This looks okay to me, but realistically speaking, is there ever going to be a case where we want to override the scheme in this way? We can always introduce an argument later, but maybe we can use @delegate.scheme on line 100 for now? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't presently have a concrete use case for the extra arg. It does pass through from def self.connect(..., **options) on line 55, so it's a little more versatile than at first glance.

Because of that pass through, my intention was to parallel the usage of Async::HTTP::Client where high-level method calls already support overriding scheme: apart from the endpoint. If that parallelism seems worth it, then perhaps keep it? Otherwise, it's no problem to remove the arg. I'll let you make the call. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def connect(authority, path, scheme: @delegate.scheme, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
def connect(authority, path, headers: nil, handler: Connection, extensions: ::Protocol::WebSocket::Extensions::Client.default, **options, &block)
scheme = @delegate.scheme

headers = ::Protocol::HTTP::Headers[headers]

extensions&.offer do |extension|
headers.add(SEC_WEBSOCKET_EXTENSIONS, extension.join("; "))
end

request = Request.new(nil, authority, path, headers, **options)
request = Request.new(scheme, authority, path, headers, **options)

pool = @delegate.pool
connection = pool.acquire
Expand Down
25 changes: 25 additions & 0 deletions test/async/websocket/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Copyright, 2023, by Thomas Morgan.

require 'async/websocket/client'
require 'async/websocket/adapters/http'

require 'sus/fixtures/async/http/server_context'

Expand Down Expand Up @@ -90,6 +91,30 @@
end
end

with '#connect' do
let(:app) do
Protocol::HTTP::Middleware.for do |request|
Async::WebSocket::Adapters::HTTP.open(request) do |connection|
connection.send_text("authority: #{request.authority}")
connection.send_text("path: #{request.path}")
connection.send_text("protocol: #{Array(request.protocol).inspect}")
connection.send_text("scheme: #{request.scheme}")
connection.close
end or Protocol::HTTP::Response[404, {}, []]
end
end

it "fully populates the request" do
connection = Async::WebSocket::Client.connect(client_endpoint)
expect(connection.read.to_str).to be =~ /authority: localhost:\d+/
expect(connection.read.to_str).to be == 'path: /'
expect(connection.read.to_str).to be == 'protocol: ["websocket"]'
expect(connection.read.to_str).to be == 'scheme: http'
ensure
connection&.close
end
end

with 'missing support for websockets' do
let(:app) do
Protocol::HTTP::Middleware.for do |request|
Expand Down
Loading