chathistory: Fix error handling on timestamp parsing

sable_ircd/src/command/client_command.rs

@@ -179,9 +179,7 @@ impl Command for ClientCommand {
     }
 
     fn notify_error(&self, err: CommandError) {
-        if let Some(n) = self.translate_command_error(err) {
-            let _ = self.response_sink().numeric(n);
-        }
+        self.send_command_error(err)
     }
 
     fn response_sink(&self) -> &dyn CommandResponse {
@@ -206,8 +204,8 @@ impl Command for ClientCommand {
 }
 
 impl ClientCommand {
-    fn translate_command_error(&self, err: CommandError) -> Option<UntargetedNumeric> {
-        match err {
+    fn send_command_error(&self, err: CommandError) {
+        let numeric = match err {
             CommandError::UnderlyingError(_) => {
                 todo!()
             }
@@ -277,6 +275,19 @@ impl ClientCommand {
                 }
             }
             CommandError::Numeric(n) => Some(n),
+            CommandError::Fail {
+                command,
+                code,
+                context,
+                description,
+            } => {
+                self.response_sink
+                    .send(message::Fail::new(command, code, &context, &description));
+                None
+            }
+        };
+        if let Some(numeric) = numeric {
+            self.response_sink.numeric(numeric)
         }
     }
 }

sable_ircd/src/command/error.rs

@@ -12,9 +12,9 @@
 #[derive(Debug)]
 pub enum CommandError {
     /// Something returned an `Error` that we don't know how to handle
     UnderlyingError(anyhow::Error),
     /// Something went wrong but we don't have an `Error` impl for it
     UnknownError(String),
     /*
         /// The command couldn't be processed successfully, and the client has already been
         /// notified
@@ -47,6 +47,17 @@
     /// The command couldn't be processed successfully; the provided
     /// numeric(messages::UntargetedNumeric) will be sent to the client to notify them
     Numeric(messages::UntargetedNumeric),
+
+    /// The command couldn't be processed successfully; the provided parameters will
+    /// be turned into a [`FAIL` standard
+    /// reply](https://ircv3.net/specs/extensions/standard-replies) and sent to the client
+    /// to notify them
+    Fail {
+        command: &'static str,
+        code: &'static str,
+        context: String,
+        description: String,
+    },
 }
 
 impl From<ValidationError> for CommandError {

sable_ircd/src/command/handlers/chathistory.rs

@@ -5,6 +5,35 @@ use sable_network::network::update::HistoricMessageTarget;
 
 use std::cmp::{max, min};
 
+fn parse_msgref(subcommand: &str, target: Option<&str>, msgref: &str) -> Result<i64, CommandError> {
+    match msgref.split_once('=') {
+        Some(("timestamp", ts)) => utils::parse_timestamp(ts).ok_or_else(|| CommandError::Fail {
+            command: "CHATHISTORY",
+            code: "INVALID_PARAMS",
+            context: subcommand.to_string(),
+            description: "Invalid timestamp".to_string(),
+        }),
+        Some(("msgid", _)) => Err(CommandError::Fail {
+            command: "CHATHISTORY",
+            code: "INVALID_MSGREFTYPE",
+            context: match target {
+                Some(target) => format!("{} {}", subcommand, target),
+                None => subcommand.to_string(),
+            },
+            description: "msgid-based history requests are not supported yet".to_string(),
+        }),
+        _ => Err(CommandError::Fail {
+            command: "CHATHISTORY",
+            code: "INVALID_MSGREFTYPE",
+            context: match target {
+                Some(target) => format!("{} {}", subcommand, target),
+                None => subcommand.to_string(),
+            },
+            description: format!("{:?} is not a valid message reference", msgref),
+        }),
+    }
+}
+
 #[command_handler("CHATHISTORY")]
 fn handle_chathistory(
     source: UserSource,
@@ -20,19 +49,10 @@ fn handle_chathistory(
 
     match subcommand.to_ascii_uppercase().as_str() {
         "TARGETS" => {
-            let from_ts = utils::parse_timestamp(arg_1);
-            let to_ts = utils::parse_timestamp(arg_2);
+            let from_ts = parse_msgref(subcommand, None, arg_1)?;
+            let to_ts = parse_msgref(subcommand, None, arg_2)?;
             let limit = arg_3.parse().ok();
 
-            if from_ts.is_none() || to_ts.is_none() {
-                response.send(message::Fail::new(
-                    "CHATHISTORY",
-                    "INVALID_PARAMS",
-                    "",
-                    "Invalid timestamp",
-                ));
-                return Ok(());
-            }
             if limit.is_none() {
                 response.send(message::Fail::new(
                     "CHATHISTORY",
@@ -48,27 +68,16 @@ fn handle_chathistory(
                 server,
                 &response,
                 source,
-                min(from_ts, to_ts),
-                max(from_ts, to_ts),
+                Some(min(from_ts, to_ts)),
+                Some(max(from_ts, to_ts)),
                 limit,
             );
         }
         "LATEST" => {
             let target = arg_1;
             let from_ts = match arg_2 {
                 "*" => None,
-                _ => match utils::parse_timestamp(arg_2) {
-                    Some(ts) => Some(ts),
-                    None => {
-                        response.send(message::Fail::new(
-                            "CHATHISTORY",
-                            "INVALID_PARAMS",
-                            "",
-                            "Invalid timestamp",
-                        ));
-                        return Ok(());
-                    }
-                },
+                _ => Some(parse_msgref(subcommand, Some(target), arg_2)?),
             };
 
             let limit = arg_3.parse().ok();
@@ -87,19 +96,8 @@ fn handle_chathistory(
             )?;
         }
         "BEFORE" => {
-            let target = arg_1.to_string();
-            let end_ts = match utils::parse_timestamp(arg_2) {
-                Some(ts) => ts,
-                None => {
-                    response.send(message::Fail::new(
-                        "CHATHISTORY",
-                        "INVALID_PARAMS",
-                        "",
-                        "Invalid timestamp",
-                    ));
-                    return Ok(());
-                }
-            };
+            let target = arg_1;
+            let end_ts = parse_msgref(subcommand, Some(target), arg_2)?;
 
             let limit = arg_3.parse().ok();
             if limit.is_none() {
@@ -117,26 +115,15 @@ fn handle_chathistory(
                 &response,
                 source,
                 subcommand,
-                &target,
+                target,
                 None,
                 Some(end_ts),
                 limit,
             )?;
         }
         "AFTER" => {
             let target = arg_1;
-            let start_ts = match utils::parse_timestamp(arg_2) {
-                Some(ts) => ts,
-                None => {
-                    response.send(message::Fail::new(
-                        "CHATHISTORY",
-                        "INVALID_PARAMS",
-                        "",
-                        "Invalid timestamp",
-                    ));
-                    return Ok(());
-                }
-            };
+            let start_ts = parse_msgref(subcommand, Some(target), arg_2)?;
 
             let limit = arg_3.parse().ok();
             if limit.is_none() {
@@ -162,18 +149,7 @@ fn handle_chathistory(
         }
         "AROUND" => {
             let target = arg_1;
-            let around_ts = match utils::parse_timestamp(arg_2) {
-                Some(ts) => ts,
-                None => {
-                    response.send(message::Fail::new(
-                        "CHATHISTORY",
-                        "INVALID_PARAMS",
-                        "",
-                        "Invalid timestamp",
-                    ));
-                    return Ok(());
-                }
-            };
+            let around_ts = parse_msgref(subcommand, Some(target), arg_2)?;
 
             let limit = match arg_3.parse::<usize>().ok() {
                 Some(limit) => limit,
@@ -211,30 +187,8 @@ fn handle_chathistory(
         }
         "BETWEEN" => {
             let target = arg_1;
-            let start_ts = match utils::parse_timestamp(arg_2) {
-                Some(ts) => ts,
-                None => {
-                    response.send(message::Fail::new(
-                        "CHATHISTORY",
-                        "INVALID_PARAMS",
-                        "",
-                        "Invalid timestamp",
-                    ));
-                    return Ok(());
-                }
-            };
-            let end_ts = match utils::parse_timestamp(arg_3) {
-                Some(ts) => ts,
-                None => {
-                    response.send(message::Fail::new(
-                        "CHATHISTORY",
-                        "INVALID_PARAMS",
-                        "",
-                        "Invalid timestamp",
-                    ));
-                    return Ok(());
-                }
-            };
+            let start_ts = parse_msgref(subcommand, Some(target), arg_2)?;
+            let end_ts = parse_msgref(subcommand, Some(target), arg_3)?;
 
             let limit = arg_4.and_then(|arg| arg.parse().ok());
             if limit.is_none() {

sable_ircd/src/command/handlers/services/mod.rs

@@ -161,6 +161,10 @@ impl<'a> Command for ServicesCommand<'a> {
                 tracing::warn!("Translating unknown error numeric from services response: {:?}", n);
                 self.notice(format_args!("Unknown error: {}", n.debug_format()));
             }
+            CommandError::Fail { command, code, context, description } => {
+                tracing::warn!("Translating unknown error numeric from services response: {} {} {} :{}", command, code, context, description);
+                self.notice(format_args!("Unknown error: {} {} {} :{}", command, code, context, description));
+            }
         }
     }
 }