Encapsulate config with Arc. Further bot code cleanup.

Only expose config settings via methods on the Config struct. This
allows default value handling to live entirely inside the config code,
solves various borrowing issues with the "create default bot config
value" solution, and allows us to avoid cloning the bot config values.
The downside is that the config must now be in an Arc since its
ownership is shared in multiple places and the matrix SDK requires
thread-safe types (perhaps in the future we re-compose traits and use
Rc for config instead).

This commit also further cleans up and splits up the bot code for the
matrix connection, notably making the main message event handler
smaller by splitting out the "should we process the message" checks
into a separate function.

src/bin/dicebot.rs

@@ -9,6 +9,7 @@ use env_logger::Env;
 use log::error;
 use std::fs;
 use std::path::PathBuf;
+use std::sync::Arc;
 
 fn read_config<P: Into<PathBuf>>(config_path: P) -> Result<Config, BotError> {
     let config_path = config_path.into();
@@ -41,7 +42,7 @@ async fn run() -> Result<(), BotError> {
         .next()
         .expect("Need a config as an argument");
 
-    let cfg = read_config(config_path)?;
+    let cfg = Arc::new(read_config(config_path)?);
     let bot_state = DiceBotState::new(&cfg).start();
 
     match DiceBot::new(&cfg, bot_state) {

src/bot.rs

@@ -18,6 +18,7 @@ use matrix_sdk_common_macros::async_trait;
 use std::clone::Clone;
 use std::ops::Sub;
 use std::path::PathBuf;
+use std::sync::Arc;
 use std::time::{Duration, SystemTime};
 use url::Url;
 
@@ -25,7 +26,7 @@ use url::Url;
 /// connected to Matrix until its run() function is called.
 pub struct DiceBot {
     /// A reference to the configuration read in on application start.
-    config: Config,
+    config: Arc<Config>,
 
     /// The matrix client.
     client: Client,
@@ -46,7 +47,7 @@ fn create_client(config: &Config) -> Result<Client, BotError> {
     let cache_dir = cache_dir()?;
     let store = JsonStore::open(&cache_dir)?;
     let client_config = ClientConfig::new().state_store(Box::new(store));
-    let homeserver_url = Url::parse(&config.matrix.home_server)?;
+    let homeserver_url = Url::parse(&config.matrix_homeserver())?;
 
     Ok(Client::new_with_config(homeserver_url, client_config)?)
 }
@@ -56,7 +57,7 @@ impl DiceBot {
     /// actor. This function returns a Result because it is possible
     /// for client creation to fail for some reason (e.g. invalid
     /// homeserver URL).
-    pub fn new(config: &Config, state_actor: Addr<DiceBotState>) -> Result<Self, BotError> {
+    pub fn new(config: &Arc<Config>, state_actor: Addr<DiceBotState>) -> Result<Self, BotError> {
         Ok(DiceBot {
             client: create_client(&config)?,
             config: config.clone(),
@@ -68,8 +69,8 @@ impl DiceBot {
     /// terminated, or a panic occurs. Originally adapted from the
     /// matrix-rust-sdk command bot example.
     pub async fn run(self) -> Result<(), BotError> {
-        let username = &self.config.matrix.username;
+        let username = &self.config.matrix_username();
-        let password = &self.config.matrix.password;
+        let password = &self.config.matrix_password();
 
         //TODO provide a device id from config.
         let mut client = self.client.clone();
@@ -82,7 +83,6 @@ impl DiceBot {
         //If the local json store has not been created yet, we need to do a single initial sync.
         //It stores data under username's localpart.
         let should_sync = {
-            let username = &self.config.matrix.username;
             let mut cache = cache_dir()?;
             cache.push(username);
             !cache.exists()
@@ -135,6 +135,45 @@ fn check_message_age(
     }
 }
 
+async fn should_process(
+    bot: &DiceBot,
+    event: &SyncMessageEvent<MessageEventContent>,
+) -> Result<(String, String), BotError> {
+    //Ignore messages that are older than configured duration.
+    if !check_message_age(event, bot.config.oldest_message_age()) {
+        let res = bot.state.send(LogSkippedOldMessages).await;
+
+        if let Err(e) = res {
+            error!("Actix error: {:?}", e);
+        };
+
+        return Err(BotError::ShouldNotProcessError);
+    }
+
+    let (msg_body, sender_username) = if let SyncMessageEvent {
+        content: MessageEventContent::Text(TextMessageEventContent { body, .. }),
+        sender,
+        ..
+    } = event
+    {
+        (
+            body.clone(),
+            format!("@{}:{}", sender.localpart(), sender.server_name()),
+        )
+    } else {
+        (String::new(), String::new())
+    };
+
+    //Command parser can handle non-commands, but faster to
+    //not parse them.
+    if !msg_body.starts_with("!") {
+        trace!("Ignoring non-command: {}", msg_body);
+        return Err(BotError::ShouldNotProcessError);
+    }
+
+    Ok((msg_body, sender_username))
+}
+
 /// This event emitter listens for messages with dice rolling commands.
 /// Originally adapted from the matrix-rust-sdk examples.
 #[async_trait]
@@ -155,48 +194,20 @@ impl EventEmitter for DiceBot {
             let room = room.read().await;
             info!("Autojoining room {}", room.display_name());
 
-            match self.client.join_room_by_id(&room.room_id).await {
+            if let Err(e) = self.client.join_room_by_id(&room.room_id).await {
-                Err(e) => warn!("Could not join room: {}", e.to_string()),
+                warn!("Could not join room: {}", e.to_string())
-                _ => (),
             }
         }
     }
 
     async fn on_room_message(&self, room: SyncRoom, event: &SyncMessageEvent<MessageEventContent>) {
         if let SyncRoom::Joined(room) = room {
-            let (msg_body, sender_username) = if let SyncMessageEvent {
+            let (msg_body, sender_username) =
-                content: MessageEventContent::Text(TextMessageEventContent { body, .. }),
+                if let Ok((msg_body, sender_username)) = should_process(self, &event).await {
-                sender,
+                    (msg_body, sender_username)
-                ..
+                } else {
-            } = event
+                    return;
-            {
+                };
-                (
-                    body.clone(),
-                    format!("@{}:{}", sender.localpart(), sender.server_name()),
-                )
-            } else {
-                (String::new(), String::new())
-            };
-
-            //Command parser can handle non-commands, but faster to
-            //not parse them.
-            if !msg_body.starts_with("!") {
-                trace!("Ignoring non-command: {}", msg_body);
-                return;
-            }
-
-            //Ignore messages that are older than configured duration.
-            if !check_message_age(event, self.config.get_oldest_message_age()) {
-                let res = self.state.send(LogSkippedOldMessages).await;
-
-                match res {
-                    Ok(_) => return,
-                    Err(e) => {
-                        error!("Actix error: {:?}", e);
-                        return;
-                    }
-                }
-            }
 
             let (plain, html) = match parse_command(&msg_body) {
                 Ok(Some(command)) => {
@@ -217,16 +228,18 @@ impl EventEmitter for DiceBot {
                 NoticeMessageEventContent::html(plain, html),
             ));
 
-            info!("{} executed: {}", sender_username, msg_body);
-
             //we clone here to hold the lock for as little time as possible.
-            let room_id = room.read().await.room_id.clone();
+            let (room_name, room_id) = {
-            let result = self.client.room_send(&room_id, content, None).await;
+                let real_room = room.read().await;
+                (real_room.display_name().clone(), real_room.room_id.clone())
+            };
 
-            match result {
+            let result = self.client.room_send(&room_id, content, None).await;
-                Err(e) => error!("Error sending message: {}", e.to_string()),
+            if let Err(e) = result {
-                Ok(_) => (),
+                error!("Error sending message: {}", e.to_string());
-            }
+            };
+
+            info!("[{}] {} executed: {}", room_name, sender_username, msg_body);
         }
     }
 }

src/config.rs

@@ -2,37 +2,31 @@ use serde::{self, Deserialize, Serialize};
 
 /// The "matrix" section of the config, which gives home server, login information, and etc.
 #[derive(Serialize, Deserialize, Clone, Debug)]
-pub struct MatrixConfig {
+struct MatrixConfig {
     /// Your homeserver of choice, as an FQDN without scheme or path
-    pub home_server: String,
+    home_server: String,
 
     /// Username to login as. Only the localpart.
-    pub username: String,
+    username: String,
 
     /// Bot account password.
-    pub password: String,
+    password: String,
 }
 
 const DEFAULT_OLDEST_MESSAGE_AGE: u64 = 15 * 60;
 
 /// The "bot" section of the config file, for bot settings.
 #[derive(Serialize, Deserialize, Clone, Debug)]
-pub struct BotConfig {
+struct BotConfig {
     /// How far back from current time should we process a message?
     oldest_message_age: Option<u64>,
 }
 
 impl BotConfig {
-    pub fn new() -> BotConfig {
-        BotConfig {
-            oldest_message_age: Some(DEFAULT_OLDEST_MESSAGE_AGE),
-        }
-    }
-
     /// Determine the oldest allowable message age, in seconds. If the
     /// setting is defined, use that value. If it is not defined, fall
     /// back to DEFAULT_OLDEST_MESSAGE_AGE (15 minutes).
-    pub fn oldest_message_age(&self) -> u64 {
+    fn oldest_message_age(&self) -> u64 {
         match self.oldest_message_age {
             Some(seconds) => seconds,
             None => DEFAULT_OLDEST_MESSAGE_AGE,
@@ -40,25 +34,29 @@ impl BotConfig {
     }
 }
 
-/// Represents the toml config file for the dicebot.
+/// Represents the toml config file for the dicebot. The sections of
+/// the config are not directly accessible; instead the config
+/// provides friendly methods that handle default values, etc.
 #[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct Config {
-    pub matrix: MatrixConfig,
+    matrix: MatrixConfig,
-    pub bot: Option<BotConfig>,
+    bot: Option<BotConfig>,
 }
 
 impl Config {
-    pub fn bot(self) -> BotConfig {
+    /// The matrix homeserver URL.
-        let none_cfg;
+    pub fn matrix_homeserver(&self) -> &str {
-        let bot_cfg = match self.bot {
+        &self.matrix.home_server
-            Some(cfg) => cfg,
+    }
-            None => {
-                none_cfg = BotConfig::new();
-                none_cfg
-            }
-        };
 
-        bot_cfg
+    /// The username used to connect to the matrix server.
+    pub fn matrix_username(&self) -> &str {
+        &self.matrix.username
+    }
+
+    /// The password used to connect to the matrix server.
+    pub fn matrix_password(&self) -> &str {
+        &self.matrix.password
     }
 
     /// Figure out the allowed oldest message age, in seconds. This will
@@ -66,11 +64,11 @@ impl Config {
     /// configuration and associated "oldest_message_age" setting are
     /// defined. If the bot config or the message setting are not defined,
     /// it will defualt to 15 minutes.
-    pub fn get_oldest_message_age(&self) -> u64 {
+    pub fn oldest_message_age(&self) -> u64 {
         self.bot
             .as_ref()
-            .unwrap_or(&BotConfig::new())
+            .map(|bc| bc.oldest_message_age())
-            .oldest_message_age()
+            .unwrap_or(DEFAULT_OLDEST_MESSAGE_AGE)
     }
 }
 
@@ -91,7 +89,7 @@ mod tests {
             }),
         };
 
-        assert_eq!(15 * 60, cfg.get_oldest_message_age());
+        assert_eq!(15 * 60, cfg.oldest_message_age());
     }
 
     #[test]
@@ -105,6 +103,6 @@ mod tests {
             bot: None,
         };
 
-        assert_eq!(15 * 60, cfg.get_oldest_message_age());
+        assert_eq!(15 * 60, cfg.oldest_message_age());
     }
 }

src/error.rs

@@ -6,6 +6,9 @@ pub enum BotError {
     #[error("the sync token could not be retrieved")]
     SyncTokenRequired,
 
+    #[error("the message should not be processed because it failed validation")]
+    ShouldNotProcessError,
+
     #[error("no cache directory found")]
     NoCacheDirectoryError,
 

src/state.rs

@@ -1,6 +1,7 @@
 use crate::config::*;
 use actix::prelude::*;
 use log::info;
+use std::sync::Arc;
 
 #[derive(Message)]
 #[rtype(result = "bool")]
@@ -12,7 +13,7 @@ pub struct LogSkippedOldMessages;
 /// change state.
 pub struct DiceBotState {
     logged_skipped_old_messages: bool,
-    config: Config,
+    config: Arc<Config>,
 }
 
 impl Actor for DiceBotState {
@@ -21,14 +22,14 @@ impl Actor for DiceBotState {
     fn started(&mut self, _ctx: &mut Self::Context) {
         info!(
             "Oldest allowable message time is {} seconds ago",
-            &self.config.get_oldest_message_age()
+            &self.config.oldest_message_age()
         );
     }
 }
 
 impl DiceBotState {
     /// Create initial dice bot state.
-    pub fn new(config: &Config) -> DiceBotState {
+    pub fn new(config: &Arc<Config>) -> DiceBotState {
         DiceBotState {
             logged_skipped_old_messages: false,
             config: config.clone(),