diff --git a/nghe-api/src/scan/start.rs b/nghe-api/src/scan/start.rs index daab8563..eaba0d91 100644 --- a/nghe-api/src/scan/start.rs +++ b/nghe-api/src/scan/start.rs @@ -7,6 +7,8 @@ pub struct Full { #[serde(default)] pub file: bool, #[serde(default)] + pub external_lyric: bool, + #[serde(default)] pub dir_picture: bool, #[serde(default)] pub information: bool, diff --git a/nghe-backend/migrations/2024-12-24-040249_change_lyrics_primary_key/up.sql b/nghe-backend/migrations/2024-12-24-040249_change_lyrics_primary_key/up.sql index 89fde7d9..2b52e30b 100644 --- a/nghe-backend/migrations/2024-12-24-040249_change_lyrics_primary_key/up.sql +++ b/nghe-backend/migrations/2024-12-24-040249_change_lyrics_primary_key/up.sql @@ -5,7 +5,7 @@ create table lyrics ( id uuid not null default gen_random_uuid() constraint lyrics_pkey primary key, song_id uuid not null, - source text, + external boolean not null, description text, language text not null, durations integer [] check (array_position(durations, null) is null), @@ -15,14 +15,17 @@ lyrics ( check (durations is null or array_length(durations, 1) = array_length(texts, 1)), constraint lyrics_song_id_fkey foreign key (song_id) references songs ( id - ) on delete cascade, - constraint lyrics_song_id_source_key unique (song_id, source) + ) on delete cascade ); select add_updated_at_leave_scanned_at('lyrics'); +create unique index lyrics_song_id_key on lyrics ( + song_id +) nulls not distinct where (external); + create unique index lyrics_song_id_description_key on lyrics ( song_id, description -) nulls not distinct where (source is null); +) nulls not distinct where (not external); create index lyrics_song_id_idx on lyrics (song_id); diff --git a/nghe-backend/src/file/lyric/mod.rs b/nghe-backend/src/file/lyric/mod.rs index a4882846..75c14492 100644 --- a/nghe-backend/src/file/lyric/mod.rs +++ b/nghe-backend/src/file/lyric/mod.rs @@ -112,15 +112,9 @@ impl Lyric<'_> { &self, database: &Database, foreign: lyrics::Foreign, - source: Option>, + external: bool, ) -> Result { - lyrics::Upsert { - foreign, - source: source.as_ref().map(AsRef::as_ref).map(Cow::Borrowed), - data: self.try_into()?, - } - .insert(database) - .await + lyrics::Upsert { foreign, external, data: self.try_into()? }.insert(database).await } pub async fn upserts_embedded( @@ -129,7 +123,7 @@ impl Lyric<'_> { lyrics: &[Self], ) -> Result, Error> { stream::iter(lyrics) - .then(async |lyric| lyric.upsert(database, foreign, None::<&str>).await) + .then(async |lyric| lyric.upsert(database, foreign, false).await) .try_collect() .await } @@ -137,11 +131,10 @@ impl Lyric<'_> { async fn set_external_scanned_at( database: &Database, song_id: Uuid, - source: impl AsRef, ) -> Result, Error> { diesel::update(lyrics::table) .filter(lyrics::song_id.eq(song_id)) - .filter(lyrics::source.eq(source.as_ref())) + .filter(lyrics::external) .set(lyrics::scanned_at.eq(crate::time::now().await)) .returning(lyrics::id) .get_result(&mut database.get().await?) @@ -168,17 +161,16 @@ impl Lyric<'_> { song_id: Uuid, song_path: Utf8TypedPath<'_>, ) -> Result, Error> { - let path = song_path.with_extension(Self::EXTERNAL_EXTENSION); - let path = path.to_path(); - Ok( if !full - && let Some(lyrics_id) = - Self::set_external_scanned_at(database, song_id, path).await? + && let Some(lyrics_id) = Self::set_external_scanned_at(database, song_id).await? { Some(lyrics_id) - } else if let Some(lyrics) = Self::load(filesystem, path).await? { - Some(lyrics.upsert(database, lyrics::Foreign { song_id }, Some(path)).await?) + } else if let Some(lyrics) = + Self::load(filesystem, song_path.with_extension(Self::EXTERNAL_EXTENSION).to_path()) + .await? + { + Some(lyrics.upsert(database, lyrics::Foreign { song_id }, true).await?) } else { None }, @@ -194,7 +186,7 @@ impl Lyric<'_> { diesel::delete(lyrics::table) .filter(lyrics::song_id.eq(song_id)) .filter(lyrics::scanned_at.lt(started_at)) - .filter(lyrics::source.is_not_null()) + .filter(lyrics::external) .execute(&mut database.get().await?) .await?; Ok(()) @@ -220,6 +212,7 @@ impl Lyric<'_> { mod test { use std::fmt::Display; + use diesel::dsl::not; use diesel::{QueryDsl, SelectableHelper}; use fake::{Dummy, Fake, Faker}; use itertools::Itertools; @@ -276,10 +269,20 @@ mod test { } impl Lyric<'static> { + pub async fn query(mock: &Mock, id: Uuid) -> Self { + lyrics::table + .filter(lyrics::id.eq(id)) + .select(lyrics::Data::as_select()) + .get_result(&mut mock.get().await) + .await + .unwrap() + .into() + } + pub async fn query_embedded(mock: &Mock, id: Uuid) -> Vec { lyrics::table .filter(lyrics::song_id.eq(id)) - .filter(lyrics::source.is_null()) + .filter(not(lyrics::external)) .select(lyrics::Data::as_select()) .order_by(lyrics::scanned_at) .get_results(&mut mock.get().await) @@ -293,7 +296,7 @@ mod test { pub async fn query_external(mock: &Mock, id: Uuid) -> Option { lyrics::table .filter(lyrics::song_id.eq(id)) - .filter(lyrics::source.is_not_null()) + .filter(lyrics::external) .select(lyrics::Data::as_select()) .get_result(&mut mock.get().await) .await @@ -374,10 +377,12 @@ mod test { #[cfg(test)] #[coverage(off)] mod tests { + use fake::{Fake, Faker}; use rstest::rstest; use super::*; - use crate::test::assets; + use crate::test::filesystem::Trait as _; + use crate::test::{Mock, assets, mock}; #[rstest] fn test_lyrics_roundtrip(#[values(true, false)] sync: bool) { @@ -422,4 +427,86 @@ mod tests { }; assert_eq!(parsed, lyrics); } + + #[rstest] + #[tokio::test] + async fn test_lyric_upsert_roundtrip( + #[future(awt)] mock: Mock, + #[values(true, false)] external: bool, + #[values(true, false)] update_lyric: bool, + #[values(true, false)] same_description: bool, + ) { + let mut music_folder = mock.music_folder(0).await; + let song_id = music_folder.add_audio().call().await.song_id(0); + + let lyric: Lyric = Faker.fake(); + let id = + lyric.upsert(mock.database(), lyrics::Foreign { song_id }, external).await.unwrap(); + + let database_lyric = Lyric::query(&mock, id).await; + assert_eq!(database_lyric, lyric); + + if update_lyric { + let update_lyric = Lyric { + description: if same_description { + lyric.description.clone() + } else { + // Force description to Some to avoid both descriptions are None. + Some(Faker.fake::().into()) + }, + ..Faker.fake() + }; + let update_id = update_lyric + .upsert(mock.database(), lyrics::Foreign { song_id }, external) + .await + .unwrap(); + let database_update_lyric = Lyric::query(&mock, id).await; + + if external || same_description { + assert_eq!(id, update_id); + assert_eq!(database_update_lyric, update_lyric); + } else { + // This will always insert a new row to the database + // since there is nothing to identify the old lyric. + assert_ne!(id, update_id); + } + } + } + + #[rstest] + #[tokio::test] + async fn test_scan_full( + #[future(awt)] + #[with(0, 1)] + mock: Mock, + #[values(true, false)] full: bool, + ) { + let mut music_folder = mock.music_folder(0).await; + let song_id = music_folder.add_audio().call().await.song_id(0); + + let lyric: Lyric = Faker.fake(); + let id = lyric.upsert(mock.database(), lyrics::Foreign { song_id }, true).await.unwrap(); + + let new_lyric = Lyric::fake_sync(); + + let filesystem = music_folder.to_impl(); + let path = filesystem.prefix().join("test"); + let path = path.to_path(); + filesystem + .write( + path.with_extension(Lyric::EXTERNAL_EXTENSION).to_path(), + new_lyric.to_string().as_bytes(), + ) + .await; + let scanned_id = Lyric::scan(mock.database(), &filesystem.main(), full, song_id, path) + .await + .unwrap() + .unwrap(); + + // They will always be the same because there can only be at most one external lyric for a + // song. + assert_eq!(scanned_id, id); + let database_lyric = Lyric::query(&mock, id).await; + assert_eq!(database_lyric, if full { new_lyric } else { lyric }); + } } diff --git a/nghe-backend/src/orm/lyrics.rs b/nghe-backend/src/orm/lyrics.rs index fbcf4e24..cb51329b 100644 --- a/nghe-backend/src/orm/lyrics.rs +++ b/nghe-backend/src/orm/lyrics.rs @@ -38,12 +38,13 @@ pub struct Foreign { pub struct Upsert<'a> { #[diesel(embed)] pub foreign: Foreign, - pub source: Option>, + pub external: bool, #[diesel(embed)] pub data: Data<'a>, } mod upsert { + use diesel::dsl::not; use diesel::{DecoratableTarget, ExpressionMethods}; use diesel_async::RunQueryDsl; use uuid::Uuid; @@ -54,10 +55,11 @@ mod upsert { impl crate::orm::upsert::Insert for Upsert<'_> { async fn insert(&self, database: &Database) -> Result { - if self.source.is_some() { + if self.external { diesel::insert_into(lyrics::table) .values(self) - .on_conflict((lyrics::song_id, lyrics::source)) + .on_conflict(lyrics::song_id) + .filter_target(lyrics::external) .do_update() .set((&self.data, lyrics::scanned_at.eq(crate::time::now().await))) .returning(lyrics::id) @@ -67,7 +69,7 @@ mod upsert { diesel::insert_into(lyrics::table) .values(self) .on_conflict((lyrics::song_id, lyrics::description)) - .filter_target(lyrics::source.is_null()) + .filter_target(not(lyrics::external)) .do_update() .set((&self.data, lyrics::scanned_at.eq(crate::time::now().await))) .returning(lyrics::id) diff --git a/nghe-backend/src/scan/scanner.rs b/nghe-backend/src/scan/scanner.rs index 93738dab..7f081f0f 100644 --- a/nghe-backend/src/scan/scanner.rs +++ b/nghe-backend/src/scan/scanner.rs @@ -179,13 +179,34 @@ impl<'db, 'fs, 'mf> Scanner<'db, 'fs, 'mf> { song_id: Uuid, song_path: Utf8TypedPath<'_>, ) -> Result<(), Error> { - lyric::Lyric::scan(&self.database, &self.filesystem, false, song_id, song_path).await?; + lyric::Lyric::scan( + &self.database, + &self.filesystem, + self.full.external_lyric, + song_id, + song_path, + ) + .await?; if let Some(started_at) = started_at.into() { lyric::Lyric::cleanup_one_external(&self.database, started_at, song_id).await?; } Ok(()) } + async fn update_external( + &self, + started_at: time::OffsetDateTime, + song_id: Uuid, + song_path: Utf8TypedPath<'_>, + dir_picture_id: Option, + ) -> Result<(), Error> { + // We also need to set album cover_art_id and external lyrics since it might be + // added or removed after the previous scan. + self.update_dir_picture(song_id, dir_picture_id).await?; + self.update_external_lyric(started_at, song_id, song_path).await?; + Ok(()) + } + #[cfg_attr( not(coverage_nightly), instrument( @@ -253,10 +274,8 @@ impl<'db, 'fs, 'mf> Scanner<'db, 'fs, 'mf> { // `song_id` can be None if there are more than two duplicated files in the same // music folder. - // We also need to set album cover_art_id and external lyrics since it might be - // added or removed after the previous scan. - self.update_dir_picture(song_path.id, dir_picture_id).await?; - self.update_external_lyric(started_at, song_path.id, absolute_path).await?; + self.update_external(started_at, song_path.id, absolute_path, dir_picture_id) + .await?; tracing::debug!("already scanned"); return Ok(song_path.id); } else if let Some(song_id) = song_id { @@ -289,9 +308,13 @@ impl<'db, 'fs, 'mf> Scanner<'db, 'fs, 'mf> { .execute(&mut database.get().await?) .await?; - // We also need to set album cover_art_id since it might be added or removed - // after the previous scan. - self.update_dir_picture(song_path.id, dir_picture_id).await?; + self.update_external( + started_at, + song_path.id, + absolute_path, + dir_picture_id, + ) + .await?; tracing::debug!("stale last_modified"); return Ok(song_path.id); } @@ -318,10 +341,8 @@ impl<'db, 'fs, 'mf> Scanner<'db, 'fs, 'mf> { .execute(&mut database.get().await?) .await?; - // We also need to set album cover_art_id and external lyrics since it might be - // added or removed after the previous scan. - self.update_dir_picture(song_path.id, dir_picture_id).await?; - self.update_external_lyric(started_at, song_path.id, absolute_path).await?; + self.update_external(started_at, song_path.id, absolute_path, dir_picture_id) + .await?; tracing::warn!( old = %song_path.relative_path, new = %relative_path, "renamed duplication" ); @@ -426,10 +447,13 @@ mod tests { let song_id = music_folder.song_id_filesystem(0).await; let filesystem_audio = music_folder.filesystem[0].clone(); + // Don't modify lyric because we won't rescan it even in full mode (it will be rescanned + // only in full lyric mode). music_folder .add_audio() .album(filesystem_audio.information.metadata.album) .file_property(filesystem_audio.information.file) + .external_lyric(None) .relative_path(filesystem_audio.relative_path) .song_id(song_id) .call() diff --git a/nghe-backend/src/schema.rs b/nghe-backend/src/schema.rs index 44e49b92..6df0f04a 100644 --- a/nghe-backend/src/schema.rs +++ b/nghe-backend/src/schema.rs @@ -101,7 +101,7 @@ diesel::table! { lyrics (id) { id -> Uuid, song_id -> Uuid, - source -> Nullable, + external -> Bool, description -> Nullable, language -> Text, durations -> Nullable>>, diff --git a/nghe-backend/src/test/mock_impl/information.rs b/nghe-backend/src/test/mock_impl/information.rs index 1999b989..7c94b2d3 100644 --- a/nghe-backend/src/test/mock_impl/information.rs +++ b/nghe-backend/src/test/mock_impl/information.rs @@ -14,10 +14,10 @@ use crate::test::file::audio::dump::Metadata as _; use crate::test::filesystem::Trait as _; #[derive(Debug, Clone, PartialEq, Eq)] -pub struct Mock<'info, 'picture, 'lyrics, 'path> { +pub struct Mock<'info, 'lyrics, 'picture, 'path> { pub information: audio::Information<'info>, - pub dir_picture: Option>, pub external_lyric: Option>, + pub dir_picture: Option>, pub relative_path: Cow<'path, str>, } @@ -148,14 +148,7 @@ impl Mock<'_, '_, '_, '_> { .unwrap(); if let Some(external_lyric) = self.external_lyric.as_ref() { - external_lyric - .upsert( - database, - lyrics::Foreign { song_id }, - Some(music_folder.absolutize(&self.relative_path)), - ) - .await - .unwrap(); + external_lyric.upsert(database, lyrics::Foreign { song_id }, true).await.unwrap(); } song_id