Conrad Irwin created
TEMP
TEMP
First pass of structured errors
Improved error handling for channel joining failurescrates/client/src/client.rs | 7
crates/collab/src/db/queries/channels.rs | 10
crates/collab/src/rpc.rs | 15
crates/rpc/proto/zed.proto | 12 +
crates/rpc/src/error.rs | 223 ++++++++++++++++++++++++++
crates/rpc/src/peer.rs | 41 ++--
crates/rpc/src/rpc.rs | 2
crates/workspace/src/workspace.rs | 26 ++
8 files changed, 292 insertions(+), 44 deletions(-)
@@ -689,12 +689,7 @@ impl Client {
Ok(())
}
Err(error) => {
- client.respond_with_error(
- receipt,
- proto::Error {
- message: format!("{:?}", error),
- },
- )?;
+ client.respond_with_error(receipt, error.to_proto())?;
Err(error)
}
}
@@ -1,5 +1,5 @@
use super::*;
-use rpc::proto::channel_member::Kind;
+use rpc::{proto::channel_member::Kind, ErrorCode, ErrorCodeExt};
use sea_orm::TryGetableMany;
impl Database {
@@ -166,7 +166,7 @@ impl Database {
}
if role.is_none() || role == Some(ChannelRole::Banned) {
- Err(anyhow!("not allowed"))?
+ Err(ErrorCode::Forbidden.anyhow())?
}
let role = role.unwrap();
@@ -1201,7 +1201,7 @@ impl Database {
Ok(channel::Entity::find_by_id(channel_id)
.one(&*tx)
.await?
- .ok_or_else(|| anyhow!("no such channel"))?)
+ .ok_or_else(|| proto::ErrorCode::NoSuchChannel.anyhow())?)
}
pub(crate) async fn get_or_create_channel_room(
@@ -1219,7 +1219,9 @@ impl Database {
let room_id = if let Some(room) = room {
if let Some(env) = room.environment {
if &env != environment {
- Err(anyhow!("must join using the {} release", env))?;
+ Err(ErrorCode::WrongReleaseChannel
+ .with_tag("required", &env)
+ .anyhow())?;
}
}
room.id
@@ -10,7 +10,7 @@ use crate::{
User, UserId,
},
executor::Executor,
- AppState, Result,
+ AppState, Error, Result,
};
use anyhow::anyhow;
use async_tungstenite::tungstenite::{
@@ -44,7 +44,7 @@ use rpc::{
self, Ack, AnyTypedEnvelope, EntityMessage, EnvelopedMessage, LiveKitConnectionInfo,
RequestMessage, ShareProject, UpdateChannelBufferCollaborators,
},
- Connection, ConnectionId, Peer, Receipt, TypedEnvelope,
+ Connection, ConnectionId, ErrorCode, ErrorCodeExt, ErrorExt, Peer, Receipt, TypedEnvelope,
};
use serde::{Serialize, Serializer};
use std::{
@@ -543,12 +543,11 @@ impl Server {
}
}
Err(error) => {
- peer.respond_with_error(
- receipt,
- proto::Error {
- message: error.to_string(),
- },
- )?;
+ let proto_err = match &error {
+ Error::Internal(err) => err.to_proto(),
+ _ => ErrorCode::Internal.message(format!("{}", error)).to_proto(),
+ };
+ peer.respond_with_error(receipt, proto_err)?;
Err(error)
}
}
@@ -197,6 +197,18 @@ message Ack {}
message Error {
string message = 1;
+ ErrorCode code = 2;
+ repeated string tags = 3;
+}
+
+enum ErrorCode {
+ Internal = 0;
+ NoSuchChannel = 1;
+ Disconnected = 2;
+ SignedOut = 3;
+ UpgradeRequired = 4;
+ Forbidden = 5;
+ WrongReleaseChannel = 6;
}
message Test {
@@ -0,0 +1,223 @@
+/// Some helpers for structured error handling.
+///
+/// The helpers defined here allow you to pass type-safe error codes from
+/// the collab server to the client; and provide a mechanism for additional
+/// structured data alongside the message.
+///
+/// When returning an error, it can be as simple as:
+///
+/// `return Err(Error::Forbidden.into())`
+///
+/// If you'd like to log more context, you can set a message. These messages
+/// show up in our logs, but are not shown visibly to users.
+///
+/// `return Err(Error::Forbidden.message("not an admin").into())`
+///
+/// If you'd like to provide enough context that the UI can render a good error
+/// message (or would be helpful to see in a structured format in the logs), you
+/// can use .with_tag():
+///
+/// `return Err(Error::WrongReleaseChannel.with_tag("required", "stable").into())`
+///
+/// When handling an error you can use .error_code() to match which error it was
+/// and .error_tag() to read any tags.
+///
+/// ```
+/// match err.error_code() {
+/// ErrorCode::Forbidden => alert("I'm sorry I can't do that.")
+/// ErrorCode::WrongReleaseChannel =>
+/// alert(format!("You need to be on the {} release channel.", err.error_tag("required").unwrap()))
+/// ErrorCode::Internal => alert("Sorry, something went wrong")
+/// }
+/// ```
+///
+use crate::proto;
+pub use proto::ErrorCode;
+
+/// ErrorCodeExt provides some helpers for structured error handling.
+///
+/// The primary implementation is on the proto::ErrorCode to easily convert
+/// that into an anyhow::Error, which we use pervasively.
+///
+/// The RPCError struct provides support for further metadata if needed.
+pub trait ErrorCodeExt {
+ /// Return an anyhow::Error containing this.
+ /// (useful in places where .into() doesn't have enough type information)
+ fn anyhow(self) -> anyhow::Error;
+
+ /// Add a message to the error (by default the error code is used)
+ fn message(self, msg: String) -> RPCError;
+
+ /// Add a tag to the error. Tags are key value pairs that can be used
+ /// to send semi-structured data along with the error.
+ fn with_tag(self, k: &str, v: &str) -> RPCError;
+}
+
+impl ErrorCodeExt for proto::ErrorCode {
+ fn anyhow(self) -> anyhow::Error {
+ self.into()
+ }
+
+ fn message(self, msg: String) -> RPCError {
+ let err: RPCError = self.into();
+ err.message(msg)
+ }
+
+ fn with_tag(self, k: &str, v: &str) -> RPCError {
+ let err: RPCError = self.into();
+ err.with_tag(k, v)
+ }
+}
+
+/// ErrorExt provides helpers for structured error handling.
+///
+/// The primary implementation is on the anyhow::Error, which is
+/// what we use throughout our codebase. Though under the hood this
+pub trait ErrorExt {
+ /// error_code() returns the ErrorCode (or ErrorCode::Internal if there is none)
+ fn error_code(&self) -> proto::ErrorCode;
+ /// error_tag() returns the value of the tag with the given key, if any.
+ fn error_tag(&self, k: &str) -> Option<&str>;
+ /// to_proto() convers the error into a proto::Error
+ fn to_proto(&self) -> proto::Error;
+}
+
+impl ErrorExt for anyhow::Error {
+ fn error_code(&self) -> proto::ErrorCode {
+ if let Some(rpc_error) = self.downcast_ref::<RPCError>() {
+ rpc_error.code
+ } else {
+ proto::ErrorCode::Internal
+ }
+ }
+
+ fn error_tag(&self, k: &str) -> Option<&str> {
+ if let Some(rpc_error) = self.downcast_ref::<RPCError>() {
+ rpc_error.error_tag(k)
+ } else {
+ None
+ }
+ }
+
+ fn to_proto(&self) -> proto::Error {
+ if let Some(rpc_error) = self.downcast_ref::<RPCError>() {
+ rpc_error.to_proto()
+ } else {
+ ErrorCode::Internal.message(format!("{}", self)).to_proto()
+ }
+ }
+}
+
+impl From<proto::ErrorCode> for anyhow::Error {
+ fn from(value: proto::ErrorCode) -> Self {
+ RPCError {
+ request: None,
+ code: value,
+ msg: format!("{:?}", value).to_string(),
+ tags: Default::default(),
+ }
+ .into()
+ }
+}
+
+#[derive(Clone, Debug)]
+pub struct RPCError {
+ request: Option<String>,
+ msg: String,
+ code: proto::ErrorCode,
+ tags: Vec<String>,
+}
+
+/// RPCError is a structured error type that is returned by the collab server.
+/// In addition to a message, it lets you set a specific ErrorCode, and attach
+/// small amounts of metadata to help the client handle the error appropriately.
+///
+/// This struct is not typically used directly, as we pass anyhow::Error around
+/// in the app; however it is useful for chaining .message() and .with_tag() on
+/// ErrorCode.
+impl RPCError {
+ /// from_proto converts a proto::Error into an anyhow::Error containing
+ /// an RPCError.
+ pub fn from_proto(error: &proto::Error, request: &str) -> anyhow::Error {
+ RPCError {
+ request: Some(request.to_string()),
+ code: error.code(),
+ msg: error.message.clone(),
+ tags: error.tags.clone(),
+ }
+ .into()
+ }
+}
+
+impl ErrorCodeExt for RPCError {
+ fn message(mut self, msg: String) -> RPCError {
+ self.msg = msg;
+ self
+ }
+
+ fn with_tag(mut self, k: &str, v: &str) -> RPCError {
+ self.tags.push(format!("{}={}", k, v));
+ self
+ }
+
+ fn anyhow(self) -> anyhow::Error {
+ self.into()
+ }
+}
+
+impl ErrorExt for RPCError {
+ fn error_tag(&self, k: &str) -> Option<&str> {
+ for tag in &self.tags {
+ let mut parts = tag.split('=');
+ if let Some(key) = parts.next() {
+ if key == k {
+ return parts.next();
+ }
+ }
+ }
+ None
+ }
+
+ fn error_code(&self) -> proto::ErrorCode {
+ self.code
+ }
+
+ fn to_proto(&self) -> proto::Error {
+ proto::Error {
+ code: self.code as i32,
+ message: self.msg.clone(),
+ tags: self.tags.clone(),
+ }
+ }
+}
+
+impl std::error::Error for RPCError {
+ fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+ None
+ }
+}
+
+impl std::fmt::Display for RPCError {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ if let Some(request) = &self.request {
+ write!(f, "RPC request {} failed: {}", request, self.msg)?
+ } else {
+ write!(f, "{}", self.msg)?
+ }
+ for tag in &self.tags {
+ write!(f, " {}", tag)?
+ }
+ Ok(())
+ }
+}
+
+impl From<proto::ErrorCode> for RPCError {
+ fn from(code: proto::ErrorCode) -> Self {
+ RPCError {
+ request: None,
+ code,
+ msg: format!("{:?}", code).to_string(),
+ tags: Default::default(),
+ }
+ }
+}
@@ -1,3 +1,5 @@
+use crate::{ErrorCode, ErrorCodeExt, ErrorExt, RPCError};
+
use super::{
proto::{self, AnyTypedEnvelope, EnvelopedMessage, MessageStream, PeerId, RequestMessage},
Connection,
@@ -423,11 +425,7 @@ impl Peer {
let (response, _barrier) = rx.await.map_err(|_| anyhow!("connection was closed"))?;
if let Some(proto::envelope::Payload::Error(error)) = &response.payload {
- Err(anyhow!(
- "RPC request {} failed - {}",
- T::NAME,
- error.message
- ))
+ Err(RPCError::from_proto(&error, T::NAME))
} else {
Ok(TypedEnvelope {
message_id: response.id,
@@ -516,9 +514,12 @@ impl Peer {
envelope: Box<dyn AnyTypedEnvelope>,
) -> Result<()> {
let connection = self.connection_state(envelope.sender_id())?;
- let response = proto::Error {
- message: format!("message {} was not handled", envelope.payload_type_name()),
- };
+ let response = ErrorCode::Internal
+ .message(format!(
+ "message {} was not handled",
+ envelope.payload_type_name()
+ ))
+ .to_proto();
let message_id = connection
.next_message_id
.fetch_add(1, atomic::Ordering::SeqCst);
@@ -692,17 +693,17 @@ mod tests {
server
.send(
server_to_client_conn_id,
- proto::Error {
- message: "message 1".to_string(),
- },
+ ErrorCode::Internal
+ .message("message 1".to_string())
+ .to_proto(),
)
.unwrap();
server
.send(
server_to_client_conn_id,
- proto::Error {
- message: "message 2".to_string(),
- },
+ ErrorCode::Internal
+ .message("message 2".to_string())
+ .to_proto(),
)
.unwrap();
server.respond(request.receipt(), proto::Ack {}).unwrap();
@@ -797,17 +798,17 @@ mod tests {
server
.send(
server_to_client_conn_id,
- proto::Error {
- message: "message 1".to_string(),
- },
+ ErrorCode::Internal
+ .message("message 1".to_string())
+ .to_proto(),
)
.unwrap();
server
.send(
server_to_client_conn_id,
- proto::Error {
- message: "message 2".to_string(),
- },
+ ErrorCode::Internal
+ .message("message 2".to_string())
+ .to_proto(),
)
.unwrap();
server.respond(request1.receipt(), proto::Ack {}).unwrap();
@@ -1,10 +1,12 @@
pub mod auth;
mod conn;
+mod error;
mod notification;
mod peer;
pub mod proto;
pub use conn::Connection;
+pub use error::*;
pub use notification::*;
pub use peer::*;
mod macros;
@@ -14,8 +14,8 @@ mod workspace_settings;
use anyhow::{anyhow, Context as _, Result};
use call::ActiveCall;
use client::{
- proto::{self, PeerId},
- Client, Status, TypedEnvelope, UserStore,
+ proto::{self, ErrorCode, PeerId},
+ Client, ErrorExt, Status, TypedEnvelope, UserStore,
};
use collections::{hash_map, HashMap, HashSet};
use dock::{Dock, DockPosition, Panel, PanelButtons, PanelHandle};
@@ -3919,10 +3919,10 @@ async fn join_channel_internal(
| Status::Reconnecting
| Status::Reauthenticating => continue,
Status::Connected { .. } => break 'outer,
- Status::SignedOut => return Err(anyhow!("not signed in")),
- Status::UpgradeRequired => return Err(anyhow!("zed is out of date")),
+ Status::SignedOut => return Err(ErrorCode::SignedOut.into()),
+ Status::UpgradeRequired => return Err(ErrorCode::UpgradeRequired.into()),
Status::ConnectionError | Status::ConnectionLost | Status::ReconnectionError { .. } => {
- return Err(anyhow!("zed is offline"))
+ return Err(ErrorCode::Disconnected.into())
}
}
}
@@ -3995,9 +3995,23 @@ pub fn join_channel(
if let Some(active_window) = active_window {
active_window
.update(&mut cx, |_, cx| {
+ let message:SharedString = match err.error_code() {
+ ErrorCode::SignedOut => {
+ "Failed to join channel\n\nPlease sign in to continue.".into()
+ },
+ ErrorCode::UpgradeRequired => {
+ "Failed to join channel\n\nPlease update to the latest version of Zed to continue.".into()
+ },
+ ErrorCode::NoSuchChannel => {
+ "Failed to find channel\n\nPlease check the link and try again.".into()
+ },
+ ErrorCode::Disconnected => "Failed to join channel\n\nPlease check your internet connection and try again.".into(),
+ ErrorCode::WrongReleaseChannel => format!("Failed to join channel\n\nOther people in the channel are using the {} release of Zed, please switch to that release instead.", err.error_tag("required").unwrap_or("other")).into(),
+ _ => format!("Failed to join channel\n\n{}\n\nPlease try again.", err).into(),
+ };
cx.prompt(
PromptLevel::Critical,
- &format!("Failed to join channel: {}", err),
+ &message,
&["Ok"],
)
})?