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.

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

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

Merged
merged 17 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
4 changes: 3 additions & 1 deletion crates/matrix-sdk/src/widget/machine/driver_req.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,11 @@ where
Self { request_meta: None, _phantom: PhantomData }
}

/// Setup a callback function that will be called once the matrix driver has
/// processed the request.
pub(crate) fn then(
self,
response_handler: impl FnOnce(Result<T, String>, &mut WidgetMachine) -> Vec<Action>
response_handler: impl FnOnce(Result<T, crate::Error>, &mut WidgetMachine) -> Vec<Action>
+ Send
+ 'static,
) {
Expand Down
66 changes: 59 additions & 7 deletions crates/matrix-sdk/src/widget/machine/from_widget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,20 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::fmt;

use ruma::{
api::client::delayed_events::{
delayed_message_event, delayed_state_event, update_delayed_event,
api::client::{
delayed_events::{delayed_message_event, delayed_state_event, update_delayed_event},
error::ErrorBody,
},
events::{AnyTimelineEvent, MessageLikeEventType, StateEventType},
serde::Raw,
OwnedEventId, OwnedRoomId,
};
use serde::{Deserialize, Serialize};
use serde_json::{json, Value};

use super::{SendEventRequest, UpdateDelayedEventRequest};
use crate::widget::StateKeySelector;
use crate::{widget::StateKeySelector, Error, HttpError};

#[derive(Deserialize, Debug)]
#[serde(tag = "action", rename_all = "snake_case", content = "data")]
Expand All @@ -41,28 +41,80 @@ pub(super) enum FromWidgetRequest {
DelayedEventUpdate(UpdateDelayedEventRequest),
}

/// The full response a client sends to a from widget request
/// in case of an error.
#[derive(Serialize)]
pub(super) struct FromWidgetErrorResponse {
error: FromWidgetError,
}

impl FromWidgetErrorResponse {
pub(super) fn new(e: impl fmt::Display) -> Self {
Self { error: FromWidgetError { message: e.to_string() } }
/// Create a error response to send to the widget from an http error.
pub(crate) fn from_http_error(error: &HttpError) -> Self {
Self {
error: FromWidgetError {
message: error.to_string(),
matrix_api_error: error.as_client_api_error().and_then(
|api_error| match &api_error.body {
ErrorBody::Standard { kind, message } => Some(FromWidgetMatrixErrorBody {
http_status: api_error.status_code.as_u16().into(),
response: json!({"errcode": kind.to_string(), "error": message }),
}),
_ => None,
},
),
},
}
}

/// Create a error response to send to the widget from a matrix sdk error.
pub(crate) fn from_error(error: &Error) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Can this take ownership of the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could but would there be any benefit to that? we will only call methods that use the ref?

Copy link
Member

Choose a reason for hiding this comment

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

Avoid cloning in the function's body, and it's a good practice whenever you can do that (as it avoids future changes to this code from cloning too).

Copy link
Contributor Author

@toger5 toger5 Dec 3, 2024

Choose a reason for hiding this comment

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

We will only call to_string and as_client_api_error on the error object. Both use &self. to_string will of course allocate new data but I am not sure there is a way to convert an error to a string without that overhead?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed there's no way, but overall passing by ownership makes for cleaner APIs, whenever we can do it.

match &error {
Error::Http(e) => FromWidgetErrorResponse::from_http_error(e),
Error::UnknownError(e) => FromWidgetErrorResponse::from_string(e.to_string()),
_ => FromWidgetErrorResponse::from_string(error.to_string()),
}
}

/// Create a error response to send to the widget from a string.
pub(crate) fn from_string<S: Into<String>>(error: S) -> Self {
Self { error: FromWidgetError { message: error.into(), matrix_api_error: None } }
}
}

/// The serializable section of an error response send by the client as a
/// response to a from widget request.
#[derive(Serialize)]
struct FromWidgetError {
/// A unspecified error message text that caused this widget action to fail.
/// This is useful to prompt the user on an issue but cannot be used to
/// decide on how to deal with the error.
message: String,
/// An optional matrix error that contains specified
/// information and helps finding a work around for specific errors.
matrix_api_error: Option<FromWidgetMatrixErrorBody>,
}

/// The serializable section of a widget response that represents a matrix
/// error.
#[derive(Serialize)]
struct FromWidgetMatrixErrorBody {
/// The status code of the http response
http_status: u32,
/// The matrix standard error response including the `errorcode` and the
/// `error` message as defined in the spec: https://spec.matrix.org/v1.12/client-server-api/#standard-error-response
response: Value,
Copy link
Member

Choose a reason for hiding this comment

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

Is it really worth saving the response field as a json::Value, instead of having the fields in there, automatically serialized by serde?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using StandardErrorBody makes sense?
This struct was a bit suspicious to me see: https://matrix.to/#/!veagCdDBjKrMsOCzrq:privacytools.io/$2ieYTtk1E4HU36rQZ7r40_bjGaWUN-zfOGz01Yfv3Ng?via=matrix.org&via=envs.net&via=kabou.ro
(I was afraid it is only supposed to be used in a very special occasion since its not even used in the ErrorBody enum)
Also the ErrorBody enum seemed to be the perfect fit here but it is not serializable.
So I did not use it before.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to store the result as an integer and the error message as a String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The errorcode is a a string as well. I used the StandardErrorBody which is exactly what we expect here.

}

/// The serializable section of a widget response containing the supported
/// versions.
#[derive(Serialize)]
pub(super) struct SupportedApiVersionsResponse {
supported_versions: Vec<ApiVersion>,
}

impl SupportedApiVersionsResponse {
/// The currently supported widget api versions from the rust widget driver.
pub(super) fn new() -> Self {
Self {
supported_versions: vec![
Expand Down
6 changes: 4 additions & 2 deletions crates/matrix-sdk/src/widget/machine/incoming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ pub(crate) enum IncomingMessage {
/// The ID of the request that this response corresponds to.
request_id: Uuid,

/// The result of the request: response data or error message.
response: Result<MatrixDriverResponse, String>,
/// The result of the request: response data or matrix sdk error.
/// Http errors will be forwarded to the widget in a specified format
/// so the widget can parse the error.
response: Result<MatrixDriverResponse, crate::Error>,
},

/// The `MatrixDriver` notified the `WidgetMachine` of a new matrix event.
Expand Down
99 changes: 62 additions & 37 deletions crates/matrix-sdk/src/widget/machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

//! No I/O logic of the [`WidgetDriver`].

use std::{fmt, iter, time::Duration};
use std::{iter, time::Duration};

use driver_req::UpdateDelayedEventRequest;
use from_widget::UpdateDelayedEventResponse;
Expand Down Expand Up @@ -48,10 +48,11 @@ use self::{
#[cfg(doc)]
use super::WidgetDriver;
use super::{
capabilities,
capabilities::{SEND_DELAYED_EVENT, UPDATE_DELAYED_EVENT},
filter::{MatrixEventContent, MatrixEventFilterInput},
Capabilities, StateKeySelector,
};
use crate::Result;

mod driver_req;
mod from_widget;
Expand Down Expand Up @@ -212,7 +213,12 @@ impl WidgetMachine {
) -> Vec<Action> {
let request = match raw_request.deserialize() {
Ok(r) => r,
Err(e) => return vec![Self::send_from_widget_error_response(raw_request, e)],
Err(e) => {
return vec![Self::send_from_widget_error_response(
raw_request,
FromWidgetErrorResponse::from_error(&crate::Error::SerdeJson(e)),
)]
}
};

match request {
Expand Down Expand Up @@ -264,16 +270,18 @@ impl WidgetMachine {
let CapabilitiesState::Negotiated(capabilities) = &self.capabilities else {
let text =
"Received send update delayed event request before capabilities were negotiated";
return vec![Self::send_from_widget_error_response(raw_request, text)];
return vec![Self::send_from_widget_error_response(
raw_request,
FromWidgetErrorResponse::from_string(text),
)];
};

if !capabilities.update_delayed_event {
return vec![Self::send_from_widget_error_response(
raw_request,
format!(
"Not allowed: missing the {} capability.",
capabilities::UPDATE_DELAYED_EVENT
),
FromWidgetErrorResponse::from_string(format!(
"Not allowed: missing the {UPDATE_DELAYED_EVENT} capability."
)),
)];
}

Expand All @@ -282,13 +290,15 @@ impl WidgetMachine {
action: req.action,
delay_id: req.delay_id,
});

request.then(|res, _machine| {
request.then(|result, _machine| {
vec![Self::send_from_widget_result_response(
raw_request,
// This is mapped to another type because the update_delay_event::Response
// does not impl Serialize
res.map(Into::<UpdateDelayedEventResponse>::into),
match result {
Ok(response) => Ok(Into::<UpdateDelayedEventResponse>::into(response)),
Err(e) => Err(FromWidgetErrorResponse::from_error(&e)),
},
)]
});

Expand All @@ -304,7 +314,10 @@ impl WidgetMachine {
) -> Option<Action> {
let CapabilitiesState::Negotiated(capabilities) = &self.capabilities else {
let text = "Received read event request before capabilities were negotiated";
return Some(Self::send_from_widget_error_response(raw_request, text));
return Some(Self::send_from_widget_error_response(
raw_request,
FromWidgetErrorResponse::from_string(text),
));
};

match request {
Expand All @@ -313,7 +326,9 @@ impl WidgetMachine {
{
return Some(Self::send_from_widget_error_response(
raw_request,
"Not allowed to read message like event",
FromWidgetErrorResponse::from_string(
"Not allowed to read message like event",
),
));
}

Expand All @@ -324,17 +339,23 @@ impl WidgetMachine {
let (request, action) = self.send_matrix_driver_request(request);

request.then(|result, machine| {
let response = result.and_then(|mut events| {
let CapabilitiesState::Negotiated(capabilities) = &machine.capabilities
else {
let err = "Received read event request before capabilities negotiation";
return Err(err.into());
};

events.retain(|e| capabilities.raw_event_matches_read_filter(e));
Ok(ReadEventResponse { events })
});

let response = match (result, &machine.capabilities) {
(Ok(mut events), CapabilitiesState::Negotiated(capabilities)) => {
events.retain(|e| capabilities.raw_event_matches_read_filter(e));
Ok(ReadEventResponse { events })
}
(Ok(_), CapabilitiesState::Unset) => {
Err(FromWidgetErrorResponse::from_string(
"Received read event request before capabilities negotiation",
))
}
(Ok(_), CapabilitiesState::Negotiating) => {
Err(FromWidgetErrorResponse::from_string(
"Received read event request while capabilities were negotiating",
))
}
(Err(e), _) => Err(FromWidgetErrorResponse::from_error(&e)),
};
vec![Self::send_from_widget_result_response(raw_request, response)]
});

Expand Down Expand Up @@ -364,14 +385,16 @@ impl WidgetMachine {
let request = ReadStateEventRequest { event_type, state_key };
let (request, action) = self.send_matrix_driver_request(request);
request.then(|result, _machine| {
let response = result.map(|events| ReadEventResponse { events });
let response = result
.map(|events| ReadEventResponse { events })
.map_err(|e| FromWidgetErrorResponse::from_error(&e));
vec![Self::send_from_widget_result_response(raw_request, response)]
});
action
} else {
Some(Self::send_from_widget_error_response(
raw_request,
"Not allowed to read state event",
FromWidgetErrorResponse::from_string("Not allowed to read state event"),
))
}
}
Expand Down Expand Up @@ -402,17 +425,16 @@ impl WidgetMachine {
if !capabilities.send_delayed_event && request.delay.is_some() {
return Some(Self::send_from_widget_error_response(
raw_request,
format!(
"Not allowed: missing the {} capability.",
capabilities::SEND_DELAYED_EVENT
),
FromWidgetErrorResponse::from_string(format!(
"Not allowed: missing the {SEND_DELAYED_EVENT} capability."
)),
));
}

if !capabilities.send.iter().any(|filter| filter.matches(&filter_in)) {
return Some(Self::send_from_widget_error_response(
raw_request,
"Not allowed to send event",
FromWidgetErrorResponse::from_string("Not allowed to send event"),
));
}

Expand All @@ -422,7 +444,10 @@ impl WidgetMachine {
if let Ok(r) = result.as_mut() {
r.set_room_id(machine.room_id.clone());
}
vec![Self::send_from_widget_result_response(raw_request, result)]
vec![Self::send_from_widget_result_response(
raw_request,
result.map_err(|e| FromWidgetErrorResponse::from_error(&e)),
)]
});

action
Expand Down Expand Up @@ -465,7 +490,7 @@ impl WidgetMachine {
fn process_matrix_driver_response(
&mut self,
request_id: Uuid,
response: Result<MatrixDriverResponse, String>,
response: Result<MatrixDriverResponse>,
) -> Vec<Action> {
match self.pending_matrix_driver_requests.extract(&request_id) {
Ok(request) => request
Expand Down Expand Up @@ -500,14 +525,14 @@ impl WidgetMachine {

fn send_from_widget_error_response(
raw_request: Raw<FromWidgetRequest>,
error: impl fmt::Display,
error: FromWidgetErrorResponse,
) -> Action {
Self::send_from_widget_response(raw_request, FromWidgetErrorResponse::new(error))
Self::send_from_widget_response(raw_request, error)
}

fn send_from_widget_result_response(
raw_request: Raw<FromWidgetRequest>,
result: Result<impl Serialize, impl fmt::Display>,
result: Result<impl Serialize, FromWidgetErrorResponse>,
) -> Action {
match result {
Ok(res) => Self::send_from_widget_response(raw_request, res),
Expand Down Expand Up @@ -613,7 +638,7 @@ impl ToWidgetRequestMeta {
}

type MatrixDriverResponseFn =
Box<dyn FnOnce(Result<MatrixDriverResponse, String>, &mut WidgetMachine) -> Vec<Action> + Send>;
Box<dyn FnOnce(Result<MatrixDriverResponse>, &mut WidgetMachine) -> Vec<Action> + Send>;

pub(crate) struct MatrixDriverRequestMeta {
response_fn: Option<MatrixDriverResponseFn>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ fn test_capabilities_failure_results_into_empty_capabilities() {

machine.process(IncomingMessage::MatrixDriverResponse {
request_id,
response: Err("OHMG!".into()),
response: Err(crate::Error::UnknownError("OHMG!".into())),
})
};

Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/widget/machine/tests/openid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ fn test_openid_fail_results_in_response_blocked() {

machine.process(IncomingMessage::MatrixDriverResponse {
request_id,
response: Err("Unlucky one".into()),
response: Err(crate::Error::UnknownError("Unlucky one".into())),
})
};

Expand Down
Loading
Loading