Always provide a non-empty WebSocket connection close reason #2080
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.
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.
🤦♂️
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.
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
datawas empty, it was being interpreted as a valid, zero-length string, and therefore replacing the defaultcloseReasonof "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.