diff --git a/Cargo.lock b/Cargo.lock index 17174b46..2c6caee6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2412,6 +2412,16 @@ dependencies = [ "syn", ] +[[package]] +name = "serde_ignored" +version = "0.1.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "115dffd5f3853e06e746965a20dcbae6ee747ae30b543d91b0e089668bb07798" +dependencies = [ + "serde", + "serde_core", +] + [[package]] name = "serde_json" version = "1.0.149" @@ -3127,6 +3137,7 @@ version = "0.1.0" dependencies = [ "dirs", "serde", + "serde_ignored", "thiserror 2.0.18", "toml", "tracing", @@ -3143,6 +3154,7 @@ dependencies = [ "dialoguer", "tempfile", "tokio-serial", + "toml", "toml_edit 0.22.27", "trx-config", ] diff --git a/src/trx-client/src/main.rs b/src/trx-client/src/main.rs index 161f929b..5cef2e98 100644 --- a/src/trx-client/src/main.rs +++ b/src/trx-client/src/main.rs @@ -50,6 +50,9 @@ struct Cli { /// Print example configuration and exit #[arg(long = "print-config")] print_config: bool, + /// Treat unknown configuration keys as a fatal error + #[arg(long = "strict-config")] + strict_config: bool, /// Remote server URL (host:port) #[arg(short = 'u', long = "url")] url: Option, @@ -135,20 +138,24 @@ async fn async_init() -> DynResult { std::process::exit(0); } - let (cfg, config_path) = if let Some(ref path) = cli.config { - let cfg = ClientConfig::load_from_file(path)?; - (cfg, Some(path.clone())) + let loaded = if let Some(ref path) = cli.config { + ClientConfig::load_from_file(path)? } else { ClientConfig::load_from_default_paths()? }; - cfg.validate() - .map_err(|e| format!("Invalid client configuration: {}", e))?; + let config_path = loaded.path.clone(); - init_logging(cfg.general.log_level.as_deref()); + // Logging comes up before any config complaint so the warnings are visible. + init_logging(loaded.config.general.log_level.as_deref()); if let Some(ref path) = config_path { info!("Loaded configuration from {}", path.display()); } + loaded.report_unknown_keys(cli.strict_config)?; + + let cfg = loaded.config; + cfg.validate() + .map_err(|e| format!("Invalid client configuration: {}", e))?; frontend_runtime.http_auth.tokens = cfg .frontends diff --git a/src/trx-config/Cargo.toml b/src/trx-config/Cargo.toml index 6e23f93c..77f5f466 100644 --- a/src/trx-config/Cargo.toml +++ b/src/trx-config/Cargo.toml @@ -17,3 +17,4 @@ thiserror = "2" trx-core = { path = "../trx-core" } trx-decode-log = { path = "../decoders/trx-decode-log" } trx-reporting = { path = "../trx-reporting" } +serde_ignored = "0.1" diff --git a/src/trx-config/src/client.rs b/src/trx-config/src/client.rs index 6722119d..24c6ff82 100644 --- a/src/trx-config/src/client.rs +++ b/src/trx-config/src/client.rs @@ -13,11 +13,11 @@ use std::collections::HashMap; use std::net::IpAddr; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::time::Duration; use serde::{Deserialize, Serialize}; -use crate::file::{ConfigError, ConfigFile}; +use crate::file::{ConfigError, ConfigFile, ConfigLoad}; use crate::shared::{validate_log_level, validate_tokens}; /// Top-level client configuration structure. @@ -609,13 +609,13 @@ impl ClientConfig { } /// Load configuration from a specific file path. - pub fn load_from_file(path: &Path) -> Result { + pub fn load_from_file(path: &Path) -> Result, ConfigError> { ::load_from_file(path) } /// Load configuration from the default search paths. /// Returns default config if no config file is found. - pub fn load_from_default_paths() -> Result<(Self, Option), ConfigError> { + pub fn load_from_default_paths() -> Result, ConfigError> { ::load_from_default_paths() } @@ -734,6 +734,22 @@ impl ConfigFile for ClientConfig { fn section_key() -> &'static str { "trx-client" } + + /// Include one `[[remotes]]` entry so unknown keys nested inside a remote + /// entry still get a suggestion. + fn reference_value() -> toml::Value { + let cfg = ClientConfig { + remotes: vec![RemoteEntry { + name: "example".to_string(), + url: "127.0.0.1:4530".to_string(), + rig_id: Some("hf".to_string()), + auth: RemoteAuthConfig::default(), + poll_interval_ms: default_poll_interval_ms(), + }], + ..Default::default() + }; + toml::Value::try_from(cfg).unwrap_or_else(|_| toml::Value::Table(toml::Table::new())) + } } #[cfg(test)] diff --git a/src/trx-config/src/file.rs b/src/trx-config/src/file.rs index beeaea14..3e6feddd 100644 --- a/src/trx-config/src/file.rs +++ b/src/trx-config/src/file.rs @@ -18,12 +18,44 @@ //! falling back to defaults. use serde::de::DeserializeOwned; +use serde::Serialize; use std::path::{Path, PathBuf}; use thiserror::Error; +use crate::unknown::{describe, UnknownKey}; + /// Every section key that may appear at the root of a combined config file. pub const SECTION_KEYS: &[&str] = &["trx-server", "trx-client"]; +/// A loaded config plus what the loader noticed on the way in. +#[derive(Debug, Clone)] +pub struct ConfigLoad { + /// The deserialized configuration. + pub config: T, + /// File the config came from; `None` when nothing was found and defaults + /// were used. + pub path: Option, + /// Keys present in the file that no config field claimed. + pub unknown_keys: Vec, +} + +impl ConfigLoad { + /// Log every unknown key as a warning. With `strict`, also return an error + /// so the caller can refuse to start. + pub fn report_unknown_keys(&self, strict: bool) -> Result<(), String> { + for key in &self.unknown_keys { + tracing::warn!("{}", key); + } + if strict && !self.unknown_keys.is_empty() { + return Err(format!( + "{} unknown config key(s); refusing to start because --strict-config is set", + self.unknown_keys.len() + )); + } + Ok(()) + } +} + #[derive(Debug, Error)] pub enum ConfigError { #[error("Failed to read config file {0}: {1}")] @@ -62,12 +94,13 @@ fn select_section(table: &toml::Table, key: &str) -> Option { /// Extract and deserialize a named section from a TOML file. /// -/// Returns `Ok(Some(cfg))` when the section is present and parses cleanly, -/// `Ok(None)` when the section is absent, or `Err` on I/O / parse failure. -fn load_section_from_file( +/// Returns `Ok(Some((cfg, unknown_keys)))` when the section is present and +/// parses cleanly, `Ok(None)` when the section is absent, or `Err` on I/O / +/// parse failure. +fn load_section_from_file( path: &Path, key: &str, -) -> Result, ConfigError> { +) -> Result)>, ConfigError> { let content = std::fs::read_to_string(path) .map_err(|e| ConfigError::ReadError(path.to_path_buf(), e.to_string()))?; @@ -78,48 +111,72 @@ fn load_section_from_file( return Ok(None); }; - // Re-serialize the section then parse as T so all serde defaults apply. - let section_toml = toml::to_string(§ion) + // Deserialize straight from the TOML value so serde applies every default, + // recording any key no field claimed. + let mut ignored: Vec = Vec::new(); + let cfg: T = serde_ignored::deserialize(section, |path| ignored.push(path.to_string())) .map_err(|e| ConfigError::ParseError(path.to_path_buf(), e.to_string()))?; - let cfg = toml::from_str::(§ion_toml) - .map_err(|e| ConfigError::ParseError(path.to_path_buf(), e.to_string()))?; - Ok(Some(cfg)) + + Ok(Some((cfg, describe(&ignored, &T::reference_value())))) } /// Trait for loading configuration from a `trx-rs.toml` section. -pub trait ConfigFile: Sized + Default + DeserializeOwned { +pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize { /// Section key in `trx-rs.toml` (e.g. `"trx-server"` or `"trx-client"`). fn section_key() -> &'static str; + /// A TOML rendering of a populated config, used to suggest corrections for + /// unknown keys. Implementations should fill in list-valued sections such + /// as `[[rigs]]` so keys nested inside them can be suggested too. + fn reference_value() -> toml::Value { + toml::Value::try_from(Self::default()) + .unwrap_or_else(|_| toml::Value::Table(toml::Table::new())) + } + /// Load the section from a specific file path. /// /// Accepts both a sectioned file (`[]` at the root) and a bare /// file whose root is the section itself. Returns an error if the file /// cannot be read, is not valid TOML, or is sectioned for some other /// component only. - fn load_from_file(path: &Path) -> Result { - load_section_from_file::(path, Self::section_key())?.ok_or_else(|| { - ConfigError::ParseError( - path.to_path_buf(), - format!("missing [{}] section", Self::section_key()), - ) + fn load_from_file(path: &Path) -> Result, ConfigError> { + let (config, unknown_keys) = load_section_from_file::(path, Self::section_key())? + .ok_or_else(|| { + ConfigError::ParseError( + path.to_path_buf(), + format!("missing [{}] section", Self::section_key()), + ) + })?; + Ok(ConfigLoad { + config, + path: Some(path.to_path_buf()), + unknown_keys, }) } /// Search default paths (`trx-rs.toml` in CWD → XDG → /etc) and load /// the first file that contains the expected section. /// - /// Returns `(config, path_where_found)` or `(Default::default(), None)` - /// when no config file is found. - fn load_from_default_paths() -> Result<(Self, Option), ConfigError> { + /// Falls back to `Self::default()` with no path when nothing is found. + fn load_from_default_paths() -> Result, ConfigError> { for path in config_search_paths() { if path.exists() { - if let Some(cfg) = load_section_from_file::(&path, Self::section_key())? { - return Ok((cfg, Some(path))); + if let Some((config, unknown_keys)) = + load_section_from_file::(&path, Self::section_key())? + { + return Ok(ConfigLoad { + config, + path: Some(path), + unknown_keys, + }); } } } - Ok((Self::default(), None)) + Ok(ConfigLoad { + config: Self::default(), + path: None, + unknown_keys: Vec::new(), + }) } } diff --git a/src/trx-config/src/lib.rs b/src/trx-config/src/lib.rs index 5bb554b2..4f0508b5 100644 --- a/src/trx-config/src/lib.rs +++ b/src/trx-config/src/lib.rs @@ -13,10 +13,12 @@ pub mod client; pub mod file; pub mod server; pub mod shared; +pub mod unknown; pub mod url; pub use client::ClientConfig; -pub use file::{ConfigError, ConfigFile}; +pub use file::{ConfigError, ConfigFile, ConfigLoad}; pub use server::ServerConfig; pub use shared::{validate_log_level, validate_tokens}; +pub use unknown::UnknownKey; pub use url::{parse_audio_url, parse_remote_url, RemoteEndpoint}; diff --git a/src/trx-config/src/server.rs b/src/trx-config/src/server.rs index d504e0a5..3139874e 100644 --- a/src/trx-config/src/server.rs +++ b/src/trx-config/src/server.rs @@ -12,10 +12,10 @@ //! 4. `/etc/trx-rs/trx-rs.toml` use std::net::IpAddr; -use std::path::{Path, PathBuf}; +use std::path::Path; use serde::{Deserialize, Serialize}; -use crate::file::{ConfigError, ConfigFile}; +use crate::file::{ConfigError, ConfigFile, ConfigLoad}; use crate::shared::{validate_log_level, validate_tokens}; pub use trx_decode_log::DecodeLogsConfig; @@ -706,13 +706,13 @@ impl ServerConfig { } /// Load configuration from a specific file path. - pub fn load_from_file(path: &Path) -> Result { + pub fn load_from_file(path: &Path) -> Result, ConfigError> { ::load_from_file(path) } /// Load configuration from the default search paths. /// Returns default config if no config file is found. - pub fn load_from_default_paths() -> Result<(Self, Option), ConfigError> { + pub fn load_from_default_paths() -> Result, ConfigError> { ::load_from_default_paths() } @@ -900,6 +900,16 @@ impl ConfigFile for ServerConfig { fn section_key() -> &'static str { "trx-server" } + + /// Include one `[[rigs]]` entry so unknown keys nested inside a rig entry + /// still get a suggestion. + fn reference_value() -> toml::Value { + let cfg = ServerConfig { + rigs: vec![RigInstanceConfig::default()], + ..Default::default() + }; + toml::Value::try_from(cfg).unwrap_or_else(|_| toml::Value::Table(toml::Table::new())) + } } #[cfg(test)] diff --git a/src/trx-config/src/unknown.rs b/src/trx-config/src/unknown.rs new file mode 100644 index 00000000..6d9e22ad --- /dev/null +++ b/src/trx-config/src/unknown.rs @@ -0,0 +1,256 @@ +// SPDX-FileCopyrightText: 2026 Stan Grams +// +// SPDX-License-Identifier: GPL-2.0-or-later + +//! Reporting for config keys the deserializer ignored. +//! +//! Every config struct is `#[serde(default)]`, so a misspelled key used to be +//! dropped without a word and the setting silently kept its default. The +//! loader now collects the ignored key paths and pairs each with the closest +//! known key at the same level, so `prot = 9999` reads as a typo instead of +//! looking like it worked. + +use std::collections::BTreeSet; +use std::fmt; + +/// A config key that the deserializer did not recognise. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UnknownKey { + /// Dotted path of the key, e.g. `listen.prot` or `rigs.0.audio.prot`. + pub path: String, + /// Closest known key at the same level, when one is near enough to suggest. + pub suggestion: Option, +} + +impl fmt::Display for UnknownKey { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match &self.suggestion { + Some(s) => write!(f, "unknown config key '{}' (did you mean '{}'?)", self.path, s), + None => write!(f, "unknown config key '{}'", self.path), + } + } +} + +/// Flatten a TOML value into the set of dotted key paths it contains. +/// +/// Array indices are normalised to `0` so a path inside `[[rigs]]` matches +/// whichever entry it came from. +pub fn flatten_paths(value: &toml::Value) -> BTreeSet { + let mut paths = BTreeSet::new(); + walk(value, "", &mut paths); + paths +} + +fn walk(value: &toml::Value, prefix: &str, paths: &mut BTreeSet) { + let join = |seg: &str| { + if prefix.is_empty() { + seg.to_string() + } else { + format!("{prefix}.{seg}") + } + }; + match value { + toml::Value::Table(table) => { + for (key, child) in table { + let path = join(key); + paths.insert(path.clone()); + walk(child, &path, paths); + } + } + toml::Value::Array(items) => { + // Every entry of an array of tables has the same shape, so collapse + // them onto index 0 and let one entry stand for all. + for item in items { + let path = join("0"); + walk(item, &path, paths); + } + } + _ => {} + } +} + +/// Replace numeric path segments with `0` so array entries compare equal. +fn normalize(path: &str) -> String { + path.split('.') + .map(|seg| { + if !seg.is_empty() && seg.bytes().all(|b| b.is_ascii_digit()) { + "0" + } else { + seg + } + }) + .collect::>() + .join(".") +} + +/// Suggest the closest known key that sits at the same level as `path`. +/// +/// Returns `None` when nothing is close enough to be worth printing. +pub fn suggest(path: &str, known: &BTreeSet) -> Option { + let normalized = normalize(path); + let (parent, leaf) = match normalized.rsplit_once('.') { + Some((parent, leaf)) => (parent, leaf), + None => ("", normalized.as_str()), + }; + + // Anything longer than this is a different word, not a typo. + let limit = (leaf.chars().count() / 3).clamp(1, 3); + let mut best: Option<(usize, &str)> = None; + + for candidate in known { + let (cand_parent, cand_leaf) = match candidate.rsplit_once('.') { + Some((p, l)) => (p, l), + None => ("", candidate.as_str()), + }; + if cand_parent != parent || cand_leaf == leaf { + continue; + } + let distance = edit_distance(leaf, cand_leaf); + if distance <= limit && best.is_none_or(|(best_d, _)| distance < best_d) { + best = Some((distance, cand_leaf)); + } + } + + best.map(|(_, leaf)| { + if parent.is_empty() { + leaf.to_string() + } else { + format!("{parent}.{leaf}") + } + }) +} + +/// Pair each ignored path with a suggestion drawn from `reference`. +pub fn describe(paths: &[String], reference: &toml::Value) -> Vec { + let known = flatten_paths(reference); + paths + .iter() + .map(|path| UnknownKey { + path: path.clone(), + suggestion: suggest(path, &known), + }) + .collect() +} + +/// Optimal string alignment distance: Levenshtein plus transpositions, so the +/// common `port` → `prot` slip counts as one mistake rather than two. +fn edit_distance(a: &str, b: &str) -> usize { + let a: Vec = a.chars().collect(); + let b: Vec = b.chars().collect(); + if a.is_empty() { + return b.len(); + } + if b.is_empty() { + return a.len(); + } + + let mut rows = vec![vec![0usize; b.len() + 1]; a.len() + 1]; + for (i, row) in rows.iter_mut().enumerate() { + row[0] = i; + } + for (j, cell) in rows[0].iter_mut().enumerate() { + *cell = j; + } + + for i in 1..=a.len() { + for j in 1..=b.len() { + let cost = usize::from(a[i - 1] != b[j - 1]); + let mut best = (rows[i - 1][j] + 1) + .min(rows[i][j - 1] + 1) + .min(rows[i - 1][j - 1] + cost); + if i > 1 && j > 1 && a[i - 1] == b[j - 2] && a[i - 2] == b[j - 1] { + best = best.min(rows[i - 2][j - 2] + 1); + } + rows[i][j] = best; + } + } + rows[a.len()][b.len()] +} + +#[cfg(test)] +mod tests { + use super::*; + + fn reference() -> toml::Value { + toml::from_str( + r#" +[general] +callsign = "N0CALL" +log_level = "info" + +[listen] +enabled = true +port = 4530 + +[[rigs]] +id = "hf" +[rigs.audio] +port = 4531 +sample_rate = 48000 +"#, + ) + .unwrap() + } + + #[test] + fn test_flatten_collects_nested_paths() { + let paths = flatten_paths(&reference()); + assert!(paths.contains("general.callsign")); + assert!(paths.contains("listen.port")); + assert!(paths.contains("rigs.0.audio.sample_rate")); + } + + #[test] + fn test_suggest_finds_close_sibling() { + let known = flatten_paths(&reference()); + assert_eq!(suggest("listen.prot", &known).as_deref(), Some("listen.port")); + } + + #[test] + fn test_suggest_inside_array_entry() { + let known = flatten_paths(&reference()); + assert_eq!( + suggest("rigs.1.audio.prot", &known).as_deref(), + Some("rigs.0.audio.port") + ); + } + + #[test] + fn test_suggest_ignores_distant_names() { + let known = flatten_paths(&reference()); + assert_eq!(suggest("listen.bananas", &known), None); + } + + #[test] + fn test_suggest_does_not_cross_levels() { + let known = flatten_paths(&reference()); + // `port` exists under [listen], but not under [general]. + assert_eq!(suggest("general.port", &known), None); + } + + #[test] + fn test_describe_formats_message() { + let described = describe(&["listen.prot".to_string()], &reference()); + assert_eq!( + described[0].to_string(), + "unknown config key 'listen.prot' (did you mean 'listen.port'?)" + ); + } + + #[test] + fn test_describe_without_suggestion() { + let described = describe(&["listen.bananas".to_string()], &reference()); + assert_eq!( + described[0].to_string(), + "unknown config key 'listen.bananas'" + ); + } + + #[test] + fn test_edit_distance_counts_transposition_once() { + assert_eq!(edit_distance("port", "prot"), 1); + assert_eq!(edit_distance("port", "port"), 0); + assert_eq!(edit_distance("", "port"), 4); + assert_eq!(edit_distance("sample_rat", "sample_rate"), 1); + } +} diff --git a/src/trx-configurator/Cargo.toml b/src/trx-configurator/Cargo.toml index 1db800a0..dc8523fe 100644 --- a/src/trx-configurator/Cargo.toml +++ b/src/trx-configurator/Cargo.toml @@ -15,6 +15,7 @@ path = "src/main.rs" clap = { workspace = true, features = ["derive"] } dialoguer = "0.11" tokio-serial = { workspace = true } +toml = "0.8" toml_edit = "0.22" trx-config = { path = "../trx-config" } diff --git a/src/trx-configurator/src/check.rs b/src/trx-configurator/src/check.rs index fa94ed57..50b670ea 100644 --- a/src/trx-configurator/src/check.rs +++ b/src/trx-configurator/src/check.rs @@ -2,80 +2,37 @@ // // SPDX-License-Identifier: GPL-2.0-or-later +//! `trx-configurator --check`: run a config file through the same loader and +//! validators the binaries use. +//! +//! This used to be a second, hand-maintained implementation — lists of known +//! keys and a handful of re-implemented range checks — which drifted out of +//! date as soon as a field was added. It now defers entirely to `trx-config`, +//! so a config that checks clean here is one the binaries will accept. + use std::fmt::Write as _; use std::path::Path; -use toml_edit::DocumentMut; +use trx_config::{ClientConfig, ServerConfig}; -/// Known top-level keys for a standalone server config. -const SERVER_KEYS: &[&str] = &[ - "general", +/// Top-level keys that only appear in a server config. Used solely to guess +/// what a section-less file is meant to be; the real key checking is done by +/// the loader. +const SERVER_MARKERS: &[&str] = &[ "rig", "rigs", - "behavior", "listen", - "audio", + "behavior", "sdr", "pskreporter", "aprsfi", "decode_logs", + "timeouts", + "audio", ]; -/// Known top-level keys for a standalone client config. -const CLIENT_KEYS: &[&str] = &["general", "remote", "remotes", "frontends"]; - -/// Known top-level keys for a combined trx-rs.toml. -const COMBINED_KEYS: &[&str] = &["trx-server", "trx-client"]; - -/// Known sub-keys within [general] (server). -const SERVER_GENERAL_KEYS: &[&str] = &["callsign", "log_level", "latitude", "longitude"]; - -/// Known sub-keys within [general] (client). -const CLIENT_GENERAL_KEYS: &[&str] = &[ - "callsign", - "log_level", - "website_url", - "website_name", - "ais_vessel_url_base", -]; - -/// Known sub-keys within [rig]. -const RIG_KEYS: &[&str] = &["model", "initial_freq_hz", "initial_mode", "access"]; - -/// Known sub-keys within [rig.access]. -const ACCESS_KEYS: &[&str] = &["type", "port", "baud", "host", "tcp_port", "args"]; - -/// Known sub-keys within [listen]. -const LISTEN_KEYS: &[&str] = &["enabled", "listen", "port", "auth"]; - -/// Known sub-keys within [audio] (server). -const AUDIO_KEYS: &[&str] = &[ - "enabled", - "listen", - "port", - "rx_enabled", - "tx_enabled", - "device", - "sample_rate", - "channels", - "frame_duration_ms", - "bitrate_bps", -]; - -/// Known sub-keys within [behavior]. -const BEHAVIOR_KEYS: &[&str] = &[ - "poll_interval_ms", - "poll_interval_tx_ms", - "max_retries", - "retry_base_delay_ms", - "vfo_prime", -]; - -/// Known sub-keys within [remote]. -const REMOTE_KEYS: &[&str] = &["url", "rig_id", "auth", "poll_interval_ms"]; - -/// Known sub-keys within [frontends]. -const FRONTENDS_KEYS: &[&str] = &["http", "rigctl", "http_json", "audio"]; +/// Top-level keys that only appear in a client config. +const CLIENT_MARKERS: &[&str] = &["remote", "remotes", "frontends"]; #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum DetectedType { @@ -100,49 +57,38 @@ pub fn check_file(path: &Path) -> Result { let content = std::fs::read_to_string(path) .map_err(|e| format!("Failed to read {}: {}", path.display(), e))?; - // Step 1: TOML syntax check - let doc: DocumentMut = content - .parse() + let table: toml::Table = toml::from_str(&content) .map_err(|e| format!("{}: TOML syntax error: {}", path.display(), e))?; + let detected = detect_type(&table); + let mut report = String::new(); let mut warnings: Vec = Vec::new(); let mut errors: Vec = Vec::new(); - let table = doc.as_table(); - - // Step 2: Detect config type - let detected = detect_type(table); writeln!(report, "{}: valid TOML", path.display()).unwrap(); writeln!(report, " Detected type: {}", detected).unwrap(); - // Step 3: Structural validation match detected { - DetectedType::Server => { - check_unknown_keys(table, SERVER_KEYS, "", &mut warnings); - check_server_sections(table, "", &mut warnings, &mut errors); - } - DetectedType::Client => { - check_unknown_keys(table, CLIENT_KEYS, "", &mut warnings); - check_client_sections(table, "", &mut warnings, &mut errors); - } + DetectedType::Server => check_server(path, &mut warnings, &mut errors), + DetectedType::Client => check_client(path, &mut warnings, &mut errors), DetectedType::Combined => { - check_unknown_keys(table, COMBINED_KEYS, "", &mut warnings); - if let Some(server) = table.get("trx-server").and_then(|v| v.as_table()) { - check_unknown_keys(server, SERVER_KEYS, "[trx-server].", &mut warnings); - check_server_sections(server, "[trx-server].", &mut warnings, &mut errors); + if table.contains_key("trx-server") { + check_server(path, &mut warnings, &mut errors); } - if let Some(client) = table.get("trx-client").and_then(|v| v.as_table()) { - check_unknown_keys(client, CLIENT_KEYS, "[trx-client].", &mut warnings); - check_client_sections(client, "[trx-client].", &mut warnings, &mut errors); + if table.contains_key("trx-client") { + check_client(path, &mut warnings, &mut errors); } } DetectedType::Unknown => { - warnings.push("Could not detect config type. Expected server, client, or combined (trx-rs.toml) layout.".to_string()); + warnings.push( + "Could not detect config type. Expected server, client, or combined \ + (trx-rs.toml) layout." + .to_string(), + ); } } - // Step 4: Format report for w in &warnings { writeln!(report, " warning: {}", w).unwrap(); } @@ -169,23 +115,54 @@ pub fn check_file(path: &Path) -> Result { } } -fn detect_type(table: &toml_edit::Table) -> DetectedType { +fn check_server(path: &Path, warnings: &mut Vec, errors: &mut Vec) { + match ServerConfig::load_from_file(path) { + Ok(loaded) => { + warnings.extend(loaded.unknown_keys.iter().map(|k| k.to_string())); + if let Err(e) = loaded.config.validate() { + errors.push(format!("[trx-server] {}", e)); + } + errors.extend( + loaded + .config + .validate_sdr() + .into_iter() + .map(|e| format!("[trx-server] {}", e)), + ); + } + Err(e) => errors.push(e.to_string()), + } +} + +fn check_client(path: &Path, warnings: &mut Vec, errors: &mut Vec) { + match ClientConfig::load_from_file(path) { + Ok(loaded) => { + warnings.extend(loaded.unknown_keys.iter().map(|k| k.to_string())); + if let Err(e) = loaded.config.validate() { + errors.push(format!("[trx-client] {}", e)); + } + } + Err(e) => errors.push(e.to_string()), + } +} + +fn detect_type(table: &toml::Table) -> DetectedType { if table.contains_key("trx-server") || table.contains_key("trx-client") { return DetectedType::Combined; } - let keys: Vec<&str> = table.iter().map(|(k, _)| k).collect(); + let keys: Vec<&str> = table.keys().map(|k| k.as_str()).collect(); - let server_score = keys.iter().filter(|k| SERVER_KEYS.contains(k)).count(); - let client_score = keys.iter().filter(|k| CLIENT_KEYS.contains(k)).count(); - - // Use distinguishing keys to break ties - if keys.contains(&"rig") || keys.contains(&"rigs") || keys.contains(&"listen") { + // Distinguishing keys first, then a simple majority. + if keys.iter().any(|k| ["rig", "rigs", "listen"].contains(k)) { return DetectedType::Server; } - if keys.contains(&"remote") || keys.contains(&"remotes") || keys.contains(&"frontends") { + if keys.iter().any(|k| CLIENT_MARKERS.contains(k)) { return DetectedType::Client; } + let server_score = keys.iter().filter(|k| SERVER_MARKERS.contains(k)).count(); + let client_score = keys.iter().filter(|k| CLIENT_MARKERS.contains(k)).count(); + if server_score > client_score { DetectedType::Server } else if client_score > server_score { @@ -197,194 +174,6 @@ fn detect_type(table: &toml_edit::Table) -> DetectedType { } } -fn check_unknown_keys( - table: &toml_edit::Table, - known: &[&str], - prefix: &str, - warnings: &mut Vec, -) { - for (key, _) in table.iter() { - if !known.contains(&key) { - warnings.push(format!("{}unknown key '{}'", prefix, key)); - } - } -} - -fn check_server_sections( - table: &toml_edit::Table, - prefix: &str, - warnings: &mut Vec, - errors: &mut Vec, -) { - if let Some(general) = table.get("general").and_then(|v| v.as_table()) { - check_unknown_keys( - general, - SERVER_GENERAL_KEYS, - &format!("{}[general].", prefix), - warnings, - ); - validate_log_level(general, &format!("{}[general]", prefix), errors); - validate_coordinates(general, &format!("{}[general]", prefix), errors); - } - - if let Some(rig) = table.get("rig").and_then(|v| v.as_table()) { - check_unknown_keys(rig, RIG_KEYS, &format!("{}[rig].", prefix), warnings); - if let Some(access) = rig.get("access").and_then(|v| v.as_table()) { - check_unknown_keys( - access, - ACCESS_KEYS, - &format!("{}[rig.access].", prefix), - warnings, - ); - validate_access(access, &format!("{}[rig.access]", prefix), errors); - } - } - - if let Some(listen) = table.get("listen").and_then(|v| v.as_table()) { - check_unknown_keys( - listen, - LISTEN_KEYS, - &format!("{}[listen].", prefix), - warnings, - ); - validate_port(listen, "port", &format!("{}[listen]", prefix), errors); - } - - if let Some(audio) = table.get("audio").and_then(|v| v.as_table()) { - check_unknown_keys(audio, AUDIO_KEYS, &format!("{}[audio].", prefix), warnings); - validate_port(audio, "port", &format!("{}[audio]", prefix), errors); - } - - if let Some(behavior) = table.get("behavior").and_then(|v| v.as_table()) { - check_unknown_keys( - behavior, - BEHAVIOR_KEYS, - &format!("{}[behavior].", prefix), - warnings, - ); - } -} - -fn check_client_sections( - table: &toml_edit::Table, - prefix: &str, - warnings: &mut Vec, - errors: &mut Vec, -) { - if let Some(general) = table.get("general").and_then(|v| v.as_table()) { - check_unknown_keys( - general, - CLIENT_GENERAL_KEYS, - &format!("{}[general].", prefix), - warnings, - ); - validate_log_level(general, &format!("{}[general]", prefix), errors); - } - - if let Some(remote) = table.get("remote").and_then(|v| v.as_table()) { - check_unknown_keys( - remote, - REMOTE_KEYS, - &format!("{}[remote].", prefix), - warnings, - ); - } - - if let Some(frontends) = table.get("frontends").and_then(|v| v.as_table()) { - check_unknown_keys( - frontends, - FRONTENDS_KEYS, - &format!("{}[frontends].", prefix), - warnings, - ); - if let Some(http) = frontends.get("http").and_then(|v| v.as_table()) { - validate_port(http, "port", &format!("{}[frontends.http]", prefix), errors); - } - if let Some(rigctl) = frontends.get("rigctl").and_then(|v| v.as_table()) { - validate_port( - rigctl, - "port", - &format!("{}[frontends.rigctl]", prefix), - errors, - ); - } - } -} - -// ── Value validators ──────────────────────────────────────────────────── - -fn validate_log_level(table: &toml_edit::Table, context: &str, errors: &mut Vec) { - if let Some(level) = table.get("log_level").and_then(|v| v.as_str()) { - if !["trace", "debug", "info", "warn", "error"].contains(&level) { - errors.push(format!( - "{}.log_level '{}' is invalid (expected: trace, debug, info, warn, error)", - context, level - )); - } - } -} - -fn validate_coordinates(table: &toml_edit::Table, context: &str, errors: &mut Vec) { - if let Some(lat) = table - .get("latitude") - .and_then(|v| v.as_float().or_else(|| v.as_integer().map(|i| i as f64))) - { - if !(-90.0..=90.0).contains(&lat) { - errors.push(format!( - "{}.latitude {} is out of range (-90..90)", - context, lat - )); - } - } - if let Some(lon) = table - .get("longitude") - .and_then(|v| v.as_float().or_else(|| v.as_integer().map(|i| i as f64))) - { - if !(-180.0..=180.0).contains(&lon) { - errors.push(format!( - "{}.longitude {} is out of range (-180..180)", - context, lon - )); - } - } - - let has_lat = table.contains_key("latitude"); - let has_lon = table.contains_key("longitude"); - if has_lat != has_lon { - errors.push(format!( - "{}: latitude and longitude must be set together or both omitted", - context - )); - } -} - -fn validate_port(table: &toml_edit::Table, key: &str, context: &str, errors: &mut Vec) { - if let Some(port) = table.get(key).and_then(|v| v.as_integer()) { - if let Some(enabled) = table.get("enabled").and_then(|v| v.as_bool()) { - if enabled && port <= 0 { - errors.push(format!("{}.{} must be > 0 when enabled", context, key)); - } - } - if !(0..=65535).contains(&port) { - errors.push(format!( - "{}.{} {} is out of range (0..65535)", - context, key, port - )); - } - } -} - -fn validate_access(table: &toml_edit::Table, context: &str, errors: &mut Vec) { - if let Some(access_type) = table.get("type").and_then(|v| v.as_str()) { - if !["serial", "tcp", "sdr"].contains(&access_type) { - errors.push(format!( - "{}.type '{}' is invalid (expected: serial, tcp, sdr)", - context, access_type - )); - } - } -} - #[cfg(test)] mod tests { use super::*; @@ -418,8 +207,7 @@ enabled = true port = 4530 "#, ); - assert!(result.is_ok()); - let report = result.unwrap(); + let report = result.expect("expected a clean report"); assert!(report.contains("Detected type: server")); assert!(report.contains("No issues found")); } @@ -432,41 +220,40 @@ port = 4530 callsign = "W1AW" [remote] -url = "localhost:4530" +url = "192.168.1.10:4530" [frontends.http] enabled = true port = 8080 "#, ); - assert!(result.is_ok()); - let report = result.unwrap(); + let report = result.expect("expected a clean report"); assert!(report.contains("Detected type: client")); + assert!(report.contains("No issues found")); } #[test] fn test_valid_combined_config() { let result = check_toml( r#" -[trx-server.general] -callsign = "W1AW" - -[trx-client.general] -callsign = "W1AW" +[trx-server.rig] +model = "ft817" +[trx-server.rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 [trx-client.remote] -url = "localhost:4530" +url = "127.0.0.1:4530" "#, ); - assert!(result.is_ok()); - let report = result.unwrap(); + let report = result.expect("expected a clean report"); assert!(report.contains("Detected type: combined")); } #[test] fn test_invalid_toml_syntax() { - let result = check_toml("this is not [valid toml"); - assert!(result.is_err()); + let result = check_toml("[general\ncallsign = \"W1AW\"\n"); assert!(result.unwrap_err().contains("TOML syntax error")); } @@ -474,19 +261,22 @@ url = "localhost:4530" fn test_unknown_key_warning() { let result = check_toml( r#" -[general] -callsign = "W1AW" - [rig] model = "ft817" +[rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 -[bogus_section] -foo = "bar" +[listen] +prot = 4530 "#, ); - assert!(result.is_ok()); - let report = result.unwrap(); - assert!(report.contains("unknown key 'bogus_section'")); + let report = result.expect("unknown keys are warnings, not errors"); + assert!( + report.contains("unknown config key 'listen.prot' (did you mean 'listen.port'?)"), + "unexpected report: {report}" + ); } #[test] @@ -498,11 +288,13 @@ log_level = "verbose" [rig] model = "ft817" +[rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 "#, ); - assert!(result.is_err()); - let report = result.unwrap_err(); - assert!(report.contains("log_level 'verbose' is invalid")); + assert!(result.unwrap_err().contains("log_level")); } #[test] @@ -510,15 +302,17 @@ model = "ft817" let result = check_toml( r#" [general] -latitude = 45.0 +latitude = 52.0 [rig] model = "ft817" +[rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 "#, ); - assert!(result.is_err()); - let report = result.unwrap_err(); - assert!(report.contains("latitude and longitude must be set together")); + assert!(result.unwrap_err().contains("longitude")); } #[test] @@ -526,16 +320,18 @@ model = "ft817" let result = check_toml( r#" [general] -latitude = 95.0 +latitude = 120.0 longitude = 10.0 [rig] model = "ft817" +[rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 "#, ); - assert!(result.is_err()); - let report = result.unwrap_err(); - assert!(report.contains("latitude 95 is out of range")); + assert!(result.unwrap_err().contains("latitude")); } #[test] @@ -547,10 +343,31 @@ model = "ft817" [rig.access] type = "usb" +port = "/dev/ttyUSB0" +baud = 9600 "#, ); - assert!(result.is_err()); - let report = result.unwrap_err(); - assert!(report.contains("type 'usb' is invalid")); + assert!(result.unwrap_err().contains("access")); + } + + /// The old checker only knew a fixed list of top-level keys and a few range + /// rules, so it passed configs the server rejects at startup. + #[test] + fn test_catches_errors_the_key_list_checker_missed() { + let result = check_toml( + r#" +[rig] +model = "ft817" +[rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 + +[audio] +enabled = true +frame_duration_ms = 7 +"#, + ); + assert!(result.unwrap_err().contains("frame_duration_ms")); } } diff --git a/src/trx-configurator/src/writer.rs b/src/trx-configurator/src/writer.rs index fc1de6f7..96edaf32 100644 --- a/src/trx-configurator/src/writer.rs +++ b/src/trx-configurator/src/writer.rs @@ -474,7 +474,9 @@ mod tests { #[test] fn test_generated_server_config_loads_and_validates() { let file = write_temp(&build_default(ConfigType::Server)); - let cfg = ServerConfig::load_from_file(file.path()).expect("generated config must load"); + let cfg = ServerConfig::load_from_file(file.path()) + .expect("generated config must load") + .config; cfg.validate().expect("generated config must validate"); assert_eq!(cfg.rig.model.as_deref(), Some("ft817")); assert_eq!(cfg.listen.port, 4530); @@ -483,7 +485,9 @@ mod tests { #[test] fn test_generated_client_config_loads_and_validates() { let file = write_temp(&build_default(ConfigType::Client)); - let cfg = ClientConfig::load_from_file(file.path()).expect("generated config must load"); + let cfg = ClientConfig::load_from_file(file.path()) + .expect("generated config must load") + .config; cfg.validate().expect("generated config must validate"); assert_eq!(cfg.remote.url.as_deref(), Some("localhost:4530")); } @@ -491,9 +495,13 @@ mod tests { #[test] fn test_generated_combined_config_loads_both_sections() { let file = write_temp(&build_default(ConfigType::Combined)); - let server = ServerConfig::load_from_file(file.path()).expect("server section must load"); + let server = ServerConfig::load_from_file(file.path()) + .expect("server section must load") + .config; server.validate().expect("server section must validate"); - let client = ClientConfig::load_from_file(file.path()).expect("client section must load"); + let client = ClientConfig::load_from_file(file.path()) + .expect("client section must load") + .config; client.validate().expect("client section must validate"); } diff --git a/src/trx-server/src/main.rs b/src/trx-server/src/main.rs index 2a42d940..8e9b156f 100644 --- a/src/trx-server/src/main.rs +++ b/src/trx-server/src/main.rs @@ -56,6 +56,9 @@ struct Cli { /// Print example configuration and exit #[arg(long = "print-config")] print_config: bool, + /// Treat unknown configuration keys as a fatal error + #[arg(long = "strict-config")] + strict_config: bool, /// Rig backend to use (e.g. ft817, ft450d) #[arg(short = 'r', long = "rig")] rig: Option, @@ -894,12 +897,22 @@ async fn main() -> DynResult<()> { return Ok(()); } - let (cfg, config_path) = if let Some(ref path) = cli.config { - let cfg = ServerConfig::load_from_file(path)?; - (cfg, Some(path.clone())) + let loaded = if let Some(ref path) = cli.config { + ServerConfig::load_from_file(path)? } else { ServerConfig::load_from_default_paths()? }; + let config_path = loaded.path.clone(); + + // Logging comes up before any config complaint so the warnings are visible. + init_logging(loaded.config.general.log_level.as_deref()); + + if let Some(ref path) = config_path { + info!("Loaded configuration from {}", path.display()); + } + loaded.report_unknown_keys(cli.strict_config)?; + + let cfg = loaded.config; cfg.validate() .map_err(|e| format!("Invalid server configuration: {}", e))?; @@ -912,12 +925,6 @@ async fn main() -> DynResult<()> { std::process::exit(1); } - init_logging(cfg.general.log_level.as_deref()); - - if let Some(ref path) = config_path { - info!("Loaded configuration from {}", path.display()); - } - let registry = Arc::new(bootstrap_ctx); // --- Resolve the effective rig list ---