From ffd0d4d82f3f67da07c38c7b424d4a58f27c090e Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 5 Jan 2025 11:06:05 +0100 Subject: [PATCH] Fetching and then instantly reloading library loses data (#256) Closes #254 Reviewed-on: https://git.thenineworlds.net/wojtek/musichoard/pulls/256 --- src/bin/musichoard-edit.rs | 8 +++--- src/core/musichoard/base.rs | 33 ++++++++-------------- src/core/musichoard/builder.rs | 43 +++++++++++----------------- src/core/musichoard/database.rs | 50 +++++++++++++++++---------------- src/core/musichoard/library.rs | 22 +++++++++++---- src/core/musichoard/mod.rs | 1 - src/main.rs | 2 +- tests/lib.rs | 4 +-- 8 files changed, 78 insertions(+), 85 deletions(-) diff --git a/src/bin/musichoard-edit.rs b/src/bin/musichoard-edit.rs index 3941786c..d6a1533 100644 --- a/src/bin/musichoard-edit.rs +++ b/src/bin/musichoard-edit.rs @@ -249,9 +249,9 @@ fn main() { let db = JsonDatabase::new(JsonDatabaseFileBackend::new(&opt.database_file_path)); - let mut music_hoard = MusicHoardBuilder::default() - .set_database(db) - .build() - .expect("failed to initialise MusicHoard"); + let mut music_hoard = MusicHoardBuilder::default().set_database(db).build(); + music_hoard + .reload_database() + .expect("failed to load MusicHoard database"); opt.command.handle(&mut music_hoard); } diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index 08a04c7..e8daf36 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -33,7 +33,7 @@ pub trait IMusicHoardBasePrivate { fn sort_artists(collection: &mut [Artist]); fn sort_albums_and_tracks<'a, C: Iterator>(collection: C); - fn merge_collections(&self) -> Collection; + fn merge_collections>(&self, database: It) -> Collection; fn filter_collection(&self) -> Collection; fn filter_artist(&self, artist: &Artist) -> Option; @@ -69,7 +69,7 @@ impl IMusicHoardBasePrivate for MusicHoard } } - fn merge_collections(&self) -> Collection { + fn merge_collections>(&self, database: It) -> Collection { let mut primary = NormalMap::::new(); let mut secondary = NormalMap::::new(); @@ -77,7 +77,7 @@ impl IMusicHoardBasePrivate for MusicHoard primary.insert(string::normalize_string(&artist.meta.id.name), artist); } - for artist in self.database_cache.iter().cloned() { + for artist in database.into_iter() { secondary.insert(string::normalize_string(&artist.meta.id.name), artist); } @@ -168,21 +168,19 @@ mod tests { let mut mh = MusicHoard { library_cache: left.clone(), - database_cache: right.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(right.clone()); assert_eq!(expected, mh.collection); // The merge is completely non-overlapping so it should be commutative. let mut mh = MusicHoard { library_cache: right.clone(), - database_cache: left.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(left.clone()); assert_eq!(expected, mh.collection); } @@ -198,21 +196,19 @@ mod tests { let mut mh = MusicHoard { library_cache: left.clone(), - database_cache: right.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(right.clone()); assert_eq!(expected, mh.collection); // The merge does not overwrite any data so it should be commutative. let mut mh = MusicHoard { library_cache: right.clone(), - database_cache: left.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(left.clone()); assert_eq!(expected, mh.collection); } @@ -241,21 +237,19 @@ mod tests { let mut mh = MusicHoard { library_cache: left.clone(), - database_cache: right.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(right.clone()); assert_eq!(expected, mh.collection); // The merge overwrites the sort data, but no data is erased so it should be commutative. let mut mh = MusicHoard { library_cache: right.clone(), - database_cache: left.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(left.clone()); assert_eq!(expected, mh.collection); } @@ -273,11 +267,10 @@ mod tests { let mut mh = MusicHoard { library_cache: left.clone(), - database_cache: right.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(right.clone()); } #[test] @@ -294,11 +287,10 @@ mod tests { let mut mh = MusicHoard { library_cache: left.clone(), - database_cache: right.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(right.clone()); } #[test] @@ -318,11 +310,10 @@ mod tests { let mut mh = MusicHoard { library_cache: left.clone(), - database_cache: right.clone(), ..Default::default() }; - mh.collection = mh.merge_collections(); + mh.collection = mh.merge_collections(right.clone()); assert_eq!(expected, mh.collection); } diff --git a/src/core/musichoard/builder.rs b/src/core/musichoard/builder.rs index 1901a9f..46c9f4c 100644 --- a/src/core/musichoard/builder.rs +++ b/src/core/musichoard/builder.rs @@ -1,8 +1,6 @@ use crate::core::{ interface::{database::IDatabase, library::ILibrary}, - musichoard::{ - database::IMusicHoardDatabase, CollectionFilter, Error, MusicHoard, NoDatabase, NoLibrary, - }, + musichoard::{CollectionFilter, MusicHoard, NoDatabase, NoLibrary}, }; /// Builder for [`MusicHoard`]. Its purpose is to make it easier to set various combinations of @@ -69,7 +67,6 @@ impl MusicHoard { collection: vec![], pre_commit: vec![], database: NoDatabase, - database_cache: vec![], library: NoLibrary, library_cache: vec![], } @@ -92,7 +89,6 @@ impl MusicHoard { collection: vec![], pre_commit: vec![], database: NoDatabase, - database_cache: vec![], library, library_cache: vec![], } @@ -101,59 +97,56 @@ impl MusicHoard { impl MusicHoardBuilder { /// Build [`MusicHoard`] with the currently set library and database. - pub fn build(self) -> Result, Error> { + pub fn build(self) -> MusicHoard { MusicHoard::database(self.database) } } impl MusicHoard { /// Create a new [`MusicHoard`] with the provided [`IDatabase`] and no library. - pub fn database(database: Database) -> Result { - let mut mh = MusicHoard { + pub fn database(database: Database) -> Self { + MusicHoard { filter: CollectionFilter::default(), filtered: vec![], collection: vec![], pre_commit: vec![], database, - database_cache: vec![], library: NoLibrary, library_cache: vec![], - }; - mh.reload_database()?; - Ok(mh) + } } } impl MusicHoardBuilder { /// Build [`MusicHoard`] with the currently set library and database. - pub fn build(self) -> Result, Error> { + pub fn build(self) -> MusicHoard { MusicHoard::new(self.database, self.library) } } impl MusicHoard { /// Create a new [`MusicHoard`] with the provided [`ILibrary`] and [`IDatabase`]. - pub fn new(database: Database, library: Library) -> Result { - let mut mh = MusicHoard { + pub fn new(database: Database, library: Library) -> Self { + MusicHoard { filter: CollectionFilter::default(), filtered: vec![], collection: vec![], pre_commit: vec![], database, - database_cache: vec![], library, library_cache: vec![], - }; - mh.reload_database()?; - Ok(mh) + } } } #[cfg(test)] mod tests { - use crate::core::{ - interface::{database::NullDatabase, library::NullLibrary}, - musichoard::library::IMusicHoardLibrary, + use crate::{ + core::{ + interface::{database::NullDatabase, library::NullLibrary}, + musichoard::library::IMusicHoardLibrary, + }, + IMusicHoardDatabase, }; use super::*; @@ -181,8 +174,7 @@ mod tests { fn no_library_with_database() { let mut mh = MusicHoardBuilder::default() .set_database(NullDatabase) - .build() - .unwrap(); + .build(); assert!(mh.reload_database().is_ok()); } @@ -191,8 +183,7 @@ mod tests { let mut mh = MusicHoardBuilder::default() .set_library(NullLibrary) .set_database(NullDatabase) - .build() - .unwrap(); + .build(); assert!(mh.rescan_library().is_ok()); assert!(mh.reload_database().is_ok()); } diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 990871c..ae9e58e 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -116,10 +116,10 @@ pub trait IMusicHoardDatabase { impl IMusicHoardDatabase for MusicHoard { fn reload_database(&mut self) -> Result<(), Error> { - self.database_cache = self.database.load()?; - Self::sort_albums_and_tracks(self.database_cache.iter_mut()); + let mut database_cache = self.database.load()?; + Self::sort_albums_and_tracks(database_cache.iter_mut()); - self.collection = self.merge_collections(); + self.collection = self.merge_collections(database_cache); self.filtered = self.filter_collection(); self.pre_commit = self.collection.clone(); @@ -504,7 +504,8 @@ mod tests { .with(predicate::eq(collection.clone())) .returning(|_| Ok(())); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); assert_eq!(music_hoard.collection, collection); assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); @@ -523,11 +524,10 @@ mod tests { #[test] fn artist_sort_set_clear() { let mut database = MockIDatabase::new(); - database.expect_load().times(1).returning(|| Ok(vec![])); database.expect_save().times(4).returning(|_| Ok(())); type MH = MusicHoard; - let mut music_hoard: MH = MusicHoard::database(database).unwrap(); + let mut music_hoard: MH = MusicHoard::database(database); let artist_1_id = ArtistId::new("the artist"); let artist_1_sort = String::from("artist, the"); @@ -574,9 +574,8 @@ mod tests { #[test] fn collection_error() { - let mut database = MockIDatabase::new(); - database.expect_load().times(1).returning(|| Ok(vec![])); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let database = MockIDatabase::new(); + let mut music_hoard = MusicHoard::database(database); let artist_id = ArtistId::new("an artist"); let actual_err = music_hoard @@ -591,12 +590,11 @@ mod tests { #[test] fn set_clear_artist_mb_ref() { let mut database = MockIDatabase::new(); - database.expect_load().times(1).returning(|| Ok(vec![])); database.expect_save().times(3).returning(|_| Ok(())); let mut artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); @@ -637,12 +635,11 @@ mod tests { #[test] fn set_clear_artist_info() { let mut database = MockIDatabase::new(); - database.expect_load().times(1).returning(|| Ok(vec![])); database.expect_save().times(3).returning(|_| Ok(())); let artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); @@ -681,12 +678,11 @@ mod tests { #[test] fn add_to_remove_from_property() { let mut database = MockIDatabase::new(); - database.expect_load().times(1).returning(|| Ok(vec![])); database.expect_save().times(3).returning(|_| Ok(())); let artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); @@ -730,12 +726,11 @@ mod tests { #[test] fn set_clear_property() { let mut database = MockIDatabase::new(); - database.expect_load().times(1).returning(|| Ok(vec![])); database.expect_save().times(3).returning(|_| Ok(())); let artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); @@ -805,7 +800,8 @@ mod tests { .with(predicate::eq(collection.clone())) .returning(|_| Ok(())); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); assert_eq!(music_hoard.collection, collection); assert!(music_hoard @@ -844,7 +840,8 @@ mod tests { .return_once(|| Ok(database_result)); database.expect_save().times(2).returning(|_| Ok(())); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); let album = &music_hoard.collection[0].albums[0]; assert_eq!(album.meta.id.mb_ref, AlbumMbRef::None); @@ -902,7 +899,8 @@ mod tests { .return_once(|| Ok(database_result)); database.expect_save().times(2).returning(|_| Ok(())); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); assert_eq!(music_hoard.collection[0].albums[0].meta.seq, AlbumSeq(0)); // Seting seq on an album not belonging to the artist is an error. @@ -941,7 +939,8 @@ mod tests { .times(1) .return_once(|| Ok(database_result)); database.expect_save().times(2).returning(|_| Ok(())); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); let meta = &music_hoard.collection[0].albums[0].meta; assert_eq!(meta.info.primary_type, None); @@ -986,7 +985,8 @@ mod tests { .times(1) .return_once(|| Ok(FULL_COLLECTION.to_owned())); - let music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); assert_eq!(music_hoard.get_collection(), &*FULL_COLLECTION); } @@ -1002,7 +1002,8 @@ mod tests { .times(1) .return_once(|| database_result); - let actual_err = MusicHoard::database(database).unwrap_err(); + let mut music_hoard = MusicHoard::database(database); + let actual_err = music_hoard.reload_database().unwrap_err(); let expected_err = Error::DatabaseError( database::LoadError::IoError(String::from("I/O error")).to_string(), ); @@ -1023,7 +1024,8 @@ mod tests { .times(1) .return_once(|_: &Collection| database_result); - let mut music_hoard = MusicHoard::database(database).unwrap(); + let mut music_hoard = MusicHoard::database(database); + music_hoard.reload_database().unwrap(); let actual_err = music_hoard .add_artist(ArtistId::new("an artist")) diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index 70eabe2..d0deb30 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -23,25 +23,28 @@ pub trait IMusicHoardLibrary { impl IMusicHoardLibrary for MusicHoard { fn rescan_library(&mut self) -> Result<(), Error> { - self.pre_commit = self.rescan_library_inner()?; + self.pre_commit = self.rescan_library_inner(vec![])?; self.commit() } } impl IMusicHoardLibrary for MusicHoard { fn rescan_library(&mut self) -> Result<(), Error> { - self.pre_commit = self.rescan_library_inner()?; + let mut database_cache = self.database.load()?; + Self::sort_albums_and_tracks(database_cache.iter_mut()); + + self.pre_commit = self.rescan_library_inner(database_cache)?; self.commit() } } impl MusicHoard { - fn rescan_library_inner(&mut self) -> Result { + fn rescan_library_inner(&mut self, database: Collection) -> Result { let items = self.library.list(&Query::new())?; self.library_cache = Self::items_to_artists(items)?; Self::sort_albums_and_tracks(self.library_cache.iter_mut()); - Ok(self.merge_collections()) + Ok(self.merge_collections(database)) } fn items_to_artists(items: Vec) -> Result { @@ -152,14 +155,21 @@ mod tests { .times(1) .return_once(|_| library_result); - database.expect_load().times(1).returning(|| Ok(vec![])); + // The database contents are not relevant in this test. + let mut seq = Sequence::new(); + database + .expect_load() + .times(1) + .in_sequence(&mut seq) + .returning(|| Ok(vec![])); database .expect_save() .with(predicate::eq(&*LIBRARY_COLLECTION)) .times(1) + .in_sequence(&mut seq) .return_once(|_| Ok(())); - let mut music_hoard = MusicHoard::new(database, library).unwrap(); + let mut music_hoard = MusicHoard::new(database, library); music_hoard.rescan_library().unwrap(); assert_eq!(music_hoard.get_collection(), &*LIBRARY_COLLECTION); diff --git a/src/core/musichoard/mod.rs b/src/core/musichoard/mod.rs index 3b3230d..bfe9c04 100644 --- a/src/core/musichoard/mod.rs +++ b/src/core/musichoard/mod.rs @@ -32,7 +32,6 @@ pub struct MusicHoard { collection: Collection, pre_commit: Collection, database: Database, - database_cache: Collection, library: Library, library_cache: Collection, } diff --git a/src/main.rs b/src/main.rs index c3cb2be..b7efbfa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -97,7 +97,7 @@ fn default_filter() -> CollectionFilter { fn with( builder: MusicHoardBuilder, ) { - let mut music_hoard = builder.build().expect("failed to initialise MusicHoard"); + let mut music_hoard = builder.build(); music_hoard.set_filter(default_filter()); // Initialize the terminal user interface. diff --git a/tests/lib.rs b/tests/lib.rs index 72f6417..3ae0b9e 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -31,7 +31,7 @@ fn merge_library_then_database() { let backend = JsonDatabaseFileBackend::new(&*database::json::DATABASE_TEST_FILE); let database = JsonDatabase::new(backend); - let mut music_hoard = MusicHoard::new(database, library).unwrap(); + let mut music_hoard = MusicHoard::new(database, library); music_hoard.rescan_library().unwrap(); music_hoard.reload_database().unwrap(); @@ -54,7 +54,7 @@ fn merge_database_then_library() { let backend = JsonDatabaseFileBackend::new(&*database::json::DATABASE_TEST_FILE); let database = JsonDatabase::new(backend); - let mut music_hoard = MusicHoard::new(database, library).unwrap(); + let mut music_hoard = MusicHoard::new(database, library); music_hoard.reload_database().unwrap(); music_hoard.rescan_library().unwrap();