From 18b9c35316c0cbdee18d7c5e2b7fcdd4d77fcca8 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 27 Jun 2024 17:41:41 +1000 Subject: [PATCH] chore(connlib): explicitly handle `invalid_version` error (#5577) Ensures we correctly deserialize `invalid_version` and don't fall-back to `Other`. Related: #5525. --- rust/connlib/clients/shared/src/eventloop.rs | 4 +- rust/phoenix-channel/src/lib.rs | 44 ++++++++++++++++++-- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/rust/connlib/clients/shared/src/eventloop.rs b/rust/connlib/clients/shared/src/eventloop.rs index 052207a6d..7e5e4ccd4 100644 --- a/rust/connlib/clients/shared/src/eventloop.rs +++ b/rust/connlib/clients/shared/src/eventloop.rs @@ -340,7 +340,9 @@ where ErrorReply::UnmatchedTopic => { self.portal.join(topic, ()); } - ErrorReply::NotFound | ErrorReply::Other => {} + reason @ (ErrorReply::InvalidVersion | ErrorReply::NotFound | ErrorReply::Other) => { + tracing::debug!(%req_id, %reason, "Request failed"); + } } } } diff --git a/rust/phoenix-channel/src/lib.rs b/rust/phoenix-channel/src/lib.rs index 80d5ea9f2..7bdcaf482 100644 --- a/rust/phoenix-channel/src/lib.rs +++ b/rust/phoenix-channel/src/lib.rs @@ -78,8 +78,8 @@ pub enum Error { TokenExpired, #[error("max retries reached")] MaxRetriesReached, - #[error("login failed")] - LoginFailed, + #[error("login failed: {0}")] + LoginFailed(ErrorReply), } impl Error { @@ -88,7 +88,7 @@ impl Error { Error::Client(s) => s == &StatusCode::UNAUTHORIZED || s == &StatusCode::FORBIDDEN, Error::TokenExpired => true, Error::MaxRetriesReached => false, - Error::LoginFailed => false, + Error::LoginFailed(_) => false, } } } @@ -391,7 +391,7 @@ where if message.topic == self.login && self.pending_join_requests.contains(&req_id) { - return Poll::Ready(Err(Error::LoginFailed)); + return Poll::Ready(Err(Error::LoginFailed(reason))); } return Poll::Ready(Ok(Event::ErrorResponse { @@ -587,12 +587,26 @@ pub enum ErrorReply { #[serde(rename = "unmatched topic")] UnmatchedTopic, NotFound, + InvalidVersion, Offline, Disabled, #[serde(other)] Other, } +impl fmt::Display for ErrorReply { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ErrorReply::UnmatchedTopic => write!(f, "unmatched topic"), + ErrorReply::NotFound => write!(f, "not found"), + ErrorReply::InvalidVersion => write!(f, "invalid version"), + ErrorReply::Offline => write!(f, "offline"), + ErrorReply::Disabled => write!(f, "disabled"), + ErrorReply::Other => write!(f, "other"), + } + } +} + #[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)] #[serde(rename_all = "snake_case")] pub enum DisconnectReason { @@ -811,6 +825,28 @@ mod tests { assert_eq!(actual_reply, expected_reply); } + #[test] + fn invalid_version_reply() { + let actual_reply = r#" + { + "event": "phx_reply", + "ref": "12", + "topic": "client", + "payload":{ + "status": "error", + "response":{ + "reason": "invalid_version" + } + } + } + "#; + let actual_reply: Payload<(), ()> = serde_json::from_str(actual_reply).unwrap(); + let expected_reply = Payload::<(), ()>::Reply(Reply::Error { + reason: ErrorReply::InvalidVersion, + }); + assert_eq!(actual_reply, expected_reply); + } + #[test] fn disabled_err_reply() { let json = r#"{"event":"phx_reply","ref":null,"topic":"client","payload":{"status":"error","response":{"reason": "disabled"}}}"#;