Use thiserror in id-validation, add more detail to mxc-uri errors

crates/ruma-api/Cargo.toml

@@ -34,7 +34,7 @@ ruma-identifiers = { version = "0.20.0", path = "../ruma-identifiers" }
 ruma-serde = { version = "0.5.0", path = "../ruma-serde" }
 serde = { version = "1.0.118", features = ["derive"] }
 serde_json = "1.0.61"
-thiserror = "1.0.23"
+thiserror = "1.0.26"
 
 [dev-dependencies]
 ruma-events = { version = "0.24.5", path = "../ruma-events" }

crates/ruma-client-api/src/r0/media/get_content.rs

@@ -60,11 +60,9 @@ impl<'a> Request<'a> {
 
     /// Creates a new `Request` with the given url.
     pub fn from_url(url: &'a MxcUri) -> Result<Self, Error> {
-        if let Some((server_name, media_id)) = url.parts() {
+        let (server_name, media_id) = url.parts()?;
+
         Ok(Self { media_id, server_name, allow_remote: true })
-        } else {
-            Err(Error::InvalidMxcUri)
-        }
     }
 }
 

crates/ruma-client-api/src/r0/media/get_content_as_filename.rs

@@ -65,11 +65,9 @@ impl<'a> Request<'a> {
 
     /// Creates a new `Request` with the given url and filename.
     pub fn from_url(url: &'a MxcUri, filename: &'a str) -> Result<Self, Error> {
-        if let Some((server_name, media_id)) = url.parts() {
+        let (server_name, media_id) = url.parts()?;
+
         Ok(Self { media_id, server_name, filename, allow_remote: true })
-        } else {
-            Err(Error::InvalidMxcUri)
-        }
     }
 }
 

crates/ruma-client-api/src/r0/media/get_content_thumbnail.rs

@@ -70,11 +70,9 @@ impl<'a> Request<'a> {
     /// Creates a new `Request` with the given url, desired thumbnail width and
     /// desired thumbnail height.
     pub fn from_url(url: &'a MxcUri, width: UInt, height: UInt) -> Result<Self, Error> {
-        if let Some((server_name, media_id)) = url.parts() {
+        let (server_name, media_id) = url.parts()?;
+
         Ok(Self { media_id, server_name, method: None, width, height, allow_remote: true })
-        } else {
-            Err(Error::InvalidMxcUri)
-        }
     }
 }
 

crates/ruma-events/Cargo.toml

@@ -33,7 +33,7 @@ ruma-identifiers = { version = "0.20.0", path = "../ruma-identifiers", features
 ruma-serde = { version = "0.5.0", path = "../ruma-serde" }
 serde = { version = "1.0.118", features = ["derive"] }
 serde_json = { version = "1.0.60", features = ["raw_value"] }
-thiserror = "1.0.25"
+thiserror = "1.0.26"
 
 [dev-dependencies]
 assign = "1.1.1"

crates/ruma-identifiers-validation/Cargo.toml

@@ -16,3 +16,6 @@ all-features = true
 
 [features]
 compat = []
+
+[dependencies]
+thiserror = "1.0.26"

crates/ruma-identifiers-validation/src/error.rs

@@ -1,66 +1,77 @@
 //! Error conditions.
 
-use std::fmt;
-
 /// An error encountered when trying to parse an invalid ID string.
-#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)]
+#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, thiserror::Error)]
 #[non_exhaustive]
 pub enum Error {
     /// The client secret is empty.
+    #[error("client secret is empty")]
     EmptyClientSecret,
 
     /// The room name is empty.
+    #[error("room name is empty")]
     EmptyRoomName,
 
     /// The room version ID is empty.
+    #[error("room version ID is empty")]
     EmptyRoomVersionId,
 
     /// The ID's localpart contains invalid characters.
     ///
     /// Only relevant for user IDs.
+    #[error("localpart contains invalid characters")]
     InvalidCharacters,
 
     /// The key algorithm is invalid (e.g. empty).
+    #[error("invalid key algorithm specified")]
     InvalidKeyAlgorithm,
 
     /// The key version contains outside of [a-zA-Z0-9_].
+    #[error("key ID version contains invalid characters")]
     InvalidKeyVersion,
 
     /// The mxc:// isn't a valid Matrix Content URI.
-    InvalidMxcUri,
+    #[error("invalid Matrix Content URI: {0}")]
+    InvalidMxcUri(#[from] MxcUriError),
 
     /// The server name part of the the ID string is not a valid server name.
+    #[error("server name is not a valid IP address or domain name")]
     InvalidServerName,
 
     /// The ID exceeds 255 bytes (or 32 codepoints for a room version ID).
+    #[error("ID exceeds 255 bytes")]
     MaximumLengthExceeded,
 
     /// The ID is missing the colon delimiter between localpart and server name, or between key
     /// algorithm and key name / version.
+    #[error("required colon is missing")]
     MissingDelimiter,
 
     /// The ID is missing the correct leading sigil.
+    #[error("leading sigil is incorrect or missing")]
     MissingLeadingSigil,
 }
 
-impl fmt::Display for Error {
+/// An error occurred while validating an MXC URI.
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, thiserror::Error)]
-        let message = match self {
+#[non_exhaustive]
-            Error::EmptyClientSecret => "client secret is empty",
+pub enum MxcUriError {
-            Error::EmptyRoomName => "room name is empty",
+    /// MXC URI did not start with `mxc://`.
-            Error::EmptyRoomVersionId => "room version ID is empty",
+    #[error("MXC URI schema was not mxc://")]
-            Error::InvalidCharacters => "localpart contains invalid characters",
+    WrongSchema,
-            Error::InvalidKeyAlgorithm => "invalid key algorithm specified",
-            Error::InvalidKeyVersion => "key ID version contains invalid characters",
-            Error::InvalidMxcUri => "the mxc:// isn't a valid Matrix Content URI",
-            Error::InvalidServerName => "server name is not a valid IP address or domain name",
-            Error::MaximumLengthExceeded => "ID exceeds 255 bytes",
-            Error::MissingDelimiter => "required colon is missing",
-            Error::MissingLeadingSigil => "leading sigil is incorrect or missing",
-        };
 
-        write!(f, "{}", message)
+    /// MXC URI did not have first slash, required for `server.name/media_id`.
-    }
+    #[error("MXC URI does not have first slash")]
-}
+    MissingSlash,
 
