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

fix: Websocket error broadcast for unsubscribed ID #506

Merged
merged 2 commits into from
Oct 16, 2024
Merged

fix: Websocket error broadcast for unsubscribed ID #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
079d1bd
Don't broadcast error for unsubscribed id
calvincestari Oct 16, 2024
e9ec645
Change test to handle missing id case
calvincestari Oct 16, 2024
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
8 changes: 4 additions & 4 deletions Tests/ApolloTests/WebSocket/WebSocketTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ class WebSocketTests: XCTestCase {
subject.cancel()
}

func testLocalErrorUnknownId() throws {
let expectation = self.expectation(description: "Unknown id for subscription")
func testLocalErrorMissingId() throws {
let expectation = self.expectation(description: "Missing id for subscription")

let subject = client.subscribe(subscription: MockSubscription<ReviewAddedData>()) { result in
defer { expectation.fulfill() }
Expand All @@ -133,10 +133,10 @@ class WebSocketTests: XCTestCase {
}
}
}


// Message data below has missing 'id' and should notify all subscribers of the error
let message : JSONEncodableDictionary = [
"type": "data",
"id": "2", // subscribing on id = 1, i.e. expecting error when receiving id = 2
"payload": [
"data": [
"reviewAdded": [
Expand Down
24 changes: 15 additions & 9 deletions apollo-ios/Sources/ApolloWebSocket/WebSocketTransport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,21 @@ public class WebSocketTransport {
}

switch messageType {
case .data,
.next,
.error:
if let id = parseHandler.id, let responseHandler = subscribers[id] {
case .data, .next, .error:
guard let id = parseHandler.id else {
let websocketError = WebSocketError(
payload: parseHandler.payload,
error: parseHandler.error,
kind: .unprocessedMessage(text)
)
self.notifyErrorAllHandlers(websocketError)

break
}

// If we have a handler ID but no subscriber exists for that ID then the
// subscriber probably unsubscribed.
if let responseHandler = subscribers[id] {
if let payload = parseHandler.payload {
responseHandler(.success(payload))
} else if let error = parseHandler.error {
Expand All @@ -207,11 +218,6 @@ public class WebSocketTransport {
kind: .neitherErrorNorPayloadReceived)
responseHandler(.failure(websocketError))
}
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative approach to silencing this error path is to move unsubscribing IDs into another array that can be checked against if the ID is not found in subscribers. This would ensure we're discarding a message for an unsubscribing handler but I'm not sold on that being necessary.

The unsubscribing array should be of [String] type since there would be no need to store the handling closure again.

let websocketError = WebSocketError(payload: parseHandler.payload,
error: parseHandler.error,
kind: .unprocessedMessage(text))
self.notifyErrorAllHandlers(websocketError)
}
case .complete:
if let id = parseHandler.id {
Expand Down
Loading