Fetching and then instantly reloading library loses data (#256)

Closes #254

Reviewed-on: #256

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

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<Item = &'a mut Artist>>(collection: C);
 
-    fn merge_collections(&self) -> Collection;
+    fn merge_collections<It: IntoIterator<Item = Artist>>(&self, database: It) -> Collection;
     fn filter_collection(&self) -> Collection;
     fn filter_artist(&self, artist: &Artist) -> Option<Artist>;
 
@@ -69,7 +69,7 @@ impl<Database, Library> IMusicHoardBasePrivate for MusicHoard<Database, Library>
         }
     }
 
-    fn merge_collections(&self) -> Collection {
+    fn merge_collections<It: IntoIterator<Item = Artist>>(&self, database: It) -> Collection {
         let mut primary = NormalMap::<Artist>::new();
         let mut secondary = NormalMap::<Artist>::new();
 
@@ -77,7 +77,7 @@ impl<Database, Library> IMusicHoardBasePrivate for MusicHoard<Database, Library>
             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);
     }
 

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<NoDatabase, NoLibrary> {
             collection: vec![],
             pre_commit: vec![],
             database: NoDatabase,
-            database_cache: vec![],
             library: NoLibrary,
             library_cache: vec![],
         }
@@ -92,7 +89,6 @@ impl<Library: ILibrary> MusicHoard<NoDatabase, Library> {
             collection: vec![],
             pre_commit: vec![],
             database: NoDatabase,
-            database_cache: vec![],
             library,
             library_cache: vec![],
         }
@@ -101,59 +97,56 @@ impl<Library: ILibrary> MusicHoard<NoDatabase, Library> {
 
 impl<Database: IDatabase> MusicHoardBuilder<Database, NoLibrary> {
     /// Build [`MusicHoard`] with the currently set library and database.
-    pub fn build(self) -> Result<MusicHoard<Database, NoLibrary>, Error> {
+    pub fn build(self) -> MusicHoard<Database, NoLibrary> {
         MusicHoard::database(self.database)
     }
 }
 
 impl<Database: IDatabase> MusicHoard<Database, NoLibrary> {
     /// Create a new [`MusicHoard`] with the provided [`IDatabase`] and no library.
-    pub fn database(database: Database) -> Result<Self, Error> {
-        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<Database: IDatabase, Library: ILibrary> MusicHoardBuilder<Database, Library> {
     /// Build [`MusicHoard`] with the currently set library and database.
-    pub fn build(self) -> Result<MusicHoard<Database, Library>, Error> {
+    pub fn build(self) -> MusicHoard<Database, Library> {
         MusicHoard::new(self.database, self.library)
     }
 }
 
 impl<Database: IDatabase, Library: ILibrary> MusicHoard<Database, Library> {
     /// Create a new [`MusicHoard`] with the provided [`ILibrary`] and [`IDatabase`].
-    pub fn new(database: Database, library: Library) -> Result<Self, Error> {
-        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::{
+    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());
     }

src/core/musichoard/database.rs

@@ -116,10 +116,10 @@ pub trait IMusicHoardDatabase {
 
 impl<Database: IDatabase, Library> IMusicHoardDatabase for MusicHoard<Database, Library> {
     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<MockIDatabase, NoLibrary>;
-        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"))

src/core/musichoard/library.rs

@@ -23,25 +23,28 @@ pub trait IMusicHoardLibrary {
 
 impl<Library: ILibrary> IMusicHoardLibrary for MusicHoard<NoDatabase, Library> {
     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<Database: IDatabase, Library: ILibrary> IMusicHoardLibrary for MusicHoard<Database, Library> {
     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<Database, Library: ILibrary> MusicHoard<Database, Library> {
-    fn rescan_library_inner(&mut self) -> Result<Collection, Error> {
+    fn rescan_library_inner(&mut self, database: Collection) -> Result<Collection, Error> {
         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<Item>) -> Result<Collection, Error> {
@@ -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);

src/core/musichoard/mod.rs

@@ -32,7 +32,6 @@ pub struct MusicHoard<Database, Library> {
     collection: Collection,
     pre_commit: Collection,
     database: Database,
-    database_cache: Collection,
     library: Library,
     library_cache: Collection,
 }

src/main.rs

@@ -97,7 +97,7 @@ fn default_filter() -> CollectionFilter {
 fn with<Database: IDatabase + 'static, Library: ILibrary + 'static>(
     builder: MusicHoardBuilder<Database, Library>,
 ) {
-    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.

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