crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.5k stars 1.62k forks source link

Expose HTTP response headers for HTTP::WebSocket #7213

Open skateman opened 5 years ago

skateman commented 5 years ago

When opening a WebSocket connection, there's an option to set the request headers via an argument. However, the response headers from the server are not exposed at all. This means that a programmer is not able to access e.g. cookies sent by the server.

I would like to have something like this:

require "http/web_socket"
ws = HTTP::WebSocket.new("ws://example.com")
ws.response_headers["Set-Cookie"] # response_headers is an instance of HTTP::Headers

The new method in the Protocol class has a handshake_response that has these headers. My idea was to store this in an instance variable in Protocol and delegate it from the WebSocket class. However, @straight-shoota is saying that this is not The Right Way™ so I'm open for suggestions about how else to do this.

straight-shoota commented 5 years ago

Thanks for clarifying! This actually helps a lot.

Your approach really couldn't work. This new method is used for establishing a client websocket connection. But those headers it receives are the request headers sent to the server. This argument can be used to send additional headers to the server. See 264-268 for the default headers added in this method.

What you want is to return handshake_response.headers. This looks like a logical enhancement. If you can send custom headers with the websocket handshake, you should also be able to receive them.

I would suggest to rename this new method to establish_connection and let it return {Protocol, Headers}. The headers don't belong into the Protocol class. This way they can be stored in an ivar in WebSocket.

We could consider if server-side WebSocket instances should also have the headers attached. This could be implemented by adding the headers to the hidden WebSocket.new methods.

skateman commented 5 years ago

@straight-shoota sorry for the late reply, busy times. So you want to call the establish_connection method in the two hidden WebSocket.new methods and pass the response headers into the instance, am I right?

Would it make sense to rename the headers to request_headers or should I keep it like it is? Asking because the response_headers sounds as a good name for this ivar and we might want to keep it symmetric.