diff --git a/src/db/queries.rs b/src/db/queries.rs index 496c783..59e5caa 100644 --- a/src/db/queries.rs +++ b/src/db/queries.rs @@ -164,20 +164,6 @@ pub fn get_album(conn: &Connection, id: i64) -> Result> { .optional() } -/// Find any existing album with this exact title, regardless of artist. -/// Used when no ALBUMARTIST tag is present to avoid creating a separate album -/// entry for every featured artist in a compilation. -/// Find an album by title, returning `(album_id, artist_id)`. -/// Used when no ALBUMARTIST tag is present so we can detect multi-artist albums. -pub fn find_album_by_title(conn: &Connection, title: &str) -> Result> { - conn.query_row( - "SELECT id, artist_id FROM albums WHERE title = ?1 LIMIT 1", - params![title], - |row| Ok((row.get(0)?, row.get(1)?)), - ) - .optional() -} - pub fn update_album_artist(conn: &Connection, album_id: i64, artist_id: i64) -> Result<()> { conn.execute( "UPDATE albums SET artist_id = ?1 WHERE id = ?2", @@ -194,6 +180,40 @@ pub fn update_album_rating(conn: &Connection, album_id: i64, rating: u8) -> Resu Ok(()) } +// ============================================================================ +// Library maintenance +// ============================================================================ + +pub struct CleanupStats { + pub albums_deleted: usize, + pub artists_deleted: usize, +} + +/// Delete albums with no tracks, then artists with no tracks and no albums. +/// Returns counts of deleted rows. +pub fn clean_orphans(conn: &Connection) -> Result { + let albums_deleted = conn.execute( + "DELETE FROM albums WHERE id NOT IN ( + SELECT DISTINCT album_id FROM tracks WHERE album_id IS NOT NULL + )", + [], + )?; + + let artists_deleted = conn.execute( + "DELETE FROM artists WHERE id NOT IN ( + SELECT DISTINCT artist_id FROM tracks + ) AND id NOT IN ( + SELECT DISTINCT artist_id FROM albums + )", + [], + )?; + + Ok(CleanupStats { + albums_deleted, + artists_deleted, + }) +} + // ============================================================================ // Genre operations // ============================================================================ @@ -487,12 +507,13 @@ pub fn list_albums( pub fn get_artist_albums(conn: &Connection, artist_id: i64) -> Result> { let mut stmt = conn.prepare( - "SELECT a.id, a.title, a.artist_id, ar.name as artist_name, a.year, a.rating, + "SELECT DISTINCT a.id, a.title, a.artist_id, ar.name as artist_name, a.year, a.rating, a.artwork_path, a.online_artwork_path, a.description, a.musicbrainz_id, a.label, a.country, a.barcode, a.album_type, a.release_status FROM albums a JOIN artists ar ON ar.id = a.artist_id WHERE a.artist_id = ?1 + OR EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id AND t.artist_id = ?1) ORDER BY a.year DESC, a.title", )?; @@ -1072,6 +1093,42 @@ mod tests { assert_eq!(albums.len(), 2); } + #[test] + fn test_get_artist_albums_includes_track_artist() { + // Albums where the artist only appears as a track artist (not album artist) + // should also be returned — covers "feat." artists and VA-upgraded albums. + let env = TestEnv::new(); + let conn = env.pool.get().unwrap(); + let source_id = insert_source(&conn, "Library", SourceType::Disk, None).unwrap(); + let album_artist_id = insert_artist(&conn, "Album Artist", None).unwrap(); + let feat_artist_id = insert_artist(&conn, "Featured Artist", None).unwrap(); + let album_id = insert_album(&conn, "Collab Album", album_artist_id, Some(2022)).unwrap(); + insert_track( + &conn, + "Collab Track", + Some(album_id), + feat_artist_id, + source_id, + std::path::Path::new("/music/collab.flac"), + 180_000, + None, + None, + None, + AudioFormat::Flac, + 1_000_000, + ) + .unwrap(); + + // feat_artist_id is not the album artist but has a track on the album + let albums = get_artist_albums(&conn, feat_artist_id).unwrap(); + assert_eq!(albums.len(), 1); + assert_eq!(albums[0].title, "Collab Album"); + + // album_artist_id still gets the album too + let albums = get_artist_albums(&conn, album_artist_id).unwrap(); + assert_eq!(albums.len(), 1); + } + // ==================================================================== // Album tests // ==================================================================== @@ -1120,23 +1177,6 @@ mod tests { assert_eq!(tracks.len(), 2); } - #[test] - fn test_find_album_by_title_returns_id_and_artist_id() { - let env = TestEnv::new(); - let conn = env.pool.get().unwrap(); - let artist_id = insert_artist(&conn, "Akhenaton", None).unwrap(); - let album_id = insert_album(&conn, "Sol Invictus", artist_id, Some(2001)).unwrap(); - let found = find_album_by_title(&conn, "Sol Invictus").unwrap(); - assert_eq!(found, Some((album_id, artist_id))); - } - - #[test] - fn test_find_album_by_title_returns_none_when_missing() { - let env = TestEnv::new(); - let conn = env.pool.get().unwrap(); - assert!(find_album_by_title(&conn, "Unknown").unwrap().is_none()); - } - #[test] fn test_update_album_artist() { let env = TestEnv::new(); @@ -1145,8 +1185,8 @@ mod tests { let artist2 = insert_artist(&conn, "Various Artists", None).unwrap(); let album_id = insert_album(&conn, "Compilation", artist1, None).unwrap(); update_album_artist(&conn, album_id, artist2).unwrap(); - let (_, returned_artist) = find_album_by_title(&conn, "Compilation").unwrap().unwrap(); - assert_eq!(returned_artist, artist2); + let album = get_album(&conn, album_id).unwrap().unwrap(); + assert_eq!(album.artist_id, artist2); } #[test] diff --git a/src/ffi.rs b/src/ffi.rs index 9792ca2..50266cc 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -158,6 +158,7 @@ mod ffi { // Artist Functions fn get_artists_page(offset: u32, limit: u32) -> String; fn get_artist_by_id(artist_id: i64) -> String; + fn get_artist_albums(artist_id: i64) -> String; // Genre Functions fn get_genres() -> String; @@ -174,6 +175,7 @@ mod ffi { fn delete_playlist(playlist_id: i64) -> String; fn add_track_to_playlist(playlist_id: i64, track_id: i64) -> String; fn remove_track_from_playlist(playlist_id: i64, position: i64) -> String; + fn import_m3u_playlist(path: &str) -> String; // Playback Control Functions fn play_track(track_id: i64) -> String; @@ -203,6 +205,9 @@ mod ffi { fn toggle_repeat() -> String; fn set_repeat_mode(mode: &str) -> String; + // Library Maintenance + fn clean_library() -> String; + // Network / NAS helpers fn clear_unavailable_tracks() -> String; @@ -290,6 +295,34 @@ fn get_library_stats() -> String { } } +fn clean_library() -> String { + match get_or_init_pool() { + Ok(pool) => { + let conn = match pool.get() { + Ok(c) => c, + Err(e) => { + return serde_json::json!({ "success": false, "error": format!("{e}") }) + .to_string(); + } + }; + match crate::db::queries::clean_orphans(&conn) { + Ok(stats) => serde_json::json!({ + "success": true, + "data": { + "albums_deleted": stats.albums_deleted, + "artists_deleted": stats.artists_deleted + } + }) + .to_string(), + Err(e) => { + serde_json::json!({ "success": false, "error": format!("{e}") }).to_string() + } + } + } + Err(e) => serde_json::json!({ "success": false, "error": format!("{e}") }).to_string(), + } +} + fn scan_library(folder_path: &str) -> String { // T013: Scan a folder for music files and add to library match get_or_init_library() { @@ -809,6 +842,42 @@ fn get_artist_by_id(artist_id: i64) -> String { } } +fn get_artist_albums(artist_id: i64) -> String { + // Get all albums for an artist (as album artist OR as track artist) + match get_or_init_pool() { + Ok(pool) => { + let conn = match pool.get() { + Ok(conn) => conn, + Err(e) => { + return serde_json::json!({ + "success": false, + "error": format!("Failed to get database connection: {}", e) + }) + .to_string(); + } + }; + + match crate::db::queries::get_artist_albums(&conn, artist_id) { + Ok(albums) => serde_json::json!({ + "success": true, + "data": { "albums": albums } + }) + .to_string(), + Err(e) => serde_json::json!({ + "success": false, + "error": format!("Failed to get artist albums: {}", e) + }) + .to_string(), + } + } + Err(e) => serde_json::json!({ + "success": false, + "error": format!("Database pool error: {}", e) + }) + .to_string(), + } +} + fn get_artists_page(offset: u32, limit: u32) -> String { // T018: Get paginated list of artists match get_or_init_pool() { @@ -1084,6 +1153,33 @@ fn remove_track_from_playlist(playlist_id: i64, position: i64) -> String { } } +fn import_m3u_playlist(path: &str) -> String { + // Import a .m3u file and create a new playlist from it + match get_or_init_pool() { + Ok(pool) => { + let playlist_service = PlaylistService::new(pool.clone()); + let file_path = std::path::Path::new(path); + match playlist_service.import_m3u(file_path) { + Ok(playlist) => serde_json::json!({ + "success": true, + "data": playlist + }) + .to_string(), + Err(e) => serde_json::json!({ + "success": false, + "error": format!("Failed to import M3U: {}", e) + }) + .to_string(), + } + } + Err(e) => serde_json::json!({ + "success": false, + "error": format!("FFI initialization failed: {}", e) + }) + .to_string(), + } +} + fn play_track(track_id: i64) -> String { // T024: Play a specific track // Sets queue to just this track and starts playback diff --git a/src/services/library.rs b/src/services/library.rs index 605c166..c3a0afc 100644 --- a/src/services/library.rs +++ b/src/services/library.rs @@ -307,43 +307,21 @@ impl LibraryService { // Get or create album if present. // - // When ALBUMARTIST is set, trust it and create/find the album by (title, artist). - // When ALBUMARTIST is absent (common in compilations/mixtapes), first look for any - // existing album with the same title and reuse it — this prevents creating a separate - // album entry for every featured artist on a "70s Mixtape" or "80's Greatest Hits". - // If a second distinct artist is detected, the album is upgraded to "Various Artists" - // so it doesn't belong to a single (arbitrary) artist. + // Album identity is always (title, artist_id) — the same UNIQUE constraint used + // in the database. When ALBUMARTIST is set, we use it as the artist; otherwise + // we use the primary track artist. + // + // This means "Greatest Hits" by The Beatles and "Greatest Hits" by Eminem are + // always stored as two separate albums, even if neither file carries an ALBUMARTIST + // tag. Compilations or multi-artist albums *must* be tagged with + // ALBUMARTIST=Various Artists to be grouped correctly. let album_id = if let Some(ref album_title) = metadata.album { - if metadata.album_artist.is_some() { - // ALBUMARTIST is set: use it, create per (title, artist) as normal - Some(queries::insert_album( - conn, - album_title, - album_artist_id, - metadata.year, - )?) - } else { - // No ALBUMARTIST: reuse any existing album with this title to avoid duplicates - match queries::find_album_by_title(conn, album_title)? { - Some((existing_id, existing_artist_id)) => { - // If a second distinct artist appears on this album, upgrade it to - // "Various Artists" so no single real artist "owns" it - if existing_artist_id != album_artist_id { - let va_id = queries::insert_artist(conn, "Various Artists", None)?; - if existing_artist_id != va_id { - queries::update_album_artist(conn, existing_id, va_id)?; - } - } - Some(existing_id) - } - None => Some(queries::insert_album( - conn, - album_title, - album_artist_id, - metadata.year, - )?), - } - } + Some(queries::insert_album( + conn, + album_title, + album_artist_id, + metadata.year, + )?) } else { None }; @@ -921,101 +899,48 @@ mod tests { assert_eq!(track.rating, Rating(0)); } - // ── Various Artists upgrade ─────────────────────────────────────────────── + // ── Album identity: (title, artist) ───────────────────────────────────── - /// When two tracks with different artists share the same album title (no ALBUMARTIST), - /// the album should be reassigned to "Various Artists" on the second import. + /// Two tracks with the same album title but different primary artists (no ALBUMARTIST tag) + /// must produce two *separate* albums, not a single "Various Artists" one. + /// This covers the "Greatest Hits" / "Best Of" scenario. #[test] - fn test_various_artists_upgrade_on_second_different_artist() { + fn test_same_title_different_artist_creates_separate_albums() { let env = TestEnv::new(); let conn = env.pool.get().unwrap(); - let source_id = - queries::insert_source(&conn, "Test", crate::models::source::SourceType::Disk, None) - .unwrap(); - let artist1 = queries::insert_artist(&conn, "Akhenaton", None).unwrap(); - let artist2 = queries::insert_artist(&conn, "Kool Shen", None).unwrap(); - - // First track: album created and owned by artist1 - let album_id = queries::insert_album(&conn, "70s Mixtape", artist1, None).unwrap(); - queries::insert_track( - &conn, - "Track 1", - Some(album_id), - artist1, - source_id, - &PathBuf::from("/music/t1.flac"), - 200_000, - None, - None, - None, - AudioFormat::Flac, - 1_000_000, - ) - .unwrap(); - - // Simulate second track import (different artist, same album title, no ALBUMARTIST) - let (existing_id, existing_artist_id) = queries::find_album_by_title(&conn, "70s Mixtape") - .unwrap() - .unwrap(); - assert_eq!(existing_id, album_id); - assert_eq!(existing_artist_id, artist1); - - // Upgrade to Various Artists - if existing_artist_id != artist2 { - let va_id = queries::insert_artist(&conn, "Various Artists", None).unwrap(); - if existing_artist_id != va_id { - queries::update_album_artist(&conn, existing_id, va_id).unwrap(); - } - } + let artist1 = queries::insert_artist(&conn, "The Beatles", None).unwrap(); + let artist2 = queries::insert_artist(&conn, "Eminem", None).unwrap(); - // Album should now belong to "Various Artists" - let album = queries::get_album(&conn, album_id).unwrap().unwrap(); - let va = queries::get_artist(&conn, album.artist_id) - .unwrap() - .unwrap(); - assert_eq!(va.name, "Various Artists"); + let id1 = queries::insert_album(&conn, "Greatest Hits", artist1, None).unwrap(); + let id2 = queries::insert_album(&conn, "Greatest Hits", artist2, None).unwrap(); + + assert_ne!( + id1, id2, + "distinct artists must produce distinct album rows" + ); + + let album1 = queries::get_album(&conn, id1).unwrap().unwrap(); + let album2 = queries::get_album(&conn, id2).unwrap().unwrap(); + assert_eq!(album1.artist_id, artist1); + assert_eq!(album2.artist_id, artist2); } - /// When two tracks share the same artist and same album, no upgrade should happen. + /// Two tracks with the same album title *and* the same primary artist (no ALBUMARTIST tag) + /// must be grouped into the same album row. #[test] - fn test_no_various_artists_upgrade_when_same_artist() { + fn test_same_title_same_artist_reuses_album() { let env = TestEnv::new(); let conn = env.pool.get().unwrap(); - let source_id = - queries::insert_source(&conn, "Test", crate::models::source::SourceType::Disk, None) - .unwrap(); let artist_id = queries::insert_artist(&conn, "Akhenaton", None).unwrap(); - let album_id = queries::insert_album(&conn, "Sol Invictus", artist_id, None).unwrap(); - queries::insert_track( - &conn, - "Track 1", - Some(album_id), - artist_id, - source_id, - &PathBuf::from("/music/t1.flac"), - 200_000, - None, - None, - None, - AudioFormat::Flac, - 1_000_000, - ) - .unwrap(); - - // Same artist on second track — no upgrade - let (existing_id, existing_artist_id) = queries::find_album_by_title(&conn, "Sol Invictus") - .unwrap() - .unwrap(); - // artist IDs match → no upgrade - assert_eq!(existing_artist_id, artist_id); + let id1 = queries::insert_album(&conn, "Sol Invictus", artist_id, None).unwrap(); + let id2 = queries::insert_album(&conn, "Sol Invictus", artist_id, None).unwrap(); - let album = queries::get_album(&conn, existing_id).unwrap().unwrap(); assert_eq!( - album.artist_id, artist_id, - "album should still belong to Akhenaton" + id1, id2, + "same title + same artist must reuse the existing album row" ); }