From 76b7c126c2fad2842c8ea16b00da29debdeb8c45 Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Tue, 24 Mar 2026 15:02:14 +0100 Subject: [PATCH] refactor: replace absolute URLs with route-based cover URLs and generalize caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remplacer les appels à `covers_absolute_url_for` par `covers_route_for` pour générer des URLs relatives basées sur les routes - Introduire le trait `CoverCacheable` pour généraliser le cache des covers (Album, Playlist, Artist, Track) - Remplacer les fonctions spécifiques (`cache_album_covers`, etc.) par une fonction générique `cache_covers` - Simplifier le code en unifiant la logique de mise en cache des covers - Corriger l'initialisation de `PMO_SERVER_URL` pour utiliser `base_url()` (incluant le port) au lieu d'une URL brute --- pmoplaylist/src/api.rs | 2 +- pmoplaylist/src/handle/read.rs | 4 +- pmoqobuz/src/api_rest.rs | 2 +- pmoqobuz/src/source.rs | 142 ++++++++++++++++++++++++--------- pmoserver/src/server.rs | 16 ++-- 5 files changed, 118 insertions(+), 48 deletions(-) diff --git a/pmoplaylist/src/api.rs b/pmoplaylist/src/api.rs index c8f6289c..4ad29953 100644 --- a/pmoplaylist/src/api.rs +++ b/pmoplaylist/src/api.rs @@ -517,7 +517,7 @@ fn playlist_track_to_response( } fn cover_url_from_pk(pk: &str) -> String { - pmocache::covers_absolute_url_for(pk, None) + pmocache::covers_route_for(pk, None) } fn normalize_cover_pk(input: Option) -> Option { diff --git a/pmoplaylist/src/handle/read.rs b/pmoplaylist/src/handle/read.rs index e3d4e672..c07e325c 100644 --- a/pmoplaylist/src/handle/read.rs +++ b/pmoplaylist/src/handle/read.rs @@ -182,7 +182,7 @@ impl ReadHandle { let _remaining = self.remaining().await?; // Convertir cover_pk en URL si présent - let album_art = cover_pk.map(|pk| pmocache::covers_absolute_url_for(&pk, None)); + let album_art = cover_pk.map(|pk| pmocache::covers_route_for(&pk, None)); Ok(Container { id: self.playlist.id.clone(), @@ -253,7 +253,7 @@ impl ReadHandle { let track_number = meta.get_track_number().await.ok().flatten(); let cover_pk = meta.get_cover_pk().await.ok().flatten(); let cover_url = if let Some(pk) = cover_pk.as_ref() { - Some(pmocache::covers_absolute_url_for(pk, None)) + Some(pmocache::covers_route_for(pk, None)) } else { meta.get_cover_url().await.ok().flatten() }; diff --git a/pmoqobuz/src/api_rest.rs b/pmoqobuz/src/api_rest.rs index d3f568cd..e1bcb500 100644 --- a/pmoqobuz/src/api_rest.rs +++ b/pmoqobuz/src/api_rest.rs @@ -299,7 +299,7 @@ async fn cache_album_image(mut album: Album, cover_cache: &Arc if let Some(ref image_url) = album.image { match cover_cache.add_from_url(image_url, None).await { Ok(pk) => { - album.image_cached = Some(pmocache::covers_absolute_url_for(&pk, None)); + album.image_cached = Some(pmocache::covers_route_for(&pk, None)); } Err(e) => { tracing::warn!("Failed to cache album image: {}", e); diff --git a/pmoqobuz/src/source.rs b/pmoqobuz/src/source.rs index 6b47fc67..e73ec244 100644 --- a/pmoqobuz/src/source.rs +++ b/pmoqobuz/src/source.rs @@ -19,6 +19,39 @@ use std::time::{Duration, SystemTime}; /// TTL pour les playlists d'albums (7 jours) const ALBUM_PLAYLIST_TTL: Duration = Duration::from_secs(7 * 24 * 3600); +/// Trait pour les types dont on peut cacher la cover image. +trait CoverCacheable { + fn image_url(&self) -> Option<&str>; + fn set_image_cached(&mut self, url: String); +} + +impl CoverCacheable for crate::models::Album { + fn image_url(&self) -> Option<&str> { self.image.as_deref() } + fn set_image_cached(&mut self, url: String) { self.image_cached = Some(url); } +} + +impl CoverCacheable for crate::models::Playlist { + fn image_url(&self) -> Option<&str> { self.image.as_deref() } + fn set_image_cached(&mut self, url: String) { self.image_cached = Some(url); } +} + +impl CoverCacheable for crate::models::Artist { + fn image_url(&self) -> Option<&str> { self.image.as_deref() } + fn set_image_cached(&mut self, url: String) { self.image_cached = Some(url); } +} + +/// Pour Track, la cover est celle de l'album. +impl CoverCacheable for crate::models::Track { + fn image_url(&self) -> Option<&str> { + self.album.as_ref()?.image.as_deref() + } + fn set_image_cached(&mut self, url: String) { + if let Some(ref mut album) = self.album { + album.image_cached = Some(url); + } + } +} + /// Default image for Qobuz (300x300 WebP, embedded in binary) const DEFAULT_IMAGE: &[u8] = include_bytes!("../assets/default.webp"); @@ -853,20 +886,50 @@ impl QobuzSource { } } - /// Cache les covers d'une liste d'albums en parallèle. - /// Retourne les albums avec `image_cached` mis à jour si la cover a pu être mise en cache. - async fn cache_album_covers(&self, albums: Vec) -> Vec { - let futs: Vec<_> = albums.into_iter().map(|mut album| { + /// Cache les covers d'une liste d'items en parallèle (générique via `CoverCacheable`). + async fn cache_covers(&self, items: Vec) -> Vec + where + T: CoverCacheable + Send + 'static, + { + let futs: Vec<_> = items.into_iter().map(|mut item| { let source = self.clone(); async move { - if let Some(ref image_url) = album.image.clone() { - if let Ok(pk) = source.inner.cache_manager.cache_cover(image_url).await { + if let Some(image_url) = item.image_url().map(str::to_string) { + if let Ok(pk) = source.inner.cache_manager.cache_cover(&image_url).await { if let Ok(url) = source.inner.cache_manager.cover_url(&pk, None) { - album.image_cached = Some(url); + item.set_image_cached(url); } } } - album + item + } + }).collect(); + tokio::task::JoinSet::from_iter(futs).join_all().await + } + + async fn cache_album_covers(&self, albums: Vec) -> Vec { + self.cache_covers(albums).await + } + + async fn cache_playlist_covers(&self, playlists: Vec) -> Vec { + self.cache_covers(playlists).await + } + + /// Cache les covers d'une liste de tracks en parallèle (via l'image de l'album). + async fn cache_track_covers(&self, tracks: Vec) -> Vec { + let futs: Vec<_> = tracks.into_iter().map(|mut track| { + let source = self.clone(); + async move { + if let Some(ref mut album) = track.album { + if let Some(ref image_url) = album.image.clone() { + if let Ok(pk) = source.inner.cache_manager.cache_cover(image_url).await { + if let Ok(url) = source.inner.cache_manager.cover_url(&pk, None) { + album.image_cached = Some(url); + } + } + } + } + track } }).collect(); tokio::task::JoinSet::from_iter(futs) @@ -874,19 +937,19 @@ impl QobuzSource { .await } - /// Cache les covers d'une liste de playlists en parallèle. - async fn cache_playlist_covers(&self, playlists: Vec) -> Vec { - let futs: Vec<_> = playlists.into_iter().map(|mut playlist| { + /// Cache les covers d'une liste d'artistes en parallèle. + async fn cache_artist_covers(&self, artists: Vec) -> Vec { + let futs: Vec<_> = artists.into_iter().map(|mut artist| { let source = self.clone(); async move { - if let Some(ref image_url) = playlist.image.clone() { + if let Some(ref image_url) = artist.image.clone() { if let Ok(pk) = source.inner.cache_manager.cache_cover(image_url).await { if let Ok(url) = source.inner.cache_manager.cover_url(&pk, None) { - playlist.image_cached = Some(url); + artist.image_cached = Some(url); } } } - playlist + artist } }).collect(); tokio::task::JoinSet::from_iter(futs) @@ -932,6 +995,7 @@ impl QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let tracks = self.cache_covers(tracks).await; let items: Vec = tracks .into_iter() .filter_map(|track| track.to_didl_item("qobuz:favorites:tracks").ok()) @@ -949,23 +1013,21 @@ impl QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let artists = self.cache_covers(artists).await; let containers: Vec = artists .into_iter() - .filter_map(|artist| { - // Créer un container pour chaque artiste - Some(Container { - id: format!("qobuz:artist:{}", artist.id), - parent_id: "qobuz:favorites:artists".to_string(), - restricted: Some("1".to_string()), - child_count: None, - searchable: Some("1".to_string()), - title: artist.name.clone(), - class: "object.container".to_string(), - artist: Some(artist.name.clone()), - album_art: artist.image_cached.clone().or_else(|| artist.image.clone()), - containers: vec![], - items: vec![], - }) + .map(|artist| Container { + id: format!("qobuz:artist:{}", artist.id), + parent_id: "qobuz:favorites:artists".to_string(), + restricted: Some("1".to_string()), + child_count: None, + searchable: Some("1".to_string()), + title: artist.name.clone(), + class: "object.container".to_string(), + artist: Some(artist.name.clone()), + album_art: artist.image_cached.clone(), + containers: vec![], + items: vec![], }) .collect(); @@ -1165,6 +1227,7 @@ impl QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let artists = self.cache_covers(artists).await; let containers: Vec = artists .into_iter() .map(|artist| Container { @@ -1176,7 +1239,7 @@ impl QobuzSource { title: artist.name.clone(), class: "object.container".to_string(), artist: Some(artist.name.clone()), - album_art: artist.image_cached.clone().or_else(|| artist.image.clone()), + album_art: artist.image_cached.clone(), containers: vec![], items: vec![], }) @@ -1194,6 +1257,7 @@ impl QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let playlists = self.cache_covers(playlists).await; let parent_id = format!("qobuz:discover:playlists:{}", tag); let containers: Vec = playlists .into_iter() @@ -1559,7 +1623,6 @@ impl MusicSource for QobuzSource { } ObjectIdType::Playlist(playlist_id) => { - // Get tracks in playlist let tracks = self .inner .client @@ -1567,6 +1630,7 @@ impl MusicSource for QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let tracks = self.cache_covers(tracks).await; let items: Vec = tracks .into_iter() .filter_map(|track| { @@ -1580,7 +1644,6 @@ impl MusicSource for QobuzSource { } ObjectIdType::Artist(artist_id) => { - // Get albums by artist let albums = self .inner .client @@ -1588,6 +1651,7 @@ impl MusicSource for QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let albums = self.cache_covers(albums).await; let containers: Vec = albums .into_iter() .filter_map(|album| { @@ -1738,6 +1802,7 @@ impl MusicSource for QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let all_tracks = self.cache_covers(all_tracks).await; let items: Vec = all_tracks .into_iter() .skip(offset) @@ -1757,15 +1822,16 @@ impl MusicSource for QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; - // Convert albums to containers and tracks to items - let containers: Vec = results - .albums + let (albums, tracks) = tokio::join!( + self.cache_covers(results.albums), + self.cache_covers(results.tracks), + ); + let containers: Vec = albums .into_iter() .filter_map(|album| album.to_didl_container("qobuz").ok()) .collect(); - let items: Vec = results - .tracks + let items: Vec = tracks .into_iter() .filter_map(|track| track.to_didl_item("qobuz").ok()) .collect(); @@ -1979,6 +2045,7 @@ impl MusicSource for QobuzSource { .await .map_err(|e| MusicSourceError::PlaylistError(e.to_string()))?; + let playlists = self.cache_covers(playlists).await; let containers: Vec = playlists .into_iter() .filter_map(|playlist| playlist.to_didl_container("qobuz").ok()) @@ -2061,6 +2128,7 @@ impl MusicSource for QobuzSource { .await .map_err(|e| MusicSourceError::BrowseError(e.to_string()))?; + let albums = self.cache_covers(albums).await; let containers: Vec = albums .into_iter() .skip(offset) diff --git a/pmoserver/src/server.rs b/pmoserver/src/server.rs index bd9e9852..b43169ae 100644 --- a/pmoserver/src/server.rs +++ b/pmoserver/src/server.rs @@ -116,17 +116,12 @@ impl Server { let base_url = base_url.into(); - // Initialiser PMO_SERVER_URL pour que tous les caches puissent construire des URLs absolues - // sans avoir besoin de propager base_url manuellement. - // SAFETY: appelé une seule fois au démarrage du serveur, avant tout thread concurrent. - unsafe { std::env::set_var("PMO_SERVER_URL", &base_url) }; - // Créer le router initial avec l'endpoint de registre let registry_route = Router::new() .route("/api/registry", get(get_api_registry)) .with_state(api_registry.clone()); - Self { + let server = Self { name: name.into(), base_url, http_port, @@ -136,7 +131,14 @@ impl Server { log_state: None, api_registry, shutdown_token: CancellationToken::new(), - } + }; + + // Initialiser PMO_SERVER_URL avec l'URL complète (incluant le port). + // base_url() normalise l'URL en ajoutant le port si absent. + // SAFETY: appelé une seule fois au démarrage du serveur, avant tout thread concurrent. + unsafe { std::env::set_var("PMO_SERVER_URL", server.base_url()) }; + + server } pub fn new_configured() -> Self {