Address review feedback and finish canonical JSON move

crates/ruma-common/src/canonical_json.rs

@@ -36,6 +36,72 @@ impl fmt::Display for CanonicalJsonError {
 
 impl std::error::Error for CanonicalJsonError {}
 
+/// Errors that can happen in redaction.
+#[cfg(feature = "canonical-json")]
+#[derive(Debug)]
+#[cfg_attr(not(feature = "unstable-exhaustive-types"), non_exhaustive)]
+pub enum RedactionError {
+    /// The field `field` is not of the correct type `of_type` ([`JsonType`]).
+    #[cfg_attr(not(feature = "unstable-exhaustive-types"), non_exhaustive)]
+    NotOfType {
+        /// The field name.
+        field: String,
+        /// The expected JSON type.
+        of_type: JsonType,
+    },
+
+    /// The given required field is missing from a JSON object.
+    JsonFieldMissingFromObject(String),
+}
+
+impl fmt::Display for RedactionError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            RedactionError::NotOfType { field, of_type } => {
+                write!(f, "Value in {field:?} must be a JSON {of_type:?}")
+            }
+            RedactionError::JsonFieldMissingFromObject(field) => {
+                write!(f, "JSON object must contain the field {field:?}")
+            }
+        }
+    }
+}
+
+impl std::error::Error for RedactionError {}
+
+impl RedactionError {
+    fn not_of_type(target: &str, of_type: JsonType) -> Self {
+        Self::NotOfType { field: target.to_owned(), of_type }
+    }
+
+    fn field_missing_from_object(target: &str) -> Self {
+        Self::JsonFieldMissingFromObject(target.to_owned())
+    }
+}
+
+/// A JSON type enum for [`RedactionError`] variants.
+#[derive(Debug)]
+#[allow(clippy::exhaustive_enums)]
+pub enum JsonType {
+    /// A JSON Object.
+    Object,
+
+    /// A JSON String.
+    String,
+
+    /// A JSON Integer.
+    Integer,
+
+    /// A JSON Array.
+    Array,
+
+    /// A JSON Boolean.
+    Boolean,
+
+    /// JSON Null.
+    Null,
+}
+
 /// Fallible conversion from a `serde_json::Map` to a `CanonicalJsonObject`.
 pub fn try_from_json_map(
     json: serde_json::Map<String, JsonValue>,
@@ -74,7 +140,7 @@ pub fn to_canonical_value<T: Serialize>(
 pub fn redact(
     object: &CanonicalJsonObject,
     version: &RoomVersionId,
-) -> Result<CanonicalJsonObject, ()> {
+) -> Result<CanonicalJsonObject, RedactionError> {
     let mut val = object.clone();
     redact_in_place(&mut val, version)?;
     Ok(val)
@@ -85,21 +151,24 @@ pub fn redact(
 /// Functionally equivalent to `redact`, only;
 /// * upon error, the event is not touched.
 /// * this'll redact the event in-place.
-pub fn redact_in_place(event: &mut CanonicalJsonObject, version: &RoomVersionId) -> Result<(), ()> {
+pub fn redact_in_place(
+    event: &mut CanonicalJsonObject,
+    version: &RoomVersionId,
+) -> Result<(), RedactionError> {
     // Get the content keys here instead of the event type, because we cant teach rust that this is
     // a disjoint borrow.
     let allowed_content_keys: &[&str] = match event.get("type") {
         Some(CanonicalJsonValue::String(event_type)) => {
             allowed_content_keys_for(event_type, version)
         }
-        Some(_) => return Err(()),
-        None => return Err(()),
+        Some(_) => return Err(RedactionError::not_of_type("type", JsonType::String)),
+        None => return Err(RedactionError::field_missing_from_object("type")),
     };
 
     if let Some(content_value) = event.get_mut("content") {
         let content = match content_value {
             CanonicalJsonValue::Object(map) => map,
-            _ => return Err(()),
+            _ => return Err(RedactionError::not_of_type("content", JsonType::Object)),
         };
 
         object_retain_keys(content, allowed_content_keys);

crates/ruma-common/src/canonical_json/value.rs

@@ -19,7 +19,7 @@ pub enum CanonicalJsonValue {
     ///
     /// ```
     /// # use serde_json::json;
-    /// # use ruma_common::serde::CanonicalJsonValue;
+    /// # use ruma_common::CanonicalJsonValue;
     /// let v: CanonicalJsonValue = json!(null).try_into().unwrap();
     /// ```
     Null,
@@ -28,7 +28,7 @@ pub enum CanonicalJsonValue {
     ///
     /// ```
     /// # use serde_json::json;
-    /// # use ruma_common::serde::CanonicalJsonValue;
+    /// # use ruma_common::CanonicalJsonValue;
     /// let v: CanonicalJsonValue = json!(true).try_into().unwrap();
     /// ```
     Bool(bool),
@@ -37,7 +37,7 @@ pub enum CanonicalJsonValue {
     ///
     /// ```
     /// # use serde_json::json;
-    /// # use ruma_common::serde::CanonicalJsonValue;
+    /// # use ruma_common::CanonicalJsonValue;
     /// let v: CanonicalJsonValue = json!(12).try_into().unwrap();
     /// ```
     Integer(Int),
@@ -46,7 +46,7 @@ pub enum CanonicalJsonValue {
     ///
     /// ```
     /// # use serde_json::json;
-    /// # use ruma_common::serde::CanonicalJsonValue;
+    /// # use ruma_common::CanonicalJsonValue;
     /// let v: CanonicalJsonValue = json!("a string").try_into().unwrap();
     /// ```
     String(String),
@@ -55,7 +55,7 @@ pub enum CanonicalJsonValue {
     ///
     /// ```
     /// # use serde_json::json;
-    /// # use ruma_common::serde::CanonicalJsonValue;
+    /// # use ruma_common::CanonicalJsonValue;
     /// let v: CanonicalJsonValue = json!(["an", "array"]).try_into().unwrap();
     /// ```
     Array(Vec<CanonicalJsonValue>),
@@ -66,7 +66,7 @@ pub enum CanonicalJsonValue {
     ///
     /// ```
     /// # use serde_json::json;
-    /// # use ruma_common::serde::CanonicalJsonValue;
+    /// # use ruma_common::CanonicalJsonValue;
     /// let v: CanonicalJsonValue = json!({ "an": "object" }).try_into().unwrap();
     /// ```
     Object(CanonicalJsonObject),

crates/ruma-signatures/src/error.rs

@@ -1,5 +1,7 @@
 use ruma_common::{
-    serde::Base64DecodeError, EventId, OwnedEventId, OwnedServerName, RoomVersionId,
+    canonical_json::{JsonType, RedactionError},
+    serde::Base64DecodeError,
+    EventId, OwnedEventId, OwnedServerName, RoomVersionId,
 };
 use thiserror::Error;
 
@@ -41,11 +43,25 @@ pub enum Error {
     PduSize,
 }
 
+impl From<RedactionError> for Error {
+    fn from(err: RedactionError) -> Self {
+        match err {
+            RedactionError::NotOfType { field: target, of_type, .. } => {
+                JsonError::NotOfType { target, of_type }.into()
+            }
+            RedactionError::JsonFieldMissingFromObject(field) => {
+                JsonError::JsonFieldMissingFromObject(field).into()
+            }
+            _ => unreachable!(),
+        }
+    }
+}
+
 /// All errors related to JSON validation/parsing.
 #[derive(Debug, Error)]
 #[non_exhaustive]
 pub enum JsonError {
-    /// Signals that `target` is not of type `of_type` ([`JsonType`]).
+    /// `target` is not of the correct type `of_type` ([`JsonType`]).
     #[error("Value in {target:?} must be a JSON {of_type:?}")]
     NotOfType {
         /// An arbitrary "target" that doesn't have the required type.
@@ -65,11 +81,11 @@ pub enum JsonError {
         of_type: JsonType,
     },
 
-    /// Signals that a specific field is missing from a JSON object.
+    /// The given required field is missing from a JSON object.
     #[error("JSON object must contain the field {0:?}")]
     JsonFieldMissingFromObject(String),
 
-    /// Signals a key missing from a JSON object.
+    /// A key is missing from a JSON object.
     ///
     /// Note that this is different from [`JsonError::JsonFieldMissingFromObject`],
     /// this error talks about an expected identifying key (`"ed25519:abcd"`)
@@ -117,29 +133,6 @@ impl JsonError {
     }
 }
 
-/// A JSON type enum for [`JsonError`] variants.
-#[derive(Debug)]
-#[allow(clippy::exhaustive_enums)]
-pub enum JsonType {
-    /// A JSON Object.
-    Object,
-
-    /// A JSON String.
-    String,
-
-    /// A JSON Integer.
-    Integer,
-
-    /// A JSON Array.
-    Array,
-
-    /// A JSON Boolean.
-    Boolean,
-
-    /// JSON Null.
-    Null,
-}
-
 /// Errors relating to verification of events and signatures.
 #[derive(Debug, Error)]
 #[non_exhaustive]

crates/ruma-signatures/src/functions.rs

@@ -8,6 +8,7 @@ use std::{
 
 use base64::{encode_config, STANDARD_NO_PAD, URL_SAFE_NO_PAD};
 use ruma_common::{
+    canonical_json::{redact, JsonType},
     serde::{base64::Standard, Base64},
     CanonicalJsonObject, CanonicalJsonValue, OwnedEventId, OwnedServerName, RoomVersionId, UserId,
 };
@@ -18,7 +19,7 @@ use crate::{
     keys::{KeyPair, PublicKeyMap},
     split_id,
     verification::{Ed25519Verifier, Verified, Verifier},
-    Error, JsonError, JsonType, ParseError, VerificationError,
+    Error, JsonError, ParseError, VerificationError,
 };
 
 const MAX_PDU_BYTES: usize = 65_535;

crates/ruma-signatures/src/lib.rs

@@ -46,13 +46,12 @@
 
 use ruma_common::serde::{AsRefStr, DisplayAsRefStr};
 
-pub use error::{Error, JsonError, JsonType, ParseError, VerificationError};
+pub use error::{Error, JsonError, ParseError, VerificationError};
 pub use functions::{
-    canonical_json, content_hash, hash_and_sign_event, redact, redact_content_in_place,
-    redact_in_place, reference_hash, sign_json, verify_event, verify_json,
+    canonical_json, content_hash, hash_and_sign_event, reference_hash, sign_json, verify_event,
+    verify_json,
 };
 pub use keys::{Ed25519KeyPair, KeyPair, PublicKeyMap, PublicKeySet};
-pub use ruma_common::{CanonicalJsonError, CanonicalJsonObject, CanonicalJsonValue};
 pub use signatures::Signature;
 pub use verification::Verified;