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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/widget/machine/driver_req.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ where

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>
Copy link
Member

Choose a reason for hiding this comment

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

Since you're around, can you add a doc comment to this function please?

+ Send
+ 'static,
) {
Expand Down
62 changes: 55 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,22 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::fmt;
use std::collections::HashMap;

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 @@ -46,17 +48,63 @@ pub(super) struct FromWidgetErrorResponse {
error: FromWidgetError,
}

impl FromWidgetErrorResponse {
pub(super) fn new(e: impl fmt::Display) -> Self {
Self { error: FromWidgetError { message: e.to_string() } }
impl From<&HttpError> for FromWidgetErrorResponse {
fn from(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(),
http_headers: HashMap::new(),
url: "".to_owned(),
response: json!({"errcode": kind.to_string(), "error": message }),
}),
_ => None,
},
),
},
}
}
}

impl From<&Error> for FromWidgetErrorResponse {
fn from(error: &Error) -> Self {
match &error {
Error::Http(e) => e.into(),
Error::UnknownError(e) => e.to_string().into(),
_ => error.to_string().into(),
}
}
}

impl From<String> for FromWidgetErrorResponse {
fn from(error: String) -> Self {
Self { error: FromWidgetError { message: error, matrix_api_error: None } }
}
}

impl From<&str> for FromWidgetErrorResponse {
fn from(error: &str) -> Self {
error.to_owned().into()
Comment on lines +82 to +90
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using From implementations, it'd be simpler to use plain functions: from_string(String) -> Self, from_error(matrix_sdk::Error) -> Self, from_http_error(...). Using into() at the call sites makes it harder to find which code is used, and thus harder to navigate the code.

}
}

#[derive(Serialize)]
struct FromWidgetError {
message: String,
matrix_api_error: Option<FromWidgetMatrixErrorBody>,
Copy link
Member

Choose a reason for hiding this comment

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

This crate is barely documented, which makes it super hard to understand how it works. Let's change that; of course I wouldn't have you change all of it at once, so let's try to do it incrementally. The doc comments don't have to be long; a single short descriptive line is sufficient to give us a rough idea of why/how it's used.

Please add a doc comment to:

  • every single struct/enum you're adding fields to (here, FromWidgetError)
  • every field/variant from those structs/enums (here, message above)
  • every new struct/enum/field/variant

}

#[derive(Serialize)]
struct FromWidgetMatrixErrorBody {
http_status: u32,
// TODO: figure out the which type to use here.
http_headers: HashMap<String, String>,
url: String,
Comment on lines +103 to +105
Copy link
Member

Choose a reason for hiding this comment

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

If these two fields are never used, let's not introduce them now?

response: Value,
Comment on lines +101 to +106
Copy link
Member

Choose a reason for hiding this comment

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

I want doc comments everywhere here too please :)

}
Copy link
Member

Choose a reason for hiding this comment

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

nit: always add a newline after a block closing

Suggested change
}
}

#[derive(Serialize)]
pub(super) struct SupportedApiVersionsResponse {
supported_versions: Vec<ApiVersion>,
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/widget/machine/incoming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub(crate) enum IncomingMessage {
request_id: Uuid,

/// The result of the request: response data or error message.
response: Result<MatrixDriverResponse, String>,
response: Result<MatrixDriverResponse, crate::Error>,
Copy link
Member

Choose a reason for hiding this comment

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

The doc comment above needs an update^^

},

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

#![warn(unreachable_pub)]

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

use driver_req::UpdateDelayedEventRequest;
use from_widget::UpdateDelayedEventResponse;
Expand Down Expand Up @@ -50,11 +50,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::widget::EventFilter;
use crate::{widget::EventFilter, Error};

mod driver_req;
mod from_widget;
Expand Down Expand Up @@ -203,7 +203,10 @@ 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, (&Error::SerdeJson(e)).into())]
}
};

match request {
Expand Down Expand Up @@ -252,28 +255,29 @@ 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, text.into())];
};
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
),
format!("Not allowed: missing the {UPDATE_DELAYED_EVENT} capability.")
.into(),
)];
}
let (request, request_action) =
self.send_matrix_driver_request(UpdateDelayedEventRequest {
action: req.action,
delay_id: req.delay_id,
});
request.then(|res, machine| {
request.then(|result, machine| {
vec![machine.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((&e).into()),
},
)]
});
request_action.map(|a| vec![a]).unwrap_or_default()
Expand All @@ -288,7 +292,7 @@ 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, text.into()));
};

match request {
Expand All @@ -297,7 +301,7 @@ impl WidgetMachine {
if !capabilities.read.iter().any(filter_fn) {
return Some(self.send_from_widget_error_response(
raw_request,
"Not allowed to read message like event",
"Not allowed to read message like event".into(),
));
}

Expand All @@ -306,16 +310,21 @@ impl WidgetMachine {
let request = ReadMessageLikeEventRequest { event_type, limit };
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("Received read event request before capabilities negotiation"
.into())
}
(Ok(_), CapabilitiesState::Negotiating) => {
Err("Received read event request while capabilities were negotiating"
.into())
}
(Err(e), _) => Err((&e).into()),
};
Comment on lines +313 to +327
Copy link
Member

Choose a reason for hiding this comment

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

This is messier like this. Can we keep the and_then, and after it call map_err to convert the error type if needs be, instead?

vec![machine.send_from_widget_result_response(raw_request, response)]
});
action
Expand Down Expand Up @@ -343,14 +352,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| (&e).into());
vec![machine.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",
"Not allowed to read state event".into(),
))
}
}
Expand Down Expand Up @@ -380,15 +391,15 @@ 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
),
format!("Not allowed: missing the {SEND_DELAYED_EVENT} capability.").into(),
));
}
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"),
self.send_from_widget_error_response(
raw_request,
"Not allowed to send event".into(),
),
);
}

Expand All @@ -397,7 +408,8 @@ impl WidgetMachine {
if let Ok(r) = result.as_mut() {
r.set_room_id(machine.room_id.clone());
}
vec![machine.send_from_widget_result_response(raw_request, result)]
vec![machine
.send_from_widget_result_response(raw_request, result.map_err(|e| (&e).into()))]
});
action
}
Expand Down Expand Up @@ -439,7 +451,7 @@ impl WidgetMachine {
fn process_matrix_driver_response(
&mut self,
request_id: Uuid,
response: Result<MatrixDriverResponse, String>,
response: Result<MatrixDriverResponse, Error>,
) -> Vec<Action> {
match self.pending_matrix_driver_requests.extract(&request_id) {
Ok(request) => request
Expand Down Expand Up @@ -472,15 +484,15 @@ impl WidgetMachine {
fn send_from_widget_error_response(
&self,
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(
&self,
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 @@ -586,7 +598,7 @@ impl ToWidgetRequestMeta {
}

type MatrixDriverResponseFn =
Box<dyn FnOnce(Result<MatrixDriverResponse, String>, &mut WidgetMachine) -> Vec<Action> + Send>;
Box<dyn FnOnce(Result<MatrixDriverResponse, Error>, &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 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 @@ -162,7 +162,7 @@ fn 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