From be196f9e4b8d6eeee4d2f4468cd6c8d1d4316099 Mon Sep 17 00:00:00 2001 From: ddidderr Date: Sat, 16 May 2026 11:49:01 +0200 Subject: [PATCH] refactor: type game availability state Game::availability used string labels that were carried through persisted library data, protocol summaries, and the Tauri-facing game payload. That allowed invalid states to exist and required legacy summary conversion code to defensively map strings back into protocol availability values. Move Availability to lanspread-db and re-export it from lanspread-proto so the persisted Game type and wire GameSummary type share one serde enum. The JSON spelling stays Ready, Downloading, or LocalOnly, so the serialized shape does not change for current library indexes or peer payloads. Add typed helpers for sentinel-derived download state. Game::set_downloaded keeps downloaded and Ready/LocalOnly in lockstep and intentionally collapses non-ready local state, including Downloading, back to LocalOnly. That matches the current local-summary contract where active operations are suppressed instead of advertised as Downloading. Game::normalized_availability keeps the legacy Game-to-summary path from publishing an inconsistent Ready value when downloaded is false. Update the follow-up status note so typed availability is no longer listed as open work. Test Plan: - just fmt - just test - just clippy - just build Refs: none --- FOLLOW_UP_2.md | 7 +- crates/lanspread-compat/src/eti.rs | 4 +- crates/lanspread-db/src/db.rs | 115 ++++++++++++++++-- crates/lanspread-peer/src/local_games.rs | 19 +-- crates/lanspread-peer/src/peer_db.rs | 25 ++-- crates/lanspread-peer/src/remote_peer.rs | 19 +-- crates/lanspread-proto/src/lib.rs | 9 +- .../src-tauri/src/lib.rs | 23 +--- 8 files changed, 128 insertions(+), 93 deletions(-) diff --git a/FOLLOW_UP_2.md b/FOLLOW_UP_2.md index e853d51..4e8ce06 100644 --- a/FOLLOW_UP_2.md +++ b/FOLLOW_UP_2.md @@ -37,15 +37,14 @@ The OperationGuard ordering fix in `handlers.rs` is the structurally most import missing-sentinel, and local-path cases. - TempDir test helper: peer tests now share `test_support::TempDir` instead of carrying per-module copies. +- Typed availability: `lanspread-db::Game::availability` now uses the shared + serde enum, with `lanspread-proto` re-exporting it for wire summaries. The + JSON spelling stays `"Ready" | "Downloading" | "LocalOnly"`. ## Still open ### Code hygiene -#### 9. `availability` field is `String`, not a typed enum - -`lanspread-db::Game::availability: String` with `AVAILABILITY_*` consts works but allows invalid values. `remote_peer::summary_from_game` already has a defensive fallback for unknown strings. A typed enum + serde would be tighter, but is a wire-format change — coordinate if/when other peers are upgraded. - #### 10. `save_library_index` non-atomic `local_games.rs:141-148` writes without temp+rename. Corrupt index → next scan rebuilds, so blast radius is small. Match the intent-log atomic pattern if touching that module. diff --git a/crates/lanspread-compat/src/eti.rs b/crates/lanspread-compat/src/eti.rs index 9fbd191..5ed6b6f 100644 --- a/crates/lanspread-compat/src/eti.rs +++ b/crates/lanspread-compat/src/eti.rs @@ -1,6 +1,6 @@ use std::path::Path; -use lanspread_db::db::{AVAILABILITY_LOCAL_ONLY, Game}; +use lanspread_db::db::{Availability, Game}; use serde::{Deserialize, Serialize}; use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; @@ -63,7 +63,7 @@ impl From for Game { size: (eti_game.game_size * 1024.0 * 1024.0 * 1024.0) as u64, downloaded: false, installed: false, - availability: AVAILABILITY_LOCAL_ONLY.to_string(), + availability: Availability::LocalOnly, eti_game_version: None, local_version: None, peer_count: 0, // ETI games start with 0 peers until peer system discovers them diff --git a/crates/lanspread-db/src/db.rs b/crates/lanspread-db/src/db.rs index 776399a..97468f0 100644 --- a/crates/lanspread-db/src/db.rs +++ b/crates/lanspread-db/src/db.rs @@ -5,10 +5,6 @@ use std::{collections::HashMap, fmt, path::Path}; use serde::{Deserialize, Serialize}; -pub const AVAILABILITY_READY: &str = "Ready"; -pub const AVAILABILITY_DOWNLOADING: &str = "Downloading"; -pub const AVAILABILITY_LOCAL_ONLY: &str = "LocalOnly"; - /// Read version from version.ini file /// # Errors /// Returns error if file cannot be read or parsed @@ -34,8 +30,30 @@ pub fn read_version_from_ini(game_dir: &Path) -> eyre::Result> { } } -fn default_availability() -> String { - AVAILABILITY_LOCAL_ONLY.to_string() +#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum Availability { + Ready, + /// Wire-compatible transitional state. Local library summaries currently + /// suppress active operations instead of advertising this value. + Downloading, + #[default] + LocalOnly, +} + +impl Availability { + #[must_use] + pub fn from_downloaded(downloaded: bool) -> Self { + if downloaded { + Self::Ready + } else { + Self::LocalOnly + } + } + + #[must_use] + pub fn is_downloaded(&self) -> bool { + matches!(self, Self::Ready) + } } /// A game @@ -66,8 +84,8 @@ pub struct Game { #[serde(default)] pub installed: bool, /// Backend-reported availability state for this game's local or peer summary. - #[serde(default = "default_availability")] - pub availability: String, + #[serde(default)] + pub availability: Availability, /// ETI game version from version.ini (YYYYMMDD format) (server) pub eti_game_version: Option, /// Local game version from version.ini (YYYYMMDD format) @@ -76,6 +94,26 @@ pub struct Game { pub peer_count: u32, } +impl Game { + /// Sets sentinel-derived download state and collapses any non-ready + /// availability, including `Downloading`, back to `LocalOnly`. + pub fn set_downloaded(&mut self, downloaded: bool) { + self.downloaded = downloaded; + self.availability = Availability::from_downloaded(downloaded); + } + + #[must_use] + pub fn normalized_availability(&self) -> Availability { + if self.downloaded { + Availability::Ready + } else if self.availability.is_downloaded() { + Availability::LocalOnly + } else { + self.availability.clone() + } + } +} + impl fmt::Debug for Game { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( @@ -166,9 +204,8 @@ impl GameDB { pub fn set_all_uninstalled(&mut self) { for game in self.games.values_mut() { - game.downloaded = false; + game.set_downloaded(false); game.installed = false; - game.availability = AVAILABILITY_LOCAL_ONLY.to_string(); game.local_version = None; } } @@ -219,10 +256,30 @@ impl fmt::Debug for GameFileDescription { mod tests { use serde_json::json; - use super::{AVAILABILITY_LOCAL_ONLY, Game, GameFileDescription}; + use super::{Availability, Game, GameFileDescription}; + + fn game_fixture() -> Game { + Game { + id: "aoe2".to_string(), + name: "Age of Empires II".to_string(), + description: "desc".to_string(), + release_year: "1999".to_string(), + publisher: "Microsoft".to_string(), + max_players: 8, + version: "1.0".to_string(), + genre: "RTS".to_string(), + size: 123_456, + downloaded: false, + installed: false, + availability: Availability::LocalOnly, + eti_game_version: None, + local_version: None, + peer_count: 0, + } + } #[test] - fn installed_defaults_to_false_when_missing() { + fn missing_client_state_defaults_to_false_and_local_only() { let raw = json!({ "id": "aoe2", "name": "Age of Empires II", @@ -246,7 +303,39 @@ mod tests { !game.installed, "missing installed flag should default to false" ); - assert_eq!(game.availability, AVAILABILITY_LOCAL_ONLY); + assert_eq!(game.availability, Availability::LocalOnly); + } + + #[test] + fn download_state_helpers_keep_ready_in_lockstep() { + assert_eq!(Availability::from_downloaded(true), Availability::Ready); + assert_eq!( + Availability::from_downloaded(false), + Availability::LocalOnly + ); + + let mut game = game_fixture(); + game.set_downloaded(true); + assert!(game.downloaded); + assert_eq!(game.availability, Availability::Ready); + + game.set_downloaded(false); + assert!(!game.downloaded); + assert_eq!(game.availability, Availability::LocalOnly); + + game.availability = Availability::Ready; + assert_eq!(game.normalized_availability(), Availability::LocalOnly); + + game.availability = Availability::Downloading; + game.set_downloaded(false); + assert!(!game.downloaded); + assert_eq!(game.availability, Availability::LocalOnly); + + game.availability = Availability::Downloading; + assert_eq!(game.normalized_availability(), Availability::Downloading); + + game.downloaded = true; + assert_eq!(game.normalized_availability(), Availability::Ready); } #[test] diff --git a/crates/lanspread-peer/src/local_games.rs b/crates/lanspread-peer/src/local_games.rs index 39d5993..7217cfd 100644 --- a/crates/lanspread-peer/src/local_games.rs +++ b/crates/lanspread-peer/src/local_games.rs @@ -8,14 +8,7 @@ use std::{ time::{SystemTime, UNIX_EPOCH}, }; -use lanspread_db::db::{ - AVAILABILITY_DOWNLOADING, - AVAILABILITY_LOCAL_ONLY, - AVAILABILITY_READY, - Game, - GameDB, - GameFileDescription, -}; +use lanspread_db::db::{Game, GameDB, GameFileDescription}; use lanspread_proto::{Availability, GameSummary}; use serde::{Deserialize, Serialize}; use tokio::io::AsyncWriteExt; @@ -436,21 +429,13 @@ pub(crate) fn game_from_summary(summary: &GameSummary) -> Game { size: summary.size, downloaded: summary.downloaded, installed: summary.installed, - availability: availability_label(&summary.availability).to_string(), + availability: summary.availability.clone(), eti_game_version: summary.eti_version.clone(), local_version: summary.eti_version.clone(), peer_count: 0, } } -pub(crate) fn availability_label(availability: &Availability) -> &'static str { - match availability { - Availability::Ready => AVAILABILITY_READY, - Availability::Downloading => AVAILABILITY_DOWNLOADING, - Availability::LocalOnly => AVAILABILITY_LOCAL_ONLY, - } -} - struct IndexUpdate { summary: Option, changed: bool, diff --git a/crates/lanspread-peer/src/peer_db.rs b/crates/lanspread-peer/src/peer_db.rs index ec72d04..0c71b42 100644 --- a/crates/lanspread-peer/src/peer_db.rs +++ b/crates/lanspread-peer/src/peer_db.rs @@ -7,10 +7,12 @@ use std::{ time::{Duration, Instant}, }; -use lanspread_db::db::{AVAILABILITY_READY, Game, GameFileDescription}; -use lanspread_proto::{Availability, GameSummary, LibraryDelta, LibrarySnapshot}; +#[cfg(test)] +use lanspread_db::db::Availability; +use lanspread_db::db::{Game, GameFileDescription}; +use lanspread_proto::{GameSummary, LibraryDelta, LibrarySnapshot}; -use crate::{library::compute_library_digest, local_games::availability_label}; +use crate::library::compute_library_digest; pub type PeerId = String; /// Information about a discovered peer. @@ -292,11 +294,9 @@ impl PeerGameDB { existing.size = game.size; } if game_is_ready(game) { - existing.downloaded = true; - existing.availability = AVAILABILITY_READY.to_string(); + existing.set_downloaded(true); } else if !existing.downloaded { - existing.availability = - availability_label(&game.availability).to_string(); + existing.availability = game.availability.clone(); } if game.installed { existing.installed = true; @@ -305,7 +305,6 @@ impl PeerGameDB { .or_insert_with(|| { let mut game_clone = summary_to_game(game); game_clone.peer_count = *peer_counts.get(&game.id).unwrap_or(&0); - game_clone.downloaded = game_is_ready(game); game_clone }); } @@ -728,7 +727,7 @@ fn create_peer_whitelist(peer_scores: HashMap) -> Vec bool { - summary.availability == Availability::Ready + summary.availability.is_downloaded() } fn summary_to_game(summary: &GameSummary) -> Game { @@ -746,9 +745,9 @@ fn summary_to_game(summary: &GameSummary) -> Game { version: "1.0".to_string(), genre: String::new(), size: summary.size, - downloaded: summary.downloaded, + downloaded: summary.availability.is_downloaded(), installed: summary.installed, - availability: availability_label(&summary.availability).to_string(), + availability: summary.availability.clone(), eti_game_version, local_version: None, peer_count: 0, @@ -759,8 +758,6 @@ fn summary_to_game(summary: &GameSummary) -> Game { mod tests { use std::net::SocketAddr; - use lanspread_db::db::AVAILABILITY_LOCAL_ONLY; - use super::*; fn addr(port: u16) -> SocketAddr { @@ -824,7 +821,7 @@ mod tests { assert_eq!(games.len(), 1); assert_eq!(games[0].peer_count, 0); assert!(!games[0].downloaded); - assert_eq!(games[0].availability, AVAILABILITY_LOCAL_ONLY); + assert_eq!(games[0].availability, Availability::LocalOnly); assert_eq!(games[0].eti_game_version, None); assert!(db.peers_with_game("game").is_empty()); diff --git a/crates/lanspread-peer/src/remote_peer.rs b/crates/lanspread-peer/src/remote_peer.rs index 868f2f0..548f207 100644 --- a/crates/lanspread-peer/src/remote_peer.rs +++ b/crates/lanspread-peer/src/remote_peer.rs @@ -2,13 +2,8 @@ use std::{collections::HashMap, net::SocketAddr, sync::Arc}; -use lanspread_db::db::{ - AVAILABILITY_DOWNLOADING, - AVAILABILITY_LOCAL_ONLY, - AVAILABILITY_READY, - Game, -}; -use lanspread_proto::{Availability, GameSummary}; +use lanspread_db::db::Game; +use lanspread_proto::GameSummary; use tokio::sync::RwLock; use crate::{ @@ -31,14 +26,6 @@ pub async fn ensure_peer_id_for_addr( } pub fn summary_from_game(game: &Game) -> GameSummary { - let availability = match game.availability.as_str() { - AVAILABILITY_READY => Availability::Ready, - AVAILABILITY_DOWNLOADING => Availability::Downloading, - AVAILABILITY_LOCAL_ONLY => Availability::LocalOnly, - _ if game.downloaded => Availability::Ready, - _ => Availability::LocalOnly, - }; - GameSummary { id: game.id.clone(), name: game.name.clone(), @@ -47,7 +34,7 @@ pub fn summary_from_game(game: &Game) -> GameSummary { installed: game.installed, eti_version: game.eti_game_version.clone(), manifest_hash: 0, - availability, + availability: game.normalized_availability(), } } diff --git a/crates/lanspread-proto/src/lib.rs b/crates/lanspread-proto/src/lib.rs index 6d5b46f..966e25e 100644 --- a/crates/lanspread-proto/src/lib.rs +++ b/crates/lanspread-proto/src/lib.rs @@ -4,14 +4,7 @@ use serde::{Deserialize, Serialize}; pub const PROTOCOL_VERSION: u32 = 2; -#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)] -pub enum Availability { - Ready, - /// Wire-compatible transitional state. Local library summaries currently - /// suppress active operations instead of advertising this value. - Downloading, - LocalOnly, -} +pub use lanspread_db::db::Availability; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)] pub struct GameSummary { diff --git a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs index e28cc42..864f557 100644 --- a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs +++ b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs @@ -9,13 +9,7 @@ use std::{ use eyre::bail; use lanspread_compat::eti::get_games; -use lanspread_db::db::{ - AVAILABILITY_LOCAL_ONLY, - AVAILABILITY_READY, - Game, - GameDB, - GameFileDescription, -}; +use lanspread_db::db::{Game, GameDB, GameFileDescription}; use lanspread_peer::{ ActiveOperation, ActiveOperationKind, @@ -372,15 +366,10 @@ fn update_game_installation_state(game: &mut Game, games_root: &Path) { } let downloaded = game_path.join("version.ini").is_file(); - game.downloaded = downloaded; + game.set_downloaded(downloaded); let installed = local_install_is_present(&game_path); game.installed = installed; - game.availability = if downloaded { - AVAILABILITY_READY.to_string() - } else { - AVAILABILITY_LOCAL_ONLY.to_string() - }; // Size stays anchored to bundled game.db; skip expensive recalculation. @@ -687,11 +676,8 @@ async fn update_local_games_in_db(local_games: Vec, app: AppHandle) { // Update installation status for games that exist locally for local_game in &local_games { if let Some(existing_game) = game_db.get_mut_game_by_id(&local_game.id) { - existing_game.downloaded = local_game.downloaded; + existing_game.set_downloaded(local_game.downloaded); existing_game.installed = local_game.installed; - existing_game - .availability - .clone_from(&local_game.availability); existing_game .local_version .clone_from(&local_game.local_version); @@ -707,9 +693,8 @@ async fn update_local_games_in_db(local_games: Vec, app: AppHandle) { "Game {} no longer exists locally, marking as uninstalled", game.id ); - game.downloaded = false; + game.set_downloaded(false); game.installed = false; - game.availability = AVAILABILITY_LOCAL_ONLY.to_string(); game.local_version = None; } }