MISC: Apply some of the remarks

lektor_nkdb/src/database/epoch.rs

@@ -4,7 +4,7 @@
 use crate::*;
 use hashbrown::hash_map;
 use serde::{Deserialize, Serialize};
-use std::{ffi::OsStr, path::Path};
+use std::{ffi::OsStr, mem, path::Path};
 
 /// The epoch contains all available karas at a certain point in time. It can be submitted and
 /// available to all readers of the database or unsubmited and only one writter can edit it.
@@ -105,9 +105,7 @@ impl Epoch {
     /// Replace the karas in the epoch by the provided new value
     /// Return the previous karas value
     pub fn data_replace(&mut self, new_data: EpochData) -> EpochData {
-        let ret = self.data.clone();
-        self.data = new_data;
-        ret
+        mem::replace(&mut self.data, new_data)
     }
 
     /// Get the number of the epoch

lektor_nkdb/src/database/pool.rs

@@ -105,6 +105,13 @@ impl From<PoolSerDeProxy> for Pool {
     }
 }
 
+impl From<Pool> for PoolSerDeProxy {
+    fn from(value: Pool) -> Self {
+        let Pool { kara_mapping, .. } = value;
+        Self { kara_mapping }
+    }
+}
+
 impl Default for Pool {
     fn default() -> Self {
         Self::from_iter(std::iter::empty::<(KId, RemoteKId, KaraState)>())

lektor_nkdb/src/database/update.rs

@@ -253,15 +253,8 @@ impl<'a, Storage: DatabaseStorage> UpdateHandler<'a, Storage> {
         .await
     }
 
-    pub async fn delete_kara_v2<'b>(&'b self, repo: &str, rkid: u64) -> Result<KId>
-    where
-        'b: 'a,
-    {
-        if let (Some(id), _) = self
-            .pool
-            .read()
-            .await
-            .get_from_remote(RemoteKId::new(rkid, repo))
+    pub async fn delete_kara_v2(&self, repo: &str, rkid: u64) -> Result<KId> {
+        if let (Some(id), _) = (self.pool.read().await).get_from_remote(RemoteKId::new(rkid, repo))
         {
             self.new_epoch.borrow_mut().content().data_mut().remove(&id);
             Ok(id)

lektor_nkdb/src/storage/test_storage.rs

@@ -5,7 +5,6 @@
 use crate::database::pool::Pool;
 use crate::{database::epoch::*, id::*, storage::DatabaseStorage, Playlist};
 use anyhow::{Context, Result};
-use futures::future;
 use kurisu_api::SHA256;
 use tokio::sync::RwLock;
 

lektor_repo/src/lib.rs

@@ -22,6 +22,13 @@ pub struct Repo {
     updating: AtomicU8,
 }
 
+#[derive(Debug, Default, Eq, PartialEq)]
+pub struct UpdateCounts {
+    pub updated: usize,
+    pub deleted: usize,
+    pub downloaded: usize,
+}
+
 impl Repo {
     /// Create a new repo downloader from the config.
     pub fn new(config: Vec<LektorRepoConfig>) -> Self {
@@ -75,25 +82,24 @@ impl Repo {
         let updated_list = match download_kara_list(&repo_config).await {
             Err(err) => {
                 log::error!("failed to download kara list for {}: {err}", repo_name);
-                vec![]
+                Default::default()
             }
 
             Ok(KaraList {
-                dbepoch,
-                dbepoch_validity,
-                karas: _,
-            }) if dbepoch != repo_info.epoch()
-                || dbepoch_validity != repo_info.epoch_validity() =>
-            {
-                log::error!("dbepoch or dbepoch_validity from karalist not matching those in dbinfo: got ({dbepoch},{dbepoch_validity}), expected ({},{}). Either it took dbinfo and karalist from 2 different repos, and then you need to fix your config, or the repo was modified between our 2 calls, and then you can retry the update operation now", repo_info.epoch(), repo_info.epoch_validity());
-                vec![]
+                dbepoch: epoch,
+                dbepoch_validity: validity,
+                ..
+            }) if epoch != repo_info.epoch() || validity != repo_info.epoch_validity() => {
+                log::error!(
+                    "epoch or epoch_validity not matching those in infos, got ({epoch},{validity}), expected ({},{}). {}",
+                    repo_info.epoch(),
+                    repo_info.epoch_validity(),
+                    "You may want to check out if the repo was modified live, or you may need to fix your config"
+                );
+                Default::default()
             }
 
-            Ok(KaraList {
-                dbepoch: _,
-                dbepoch_validity: _,
-                karas,
-            }) => karas,
+            Ok(KaraList { karas, .. }) => karas,
         };
 
         self.update_epochs_from_karas(handler, updated_list, repo_name)
@@ -129,7 +135,11 @@ impl Repo {
                 || got_since != since
                 || got_dbepoch != repo_info.epoch() =>
             {
-                log::error!("dbepoch or dbepoch_validity from karalist not matching those in changediff: got ({got_dbepoch},{got_dbepoch_validity},{got_since}), expected ({},{},{since}). Either it took dbinfo and changediff from 2 different repos, and then you need to fix your config, or the repo was modified between our 2 calls, and then you can retry the update operation now", repo_info.epoch(), repo_info.epoch_validity());
+                log::error!(
+                    "dbepoch or dbepoch_validity from karalist not matching those in changediff: got ({got_dbepoch},{got_dbepoch_validity},{got_since}), expected ({},{},{since}). Either it took dbinfo and changediff from 2 different repos, and then you need to fix your config, or the repo was modified between our 2 calls, and then you can retry the update operation now",
+                    repo_info.epoch(),
+                    repo_info.epoch_validity()
+                );
                 return (vec![], vec![]);
             }
 
@@ -202,66 +212,59 @@ impl Repo {
             .collect()
     }
 
-    /// Update the database throu the handler and returns the number of downloaded karas.
+    /// Update the database through the handler and returns the number of downloaded karas.
     pub async fn update_with<'a, Storage: DatabaseStorage + 'static>(
         &self,
         handler: &'a UpdateHandler<'a, Storage>,
-    ) -> Result<Option<(usize, usize, usize)>> {
+    ) -> Result<Option<UpdateCounts>> {
         match self
             .updating
             .compare_exchange(0, 1, Ordering::Acquire, Ordering::Relaxed)
         {
             Err(1) => bail!("repo is already updating"),
             Ok(0) => {
-                log::info!("begin the download process");
+                let (config, infos) = get_freshest_repo(&self.configs)
+                    .await
+                    .context("could not find any valid repo to update, aborting")?;
 
-                if let Some((repo_config, repo_info)) = get_freshest_repo(&self.configs).await {
-                    let repo_name = repo_config.name.as_str();
                 log::info!(
                     "selecting repo {} because it is the most up to date (epoch = {})",
-                        repo_name,
-                        repo_info.epoch(),
+                    config.name.as_str(),
+                    infos.epoch(),
                 );
 
-                    if handler.is_up_to_date(repo_info.clone().into()).await {
+                if handler.is_up_to_date(infos.clone().into()).await {
                     log::info!("already up to date against remote repo");
                     return Ok(None);
                 }
 
-                    handler.set_epoch_infos(repo_info.clone().into()).await;
+                handler.set_epoch_infos(infos.clone().into()).await;
 
-                    let (updated, deleted) =
-                        match handler.recoverable_since(repo_info.clone().into()).await {
+                let (updated, deleted) = match handler.recoverable_since(infos.clone().into()).await
+                {
                     Some(since) => {
-                                let _ = handler
-                                    .copy_last_epoch_data()
-                                    .await
-                                    .context("unable to copy data from last epoch");
+                        (handler.copy_last_epoch_data().await)
+                            .context("unable to copy data from last epoch")?;
 
-                                self.do_epoch_with_changediff(
-                                    handler,
-                                    repo_config.clone(),
-                                    repo_info,
-                                    since,
-                                )
+                        self.do_epoch_with_changediff(handler, config.clone(), infos, since)
                             .await
                     }
 
                     None => (
-                                self.do_epoch_from_scratch(handler, repo_config.clone(), repo_info)
+                        self.do_epoch_from_scratch(handler, config.clone(), infos)
                             .await,
                         vec![],
                     ),
                 };
 
-                    let updated_count = updated.len();
-                    let downloaded = self.do_downloads(updated, handler, repo_config).await;
+                let counts = UpdateCounts {
+                    updated: updated.len(),
+                    deleted: deleted.len(),
+                    downloaded: self.do_downloads(updated, handler, config).await.len(),
+                };
 
                 self.updating.store(0, Ordering::SeqCst);
-                    Ok(Some((updated_count, deleted.len(), downloaded.len())))
-                } else {
-                    bail!("Could not find any valid repo, aborting");
-                }
+                Ok(Some(counts))
             }
             state => panic!("repo is in incoherent state, update flag is {state:?}"),
         }

lektord/src/app/routes.rs

@@ -16,6 +16,7 @@ use axum::{
 };
 use lektor_nkdb::*;
 use lektor_payloads::*;
+use lektor_repo::UpdateCounts;
 use lektor_search::KaraStore;
 use lektor_utils::decode_base64_json;
 use rand::seq::SliceRandom;
@@ -276,8 +277,7 @@ pub(crate) async fn adm_update(
         local.spawn_local(async move {
             match tokio::task::spawn_local(async move {
                 let handle = state.database.update().await;
-                let updated = state.repo.update_with(&handle).await;
-                match updated {
+                match state.repo.update_with(&handle).await {
                     Ok(Some(counts)) => {
                         handle.finished().await;
                         Ok::<_, anyhow::Error>(counts)
@@ -285,7 +285,7 @@ pub(crate) async fn adm_update(
 
                     Ok(None) => {
                         log::info!("update had nothing to do, no need to write updated info");
-                        Ok::<_, anyhow::Error>((0, 0, 0))
+                        Ok::<_, anyhow::Error>(Default::default())
                     },
 
                     Err(err) => Err(err)
@@ -294,7 +294,9 @@ pub(crate) async fn adm_update(
             .await
             .map_err(|err| anyhow!("{err}"))
             {
-                Ok(Ok((updated, deleted, downloaded))) => log::info!("finished updating database by updating {updated} karas: ({downloaded} of which were downloaded) and deleting {deleted} karas"),
+                Ok(Ok(UpdateCounts { updated, deleted, downloaded })) => {
+                    log::info!("finished updating {updated} karas - {downloaded} of which were downloaded - and deleting {deleted} karas")
+                },
                 Err(err) | Ok(Err(err)) => log::error!("{err}"),
             }
         });