Improve errors when MAS contacts the Synapse homeserver (#2794)

* Add some drive-by docstrings * Change text rendering of catch_http_codes::HttpError Using `#[source]` is unnatural here because it makes it look like two distinct errors (one being a cause of the other), when in reality it is just one error, with 2 parts. Using `Display` formatting for that leads to a more natural error. * Add constraints to `catch_http_code{,s}` methods Not strictly required, but does two things: - documents what kind of function is expected - provides a small extra amount of type enforcement at the call site, rather than later on when you find the result doesn't implement Service * Add a `catch_http_errors` shorthand Nothing major, just a quality of life improvement so you don't have to repetitively write out what a HTTP error is * Unexpected error page: remove leading whitespace from preformatted 'details' section The extra whitespace was probably unintentional and makes the error harder to read, particularly when it wraps onto a new line unnecessarily * Capture and log Matrix errors received from Synapse * Drive-by clippy fix: use clamp instead of min().max() * Convert `err(Display)` to `err(Debug)` for `anyhow::Error`s in matrix-synapse support module
matrix-org · Jun 7, 2024 · 49e8fe5 · 49e8fe5
1 parent d76b54b
commit 49e8fe5
Show file tree

Hide file tree

Showing 9 changed files with 237 additions and 47 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/axum-utils/src/language_detection.rs b/crates/axum-utils/src/language_detection.rs
@@ -109,7 +109,7 @@ impl Header for AcceptLanguage {
                     let quality = std::str::from_utf8(quality).map_err(|_e| Error::invalid())?;
                     let quality = quality.parse::<f64>().map_err(|_e| Error::invalid())?;
                     // Bound the quality between 0 and 1
-                    let quality = quality.min(1_f64).max(0_f64);
+                    let quality = quality.clamp(0_f64, 1_f64);
 
                     // Make sure the iterator is empty
                     if it.next().is_some() {

diff --git a/crates/http/src/ext.rs b/crates/http/src/ext.rs
@@ -14,7 +14,7 @@
 
 use std::{ops::RangeBounds, sync::OnceLock};
 
-use http::{header::HeaderName, Request, StatusCode};
+use http::{header::HeaderName, Request, Response, StatusCode};
 use tower::Service;
 use tower_http::cors::CorsLayer;
 
@@ -70,6 +70,9 @@ pub trait ServiceExt<Body>: Sized {
         BytesToBodyRequest::new(self)
     }
 
+    /// Adds a layer which collects all the response body into a contiguous
+    /// byte buffer.
+    /// This makes the response type `Response<Bytes>`.
     fn response_body_to_bytes(self) -> BodyToBytesResponse<Self> {
         BodyToBytesResponse::new(self)
     }
@@ -86,20 +89,40 @@ pub trait ServiceExt<Body>: Sized {
         FormUrlencodedRequest::new(self)
     }
 
-    fn catch_http_code<M>(self, status_code: StatusCode, mapper: M) -> CatchHttpCodes<Self, M>
+    /// Catches responses with the given status code and then maps those
+    /// responses to an error type using the provided `mapper` function.
+    fn catch_http_code<M, ResBody, E>(
+        self,
+        status_code: StatusCode,
+        mapper: M,
+    ) -> CatchHttpCodes<Self, M>
     where
-        M: Clone,
+        M: Fn(Response<ResBody>) -> E + Send + Clone + 'static,
     {
         self.catch_http_codes(status_code..=status_code, mapper)
     }
 
-    fn catch_http_codes<B, M>(self, bounds: B, mapper: M) -> CatchHttpCodes<Self, M>
+    /// Catches responses with the given status codes and then maps those
+    /// responses to an error type using the provided `mapper` function.
+    fn catch_http_codes<B, M, ResBody, E>(self, bounds: B, mapper: M) -> CatchHttpCodes<Self, M>
     where
         B: RangeBounds<StatusCode>,
-        M: Clone,
+        M: Fn(Response<ResBody>) -> E + Send + Clone + 'static,
     {
         CatchHttpCodes::new(self, bounds, mapper)
     }
+
+    /// Shorthand for [`Self::catch_http_codes`] which catches all client errors
+    /// (4xx) and server errors (5xx).
+    fn catch_http_errors<M, ResBody, E>(self, mapper: M) -> CatchHttpCodes<Self, M>
+    where
+        M: Fn(Response<ResBody>) -> E + Send + Clone + 'static,
+    {
+        self.catch_http_codes(
+            StatusCode::from_u16(400).unwrap()..StatusCode::from_u16(600).unwrap(),
+            mapper,
+        )
+    }
 }
 
 impl<S, B> ServiceExt<B> for S where S: Service<Request<B>> {}
diff --git a/crates/http/src/layers/catch_http_codes.rs b/crates/http/src/layers/catch_http_codes.rs
@@ -24,12 +24,8 @@ pub enum Error<S, E> {
     #[error(transparent)]
     Service { inner: S },
 
-    #[error("request failed with status {status_code}")]
-    HttpError {
-        status_code: StatusCode,
-        #[source]
-        inner: E,
-    },
+    #[error("request failed with status {status_code}: {inner}")]
+    HttpError { status_code: StatusCode, inner: E },
 }
 
 impl<S, E> Error<S, E> {
@@ -45,10 +41,16 @@ impl<S, E> Error<S, E> {
     }
 }
 
+/// A layer that catches responses with the HTTP status codes lying within
+/// `bounds` and then maps the requests into a custom error type using `mapper`.
 #[derive(Clone)]
 pub struct CatchHttpCodes<S, M> {
+    /// The inner service
     inner: S,
+    /// Which HTTP status codes to catch
     bounds: (Bound<StatusCode>, Bound<StatusCode>),
+    /// The function used to convert errors, which must be
+    /// `Fn(Response<ResBody>) -> E + Send + Clone + 'static`.
     mapper: M,
 }
 

diff --git a/crates/http/src/layers/json_response.rs b/crates/http/src/layers/json_response.rs
@@ -23,6 +23,7 @@ use tower::{Layer, Service};
 
 #[derive(Debug, Error)]
 pub enum Error<Service> {
+    /// An error from the inner service.
     #[error(transparent)]
     Service { inner: Service },
 

diff --git a/crates/matrix-synapse/Cargo.toml b/crates/matrix-synapse/Cargo.toml
@@ -16,6 +16,7 @@ anyhow.workspace = true
 async-trait.workspace = true
 http.workspace = true
 serde.workspace = true
+serde_json.workspace = true
 tower.workspace = true
 tracing.workspace = true
 url.workspace = true

diff --git a/crates/matrix-synapse/src/error.rs b/crates/matrix-synapse/src/error.rs
@@ -0,0 +1,64 @@
+use std::{error::Error, fmt::Display};
+
+use http::Response;
+use mas_axum_utils::axum::body::Bytes;
+use serde::Deserialize;
+use tracing::debug;
+
+/// Represents a Matrix error
+/// Ref: <https://spec.matrix.org/v1.10/client-server-api/#standard-error-response>
+#[derive(Debug, Deserialize)]
+struct MatrixError {
+    errcode: String,
+    error: String,
+}
+
+/// Represents an error received from the homeserver.
+/// Where possible, we capture the Matrix error from the JSON response body.
+///
+/// Note that the `CatchHttpCodes` layer already captures the `StatusCode` for
+/// us; we don't need to do that twice.
+#[derive(Debug)]
+pub(crate) struct HomeserverError {
+    matrix_error: Option<MatrixError>,
+}
+
+impl Display for HomeserverError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if let Some(matrix_error) = &self.matrix_error {
+            write!(f, "{matrix_error}")
+        } else {
+            write!(f, "(no specific error)")
+        }
+    }
+}
+
+impl Error for HomeserverError {}
+
+impl HomeserverError {
+    /// Return the error code (`errcode`)
+    pub fn errcode(&self) -> Option<&str> {
+        self.matrix_error.as_ref().map(|me| me.errcode.as_str())
+    }
+}
+
+/// Parses a JSON-encoded Matrix error from the response body
+/// Spec reference: <https://spec.matrix.org/v1.10/client-server-api/#standard-error-response>
+#[allow(clippy::needless_pass_by_value)]
+pub(crate) fn catch_homeserver_error(response: Response<Bytes>) -> HomeserverError {
+    let matrix_error: Option<MatrixError> = match serde_json::from_slice(response.body().as_ref()) {
+        Ok(body) => Some(body),
+        Err(err) => {
+            debug!("failed to deserialise expected homeserver error: {err:?}");
+            None
+        }
+    };
+    HomeserverError { matrix_error }
+}
+
+impl Display for MatrixError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let MatrixError { errcode, error } = &self;
+        write!(f, "{errcode}: {error}")
+    }
+}