diff --git a/.gitignore b/.gitignore index 1f472a01..9469b02d 100644 --- a/.gitignore +++ b/.gitignore @@ -49,3 +49,4 @@ RF.json RF_old.json .claude/ .claude.old +Kilo-session.md diff --git a/.kilo/plans/1775285337131-neon-mountain.md b/.kilo/plans/1775285337131-neon-mountain.md new file mode 100644 index 00000000..e04f00ef --- /dev/null +++ b/.kilo/plans/1775285337131-neon-mountain.md @@ -0,0 +1,225 @@ +# Évaluation du plan : centraliser_base_url_axum_middleware + +## Résumé de l'audit + +Le plan est **bien pensé et cohérent**. Il identifie correctement le problème et la solution. Cependant, j'ai identifié plusieurs points nécessitant des amendements. + +--- + +## Points validés (conformes au code actuel) + +1. **Problème bien identifié** : URLs hardcodées avec IP locale (`PMO_SERVER_URL`) retournées au frontend via reverse proxy. + +2. **`get_request_base_url` existe déjà** à `pmoserver/src/lib.rs:199` — pas besoin de la recréer. + +3. **`covers_route_for` existe déjà** dans `pmocache/src/lib.rs:149`. + +4. **`covers_absolute_url_for` utilisée dans les contextes UPnP** : + - `pmoupnp/src/cache_registry.rs:57` + - `pmoradiofrance/src/metadata_cache.rs:263` + - `pmoparadise/src/source.rs:216` + - `pmoaudio-ext/src/sinks/streaming_icyflac_sink.rs:91` + +5. **Route audio correcte** : `/audio/tracks/{pk}` (pas `/audio/flac/{pk}`). + +6. **Architecture du Server** : Les routes sont construites dynamiquement via `Arc>`. Le layer devra être ajouté dans la construction du router, pas après. + +--- + +## Points à amender + +### 1. Ajout du layer dans le Server + +Le plan suggère d'ajouter le layer "dans `server.rs`" mais la structure du router est complexe : +- Les routes sont dynamiques (`RwLock`) +- Le router final est un fallback qui délègue + +**Correction** : Ajouter le layer directement lors de la création du `registry_route` initial (ligne 120-122) : + +```rust +let registry_route = Router::new() + .route("/api/registry", get(get_api_registry)) + .with_state(api_registry.clone()) + .layer(base_url_layer()); // ← ici +``` + +### 2. Comportement requis pour LAN vs WAN + +Le middleware doit supporter les deux cas d'usage : + +- **LAN (sans reverse proxy)** : Pas de headers `X-Forwarded-*` → utiliser l'adresse IP locale du serveur (`PMO_SERVER_URL`) +- **WAN (via reverse proxy)** : Headers `X-Forwarded-*` présents → utiliser l'URL publique du reverse proxy + +**Important** : `get_request_base_url` dans `pmoserver/src/lib.rs:199` lit déjà ces headers. Le fallback doit être `PMO_SERVER_URL` qui est configuré au démarrage avec l'IP locale. + +### 3. Chemin du middleware dans la pile + +Le plan dit d'appliquer le layer "avant" les autres. En réalité, Tower/Acorn applique les couches dans l'ordre où elles sont ajoutées — le premier layer ajouté est le plus extérieur (exécuté en premier). Le `base_url_layer` doit donc être ajouté en **premier** (le plus intérieur) pour voir les headers nettoyés. + +### 3. Les handlers n'ont PAS besoin de BaseUrl + +Après analyse, **aucun handler** dans le codebase actuel n'appelle `covers_absolute_url_for()` directement pour le frontend. Les `album_art_uri` sont : +- Soit **propagés** depuis les réponses UPnP des media servers (pas des URLs pmomusic) +- Soit **construits en tâche de fond** dans les caches (RadioFrance, RadioParadise) + +**Correction** : Le plan surestime le nombre de handlers à modifier. La vraie question est : d'où viennent les URLs incorrectes ? + +### 4. Source du problème à clarifier + +Les URLs incorrectes ne viennent pas des handlers REST classiques. Elles viennent probablement de : + +**a) Tâches de fond** (background tasks) qui stockent des URLs complètes : +- `pmoradiofrance/src/metadata_cache.rs:263` — construit `covers_absolute_url_for()` dans le cache +- `pmoparadise/src/source.rs:216` — même problème + +**b) API Qobuz** (`pmoqobuz/src/api_rest.rs:302`) — utilise `covers_route_for` (route relative, OK) + +**c) Playlist** (`pmoplaylist/src/handle/read.rs:204,271,355`) — utilise `covers_route_for` (OK) + +### 5. Correction du fallback + +Le plan suggère `localhost:8080` ou `0.0.0.0:8080` comme fallback. Le port doit provenir de la configuration du serveur (`get_server_base_url()` existe déjà dans `pmoserver/src/lib.rs`). + +**Correction** : Le fallback utilise `get_server_base_url()` (disponible via `GLOBAL_SERVER`) : +- En LAN : pas de `X-Forwarded-*` → `get_server_base_url()` → URLs en IP locale +- En WAN : `X-Forwarded-*` présents → URLs en URL publique du reverse proxy + +### 6. Fonction `audio_route_for` pas nécessaire maintenant + +Le plan propose d'ajouter `audio_route_for` dans `pmoaudiocache`. Mais : +- Les fichiers audio sont servis par `pmoaudiocache` lui-même (routes internes) +-Aucune URL audio n'est retournée au frontend via JSON + +**Supprimer** cette étape du plan. + +--- + +## Plan amendé + +### Étape 0 — Audit spécifique (à faire avant implémentation) + +```bash +# Trouver les constructions d'URLs dans les tâches de fond (caches, sources) +grep -rn "covers_absolute_url_for\|PMO_SERVER_URL" --include="*.rs" | grep -v "pmocontrol\|pmoplaylist\|pmoqobuz" + +# Vérifier les URLs dans les réponses JSON des handlers +grep -rn "album_art_uri" --include="*.rs" | grep -E "fn |->" +``` + +Identifier spécifiquement quels endpoints REST retournent des URLs au frontend. + +### Étape 1 — `pmoserver/src/lib.rs` : Ajouter `BaseUrl` + middleware + +```rust +use axum::{extract::Request, middleware::Next, response::Response}; + +#[derive(Debug, Clone)] +pub struct BaseUrl(pub String); + +impl BaseUrl { + pub fn url_for(&self, route: &str) -> String { + debug_assert!(route.starts_with('/'), "route must start with '/'"); + format!("{}{}", self.0.trim_end_matches('/'), route) + } +} + +pub async fn base_url_middleware(mut request: Request, next: Next) -> Response { + // Priorité : 1) X-Forwarded-* (reverse proxy), 2) get_server_base_url() (adresse configurée) + let base = get_request_base_url(request.headers()) + .or_else(|| get_server_base_url()) + .unwrap_or_else(|| { + panic!( + "BaseUrl: impossible de déterminer l'URL de base.\n\ + Configurer PMO_SERVER_URL ou démarrer le serveur avant les handlers HTTP." + ); + }); + tracing::debug!("BaseUrl calculée : {}", base); + request.extensions_mut().insert(BaseUrl(base)); + next.run(request).await +} + +pub fn base_url_layer() -> axum::middleware::FromFnLayer { + axum::middleware::from_fn(base_url_middleware) +} +``` + +**Comportement** : +- Accès LAN (pas de proxy) : `get_server_base_url()` → URLs en IP locale configurée +- Accès WAN (reverse proxy) : `X-Forwarded-*` → URLs en URL publique + +**Note** : Si ni les headers ni le serveur ne sont disponibles, le middleware panic (fail-fast) car c'est une erreur de configuration. + +### Étape 2 — `pmoserver/src/server.rs` : Appliquer le layer + +Dans `Server::new()`, ligne ~120-122 : + +```rust +let registry_route = Router::new() + .route("/api/registry", get(get_api_registry)) + .with_state(api_registry.clone()) + .layer(base_url_layer()); // ← Ajouter ici (couche la plus intérieure) +``` + +### Étape 3 — `pmocache/src/lib.rs` : Renommer sans déprecation + +```rust +// Rename direct - pas de déprecation (soft en cours de dev, pas une library) +pub fn covers_absolute_url_for_upnp(pk: &str, param: Option<&str>) -> String { + // PMO_SERVER_URL contient l'IP locale (LAN) - utilisé uniquement pour UPnP + // Fallback sur get_server_base_url() si dispo, sinon erreur + let base = std::env::var("PMO_SERVER_URL") + .or_else(|_| pmoserver::get_server_base_url().ok_or("PMO_SERVER_URL not set")) + .unwrap_or_else(|e| { + tracing::error!("covers_absolute_url_for_upnp: {}", e); + panic!("BaseUrl non disponible pour UPnP"); + }); + format!("{}{}", base.trim_end_matches('/'), covers_route_for(pk, param)) +} +``` + +### Étape 4 — Mettre à jour les appels UPnP + +```bash +grep -rn "covers_absolute_url_for" --include="*.rs" +``` + +Modifier `pmoupnp/src/cache_registry.rs:57` → `covers_absolute_url_for_upnp` + +### Étape 5 — Tâches de fond : stocker la route, pas l'URL + +**pmoradiofrance/src/metadata_cache.rs:263** : +```rust +// Avant : +let public_url = pmocache::covers_absolute_url_for(&pk, None); + +// Après : stocker la route relative +let album_art_route = pmocache::covers_route_for(&pk, None); +``` + +Le handler REST qui retourne ces métadonnées devra extraire `Extension` et appliquer `base_url.url_for()`. + +**pmoparadise/src/source.rs:216** : Même traitement. + +### Étape 6 — Vérification et tests + +```bash +# Plus d'appels à covers_absolute_url_for dans les contextes HTTP +grep -rn "covers_absolute_url_for" --include="*.rs" | grep -v "pmocache\|pmoupnp" + +# Tests du middleware +cargo test base_url +``` + +--- + +## Questions en suspens + +1. **Fallback avec panic** : Si ni les headers ni le serveur ne sont disponibles, le middleware panic au démarrage avec un message clair (ex: "BaseUrl: configurer PMO_SERVER_URL ou démarrer le serveur avant les handlers HTTP"). + → **Décision utilisateur** : OK, panic avec message clair. + +2. **Reverse proxy avec Authelia** : NPM ajoutera les headers `X-Forwarded-*`. Authelia gère l'authentification separately. Pas de vérification de header supplémentaire nécessaire pour le middleware BaseUrl. + → **Décision** : Pas de vérification supplémentaire. + +--- + +**Le plan original est bon mais surestime le travail.** La correction principale est de clarifier que le problème vient des tâches de fond (background tasks), pas des handlers REST. \ No newline at end of file diff --git a/pmoapp/webapp/package-lock.json b/pmoapp/webapp/package-lock.json index 1c4d70b6..a122e80a 100644 --- a/pmoapp/webapp/package-lock.json +++ b/pmoapp/webapp/package-lock.json @@ -620,9 +620,6 @@ "arm" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -637,9 +634,6 @@ "arm" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -654,9 +648,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -671,9 +662,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -688,9 +676,6 @@ "loong64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -705,9 +690,6 @@ "loong64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -722,9 +704,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -739,9 +718,6 @@ "ppc64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -756,9 +732,6 @@ "riscv64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -773,9 +746,6 @@ "riscv64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -790,9 +760,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -807,9 +774,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -824,9 +788,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ diff --git a/pmoaudio-ext/src/sinks/streaming_icyflac_sink.rs b/pmoaudio-ext/src/sinks/streaming_icyflac_sink.rs index 85b7a5f5..dda47325 100644 --- a/pmoaudio-ext/src/sinks/streaming_icyflac_sink.rs +++ b/pmoaudio-ext/src/sinks/streaming_icyflac_sink.rs @@ -88,7 +88,7 @@ impl IcyClientStream { // Add cover URL if we have a cover_pk if let Some(pk) = &meta.cover_pk { - let cover_url = pmocache::covers_absolute_url_for(pk, None); + let cover_url = pmocache::covers_absolute_url_for_upnp(pk, None); metadata_str.push_str(&format!("StreamUrl='{}';", cover_url)); } else if let Some(url) = &meta.cover_url { // Fallback to external cover URL if no local pk diff --git a/pmoaudio/src/audio_segment.rs b/pmoaudio/src/audio_segment.rs index 5d22acc4..1121c27b 100755 --- a/pmoaudio/src/audio_segment.rs +++ b/pmoaudio/src/audio_segment.rs @@ -323,37 +323,42 @@ impl AudioSegment { /// Convertit l'AudioChunk vers F32 si c'est un chunk audio pub fn to_f32_chunk(&self) -> Option { - self.as_chunk().map(|chunk| chunk.to_f32()) + self.as_chunk() + .map(|chunk: &Arc| chunk.to_f32()) } /// Convertit l'AudioChunk vers I32 si c'est un chunk audio pub fn to_i32_chunk(&self) -> Option { - self.as_chunk().map(|chunk| chunk.to_i32()) + self.as_chunk() + .map(|chunk: &Arc| chunk.to_i32()) } /// Récupère le sample rate du chunk audio pub fn sample_rate(&self) -> Option { - self.as_chunk().map(|chunk| chunk.sample_rate()) + self.as_chunk() + .map(|chunk: &Arc| chunk.sample_rate()) } /// Récupère le nombre de frames du chunk audio pub fn frame_count(&self) -> Option { - self.as_chunk().map(|chunk| chunk.len()) + self.as_chunk().map(|chunk: &Arc| chunk.len()) } /// Récupère le gain en dB du chunk audio pub fn gain_db(&self) -> Option { - self.as_chunk().map(|chunk| chunk.gain_db()) + self.as_chunk() + .map(|chunk: &Arc| chunk.gain_db()) } /// Récupère le type du chunk audio (nom du type: "i32", "f32", etc.) pub fn chunk_type_name(&self) -> Option<&'static str> { - self.as_chunk().map(|chunk| chunk.type_name()) + self.as_chunk() + .map(|chunk: &Arc| chunk.type_name()) } /// Crée un nouveau segment avec le gain modifié (si c'est un chunk audio) pub fn with_gain_db(&self, gain_db: f64) -> Option> { - self.as_chunk().map(|chunk| { + self.as_chunk().map(|chunk: &Arc| { let new_chunk = chunk.set_gain_db(gain_db); Arc::new(Self { order: self.order, @@ -365,7 +370,7 @@ impl AudioSegment { /// Crée un nouveau segment avec le gain ajusté (relatif, si c'est un chunk audio) pub fn adjust_gain_db(&self, delta_db: f64) -> Option> { - self.as_chunk().map(|chunk| { + self.as_chunk().map(|chunk: &Arc| { let new_gain = chunk.gain_db() + delta_db; let new_chunk = chunk.set_gain_db(new_gain); Arc::new(Self { diff --git a/pmoaudio/src/lib.rs b/pmoaudio/src/lib.rs index 5a2bfd72..ac92ce00 100755 --- a/pmoaudio/src/lib.rs +++ b/pmoaudio/src/lib.rs @@ -77,7 +77,6 @@ async fn main() { - **Backpressure** : Channels bounded avec `try_send` pour éviter les blocages - **RwLock** : Pour partage concurrent du compteur [`TimerNode`] "#] -#[cfg(feature = "simd")] // use std::simd::*; // Not actually used in this file, modules import their own simd mod audio_chunk; diff --git a/pmocache/src/lib.rs b/pmocache/src/lib.rs index b1833226..46b63d36 100644 --- a/pmocache/src/lib.rs +++ b/pmocache/src/lib.rs @@ -155,9 +155,15 @@ pub fn covers_route_for(pk: &str, param: Option<&str>) -> String { } /// Retourne l'URL absolue pour une cover via `PMO_SERVER_URL` -pub fn covers_absolute_url_for(pk: &str, param: Option<&str>) -> String { +/// Utilisée uniquement pour les contextes UPnP (LAN) +pub fn covers_absolute_url_for_upnp(pk: &str, param: Option<&str>) -> String { let base = std::env::var("PMO_SERVER_URL") - .unwrap_or_else(|_| "http://localhost:8080".to_string()); + .unwrap_or_else(|_| { + panic!( + "covers_absolute_url_for_upnp: PMO_SERVER_URL non configuré.\n\ + Le serveur doit être initialisé avant toute utilisation UPnP." + ); + }); format!("{}{}", base.trim_end_matches('/'), covers_route_for(pk, param)) } pub use db::{CacheEntry, DB}; diff --git a/pmoparadise/src/source.rs b/pmoparadise/src/source.rs index dbd4f3b3..b2e1cf58 100644 --- a/pmoparadise/src/source.rs +++ b/pmoparadise/src/source.rs @@ -211,9 +211,10 @@ impl RadioParadiseSource { let year = json["year"].as_u64().map(|y| y as u32); // Préférer l'URL de cache si cover_pk est fourni par le pipeline let cover_pk = json["cover_pk"].as_str().map(|s| s.to_string()); + // Stocker la route relative (le handler REST appliquera base_url.url_for()) let cover_url = cover_pk .as_ref() - .map(|pk| pmocache::covers_absolute_url_for(pk, None)) + .map(|pk| pmocache::covers_route_for(pk, None)) .or_else(|| json["cover_url"].as_str().map(|s| s.to_string())) .or_else(|| Some(self.default_cover_url())); diff --git a/pmoradiofrance/src/metadata_cache.rs b/pmoradiofrance/src/metadata_cache.rs index 7da5b86b..db623ea9 100644 --- a/pmoradiofrance/src/metadata_cache.rs +++ b/pmoradiofrance/src/metadata_cache.rs @@ -253,34 +253,28 @@ impl CachedMetadata { } }; - // Tenter de cacher la cover + // Tenter de catcher la cover match cache.add_from_url(&cover_url, Some("radiofrance")).await { Ok(pk) => { - // Construire l'URL publique - // Note: add_from_url() lance le téléchargement complet en arrière-plan. - // L'URL est valide immédiatement — si le fichier n'est pas encore prêt, - // le client web doit réessayer (retry avec backoff). - let public_url = pmocache::covers_absolute_url_for(&pk, None); + // Stocker la route relative (le handler REST appliquera base_url.url_for()) + let album_art_route = pmocache::covers_route_for(&pk, None); #[cfg(feature = "logging")] tracing::debug!( - "Cached cover - url: {}, PK: {}, public_url: {}", + "Cached cover - url: {}, PK: {}, route: {}", cover_url, pk, - public_url + album_art_route ); - (Some(public_url), Some(pk)) + (Some(album_art_route), Some(pk)) } Err(e) => { #[cfg(feature = "logging")] tracing::warn!("Failed to cache Radio France cover {}: {}", cover_url, e); - // Fallback sur le logo par défaut en cas d'erreur - let logo_url = format!( - "{}/api/radiofrance/default-logo", - server_base_url.trim_end_matches('/') - ); - (Some(logo_url), None) + // Fallback sur le logo par défaut - route relative + let logo_route = "/api/radiofrance/default-logo".to_string(); + (Some(logo_route), None) } } } diff --git a/pmoserver/src/lib.rs b/pmoserver/src/lib.rs index 9f3e60fa..20d3fd13 100644 --- a/pmoserver/src/lib.rs +++ b/pmoserver/src/lib.rs @@ -220,3 +220,36 @@ pub fn get_server_base_url() -> Option { } }) } + +// ============================================================================ +// BaseUrl pour les handlers +// ============================================================================ + +/// URL de base effective pour la requête. +/// Calculée depuis X-Forwarded-Proto/Host ou Host header. +/// Les handlers peuvent appeler get_base_url_from_request(headers) pour l'obtenir. +#[derive(Debug, Clone)] +pub struct BaseUrl(pub String); + +impl BaseUrl { + /// Construit une URL absolue en combinant la base URL de la requête avec une route relative. + /// Usage : BaseUrl::url_for(&pmocache::covers_route_for(pk, None)) + pub fn url_for(&self, route: &str) -> String { + debug_assert!(route.starts_with('/'), "route must start with '/'"); + format!("{}{}", self.0.trim_end_matches('/'), route) + } +} + +/// Récupère la BaseUrl depuis les headers de la requête. +/// Calcule la BaseUrl depuis X-Forwarded-*/Host ou utilise le serveur global. +/// Cette fonction peut être appelée par les handlers qui ont besoin de construire des URLs. +pub fn get_base_url_from_request(headers: &axum::http::HeaderMap) -> String { + get_request_base_url(headers) + .or_else(|| get_server_base_url()) + .unwrap_or_else(|| { + panic!( + "BaseUrl: impossible de déterminer l'URL de base.\n\ + Configurer PMO_SERVER_URL ou démarrer le serveur avant les handlers HTTP." + ); + }) +} diff --git a/pmoserver/src/server.rs b/pmoserver/src/server.rs index b43169ae..96ec13d1 100644 --- a/pmoserver/src/server.rs +++ b/pmoserver/src/server.rs @@ -117,6 +117,7 @@ impl Server { let base_url = base_url.into(); // Créer le router initial avec l'endpoint de registre + // Note: le base_url_layer est appliqué plus tard via le fallback dynamique let registry_route = Router::new() .route("/api/registry", get(get_api_registry)) .with_state(api_registry.clone()); diff --git a/pmoupnp/src/cache_registry.rs b/pmoupnp/src/cache_registry.rs index 572fa5f7..ee0a61d2 100644 --- a/pmoupnp/src/cache_registry.rs +++ b/pmoupnp/src/cache_registry.rs @@ -54,7 +54,7 @@ pub fn get_audio_cache() -> Option> { /// // url = "http://localhost:8080/covers/images/abc123/300" /// ``` pub fn build_cover_url(pk: &str, size: Option) -> anyhow::Result { - Ok(pmocache::covers_absolute_url_for( + Ok(pmocache::covers_absolute_url_for_upnp( pk, size.map(|s| s.to_string()).as_deref(), ))