-
Notifications
You must be signed in to change notification settings - Fork 707
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.
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
Always provide a non-empty WebSocket connection close reason #2080
base: main
Are you sure you want to change the base?
Conversation
@s3cur3: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
@@ -813,7 +813,8 @@ public final class WebSocket: NSObject, WebSocketClient, StreamDelegate, WebSock | |||
|
|||
if receivedOpcode == .connectionClose { | |||
var closeReason = "connection closed by server" | |||
if let customCloseReason = String(data: data, encoding: .utf8) { | |||
if let customCloseReason = String(data: data, encoding: .utf8), | |||
customCloseReason.isEmpty { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is inverted - this would mean that if the custom close reason is empty, it would fall into the if
, while if it was not empty, it'd fall into the default case.
There's a .apollo.isNotEmpty
extension floating around somewhere in ApolloUtils
that would make this clearer than just a !
, fwiw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️
This is what I get for trying to write one line of code without a test. I'll fix it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. Could we get a simple test added for it as well? I'd be happy to merge this once we have a test!
We are going to update this code to be compatible with recent changes and add a unit test prior to merging this for the next patch release. |
This is a tiny fix for something I encountered. When the connection close
data
was empty, it was being interpreted as a valid, zero-length string, and therefore replacing the defaultcloseReason
of "connection closed by server". The default in this case is more informative, so it probably makes sense to guard against the empty case before replacing it.