Improve error logging in project delete route (#5388)

* Improve error logging in project delete route

* remove_documents more error logging

* fix ci

* try fix ci? idk man

Author: aecsocket

apps/labrinth/src/routes/v2/projects.rs

@@ -928,6 +928,7 @@ pub async fn project_delete(
         session_queue,
     )
     .await
+    .map(|()| HttpResponse::NoContent().body(""))
     .or_else(v2_reroute::flatten_404_error)
 }
 

apps/labrinth/src/routes/v3/projects.rs

@@ -29,12 +29,14 @@ use crate::queue::session::AuthQueue;
 use crate::routes::ApiError;
 use crate::search::indexing::remove_documents;
 use crate::search::{SearchConfig, SearchError, search_for_project};
+use crate::util::error::Context;
 use crate::util::img;
 use crate::util::img::{delete_old_images, upload_image_optimized};
 use crate::util::routes::read_limited_from_payload;
 use crate::util::validate::validation_errors_to_string;
 use actix_web::{HttpRequest, HttpResponse, web};
 use chrono::Utc;
+use eyre::eyre;
 use futures::TryStreamExt;
 use itertools::Itertools;
 use serde::{Deserialize, Serialize};
@@ -980,7 +982,8 @@ pub async fn project_edit(
                 .collect::<Vec<_>>(),
             &search_config,
         )
-        .await?;
+        .await
+        .wrap_internal_err("failed to remove documents")?;
     }
 
     Ok(HttpResponse::NoContent().body(""))