-impl std::error::Error for Error {}
+    /// Media identifier malformed due to invalid characters detected.
+    ///
+    /// Valid characters are (in regex notation) `[A-Za-z0-9_-]+`.
+    /// See [here](https://matrix.org/docs/spec/client_server/r0.6.1#id408) for more details.
+    #[error("Media Identifier malformed, invalid characters")]
+    MediaIdMalformed,
+
+    /// Server identifier malformed: invalid IP or domain name.
+    #[error("invalid Server Name")]
+    ServerNameMalformed,
+}

crates/ruma-identifiers-validation/src/mxc_uri.rs

@@ -1,18 +1,18 @@
 use std::num::NonZeroU8;
 
-use crate::{server_name, Error};
+use crate::{error::MxcUriError, server_name};
 
 const PROTOCOL: &str = "mxc://";
 
-pub fn validate(uri: &str) -> Result<NonZeroU8, Error> {
+pub fn validate(uri: &str) -> Result<NonZeroU8, MxcUriError> {
     let uri = match uri.strip_prefix(PROTOCOL) {
         Some(uri) => uri,
-        None => return Err(Error::InvalidMxcUri),
+        None => return Err(MxcUriError::WrongSchema),
     };
 
     let index = match uri.find('/') {
         Some(index) => index,
-        None => return Err(Error::InvalidMxcUri),
+        None => return Err(MxcUriError::MissingSlash),
     };
 
     let server_name = &uri[..index];
@@ -21,9 +21,11 @@ pub fn validate(uri: &str) -> Result<NonZeroU8, Error> {
     let media_id_is_valid =
         media_id.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'z' | b'A'..=b'Z' | b'-' ));
 
-    if media_id_is_valid && server_name::validate(server_name).is_ok() {
+    if !media_id_is_valid {
-        Ok(NonZeroU8::new((index + 6) as u8).unwrap())
+        Err(MxcUriError::MediaIdMalformed)
+    } else if server_name::validate(server_name).is_err() {
+        Err(MxcUriError::ServerNameMalformed)
     } else {
-        Err(Error::InvalidMxcUri)
+        Ok(NonZeroU8::new((index + 6) as u8).unwrap())
     }
 }

crates/ruma-identifiers/src/mxc_uri.rs

@@ -4,33 +4,35 @@
 
 use std::{convert::TryFrom, fmt, num::NonZeroU8};
 
-use ruma_identifiers_validation::mxc_uri::validate;
+use ruma_identifiers_validation::{error::MxcUriError, mxc_uri::validate};
 
 use crate::ServerName;
 
+type Result<T> = std::result::Result<T, MxcUriError>;
+
 /// A URI that should be a Matrix-spec compliant [MXC URI].
 ///
 /// [MXC URI]: https://matrix.org/docs/spec/client_server/r0.6.1#mxc-uri
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct MxcUri {
     full_uri: Box<str>,
-    slash_idx: Option<NonZeroU8>,
 }
 
 impl MxcUri {
     /// If this is a valid MXC URI, returns the media ID.
-    pub fn media_id(&self) -> Option<&str> {
+    pub fn media_id(&self) -> Result<&str> {
         self.parts().map(|(_, s)| s)
     }
 
     /// If this is a valid MXC URI, returns the server name.
-    pub fn server_name(&self) -> Option<&ServerName> {
+    pub fn server_name(&self) -> Result<&ServerName> {
         self.parts().map(|(s, _)| s)
     }
 
-    /// If this is a valid MXC URI, returns a `(server_name, media_id)` tuple.
+    /// If this is a valid MXC URI, returns a `(server_name, media_id)` tuple, else it returns the
-    pub fn parts(&self) -> Option<(&ServerName, &str)> {
+    /// error.
-        self.slash_idx.map(|idx| {
+    pub fn parts(&self) -> Result<(&ServerName, &str)> {
+        self.extract_slash_idx().map(|idx| {
             (
                 <&ServerName>::try_from(&self.full_uri[6..idx.get() as usize]).unwrap(),
                 &self.full_uri[idx.get() as usize + 1..],
@@ -38,15 +40,28 @@ impl MxcUri {
         })
     }
 
-    /// Returns if this is a spec-compliant MXC URI.
+    /// Validates the URI and returns an error if it failed.
+    pub fn validate(&self) -> Result<()> {
+        self.extract_slash_idx().map(|_| ())
+    }
+
+    /// Convenience method for `.validate().is_ok()`.
+    #[inline(always)]
     pub fn is_valid(&self) -> bool {
-        self.slash_idx.is_some()
+        self.validate().is_ok()
     }
 
     /// Create a string slice from this MXC URI.
+    #[inline(always)]
     pub fn as_str(&self) -> &str {
         &self.full_uri
     }
+
+    // convenience method for calling validate(self)
+    #[inline(always)]
+    fn extract_slash_idx(&self) -> Result<NonZeroU8> {
+        validate(self.as_str())
+    }
 }
 
 impl fmt::Debug for MxcUri {
@@ -65,10 +80,7 @@ fn from<S>(uri: S) -> MxcUri
 where
     S: AsRef<str> + Into<Box<str>>,
 {
-    match validate(uri.as_ref()) {
+    MxcUri { full_uri: uri.into() }
-        Ok(idx) => MxcUri { full_uri: uri.into(), slash_idx: Some(idx) },
-        Err(_) => MxcUri { full_uri: uri.into(), slash_idx: None },
-    }
 }
 
 impl From<&str> for MxcUri {
@@ -85,7 +97,7 @@ impl From<String> for MxcUri {
 
 #[cfg(feature = "serde")]
 impl<'de> serde::Deserialize<'de> for MxcUri {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
     where
         D: serde::Deserializer<'de>,
     {
@@ -95,7 +107,7 @@ impl<'de> serde::Deserialize<'de> for MxcUri {
 
 #[cfg(feature = "serde")]
 impl serde::Serialize for MxcUri {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
     where
         S: serde::Serializer,
     {
@@ -107,6 +119,8 @@ impl serde::Serialize for MxcUri {
 mod tests {
     use std::convert::TryFrom;
 
+    use ruma_identifiers_validation::error::MxcUriError;
+
     use crate::ServerName;
 
     use super::MxcUri;
@@ -118,7 +132,7 @@ mod tests {
         assert!(mxc.is_valid());
         assert_eq!(
             mxc.parts(),
-            Some((
+            Ok((
                 <&ServerName>::try_from("127.0.0.1").expect("Failed to create ServerName"),
                 "asd32asdfasdsd"
             ))
@@ -130,7 +144,7 @@ mod tests {
         let mxc = MxcUri::from("mxc://127.0.0.1");
 
         assert!(!mxc.is_valid());
-        assert_eq!(mxc.parts(), None);
+        assert_eq!(mxc.parts(), Err(MxcUriError::MissingSlash));
     }
 
     #[test]
@@ -158,10 +172,7 @@ mod tests {
         assert!(mxc.is_valid());
         assert_eq!(
             mxc.parts(),
-            Some((
+            Ok((<&ServerName>::try_from("server").expect("Failed to create ServerName"), "1234id"))
-                <&ServerName>::try_from("server").expect("Failed to create ServerName"),
-                "1234id"
-            ))
         );
     }
 }

crates/ruma-signatures/Cargo.toml

@@ -28,5 +28,5 @@ ruma-identifiers = { version = "0.20.0", path = "../ruma-identifiers" }
 ruma-serde = { version = "0.5.0", path = "../ruma-serde" }
 serde_json = "1.0.60"
 sha2 = "0.9.5"
-thiserror = "1.0.23"
+thiserror = "1.0.26"
 tracing = { version = "0.1.25", optional = true }

crates/ruma-state-res/Cargo.toml

@@ -27,7 +27,7 @@ ruma-identifiers = { version = "0.20.0", path = "../ruma-identifiers" }
 ruma-serde = { version = "0.5.0", path = "../ruma-serde" }
 serde = { version = "1.0.118", features = ["derive"] }
 serde_json = "1.0.60"
-thiserror = "1.0.22"
+thiserror = "1.0.26"
 tracing = "0.1.26"
 
 [dev-dependencies]