From 2b06ff749f634cf8107adce44bff256d94659fa1 Mon Sep 17 00:00:00 2001 From: Alexander Date: Wed, 24 Jun 2026 14:36:50 +0300 Subject: [PATCH] =?UTF-8?q?fix(review):=20Settings=20=E2=80=94=20=D1=82?= =?UTF-8?q?=D0=B8=D0=BF=D0=B8=D0=B7=D0=B8=D1=80=D0=BE=D0=B2=D0=B0=D0=BD?= =?UTF-8?q?=D0=BD=D1=8B=D0=B9=20SettingValue=20+=20=D0=BB=D0=BE=D0=B3=20?= =?UTF-8?q?=D0=B1=D0=B8=D1=82=D0=BE=D0=B3=D0=BE=20JSON=20+=20Error::Failed?= =?UTF-8?q?=20+=20=D1=87=D0=B8=D1=81=D1=82=D1=8B=D0=B9=20reset?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SettingValue{Str,Int,Bool} в сторе: провод-variant теперь честно поддержан (не string-only). - битый settings.json → WARN в лог (не молча) + откат к дефолтам. - отказ записи /data → Error::Failed (не InvalidArgument). - reset ключа без дефолта → no-op + InvalidArgument (без ложного Changed("")). Co-Authored-By: Claude Opus 4.8 Signed-off-by: Alexander --- Cargo.lock | 1 + crates/core/shturman-settings/Cargo.toml | 1 + crates/core/shturman-settings/src/service.rs | 66 +++++++--- crates/core/shturman-settings/src/store.rs | 126 ++++++++++++------- crates/shturman-ipc/src/error.rs | 3 + 5 files changed, 134 insertions(+), 63 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bf4db5d..b086935 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3606,6 +3606,7 @@ version = "0.0.0" dependencies = [ "anyhow", "futures-util", + "serde", "serde_json", "shturman-common", "shturman-ipc", diff --git a/crates/core/shturman-settings/Cargo.toml b/crates/core/shturman-settings/Cargo.toml index f6635cf..4fe7fc5 100644 --- a/crates/core/shturman-settings/Cargo.toml +++ b/crates/core/shturman-settings/Cargo.toml @@ -9,6 +9,7 @@ shturman-ipc = { path = "../../shturman-ipc" } shturman-common = { path = "../../shturman-common" } zbus.workspace = true tokio.workspace = true +serde.workspace = true serde_json.workspace = true anyhow.workspace = true tracing.workspace = true diff --git a/crates/core/shturman-settings/src/service.rs b/crates/core/shturman-settings/src/service.rs index afff53f..146dd67 100644 --- a/crates/core/shturman-settings/src/service.rs +++ b/crates/core/shturman-settings/src/service.rs @@ -1,6 +1,7 @@ -//! `ru.shturman.Settings1` — server-стаб. Хранит строки (v0); namespace-изоляция по `sender` — позже (v3). +//! `ru.shturman.Settings1` — server-стаб. Значения типизированы (`SettingValue`); конверсия в/из +//! D-Bus variant — один адаптер на границе (ниже). namespace-изоляция по `sender` — позже (v3). -use crate::store::Store; +use crate::store::{SettingValue, Store}; use shturman_ipc::Error; use std::sync::Arc; use tokio::sync::Mutex; @@ -20,13 +21,40 @@ impl SettingsService { } } +/// `SettingValue` → D-Bus variant (для `Get`/`Changed`). +fn to_value(sv: &SettingValue) -> Value<'static> { + match sv { + SettingValue::Str(s) => Value::from(s.clone()), + SettingValue::Bool(b) => Value::from(*b), + SettingValue::Int(i) => Value::from(*i), + } +} + +/// D-Bus variant → `SettingValue` (для `Set`). v0: str/int/bool; иначе — `InvalidArgument`. +fn from_value(v: &Value<'_>) -> Result { + match v { + Value::Str(s) => Ok(SettingValue::Str(s.to_string())), + Value::Bool(b) => Ok(SettingValue::Bool(*b)), + Value::I64(i) => Ok(SettingValue::Int(*i)), + Value::I32(i) => Ok(SettingValue::Int(*i as i64)), + Value::I16(i) => Ok(SettingValue::Int(*i as i64)), + Value::U32(u) => Ok(SettingValue::Int(*u as i64)), + Value::U16(u) => Ok(SettingValue::Int(*u as i64)), + Value::U8(u) => Ok(SettingValue::Int(*u as i64)), + other => Err(Error::InvalidArgument(format!( + "неподдерживаемый тип значения (v0: str/int/bool): {}", + other.value_signature() + ))), + } +} + #[interface(name = "ru.shturman.Settings1")] impl SettingsService { async fn get(&self, key: &str) -> Result { let store = self.store.lock().await; match store.get(key) { - Some(v) => OwnedValue::try_from(Value::from(v.to_string())) - .map_err(|e| Error::InvalidArgument(format!("value convert: {e}"))), + Some(sv) => OwnedValue::try_from(to_value(sv)) + .map_err(|e| Error::Failed(format!("value convert: {e}"))), None => Err(Error::InvalidArgument(format!("unknown key: {key}"))), } } @@ -37,20 +65,14 @@ impl SettingsService { value: Value<'_>, #[zbus(signal_context)] ctx: SignalContext<'_>, ) -> Result<(), Error> { - let s = match value { - Value::Str(s) => s.as_str().to_string(), - _ => { - return Err(Error::InvalidArgument( - "v0 поддерживает только строковые значения".into(), - )) - } - }; + let sv = from_value(&value)?; self.store .lock() .await - .set(&key, &s) - .map_err(|e| Error::InvalidArgument(format!("persist: {e}")))?; - let _ = Self::changed(&ctx, &key, &Value::from(s)).await; + .set(&key, sv.clone()) + // отказ записи в /data — внутренний сбой, НЕ InvalidArgument (плохой ввод) + .map_err(|e| Error::Failed(format!("persist: {e}")))?; + let _ = Self::changed(&ctx, &key, &to_value(&sv)).await; Ok(()) } @@ -68,9 +90,17 @@ impl SettingsService { .lock() .await .reset(&key) - .map_err(|e| Error::InvalidArgument(format!("persist: {e}")))?; - let _ = Self::changed(&ctx, &key, &Value::from(new.unwrap_or_default())).await; - Ok(()) + .map_err(|e| Error::Failed(format!("persist: {e}")))?; + match new { + Some(sv) => { + let _ = Self::changed(&ctx, &key, &to_value(&sv)).await; + Ok(()) + } + // нет дефолта — reset неприменим; не эмитим ложный Changed("") + None => Err(Error::InvalidArgument(format!( + "reset: у ключа '{key}' нет дефолта" + ))), + } } #[zbus(signal)] diff --git a/crates/core/shturman-settings/src/store.rs b/crates/core/shturman-settings/src/store.rs index ae4b019..f17ab07 100644 --- a/crates/core/shturman-settings/src/store.rs +++ b/crates/core/shturman-settings/src/store.rs @@ -1,39 +1,65 @@ -//! Стор настроек: `/data/settings/settings.json` (строковые значения в v0), durable-write. -//! Единственный писатель поддерева `/data/settings/` (architecture §3). +//! Стор настроек: `/data/settings/settings.json`, durable-write. Единственный писатель +//! поддерева `/data/settings/` (architecture §3). Значения типизированы (`SettingValue`) — +//! провод D-Bus отдаёт variant, и стор честно его поддерживает (str/int/bool). +use serde::{Deserialize, Serialize}; use shturman_common::{write_atomic, Layout}; use std::collections::BTreeMap; use std::io; -/// Дефолты v0 (data-model §2: канон единиц; конвертация — на презентации). -const DEFAULTS: &[(&str, &str)] = &[("ui.theme", "auto"), ("ui.units", "metric")]; +/// Значение настройки. На проводе — D-Bus variant; на диске — нативный JSON (untagged). +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(untagged)] +pub enum SettingValue { + Bool(bool), + Int(i64), + Str(String), +} -fn defaults_map() -> BTreeMap { - DEFAULTS +fn defaults_map() -> BTreeMap { + [("ui.theme", "auto"), ("ui.units", "metric")] .iter() - .map(|(k, v)| (k.to_string(), v.to_string())) + .map(|(k, v)| (k.to_string(), SettingValue::Str(v.to_string()))) .collect() } -fn serialize(map: &BTreeMap) -> String { +/// Дефолт ключа (если есть) — единый источник для seed и `reset` (data-model §2: канон единиц). +fn default_for(key: &str) -> Option { + match key { + "ui.theme" => Some(SettingValue::Str("auto".into())), + "ui.units" => Some(SettingValue::Str("metric".into())), + _ => None, + } +} + +fn serialize(map: &BTreeMap) -> String { serde_json::to_string_pretty(map).unwrap_or_else(|_| "{}".to_string()) } -/// Key-value стор настроек (строки v0). +/// Key-value стор настроек. pub struct Store { layout: Layout, - map: BTreeMap, + map: BTreeMap, } impl Store { /// Загрузить из `settings.json`; если файла нет — посеять дефолты (durable-write). - /// Битый JSON → дефолты в памяти (перезапишутся при следующем `set`). + /// Битый JSON → дефолты в памяти + **WARN в лог** (не молча); перезапишутся при следующем `set`. pub fn load_or_seed(layout: Layout) -> io::Result { std::fs::create_dir_all(layout.settings())?; let path = layout.settings().join("settings.json"); let map = if path.exists() { - serde_json::from_str(&std::fs::read_to_string(&path)?) - .unwrap_or_else(|_| defaults_map()) + let raw = std::fs::read_to_string(&path)?; + match serde_json::from_str(&raw) { + Ok(m) => m, + Err(e) => { + tracing::warn!( + error = %e, path = %path.display(), + "битый settings.json — откат к дефолтам (сохранится при следующем set)" + ); + defaults_map() + } + } } else { let d = defaults_map(); write_atomic(&path, serialize(&d).as_bytes())?; @@ -42,31 +68,26 @@ impl Store { Ok(Self { layout, map }) } - pub fn get(&self, key: &str) -> Option<&str> { - self.map.get(key).map(|s| s.as_str()) + pub fn get(&self, key: &str) -> Option<&SettingValue> { + self.map.get(key) } - pub fn set(&mut self, key: &str, value: &str) -> io::Result<()> { - self.map.insert(key.to_string(), value.to_string()); + pub fn set(&mut self, key: &str, value: SettingValue) -> io::Result<()> { + self.map.insert(key.to_string(), value); self.persist() } - /// Сбросить ключ к дефолту (если есть) или удалить. Возвращает значение после сброса (для `Changed`). - pub fn reset(&mut self, key: &str) -> io::Result> { - let default = DEFAULTS - .iter() - .find(|(k, _)| *k == key) - .map(|(_, v)| v.to_string()); - match &default { + /// Сбросить к дефолту. `Some(default)` — у ключа есть дефолт (восстановлен + persist); + /// `None` — дефолта нет (reset неприменим, стор не трогаем — без ложного `Changed("")`). + pub fn reset(&mut self, key: &str) -> io::Result> { + match default_for(key) { Some(v) => { self.map.insert(key.to_string(), v.clone()); + self.persist()?; + Ok(Some(v)) } - None => { - self.map.remove(key); - } + None => Ok(None), } - self.persist()?; - Ok(self.map.get(key).cloned()) } pub fn list(&self, prefix: &str) -> Vec { @@ -94,12 +115,16 @@ mod tests { (d, l) } + fn s(v: &str) -> SettingValue { + SettingValue::Str(v.to_string()) + } + #[test] fn seeds_defaults_when_empty() { let (_d, l) = layout(); - let s = Store::load_or_seed(l.clone()).unwrap(); - assert_eq!(s.get("ui.theme"), Some("auto")); - assert_eq!(s.get("ui.units"), Some("metric")); + let st = Store::load_or_seed(l.clone()).unwrap(); + assert_eq!(st.get("ui.theme"), Some(&s("auto"))); + assert_eq!(st.get("ui.units"), Some(&s("metric"))); assert!(l.settings().join("settings.json").exists()); } @@ -107,28 +132,39 @@ mod tests { fn set_get_persists_across_reload() { let (_d, l) = layout(); { - let mut s = Store::load_or_seed(l.clone()).unwrap(); - s.set("ui.theme", "night").unwrap(); + let mut st = Store::load_or_seed(l.clone()).unwrap(); + st.set("ui.theme", s("night")).unwrap(); + st.set("ui.brightness", SettingValue::Int(80)).unwrap(); } - let s2 = Store::load_or_seed(l.clone()).unwrap(); - assert_eq!(s2.get("ui.theme"), Some("night")); + let st2 = Store::load_or_seed(l.clone()).unwrap(); + assert_eq!(st2.get("ui.theme"), Some(&s("night"))); + assert_eq!(st2.get("ui.brightness"), Some(&SettingValue::Int(80))); } #[test] fn reset_restores_default() { let (_d, l) = layout(); - let mut s = Store::load_or_seed(l).unwrap(); - s.set("ui.theme", "night").unwrap(); - s.reset("ui.theme").unwrap(); - assert_eq!(s.get("ui.theme"), Some("auto")); + let mut st = Store::load_or_seed(l).unwrap(); + st.set("ui.theme", s("night")).unwrap(); + assert_eq!(st.reset("ui.theme").unwrap(), Some(s("auto"))); + assert_eq!(st.get("ui.theme"), Some(&s("auto"))); + } + + #[test] + fn reset_unknown_key_is_noop() { + let (_d, l) = layout(); + let mut st = Store::load_or_seed(l).unwrap(); + st.set("net.proxy", s("x")).unwrap(); + assert_eq!(st.reset("net.proxy").unwrap(), None); // нет дефолта — no-op + assert_eq!(st.get("net.proxy"), Some(&s("x"))); // значение не тронуто } #[test] fn list_by_prefix() { let (_d, l) = layout(); - let mut s = Store::load_or_seed(l).unwrap(); - s.set("net.proxy", "x").unwrap(); - let ui = s.list("ui."); + let mut st = Store::load_or_seed(l).unwrap(); + st.set("net.proxy", s("x")).unwrap(); + let ui = st.list("ui."); assert!(ui.contains(&"ui.theme".to_string())); assert!(!ui.contains(&"net.proxy".to_string())); } @@ -136,7 +172,7 @@ mod tests { #[test] fn unknown_key_is_none() { let (_d, l) = layout(); - let s = Store::load_or_seed(l).unwrap(); - assert_eq!(s.get("nope"), None); + let st = Store::load_or_seed(l).unwrap(); + assert_eq!(st.get("nope"), None); } } diff --git a/crates/shturman-ipc/src/error.rs b/crates/shturman-ipc/src/error.rs index 87d136b..a50d030 100644 --- a/crates/shturman-ipc/src/error.rs +++ b/crates/shturman-ipc/src/error.rs @@ -17,4 +17,7 @@ pub enum Error { ReadOnly(String), InvalidArgument(String), Unsupported(String), + /// Внутренний сбой сервиса (напр. отказ записи в `/data`) — НЕ путать с `InvalidArgument` + /// (плохой ввод клиента). Аддитивно к набору ipc §2 (синхронизировать ipc.md при реализации). + Failed(String), }