eric/pmomusic
1
0
Fork 0
Code Issues 5 Pull Requests Actions Packages Projects Releases 3 Wiki Activity

🔧 refactor(base_url): centraliser gestion des URLs absolues via middleware

Browse Source 
- Ajout d'un BaseUrl layer dans pmoserver pour gérer les URLs absolues (LAN/WAN)
- Renommage de `covers_absolute_url_for` → ` covers_relative_route`, stocker les routes relatives
- Mise à jour des appels UPnP vers `covers_absolute_url_for_upnp` (fallback PMO_SERVER_URL)
- Correction des tâches de fond pour stocker les routes, pas l'URL complète
- Suppression du feature gate `simd` inutilisé dans pmoaudio/src/lib.rs
This commit is contained in:
Eric Coissac
2026-04-04 10:14:18 +02:00
parent ff699d220f
commit 718c0d2aed
12 changed files with 294 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
.gitignore vendored
View File

@@ -49,3 +49,4 @@ RF.json
RF_old.json
.claude/
.claude.old
Kilo-session.md

225
.kilo/plans/1775285337131-neon-mountain.md Normal file
View File

@@ -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<RwLock<Router>>`. 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<Router>`)
- 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<BaseUrl>` 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.

39
pmoapp/webapp/package-lock.json generated
View File

@@ -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": [

2
pmoaudio-ext/src/sinks/streaming_icyflac_sink.rs
View File

@@ -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

21
pmoaudio/src/audio_segment.rs
View File

@@ -323,37 +323,42 @@ impl AudioSegment {
/// Convertit l'AudioChunk vers F32 si c'est un chunk audio
pub fn to_f32_chunk(&self) -> Option<AudioChunk> {
self.as_chunk().map(|chunk| chunk.to_f32())
self.as_chunk()
.map(|chunk: &Arc<AudioChunk>| chunk.to_f32())
}
/// Convertit l'AudioChunk vers I32 si c'est un chunk audio
pub fn to_i32_chunk(&self) -> Option<AudioChunk> {
self.as_chunk().map(|chunk| chunk.to_i32())
self.as_chunk()
.map(|chunk: &Arc<AudioChunk>| chunk.to_i32())
}
/// Récupère le sample rate du chunk audio
pub fn sample_rate(&self) -> Option<u32> {
self.as_chunk().map(|chunk| chunk.sample_rate())
self.as_chunk()
.map(|chunk: &Arc<AudioChunk>| chunk.sample_rate())
}
/// Récupère le nombre de frames du chunk audio
pub fn frame_count(&self) -> Option<usize> {
self.as_chunk().map(|chunk| chunk.len())
self.as_chunk().map(|chunk: &Arc<AudioChunk>| chunk.len())
}
/// Récupère le gain en dB du chunk audio
pub fn gain_db(&self) -> Option<f64> {
self.as_chunk().map(|chunk| chunk.gain_db())
self.as_chunk()
.map(|chunk: &Arc<AudioChunk>| 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<AudioChunk>| 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<Arc<Self>> {
self.as_chunk().map(|chunk| {
self.as_chunk().map(|chunk: &Arc<AudioChunk>| {
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<Arc<Self>> {
self.as_chunk().map(|chunk| {
self.as_chunk().map(|chunk: &Arc<AudioChunk>| {
let new_gain = chunk.gain_db() + delta_db;
let new_chunk = chunk.set_gain_db(new_gain);
Arc::new(Self {

1
pmoaudio/src/lib.rs
View File

@@ -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;

10
pmocache/src/lib.rs
View File

@@ -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};

3
pmoparadise/src/source.rs
View File

@@ -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()));

24
pmoradiofrance/src/metadata_cache.rs
View File

@@ -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)
}
}
}

33
pmoserver/src/lib.rs
View File

@@ -220,3 +220,36 @@ pub fn get_server_base_url() -> Option<String> {
}
})
}
// ============================================================================
// 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."
);
})
}

1
pmoserver/src/server.rs
View File

@@ -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());

2
pmoupnp/src/cache_registry.rs
View File

@@ -54,7 +54,7 @@ pub fn get_audio_cache() -> Option<Arc<AudioCache>> {
/// // url = "http://localhost:8080/covers/images/abc123/300"
/// ```
pub fn build_cover_url(pk: &str, size: Option<usize>) -> anyhow::Result<String> {
Ok(pmocache::covers_absolute_url_for(
Ok(pmocache::covers_absolute_url_for_upnp(
pk,
size.map(|s| s.to_string()).as_deref(),
))