diff --git a/src/app/actor.rs b/src/app/actor.rs index 35be4a7..435b660 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -201,7 +201,7 @@ async fn on_input( handle_patchset_details(app, input, terminal_handle).await?; } CurrentScreen::EditConfig => { - handle_edit_config(app, input)?; + handle_edit_config(app, input).await?; } CurrentScreen::LatestPatchsets => { handle_latest_patchsets(app, input, loading).await?; @@ -227,7 +227,7 @@ mod tests { state::{AppState, ConfigUiState, LoreUiState, NavigationState, UserLoreState}, AppServices, }, - config::ConfigState, + config::{ConfigHandle, ConfigState}, infrastructure::{ env::MockEnvTrait, file_system::MockFileSystemTrait, shell::MockShellTrait, }, @@ -253,26 +253,9 @@ mod tests { use super::*; - struct NullConfigService; - - impl crate::config::ConfigServiceApi for NullConfigService { - fn snapshot(&self) -> crate::config::ConfigSnapshot { - ConfigState::default().to_snapshot() - } - - fn validate_update( - &self, - _: crate::config::ConfigUpdateDraft, - ) -> Result { - unimplemented!() - } - - fn apply_update( - &mut self, - _: crate::config::ValidatedConfigUpdate, - ) -> Result { - unimplemented!() - } + fn dummy_config_handle() -> ConfigHandle { + let (config_tx, _config_rx) = mpsc::channel(1); + ConfigHandle::new(config_tx) } fn minimal_app_with_env(env: MockEnvTrait) -> App { @@ -313,7 +296,7 @@ mod tests { shell: Box::new(MockShellTrait::new()), fs: Box::new(MockFileSystemTrait::new()), env: Box::new(env), - config: Box::new(NullConfigService), + config: dummy_config_handle(), }, } } @@ -417,7 +400,8 @@ mod tests { let ui_handle = UiActor::spawn(); let app = App::new( - Box::new(NullConfigService), + ConfigState::default().to_snapshot(), + dummy_config_handle(), bootstrap, Box::new(MockFileSystemTrait::new()), Box::new(MockShellTrait::new()), diff --git a/src/app/flows/edit_config.rs b/src/app/flows/edit_config.rs index 0895486..44b2157 100644 --- a/src/app/flows/edit_config.rs +++ b/src/app/flows/edit_config.rs @@ -5,50 +5,68 @@ use crate::{ input::event::InputEvent, }; -pub fn handle_edit_config(app: &mut App, input: InputEvent) -> color_eyre::Result<()> { - if let Some(edit_config_state) = app.state.config_state.edit_config.as_mut() { - match edit_config_state.is_editing() { - true => match input { - InputEvent::CancelConfigEdit => { - edit_config_state.clear_edit(); - edit_config_state.toggle_editing(); - } - InputEvent::Backspace => { - edit_config_state.backspace_edit(); - } - InputEvent::TextInput(ch) => { - edit_config_state.append_edit(ch); - } - InputEvent::StageConfigEdit => { - edit_config_state.stage_edit(); - edit_config_state.clear_edit(); - edit_config_state.toggle_editing(); - } - _ => {} - }, - false => match input { - InputEvent::OpenHelp => { - let popup = generate_help_popup(); - app.state.popup = Some(popup); - } - InputEvent::SaveConfig => { - debug!("saving edited configuration"); - app.consolidate_edit_config()?; - app.reset_edit_config(); - app.set_current_screen(CurrentScreen::MailingListSelection); +pub async fn handle_edit_config(app: &mut App, input: InputEvent) -> color_eyre::Result<()> { + let Some(is_editing) = app + .state + .config_state + .edit_config + .as_ref() + .map(|edit_config_state| edit_config_state.is_editing()) + else { + return Ok(()); + }; + + match is_editing { + true => { + if let Some(edit_config_state) = app.state.config_state.edit_config.as_mut() { + match input { + InputEvent::CancelConfigEdit => { + edit_config_state.clear_edit(); + edit_config_state.toggle_editing(); + } + InputEvent::Backspace => { + edit_config_state.backspace_edit(); + } + InputEvent::TextInput(ch) => { + edit_config_state.append_edit(ch); + } + InputEvent::StageConfigEdit => { + edit_config_state.stage_edit(); + edit_config_state.clear_edit(); + edit_config_state.toggle_editing(); + } + _ => {} } - InputEvent::EditConfigField => { + } + } + false => match input { + InputEvent::OpenHelp => { + let popup = generate_help_popup(); + app.state.popup = Some(popup); + } + InputEvent::SaveConfig => { + debug!("saving edited configuration"); + app.consolidate_edit_config().await?; + app.reset_edit_config(); + app.set_current_screen(CurrentScreen::MailingListSelection); + } + InputEvent::EditConfigField => { + if let Some(edit_config_state) = app.state.config_state.edit_config.as_mut() { edit_config_state.toggle_editing(); } - InputEvent::NavigateDown => { + } + InputEvent::NavigateDown => { + if let Some(edit_config_state) = app.state.config_state.edit_config.as_mut() { edit_config_state.highlight_next(); } - InputEvent::NavigateUp => { + } + InputEvent::NavigateUp => { + if let Some(edit_config_state) = app.state.config_state.edit_config.as_mut() { edit_config_state.highlight_prev(); } - _ => {} - }, - } + } + _ => {} + }, } Ok(()) } diff --git a/src/app/mod.rs b/src/app/mod.rs index d9f5599..0e87528 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -27,7 +27,7 @@ use tracing::{debug, event, info, warn, Level}; use std::path::PathBuf; use crate::{ - config::ConfigServiceApi, + config::{ConfigHandle, ConfigSnapshot}, infrastructure::{ env::EnvTrait, file_system::FileSystemTrait, @@ -62,7 +62,7 @@ pub struct AppServices { pub shell: Box, pub fs: Box, pub env: Box, - pub config: Box, + pub config: ConfigHandle, } /// Result type signalling whether a patchset was successfully loaded. @@ -78,14 +78,17 @@ pub struct App { } impl App { - /// Creates a new instance of `App`. Configuration comes from [`ConfigServiceApi::snapshot`]; - /// Lore bootstrap uses already-warmed cache from `lore_service`. + /// Creates a new instance of `App`. + /// + /// Configuration starts from the already-bootstrapped snapshot owned by the + /// Config actor. Lore bootstrap uses already-warmed cache from `lore_service`. /// /// # Returns /// /// `App` instance with loading configurations and app data. pub fn new( - config_service: Box, + config: ConfigSnapshot, + config_handle: ConfigHandle, bootstrap: BootstrapLoreData, fs: Box, shell: Box, @@ -93,8 +96,6 @@ impl App { lore_api: LoreApiHandle, render: RenderHandle, ) -> color_eyre::Result { - let config = config_service.snapshot(); - event!(Level::INFO, "patch-hub started"); collect_garbage(&config); @@ -130,7 +131,7 @@ impl App { shell, fs, env, - config: config_service, + config: config_handle, }, }) } @@ -489,12 +490,16 @@ impl App { } /// Applies edited values from [`ConfigUiState::edit_config`] into [`AppState::config`]. - pub fn consolidate_edit_config(&mut self) -> color_eyre::Result<()> { + pub async fn consolidate_edit_config(&mut self) -> color_eyre::Result<()> { if let Some(edit_config) = &self.state.config_state.edit_config { debug!("validating and applying config update"); let draft = edit_config.to_update_draft(); - let validated = self.services.config.validate_update(draft)?; - let snapshot = self.services.config.apply_update(validated)?; + let snapshot = self + .services + .config + .validate_and_apply(draft) + .await + .map_err(|e| eyre!("{e:#?}"))?; self.state.config = snapshot; info!("configuration updated and persisted"); } diff --git a/src/app/screens/edit_config.rs b/src/app/screens/edit_config.rs index c56d5cd..a629cb7 100644 --- a/src/app/screens/edit_config.rs +++ b/src/app/screens/edit_config.rs @@ -113,7 +113,7 @@ impl EditConfigState { } } - /// Raw form values for [`crate::config::ConfigServiceApi::validate_update`]. + /// Raw form values for config validation. pub fn to_update_draft(&self) -> ConfigUpdateDraft { ConfigUpdateDraft { page_size: self.config_buffer.get(&EditableConfig::PageSize).cloned(), diff --git a/src/config/actor.rs b/src/config/actor.rs new file mode 100644 index 0000000..a7f56b7 --- /dev/null +++ b/src/config/actor.rs @@ -0,0 +1,257 @@ +//! Configuration actor: serializes access to mutable config state. +//! +//! All config reads and edits go through [`ConfigHandle`](crate::config::ConfigHandle) +//! as typed request/reply messages. The actor owns [`ConfigState`](crate::config::ConfigState) +//! and persists successful updates through [`JsonConfigRepository`](crate::config::JsonConfigRepository). +use std::ops::ControlFlow; + +use tokio::sync::{mpsc, oneshot}; + +use crate::{ + config::{ + handle::ConfigHandle, + messages::{ConfigMessage, ConfigResult}, + repository::{ConfigRepository, JsonConfigRepository}, + service::{ensure_directories, validate_update}, + state::{normalize_derived_paths, ConfigState}, + ConfigSnapshot, ConfigUpdateDraft, + }, + infrastructure::file_system::FileSystemTrait, +}; + +pub const DEFAULT_CONFIG_CHANNEL_SIZE: usize = 16; + +pub struct ConfigActor { + state: ConfigState, + repo: JsonConfigRepository, + rx: mpsc::Receiver, +} + +impl ConfigActor +where + FS: FileSystemTrait + Send + Sync + 'static, +{ + pub fn new( + state: ConfigState, + repo: JsonConfigRepository, + rx: mpsc::Receiver, + ) -> Self { + Self { state, repo, rx } + } + + pub fn spawn(state: ConfigState, repo: JsonConfigRepository) -> ConfigHandle { + let (tx, rx) = mpsc::channel(DEFAULT_CONFIG_CHANNEL_SIZE); + tracing::debug!( + channel_size = DEFAULT_CONFIG_CHANNEL_SIZE, + "spawning config actor" + ); + tokio::spawn(Self::new(state, repo, rx).run()); + ConfigHandle::new(tx) + } + + pub async fn run(mut self) { + tracing::info!("config actor started"); + while let Some(message) = self.rx.recv().await { + if let ControlFlow::Break(()) = self.handle_message(message) { + break; + } + } + tracing::info!("config actor stopped"); + } + + fn handle_message(&mut self, message: ConfigMessage) -> ControlFlow<()> { + let message_name = message.name(); + tracing::debug!(message = message_name, "config request received"); + + match message { + ConfigMessage::GetSnapshot { reply } => { + send_config_snapshot_reply(message_name, reply, self.state.to_snapshot()); + ControlFlow::Continue(()) + } + ConfigMessage::ValidateAndApply { draft, reply } => { + let result = self.apply(draft); + send_config_reply(message_name, reply, result); + ControlFlow::Continue(()) + } + ConfigMessage::Shutdown => { + tracing::debug!("config actor shutting down"); + ControlFlow::Break(()) + } + } + } + + fn apply(&mut self, draft: ConfigUpdateDraft) -> ConfigResult { + let update = validate_update(draft, self.repo.fs())?; + self.state.apply_update(&update); + normalize_derived_paths(&mut self.state); + ensure_directories(&self.state, self.repo.fs())?; + self.repo.save(&self.state)?; + Ok(self.state.to_snapshot()) + } +} + +fn send_config_snapshot_reply( + message_name: &'static str, + reply: oneshot::Sender, + snapshot: ConfigSnapshot, +) { + if reply.send(snapshot).is_err() { + tracing::warn!( + message = message_name, + "config reply receiver dropped before response" + ); + } +} + +fn send_config_reply( + message_name: &'static str, + reply: oneshot::Sender>, + result: ConfigResult, +) { + if let Err(error) = &result { + tracing::warn!( + message = message_name, + error = %error, + "config request failed" + ); + } + + if reply.send(result).is_err() { + tracing::warn!( + message = message_name, + "config reply receiver dropped before response" + ); + } +} + +#[cfg(test)] +mod tests { + use std::{ + env::VarError, + fs, + path::PathBuf, + sync::atomic::{AtomicU64, Ordering}, + }; + + use crate::{ + config::{ + service::bootstrap_parts, ConfigError, ConfigUpdateDraft, DEFAULT_CONFIG_PATH_SUFFIX, + }, + infrastructure::{env::MockEnvTrait, file_system::OsFileSystem}, + }; + + use super::*; + + static TEST_SEQ: AtomicU64 = AtomicU64::new(0); + + fn unique_test_dir(prefix: &str) -> PathBuf { + let n = TEST_SEQ.fetch_add(1, Ordering::SeqCst); + let p = std::env::temp_dir().join(format!("patch-hub-{prefix}-{}-{n}", std::process::id())); + fs::create_dir_all(&p).unwrap(); + p + } + + fn default_env() -> (MockEnvTrait, PathBuf) { + let home = unique_test_dir("actor-home"); + let home_s = home.to_string_lossy().into_owned(); + let mut mock = MockEnvTrait::new(); + mock.expect_var() + .withf(|key| key == "PATCH_HUB_CONFIG_PATH") + .returning(|_| Err(VarError::NotPresent.into())); + mock.expect_var() + .withf(move |key| key == "HOME") + .returning(move |_| Ok(home_s.clone())); + mock.expect_var() + .withf(|key| { + matches!( + key, + "PATCH_HUB_PAGE_SIZE" + | "PATCH_HUB_CACHE_DIR" + | "PATCH_HUB_DATA_DIR" + | "PATCH_HUB_GIT_SEND_EMAIL_OPTIONS" + | "PATCH_HUB_PATCH_RENDERER" + ) + }) + .returning(|_| Err(VarError::NotPresent.into())); + (mock, home) + } + + fn spawn_test_actor() -> (ConfigHandle, PathBuf) { + let (env, home) = default_env(); + let (state, repo) = bootstrap_parts(&env, OsFileSystem).unwrap(); + (ConfigActor::spawn(state, repo), home) + } + + #[tokio::test] + async fn get_snapshot_returns_actor_state() { + let (handle, _home) = spawn_test_actor(); + + let snapshot = handle.get_snapshot().await.unwrap(); + + assert_eq!(30, snapshot.page_size()); + handle.shutdown().await; + } + + #[tokio::test] + async fn validate_and_apply_returns_updated_snapshot() { + let (handle, _home) = spawn_test_actor(); + + let snapshot = handle + .validate_and_apply(ConfigUpdateDraft { + page_size: Some("77".into()), + ..Default::default() + }) + .await + .unwrap(); + + assert_eq!(77, snapshot.page_size()); + assert_eq!(77, handle.get_snapshot().await.unwrap().page_size()); + handle.shutdown().await; + } + + #[tokio::test] + async fn validate_and_apply_persists_update() { + let (handle, home) = spawn_test_actor(); + let cfg_path = home.join(DEFAULT_CONFIG_PATH_SUFFIX); + + handle + .validate_and_apply(ConfigUpdateDraft { + page_size: Some("88".into()), + ..Default::default() + }) + .await + .unwrap(); + + let raw = fs::read_to_string(&cfg_path).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert_eq!(parsed["page_size"], 88); + handle.shutdown().await; + } + + #[tokio::test] + async fn invalid_update_keeps_existing_state() { + let (handle, _home) = spawn_test_actor(); + + let err = handle + .validate_and_apply(ConfigUpdateDraft { + page_size: Some("not-a-number".into()), + ..Default::default() + }) + .await + .unwrap_err(); + + assert!(matches!(err, ConfigError::InvalidPageSize(ref s) if s == "not-a-number")); + assert_eq!(30, handle.get_snapshot().await.unwrap().page_size()); + handle.shutdown().await; + } + + #[tokio::test] + async fn shutdown_stops_actor() { + let (handle, _home) = spawn_test_actor(); + + handle.shutdown().await; + let err = handle.get_snapshot().await.unwrap_err(); + + assert!(matches!(err, ConfigError::ActorUnavailable(_))); + } +} diff --git a/src/config/errors.rs b/src/config/errors.rs index 72d49af..42478c1 100644 --- a/src/config/errors.rs +++ b/src/config/errors.rs @@ -4,6 +4,8 @@ use crate::infrastructure::file_system::FileSystemError; #[derive(Debug, Error)] pub enum ConfigError { + #[error("config actor unavailable: {0}")] + ActorUnavailable(String), #[error("failed to save config: {0}")] Save(String), #[error("invalid page size: {0}")] diff --git a/src/config/handle.rs b/src/config/handle.rs new file mode 100644 index 0000000..6abfe5e --- /dev/null +++ b/src/config/handle.rs @@ -0,0 +1,65 @@ +use tokio::sync::{mpsc, oneshot}; + +use crate::config::{ + messages::{ConfigMessage, ConfigResult}, + ConfigError, ConfigSnapshot, ConfigUpdateDraft, +}; + +#[derive(Clone)] +pub struct ConfigHandle { + tx: mpsc::Sender, +} + +impl ConfigHandle { + pub fn new(tx: mpsc::Sender) -> Self { + Self { tx } + } + + pub async fn get_snapshot(&self) -> ConfigResult { + self.request(|reply| ConfigMessage::GetSnapshot { reply }) + .await + } + + pub async fn validate_and_apply( + &self, + draft: ConfigUpdateDraft, + ) -> ConfigResult { + self.request_result(|reply| ConfigMessage::ValidateAndApply { draft, reply }) + .await + } + + /// Signals the actor to stop processing messages and exit its run loop. + /// + /// Callers should invoke this after the last request that uses this handle has + /// completed. Dropping all clones of the handle also stops the actor, but + /// calling `shutdown` makes the intent explicit and allows ordered teardown. + pub async fn shutdown(&self) { + self.tx.send(ConfigMessage::Shutdown).await.ok(); + } + + async fn request_result( + &self, + build_message: impl FnOnce(oneshot::Sender>) -> ConfigMessage, + ) -> ConfigResult + where + T: Send + 'static, + { + self.request(build_message).await? + } + + async fn request( + &self, + build_message: impl FnOnce(oneshot::Sender) -> ConfigMessage, + ) -> ConfigResult + where + T: Send + 'static, + { + let (reply, rx) = oneshot::channel(); + self.tx + .send(build_message(reply)) + .await + .map_err(|_| ConfigError::ActorUnavailable("request channel closed".to_string()))?; + rx.await + .map_err(|_| ConfigError::ActorUnavailable("reply channel closed".to_string())) + } +} diff --git a/src/config/messages.rs b/src/config/messages.rs new file mode 100644 index 0000000..94bee87 --- /dev/null +++ b/src/config/messages.rs @@ -0,0 +1,26 @@ +use tokio::sync::oneshot; + +use crate::config::{ConfigError, ConfigSnapshot, ConfigUpdateDraft}; + +pub type ConfigResult = Result; + +pub enum ConfigMessage { + GetSnapshot { + reply: oneshot::Sender, + }, + ValidateAndApply { + draft: ConfigUpdateDraft, + reply: oneshot::Sender>, + }, + Shutdown, +} + +impl ConfigMessage { + pub fn name(&self) -> &'static str { + match self { + ConfigMessage::GetSnapshot { .. } => "GetSnapshot", + ConfigMessage::ValidateAndApply { .. } => "ValidateAndApply", + ConfigMessage::Shutdown => "Shutdown", + } + } +} diff --git a/src/config/mod.rs b/src/config/mod.rs index 13ad3a6..f70e22a 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,17 +1,22 @@ //! Configuration bounded context: state, persistence, bootstrap, and snapshots. #![allow(unused_imports)] // `pub use` re-exports are the public API of this module +pub mod actor; mod env_overrides; mod errors; +pub mod handle; +pub mod messages; mod parsing; mod repository; mod service; mod state; mod update; +pub use actor::ConfigActor; pub use errors::ConfigError; +pub use handle::ConfigHandle; pub use repository::{resolve_config_path, ConfigRepository, JsonConfigRepository}; -pub use service::{ConfigService, ConfigServiceApi}; +pub(crate) use service::bootstrap_parts; pub use state::{normalize_derived_paths, ConfigSnapshot, ConfigState, KernelTree}; pub use update::{ConfigUpdateDraft, ValidatedConfigUpdate}; diff --git a/src/config/service.rs b/src/config/service.rs index 200470a..5a77420 100644 --- a/src/config/service.rs +++ b/src/config/service.rs @@ -4,186 +4,151 @@ use crate::config::env_overrides; use crate::config::errors::ConfigError; use crate::config::parsing::{parse_cover_renderer, parse_patch_renderer}; use crate::config::repository::{ConfigRepository, JsonConfigRepository}; -use crate::config::state::ConfigSnapshot; use crate::config::state::{normalize_derived_paths, ConfigState}; use crate::config::update::{ConfigUpdateDraft, ValidatedConfigUpdate}; use crate::infrastructure::{env::EnvTrait, file_system::FileSystemTrait}; -/// Public surface for configuration. -pub trait ConfigServiceApi: Send + Sync { - fn snapshot(&self) -> ConfigSnapshot; - fn validate_update( - &self, - draft: ConfigUpdateDraft, - ) -> Result; - fn apply_update( - &mut self, - update: ValidatedConfigUpdate, - ) -> Result; -} - -pub struct ConfigService { - repo: JsonConfigRepository, - state: ConfigState, -} - -impl ConfigService { - /// Bootstrap configuration: load file or defaults, persist, apply env overrides, ensure dirs. - /// - /// Loads file or defaults, saves, applies env overrides, ensures directories exist. - pub fn bootstrap(env: &dyn EnvTrait, fs: FS) -> Result { - let repo = JsonConfigRepository::new(env, fs); - let mut state = Self::load_initial_state(env, &repo); - if let Err(e) = repo.save(&state) { - eprintln!("Failed to save default config: {e}"); - } - env_overrides::apply_env_overrides(&mut state, env)?; - normalize_derived_paths(&mut state); - let service = Self { repo, state }; - service.ensure_directories()?; - Ok(service) - } - - fn load_initial_state(env: &dyn EnvTrait, repo: &JsonConfigRepository) -> ConfigState { - let path = repo.config_path(); - let fs = repo.fs(); - if fs.is_file(Path::new(path)) { - match fs.read_to_string(Path::new(path)) { - Ok(file_contents) => match serde_json::from_str(&file_contents) { - Ok(config) => return config, - Err(e) => eprintln!("Failed to parse config file {path}: {e}"), - }, - Err(e) => { - eprintln!("Failed to read config file {path}: {e}"); - } - } - } - ConfigState::new_with_defaults(env) +/// Loads file or defaults, saves, applies env overrides, normalizes paths, and ensures directories. +pub(crate) fn bootstrap_parts( + env: &dyn EnvTrait, + fs: FS, +) -> Result<(ConfigState, JsonConfigRepository), ConfigError> { + let repo = JsonConfigRepository::new(env, fs); + let mut state = load_initial_state(env, &repo); + if let Err(e) = repo.save(&state) { + eprintln!("Failed to save default config: {e}"); } + env_overrides::apply_env_overrides(&mut state, env)?; + normalize_derived_paths(&mut state); + ensure_directories(&state, repo.fs())?; + Ok((state, repo)) +} - fn ensure_directories(&self) -> Result<(), ConfigError> { - let fs = self.repo.fs(); - let paths = [ - self.state.cache_dir.as_str(), - self.state.data_dir.as_str(), - self.state.patchsets_cache_dir.as_str(), - self.state.logs_path.as_str(), - ]; - for path in paths { - if fs.metadata(Path::new(path)).is_err() { - fs.create_dir_all(Path::new(path))?; +fn load_initial_state( + env: &dyn EnvTrait, + repo: &JsonConfigRepository, +) -> ConfigState { + let path = repo.config_path(); + let fs = repo.fs(); + if fs.is_file(Path::new(path)) { + match fs.read_to_string(Path::new(path)) { + Ok(file_contents) => match serde_json::from_str(&file_contents) { + Ok(config) => return config, + Err(e) => eprintln!("Failed to parse config file {path}: {e}"), + }, + Err(e) => { + eprintln!("Failed to read config file {path}: {e}"); } } - Ok(()) } + ConfigState::new_with_defaults(env) +} - fn validate_dir(fs: &dyn FileSystemTrait, dir_path: &str) -> Result<(), ConfigError> { - let path = Path::new(dir_path); - if fs.exists(path) && fs.is_dir(path) { - return Ok(()); +pub(crate) fn ensure_directories( + state: &ConfigState, + fs: &dyn FileSystemTrait, +) -> Result<(), ConfigError> { + let paths = [ + state.cache_dir.as_str(), + state.data_dir.as_str(), + state.patchsets_cache_dir.as_str(), + state.logs_path.as_str(), + ]; + for path in paths { + if fs.metadata(Path::new(path)).is_err() { + fs.create_dir_all(Path::new(path))?; } - fs.create_dir_all(path) - .map_err(|_| ConfigError::InvalidDirectory(dir_path.to_string()))?; - Ok(()) } + Ok(()) } -impl ConfigServiceApi for ConfigService { - fn snapshot(&self) -> ConfigSnapshot { - self.state.to_snapshot() - } - - fn validate_update( - &self, - draft: ConfigUpdateDraft, - ) -> Result { - let fs = self.repo.fs(); - - let page_size = match &draft.page_size { - None => None, - Some(s) if s.trim().is_empty() => { - return Err(ConfigError::InvalidPageSize(s.clone())); - } - Some(s) => Some( - s.trim() - .parse::() - .map_err(|_| ConfigError::InvalidPageSize(s.clone()))?, - ), - }; - - let cache_dir = match &draft.cache_dir { - None => None, - Some(s) if s.trim().is_empty() => { - return Err(ConfigError::InvalidDirectory( - "cache directory is empty".into(), - )); - } - Some(s) => { - let t = s.trim(); - Self::validate_dir(fs, t)?; - Some(t.to_string()) - } - }; - - let data_dir = match &draft.data_dir { - None => None, - Some(s) if s.trim().is_empty() => { - return Err(ConfigError::InvalidDirectory( - "data directory is empty".into(), - )); - } - Some(s) => { - let t = s.trim(); - Self::validate_dir(fs, t)?; - Some(t.to_string()) - } - }; - - let git_send_email_option = draft.git_send_email_option.clone(); - let git_am_option = draft.git_am_option.clone(); +pub(crate) fn validate_update( + draft: ConfigUpdateDraft, + fs: &dyn FileSystemTrait, +) -> Result { + let page_size = match &draft.page_size { + None => None, + Some(s) if s.trim().is_empty() => { + return Err(ConfigError::InvalidPageSize(s.clone())); + } + Some(s) => Some( + s.trim() + .parse::() + .map_err(|_| ConfigError::InvalidPageSize(s.clone()))?, + ), + }; + + let cache_dir = match &draft.cache_dir { + None => None, + Some(s) if s.trim().is_empty() => { + return Err(ConfigError::InvalidDirectory( + "cache directory is empty".into(), + )); + } + Some(s) => { + let t = s.trim(); + validate_dir(fs, t)?; + Some(t.to_string()) + } + }; + + let data_dir = match &draft.data_dir { + None => None, + Some(s) if s.trim().is_empty() => { + return Err(ConfigError::InvalidDirectory( + "data directory is empty".into(), + )); + } + Some(s) => { + let t = s.trim(); + validate_dir(fs, t)?; + Some(t.to_string()) + } + }; - let patch_renderer = match &draft.patch_renderer { - None => None, - Some(s) => Some(parse_patch_renderer(s)?), - }; + let git_send_email_option = draft.git_send_email_option.clone(); + let git_am_option = draft.git_am_option.clone(); - let cover_renderer = match &draft.cover_renderer { - None => None, - Some(s) => Some(parse_cover_renderer(s)?), - }; + let patch_renderer = match &draft.patch_renderer { + None => None, + Some(s) => Some(parse_patch_renderer(s)?), + }; - let max_log_age = match &draft.max_log_age { - None => None, - Some(s) if s.trim().is_empty() => { - return Err(ConfigError::InvalidMaxLogAge(s.clone())); - } - Some(s) => Some( - s.trim() - .parse::() - .map_err(|_| ConfigError::InvalidMaxLogAge(s.clone()))?, - ), - }; + let cover_renderer = match &draft.cover_renderer { + None => None, + Some(s) => Some(parse_cover_renderer(s)?), + }; - Ok(ValidatedConfigUpdate { - page_size, - cache_dir, - data_dir, - git_send_email_option, - git_am_option, - patch_renderer, - cover_renderer, - max_log_age, - }) - } + let max_log_age = match &draft.max_log_age { + None => None, + Some(s) if s.trim().is_empty() => { + return Err(ConfigError::InvalidMaxLogAge(s.clone())); + } + Some(s) => Some( + s.trim() + .parse::() + .map_err(|_| ConfigError::InvalidMaxLogAge(s.clone()))?, + ), + }; + + Ok(ValidatedConfigUpdate { + page_size, + cache_dir, + data_dir, + git_send_email_option, + git_am_option, + patch_renderer, + cover_renderer, + max_log_age, + }) +} - fn apply_update( - &mut self, - update: ValidatedConfigUpdate, - ) -> Result { - self.state.apply_update(&update); - normalize_derived_paths(&mut self.state); - self.ensure_directories()?; - self.repo.save(&self.state)?; - Ok(self.state.to_snapshot()) +fn validate_dir(fs: &dyn FileSystemTrait, dir_path: &str) -> Result<(), ConfigError> { + let path = Path::new(dir_path); + if fs.exists(path) && fs.is_dir(path) { + return Ok(()); } + fs.create_dir_all(path) + .map_err(|_| ConfigError::InvalidDirectory(dir_path.to_string()))?; + Ok(()) } diff --git a/src/config/tests.rs b/src/config/tests.rs index 209f5bb..db9a593 100644 --- a/src/config/tests.rs +++ b/src/config/tests.rs @@ -7,12 +7,15 @@ use std::{ sync::atomic::{AtomicU64, Ordering}, }; +use crate::config::actor::ConfigActor; use crate::config::repository::{ConfigRepository, JsonConfigRepository}; +use crate::config::service::{bootstrap_parts, validate_update}; use crate::config::state::{normalize_derived_paths, ConfigState}; -use crate::config::{ - ConfigError, ConfigService, ConfigServiceApi, ConfigUpdateDraft, DEFAULT_CONFIG_PATH_SUFFIX, +use crate::config::{ConfigError, ConfigSnapshot, ConfigUpdateDraft, DEFAULT_CONFIG_PATH_SUFFIX}; +use crate::infrastructure::{ + env::{EnvTrait, MockEnvTrait}, + file_system::OsFileSystem, }; -use crate::infrastructure::{env::MockEnvTrait, file_system::OsFileSystem}; static TEST_SEQ: AtomicU64 = AtomicU64::new(0); @@ -27,6 +30,10 @@ fn unique_test_dir(prefix: &str) -> PathBuf { p } +fn bootstrap_snapshot(env: &dyn EnvTrait) -> ConfigSnapshot { + bootstrap_parts(env, os_fs()).unwrap().0.to_snapshot() +} + /// Writable `HOME` and mock env: no `PATCH_HUB_CONFIG_PATH` (uses `HOME/.config/...`). fn default_env() -> (MockEnvTrait, PathBuf) { let home = unique_test_dir("home"); @@ -100,8 +107,7 @@ fn config_fixture_json(root: &Path) -> String { fn bootstrap_with_default_values() { let (env, home) = default_env(); let h = home.to_string_lossy(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); - let config = service.snapshot(); + let config = bootstrap_snapshot(&env); assert_eq!(30, config.page_size()); assert_eq!( @@ -165,8 +171,7 @@ fn bootstrap_with_config_file() { }) .returning(|_| Err(VarError::NotPresent.into())); - let service = ConfigService::bootstrap(&mock, os_fs()).unwrap(); - let config = service.snapshot(); + let config = bootstrap_snapshot(&mock); // `normalize_derived_paths` recomputes these from `cache_dir` / `data_dir` after load. let patchsets_cache_dir = fixture_root.join("cache_dir").join("patchsets"); @@ -253,8 +258,7 @@ fn bootstrap_with_env_vars() { .withf(|key| key == "PATCH_HUB_PATCH_RENDERER") .returning(|_| Err(VarError::NotPresent.into())); - let service = ConfigService::bootstrap(&mock, os_fs()).unwrap(); - let config = service.snapshot(); + let config = bootstrap_snapshot(&mock); assert_eq!(42, config.page_size()); assert_eq!( @@ -286,8 +290,7 @@ fn bootstrap_with_env_vars() { #[test] fn bootstrap_config_precedence() { let (env, home) = default_env(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); - assert_eq!(30, service.snapshot().page_size()); + assert_eq!(30, bootstrap_snapshot(&env).page_size()); let fixture_root = unique_test_dir("prec"); let tmp_path = fixture_root.join("config.json"); @@ -318,8 +321,7 @@ fn bootstrap_config_precedence() { }) .returning(|_| Err(VarError::NotPresent.into())); - let service = ConfigService::bootstrap(&env_with_file, os_fs()).unwrap(); - assert_eq!(1234, service.snapshot().page_size()); + assert_eq!(1234, bootstrap_snapshot(&env_with_file).page_size()); let tmp_path_s2 = tmp_path.to_string_lossy().into_owned(); let home_s2 = home.to_string_lossy().into_owned(); @@ -349,8 +351,7 @@ fn bootstrap_config_precedence() { }) .returning(|_| Err(VarError::NotPresent.into())); - let service = ConfigService::bootstrap(&env_with_file_and_var, os_fs()).unwrap(); - assert_eq!(42, service.snapshot().page_size()); + assert_eq!(42, bootstrap_snapshot(&env_with_file_and_var).page_size()); let _ = fs::remove_file(&tmp_path); } @@ -394,7 +395,7 @@ fn bootstrap_rejects_invalid_patch_hub_page_size_env() { }) .returning(|_| Err(VarError::NotPresent.into())); - match ConfigService::bootstrap(&mock, os_fs()) { + match bootstrap_parts(&mock, os_fs()) { Ok(_) => panic!("expected bootstrap to fail"), Err(err) => { assert!(matches!(err, ConfigError::InvalidPageSize(ref s) if s == "not-a-number")) @@ -430,7 +431,7 @@ fn bootstrap_rejects_invalid_patch_hub_patch_renderer_env() { .withf(|key| key == "PATCH_HUB_PATCH_RENDERER") .returning(|_| Ok("not-a-real-renderer".into())); - match ConfigService::bootstrap(&mock, os_fs()) { + match bootstrap_parts(&mock, os_fs()) { Ok(_) => panic!("expected bootstrap to fail"), Err(err) => assert!(matches!( err, @@ -471,27 +472,27 @@ fn normalize_derived_paths_recomputes_cache_and_data_subpaths() { #[test] fn validate_update_rejects_invalid_page_size() { - let (env, _home) = default_env(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); - let err = service - .validate_update(ConfigUpdateDraft { + let err = validate_update( + ConfigUpdateDraft { page_size: Some("xyz".into()), ..Default::default() - }) - .unwrap_err(); + }, + &os_fs(), + ) + .unwrap_err(); assert!(matches!(err, ConfigError::InvalidPageSize(ref s) if s == "xyz")); } #[test] fn validate_update_rejects_invalid_patch_renderer() { - let (env, _home) = default_env(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); - let err = service - .validate_update(ConfigUpdateDraft { + let err = validate_update( + ConfigUpdateDraft { patch_renderer: Some("nope".into()), ..Default::default() - }) - .unwrap_err(); + }, + &os_fs(), + ) + .unwrap_err(); assert!(matches!( err, ConfigError::InvalidPatchRenderer(ref s) if s == "nope" @@ -500,14 +501,14 @@ fn validate_update_rejects_invalid_patch_renderer() { #[test] fn validate_update_rejects_invalid_cover_renderer() { - let (env, _home) = default_env(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); - let err = service - .validate_update(ConfigUpdateDraft { + let err = validate_update( + ConfigUpdateDraft { cover_renderer: Some("delta".into()), ..Default::default() - }) - .unwrap_err(); + }, + &os_fs(), + ) + .unwrap_err(); assert!(matches!( err, ConfigError::InvalidCoverRenderer(ref s) if s == "delta" @@ -516,14 +517,14 @@ fn validate_update_rejects_invalid_cover_renderer() { #[test] fn validate_update_rejects_invalid_max_log_age() { - let (env, _home) = default_env(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); - let err = service - .validate_update(ConfigUpdateDraft { + let err = validate_update( + ConfigUpdateDraft { max_log_age: Some("not-a-number".into()), ..Default::default() - }) - .unwrap_err(); + }, + &os_fs(), + ) + .unwrap_err(); assert!(matches!( err, ConfigError::InvalidMaxLogAge(ref s) if s == "not-a-number" @@ -532,17 +533,17 @@ fn validate_update_rejects_invalid_max_log_age() { #[test] fn validate_update_rejects_cache_dir_that_is_existing_file() { - let (env, _home) = default_env(); - let service = ConfigService::bootstrap(&env, os_fs()).unwrap(); let root = unique_test_dir("not-a-dir"); let blocking = root.join("blocking-file"); fs::write(&blocking, b"x").unwrap(); - let err = service - .validate_update(ConfigUpdateDraft { + let err = validate_update( + ConfigUpdateDraft { cache_dir: Some(blocking.to_string_lossy().into_owned()), ..Default::default() - }) - .unwrap_err(); + }, + &os_fs(), + ) + .unwrap_err(); assert!(matches!(err, ConfigError::InvalidDirectory(_))); } @@ -578,22 +579,24 @@ fn json_config_repository_save_creates_parent_and_leaves_no_tmp_stale() { ); } -#[test] -fn apply_update_persists_to_config_file() { +#[tokio::test] +async fn validate_and_apply_persists_to_config_file() { let (env, home) = default_env(); - let mut service = ConfigService::bootstrap(&env, os_fs()).unwrap(); + let (state, repo) = bootstrap_parts(&env, os_fs()).unwrap(); + let handle = ConfigActor::spawn(state, repo); let cfg_path = home.join(DEFAULT_CONFIG_PATH_SUFFIX); - let validated = service - .validate_update(ConfigUpdateDraft { + let snapshot = handle + .validate_and_apply(ConfigUpdateDraft { page_size: Some("77".into()), ..Default::default() }) + .await .unwrap(); - service.apply_update(validated).unwrap(); - assert_eq!(service.snapshot().page_size(), 77); + assert_eq!(snapshot.page_size(), 77); let raw = fs::read_to_string(&cfg_path).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&raw).unwrap(); assert_eq!(parsed["page_size"], 77); + handle.shutdown().await; } diff --git a/src/main.rs b/src/main.rs index f06a3e3..cb8de29 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14,7 +14,7 @@ use app::{actor::AppActor, App}; use clap::Parser; use cli::Cli; use color_eyre::eyre::eyre; -use config::{ConfigService, ConfigServiceApi}; +use config::{bootstrap_parts, ConfigActor}; use infrastructure::{ env::OsEnv, file_system::OsFileSystem, @@ -55,9 +55,8 @@ async fn main() -> color_eyre::Result<()> { infrastructure::errors::install_hooks()?; let env = OsEnv; - let config_service: Box = - Box::new(ConfigService::bootstrap(&env, OsFileSystem).map_err(|e| eyre!(e))?); - let config = config_service.snapshot(); + let (config_state, config_repo) = bootstrap_parts(&env, OsFileSystem).map_err(|e| eyre!(e))?; + let config = config_state.to_snapshot(); // with the config we can update log directory let _guards = multi_log_file_writer.update_log_writer_with_config( @@ -71,6 +70,7 @@ async fn main() -> color_eyre::Result<()> { ControlFlow::Continue(()) => {} } + let config_handle = ConfigActor::spawn(config_state, config_repo); let terminal_handle = TerminalActor::spawn(Box::new(CrosstermTerminalSession::new(init()?))); let ui_handle = UiActor::spawn(); @@ -110,7 +110,11 @@ async fn main() -> color_eyre::Result<()> { let bootstrap = lore_api.get_bootstrap_data().await.unwrap_or_default(); let app = App::new( - config_service, + config_handle + .get_snapshot() + .await + .map_err(|error| eyre!("{error}"))?, + config_handle.clone(), bootstrap, Box::new(OsFileSystem), Box::new(OsShell), @@ -127,10 +131,11 @@ async fn main() -> color_eyre::Result<()> { // Shutdown ordering: // 1. AppActor — exits when the user quits (input channel closes) - // 2. LoreApiActor — no further requests once App is gone - // 3. RenderActor — no further requests once App is gone - // 4. UiActor — no further scene builds once App is gone - // 5. TerminalActor — restores the terminal last so the screen stays usable + // 2. ConfigActor — no further configuration requests once App is gone + // 3. LoreApiActor — no further requests once App is gone + // 4. RenderActor — no further requests once App is gone + // 5. UiActor — no further scene builds once App is gone + // 6. TerminalActor — restores the terminal last so the screen stays usable // during the steps above AppActor::spawn( app, @@ -141,6 +146,7 @@ async fn main() -> color_eyre::Result<()> { ) .run_until_done() .await?; + config_handle.shutdown().await; lore_api.shutdown().await; render.shutdown().await; ui_handle.shutdown().await;