feat(storage): use computed fields in favor of calling repair all the time (#311)

closes #308 

I'd like to make better use of SurrealDB's features for things like
automatically deleting orphaned items as well, but that's a task for
future me.

This does cause a major (breaking) change to the DB schema though so
v0.4.0 will be coming soon!

Somewhere along the line, I also fixed #301

Author: AnthonyMichaelTDM

cli/src/handlers/snapshots/mecomp_cli__handlers__smoke_tests__handlers_smoke_tests_test_playlist_command_case_02@stdout.snap

@@ -3,4 +3,4 @@ source: cli/src/handlers/smoke_tests.rs
 expression: "String::from_utf8(stdout.0.clone()).unwrap()"
 ---
 Daemon response:
-Err(Database("SurrealDB error: Serialization error: missing field `name`"))
+Ok("album added to playlist")

core/src/logger.rs

@@ -1,4 +1,4 @@
-use std::time::{Duration, Instant};
+use std::time::Instant;
 use std::{io::Write, sync::LazyLock};
 
 use env_logger::fmt::style::{RgbColor, Style};
@@ -259,7 +259,7 @@ pub fn init_tracing() -> impl tracing::Subscriber {
 
     #[cfg(feature = "tokio_console")]
     let console_layer = console_subscriber::Builder::default()
-        .retention(Duration::from_secs(60 * 20)) // 20 minutes
+        .retention(std::time::Duration::from_secs(60 * 20)) // 20 minutes
         .enable_self_trace(true)
         .spawn();
     #[cfg(feature = "tokio_console")]

daemon/Cargo.toml

@@ -63,7 +63,7 @@ tracing = { workspace = true }
 walkdir = { workspace = true }
 
 # MECOMP dependencies
-mecomp-core = { workspace = true, features = ["rpc", "audio", "tokio_console"] }
+mecomp-core = { workspace = true, features = ["rpc", "audio"] }
 mecomp-storage = { workspace = true, features = ["serde", "db"] }
 mecomp-analysis = { workspace = true, optional = true }
 one-or-many = { workspace = true }

daemon/src/controller.rs

@@ -1137,6 +1137,9 @@ impl MusicPlayer for MusicPlayerServer {
         let (parsed_name, song_paths) =
             import_playlist(file).tap_err(|e| warn!("Error in playlist_import: {e}"))?;
 
+        log::debug!("Parsed playlist name: {parsed_name:?}");
+        log::debug!("Parsed song paths: {song_paths:?}");
+
         let name = match (name, parsed_name) {
             (Some(name), _) | (None, Some(name)) => name,
             (None, None) => "Imported Playlist".to_owned(),

daemon/src/services/library.rs

@@ -85,7 +85,7 @@ pub async fn rescan<C: Connection>(
                     };
                     info!(
                         "{} has conflicting metadata with index, {log_postfix}",
-                        path.to_string_lossy(),
+                        path.display(),
                     );
 
                     match conflict_resolution_mode {
@@ -103,9 +103,8 @@ pub async fn rescan<C: Connection>(
                 // if we have an error, delete the song from the library
                 Err(e) => {
                     warn!(
-                        "Error reading metadata for {}: {}",
-                        path.to_string_lossy(),
-                        e
+                        "Error reading metadata for {}: {e}",
+                        path.display()
                     );
                     info!("assuming the file isn't a song or doesn't exist anymore, removing from library");
                     Song::delete(db, song.id).await?;
@@ -143,6 +142,8 @@ pub async fn rescan<C: Connection>(
 
             visited_paths.insert(path.path().to_owned());
 
+            let displayable_path = path.path().display();
+
             // if the file is a song, add it to the library
             match SongMetadata::load_from_path(
                 path.path().to_owned(),
@@ -151,14 +152,10 @@ pub async fn rescan<C: Connection>(
                 genre_separator,
             ) {
                 Ok(metadata) => Song::try_load_into_db(db, metadata).await.map_or_else(
-                    |e| warn!("Error indexing {}: {}", path.path().to_string_lossy(), e),
-                    |_| debug!("Indexed {}", path.path().to_string_lossy()),
-                ),
-                Err(e) => warn!(
-                    "Error reading metadata for {}: {}",
-                    path.path().to_string_lossy(),
-                    e
+                    |e| warn!("Error indexing {displayable_path}: {e}"),
+                    |_| debug!("Indexed {displayable_path}"),
                 ),
+                Err(e) => warn!("Error reading metadata for {displayable_path}: {e}"),
             }
         }
 
@@ -168,52 +165,34 @@ pub async fn rescan<C: Connection>(
     .await?;
 
     // find and delete any remaining orphaned albums and artists
-    // TODO: create a custom query for this
 
-    async {
-        for album in Album::read_all(db).await? {
-            if Album::repair(db, album.id.clone()).await? {
-                info!("Deleted orphaned album {}", album.id.clone());
-                Album::delete(db, album.id.clone()).await?;
-            }
-        }
-        <Result<(), Error>>::Ok(())
+    let orphans = Album::delete_orphaned(db)
+        .instrument(tracing::info_span!("Deleting orphaned albums"))
+        .await?;
+    if !orphans.is_empty() {
+        info!("Deleted orphaned albums: {orphans:?}");
     }
-    .instrument(tracing::info_span!("Repairing albums"))
-    .await?;
-    async {
-        for artist in Artist::read_all(db).await? {
-            if Artist::repair(db, artist.id.clone()).await? {
-                info!("Deleted orphaned artist {}", artist.id.clone());
-                Artist::delete(db, artist.id.clone()).await?;
-            }
-        }
-        <Result<(), Error>>::Ok(())
+
+    let orphans = Artist::delete_orphaned(db)
+        .instrument(tracing::info_span!("Repairing artists"))
+        .await?;
+    if !orphans.is_empty() {
+        info!("Deleted orphaned artists: {orphans:?}");
     }
-    .instrument(tracing::info_span!("Repairing artists"))
-    .await?;
-    async {
-        for collection in Collection::read_all(db).await? {
-            if Collection::repair(db, collection.id.clone()).await? {
-                info!("Deleted orphaned collection {}", collection.id.clone());
-                Collection::delete(db, collection.id.clone()).await?;
-            }
-        }
-        <Result<(), Error>>::Ok(())
+
+    let orphans = Collection::delete_orphaned(db)
+        .instrument(tracing::info_span!("Repairing collections"))
+        .await?;
+    if !orphans.is_empty() {
+        info!("Deleted orphaned collections: {orphans:?}");
     }
-    .instrument(tracing::info_span!("Repairing collections"))
-    .await?;
-    async {
-        for playlist in Playlist::read_all(db).await? {
-            if Playlist::repair(db, playlist.id.clone()).await? {
-                info!("Deleted orphaned playlist {}", playlist.id.clone());
-                Playlist::delete(db, playlist.id.clone()).await?;
-            }
-        }
-        <Result<(), Error>>::Ok(())
+
+    let orphans = Playlist::delete_orphaned(db)
+        .instrument(tracing::info_span!("Repairing playlists"))
+        .await?;
+    if !orphans.is_empty() {
+        info!("Deleted orphaned playlists: {orphans:?}");
     }
-    .instrument(tracing::info_span!("Repairing playlists"))
-    .await?;
 
     info!("Library rescan complete");
     info!("Library brief: {:?}", brief(db).await?);
@@ -747,6 +726,11 @@ mod tests {
         // check that the album and artist deleted
         assert_eq!(Song::read_all(&db).await.unwrap().len(), 0);
         assert_eq!(Album::read_all(&db).await.unwrap().len(), 0);
+        let artists = Artist::read_all(&db).await.unwrap();
+        for artist in artists {
+            assert_eq!(artist.album_count, 0);
+            assert_eq!(artist.song_count, 0);
+        }
         assert_eq!(Artist::read_all(&db).await.unwrap().len(), 0);
     }
 

storage/src/db/crud/album.rs

@@ -158,8 +158,6 @@ impl Album {
             .bind(("songs", song_ids))
             .await?;
 
-        Self::repair(db, id).await?;
-
         Ok(())
     }
 
@@ -181,12 +179,12 @@ impl Album {
         db: &Surreal<C>,
         id: AlbumId,
         song_ids: Vec<SongId>,
-    ) -> StorageResult<bool> {
+    ) -> StorageResult<()> {
         db.query(remove_songs())
             .bind(("album", id.clone()))
             .bind(("songs", song_ids))
             .await?;
-        Self::repair(db, id).await
+        Ok(())
     }
 
     #[instrument]
@@ -197,32 +195,15 @@ impl Album {
         Ok(db.query(read_artist()).bind(("id", id)).await?.take(0)?)
     }
 
-    /// update counts and runtime
+    /// Deletes all orphaned albums from the database
     ///
-    /// # Arguments
-    ///
-    /// * `id` - The id of the album to repair
-    ///
-    /// # Returns
-    ///
-    /// Returns a boolean indicating if the album should be removed (if it has no songs left in it)
-    #[instrument()]
-    pub async fn repair<C: Connection>(db: &Surreal<C>, id: AlbumId) -> StorageResult<bool> {
-        // remove or update the album and return
-        let songs = Self::read_songs(db, id.clone()).await?;
-
-        Self::update(
-            db,
-            id,
-            AlbumChangeSet {
-                song_count: Some(songs.len()),
-                runtime: Some(songs.iter().map(|s| s.runtime).sum()),
-                ..Default::default()
-            },
-        )
-        .await?;
-
-        Ok(songs.is_empty())
+    /// An orphaned album is an album that has no songs in it
+    #[instrument]
+    pub async fn delete_orphaned<C: Connection>(db: &Surreal<C>) -> StorageResult<Vec<Self>> {
+        Ok(db
+            .query("DELETE FROM album WHERE type::int(song_count) = 0 RETURN BEFORE")
+            .await?
+            .take(0)?)
     }
 }
 
@@ -551,8 +532,11 @@ mod tests {
             .ok_or_else(|| anyhow!("Failed to create song"))?;
 
         Album::add_songs(&db, album.id.clone(), vec![song.id.clone()]).await?;
-        assert!(Album::remove_songs(&db, album.id.clone(), vec![song.id.clone()]).await?);
+        let read = Album::read_songs(&db, album.id.clone()).await?;
+        assert_eq!(read.len(), 1);
+        assert_eq!(read[0], song);
 
+        Album::remove_songs(&db, album.id.clone(), vec![song.id.clone()]).await?;
         let read = Album::read_songs(&db, album.id.clone()).await?;
         assert_eq!(read.len(), 0);
         Ok(())