@@ -2157,25 +2160,29 @@ pub async fn project_delete(
     redis: web::Data<RedisPool>,
     search_config: web::Data<SearchConfig>,
     session_queue: web::Data<AuthQueue>,
-) -> Result<HttpResponse, ApiError> {
-    let user = get_user_from_headers(
+) -> Result<(), ApiError> {
+    let (_, user) = get_user_from_headers(
         &req,
         &**pool,
         &redis,
         &session_queue,
         Scopes::PROJECT_DELETE,
     )
-    .await?
-    .1;
+    .await?;
     let string = info.into_inner().0;
 
+    // In two cases, we return `The specified project does not exist!`:
+    // - the project really doesn't exist
+    // - the project is hidden from the user
+    //
+    // We use an `ApiError::Auth` for this case instead of a `ApiError::Request`,
+    // because our permissions tests assert that failing under the 2nd use                  case
+    // gives a 401 or 404, but `Request` gives only a 400.
+
     let project = db_models::DBProject::get(&string, &**pool, &redis)
-        .await?
-        .ok_or_else(|| {
-            ApiError::InvalidInput(
-                "The specified project does not exist!".to_string(),
-            )
-        })?;
+        .await
+        .wrap_internal_err("failed to get project")?
+        .wrap_auth_err("The specified project does not exist!")?;
 
     if !user.role.is_admin() {
         let (team_member, organization_team_member) =
@@ -2184,13 +2191,14 @@ pub async fn project_delete(
                 user.id.into(),
                 &**pool,
             )
-            .await?;
+            .await
+            .wrap_internal_err("failed to get user team member permissions")?;
 
         // Hide the project
         if team_member.is_none() && organization_team_member.is_none() {
-            return Err(ApiError::CustomAuthentication(
-                "The specified project does not exist!".to_string(),
-            ));
+            return Err(ApiError::Auth(eyre!(
+                "The specified project does not exist!"
+            )));
         }
 
         let permissions = ProjectPermissions::get_permissions_by_role(
@@ -2207,15 +2215,23 @@ pub async fn project_delete(
         }
     }
 
-    let mut transaction = pool.begin().await?;
+    let mut transaction = pool
+        .begin()
+        .await
+        .wrap_internal_err("failed to start transaction")?;
     let context = ImageContext::Project {
         project_id: Some(project.inner.id.into()),
     };
     let uploaded_images =
         db_models::DBImage::get_many_contexted(context, &mut transaction)
-            .await?;
+            .await
+            .wrap_internal_err("failed to get project images")?;
     for image in uploaded_images {
-        image_item::DBImage::remove(image.id, &mut transaction, &redis).await?;
+        image_item::DBImage::remove(image.id, &mut transaction, &redis)
+            .await
+            .wrap_internal_err_with(|| {
+                eyre!("failed to remove project image `{:?}`", image.id)
+            })?;
     }
 
     sqlx::query!(
@@ -2226,16 +2242,21 @@ pub async fn project_delete(
         project.inner.id as db_ids::DBProjectId,
     )
     .execute(&mut transaction)
-    .await?;
+    .await
+    .wrap_internal_err("failed to delete project from collections_mods")?;
 
     let result = db_models::DBProject::remove(
         project.inner.id,
         &mut transaction,
         &redis,
     )
-    .await?;
+    .await
+    .wrap_internal_err("failed to remove project")?;
 
-    transaction.commit().await?;
+    transaction
+        .commit()
+        .await
+        .wrap_internal_err("failed to commit transaction")?;
 
     remove_documents(
         &project
@@ -2245,10 +2266,11 @@ pub async fn project_delete(
             .collect::<Vec<_>>(),
         &search_config,
     )
-    .await?;
+    .await
+    .wrap_internal_err("failed to remove project version documents")?;
 
     if result.is_some() {
-        Ok(HttpResponse::NoContent().body(""))
+        Ok(())
     } else {
         Err(ApiError::NotFound)
     }

apps/labrinth/src/routes/v3/versions.rs

@@ -27,6 +27,7 @@ use crate::models::teams::ProjectPermissions;
 use crate::queue::session::AuthQueue;
 use crate::search::SearchConfig;
 use crate::search::indexing::remove_documents;
+use crate::util::error::Context;
 use crate::util::img;
 use crate::util::validate::validation_errors_to_string;
 use actix_web::{HttpRequest, HttpResponse, web};
@@ -985,7 +986,9 @@ pub async fn version_delete(
         &redis,
     )
     .await?;
-    remove_documents(&[version.inner.id.into()], &search_config).await?;
+    remove_documents(&[version.inner.id.into()], &search_config)
+        .await
+        .wrap_internal_err("failed to remove documents")?;
 
     if result.is_some() {
         Ok(HttpResponse::NoContent().body(""))

apps/labrinth/src/search/indexing/mod.rs

@@ -6,7 +6,9 @@ use std::time::Duration;
 use crate::database::PgPool;
 use crate::database::redis::RedisPool;
 use crate::search::{SearchConfig, UploadSearchProject};
+use crate::util::error::Context;
 use ariadne::ids::base62_impl::to_base62;
+use eyre::eyre;
 use futures::StreamExt;
 use futures::stream::FuturesOrdered;
 use local_import::index_local;
@@ -52,17 +54,23 @@ fn search_operation_timeout() -> std::time::Duration {
 pub async fn remove_documents(
     ids: &[crate::models::ids::VersionId],
     config: &SearchConfig,
-) -> Result<(), IndexingError> {
-    let mut indexes = get_indexes_for_indexing(config, false, false).await?;
-    let indexes_next = get_indexes_for_indexing(config, true, false).await?;
+) -> eyre::Result<()> {
+    let mut indexes = get_indexes_for_indexing(config, false, false)
+        .await
+        .wrap_err("failed to get current indexes")?;
+    let indexes_next = get_indexes_for_indexing(config, true, false)
+        .await
+        .wrap_err("failed to get next indexes")?;
 
     for list in &mut indexes {
         for alt_list in &indexes_next {
             list.extend(alt_list.iter().cloned());
         }
     }
 
-    let client = config.make_batch_client()?;
+    let client = config
+        .make_batch_client()
+        .wrap_err("failed to create batch client")?;
     let client = &client;
 
     let ids_base62 = ids.iter().map(|x| to_base62(x.0)).collect::<Vec<_>>();
@@ -75,13 +83,19 @@ pub async fn remove_documents(
             deletion_tasks.push_back(async move {
                 index
                     .delete_documents(ids_base62_ref)
-                    .await?
+                    .await
+                    .wrap_err_with(|| {
+                        eyre!("failed to request to delete documents {ids_base62_ref:?}")
+                    })?
                     .wait_for_completion(
                         &owned_client,
                         None,
                         Some(Duration::from_secs(15)),
                     )
                     .await
+                    .wrap_err_with(|| {
+                        eyre!("failed to delete documents {ids_base62_ref:?}")
+                    })
             });
         }
     });