From 9bc9a7af1f4e9f9f3f15326345d1fd3decb130f8 Mon Sep 17 00:00:00 2001 From: aecsocket <43144841+aecsocket@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:08:47 +0100 Subject: [PATCH 1/2] fix: unfollow race condition --- ...1fe2f8a5e6edd57f4b325c5842c6571eb16b4.json | 23 --- ...034c7a4450e5e89d24a6f2e2b326da6abcbe.json} | 4 +- ...244fa341ff98f31b3eb1010697e3939ab6d4.json} | 4 +- apps/labrinth/src/routes/v3/projects.rs | 171 +++++++++--------- apps/labrinth/tests/project.rs | 97 +++++++++- 5 files changed, 181 insertions(+), 118 deletions(-) delete mode 100644 apps/labrinth/.sqlx/query-0fb1cca8a2a37107104244953371fe2f8a5e6edd57f4b325c5842c6571eb16b4.json rename apps/labrinth/.sqlx/{query-371048e45dd74c855b84cdb8a6a565ccbef5ad166ec9511ab20621c336446da6.json => query-92f99f640e78d43d03b9ccd9f206034c7a4450e5e89d24a6f2e2b326da6abcbe.json} (60%) rename apps/labrinth/.sqlx/{query-c55d2132e3e6e92dd50457affab758623dca175dc27a2d3cd4aace9cfdecf789.json => query-b4a8dcdf9db2a62c445383600cbd244fa341ff98f31b3eb1010697e3939ab6d4.json} (58%) diff --git a/apps/labrinth/.sqlx/query-0fb1cca8a2a37107104244953371fe2f8a5e6edd57f4b325c5842c6571eb16b4.json b/apps/labrinth/.sqlx/query-0fb1cca8a2a37107104244953371fe2f8a5e6edd57f4b325c5842c6571eb16b4.json deleted file mode 100644 index 99c9329876..0000000000 --- a/apps/labrinth/.sqlx/query-0fb1cca8a2a37107104244953371fe2f8a5e6edd57f4b325c5842c6571eb16b4.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n SELECT EXISTS(SELECT 1 FROM mod_follows mf WHERE mf.follower_id = $1 AND mf.mod_id = $2)\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "exists", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Int8", - "Int8" - ] - }, - "nullable": [ - null - ] - }, - "hash": "0fb1cca8a2a37107104244953371fe2f8a5e6edd57f4b325c5842c6571eb16b4" -} diff --git a/apps/labrinth/.sqlx/query-371048e45dd74c855b84cdb8a6a565ccbef5ad166ec9511ab20621c336446da6.json b/apps/labrinth/.sqlx/query-92f99f640e78d43d03b9ccd9f206034c7a4450e5e89d24a6f2e2b326da6abcbe.json similarity index 60% rename from apps/labrinth/.sqlx/query-371048e45dd74c855b84cdb8a6a565ccbef5ad166ec9511ab20621c336446da6.json rename to apps/labrinth/.sqlx/query-92f99f640e78d43d03b9ccd9f206034c7a4450e5e89d24a6f2e2b326da6abcbe.json index e67642945e..2cdb8bc652 100644 --- a/apps/labrinth/.sqlx/query-371048e45dd74c855b84cdb8a6a565ccbef5ad166ec9511ab20621c336446da6.json +++ b/apps/labrinth/.sqlx/query-92f99f640e78d43d03b9ccd9f206034c7a4450e5e89d24a6f2e2b326da6abcbe.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n UPDATE mods\n SET follows = follows - 1\n WHERE id = $1\n ", + "query": "\n UPDATE mods\n SET follows = GREATEST(follows - 1, 0)\n WHERE id = $1\n ", "describe": { "columns": [], "parameters": { @@ -10,5 +10,5 @@ }, "nullable": [] }, - "hash": "371048e45dd74c855b84cdb8a6a565ccbef5ad166ec9511ab20621c336446da6" + "hash": "92f99f640e78d43d03b9ccd9f206034c7a4450e5e89d24a6f2e2b326da6abcbe" } diff --git a/apps/labrinth/.sqlx/query-c55d2132e3e6e92dd50457affab758623dca175dc27a2d3cd4aace9cfdecf789.json b/apps/labrinth/.sqlx/query-b4a8dcdf9db2a62c445383600cbd244fa341ff98f31b3eb1010697e3939ab6d4.json similarity index 58% rename from apps/labrinth/.sqlx/query-c55d2132e3e6e92dd50457affab758623dca175dc27a2d3cd4aace9cfdecf789.json rename to apps/labrinth/.sqlx/query-b4a8dcdf9db2a62c445383600cbd244fa341ff98f31b3eb1010697e3939ab6d4.json index 1d66c5b202..7adbadc2c3 100644 --- a/apps/labrinth/.sqlx/query-c55d2132e3e6e92dd50457affab758623dca175dc27a2d3cd4aace9cfdecf789.json +++ b/apps/labrinth/.sqlx/query-b4a8dcdf9db2a62c445383600cbd244fa341ff98f31b3eb1010697e3939ab6d4.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n INSERT INTO mod_follows (follower_id, mod_id)\n VALUES ($1, $2)\n ", + "query": "\n INSERT INTO mod_follows (follower_id, mod_id)\n VALUES ($1, $2)\n ON CONFLICT DO NOTHING\n ", "describe": { "columns": [], "parameters": { @@ -11,5 +11,5 @@ }, "nullable": [] }, - "hash": "c55d2132e3e6e92dd50457affab758623dca175dc27a2d3cd4aace9cfdecf789" + "hash": "b4a8dcdf9db2a62c445383600cbd244fa341ff98f31b3eb1010697e3939ab6d4" } diff --git a/apps/labrinth/src/routes/v3/projects.rs b/apps/labrinth/src/routes/v3/projects.rs index ee99c2b059..5e00ab17f1 100644 --- a/apps/labrinth/src/routes/v3/projects.rs +++ b/apps/labrinth/src/routes/v3/projects.rs @@ -3318,7 +3318,7 @@ pub async fn project_follow_internal( .1; let string = info.into_inner().0; - let result = db_models::DBProject::get(&string, &**pool, &redis) + let project = db_models::DBProject::get(&string, &**pool, &redis) .await .wrap_api_err("fetching project from database")? .wrap_request_err_with(|| { @@ -3326,68 +3326,66 @@ pub async fn project_follow_internal( })?; let user_id: db_ids::DBUserId = user.id.into(); - let project_id: db_ids::DBProjectId = result.inner.id; + let project_id: db_ids::DBProjectId = project.inner.id; - if !is_visible_project(&result.inner, &Some(user), &pool, false) + if !is_visible_project(&project.inner, &Some(user), &pool, false) .await .wrap_api_err("checking project visibility")? { return Err(ApiError::NotFound(eyre::eyre!("resource not found"))); } - let following = sqlx::query!( + let mut transaction = pool + .begin() + .await + .wrap_internal_err("starting database transaction")?; + + let insert_result = sqlx::query!( " - SELECT EXISTS(SELECT 1 FROM mod_follows mf WHERE mf.follower_id = $1 AND mf.mod_id = $2) - ", + INSERT INTO mod_follows (follower_id, mod_id) + VALUES ($1, $2) + ON CONFLICT DO NOTHING + ", user_id as db_ids::DBUserId, project_id as db_ids::DBProjectId ) - .fetch_one(&**pool) - .await.wrap_internal_err("fetching project follow status from database")? - .exists - .unwrap_or(false); - - if !following { - let mut transaction = pool - .begin() - .await - .wrap_internal_err("starting database transaction")?; + .execute(&mut transaction) + .await + .wrap_internal_err("querying database for `project_follow_internal`")?; - sqlx::query!( - " + if insert_result.rows_affected() == 0 { + return Err(ApiError::Request(eyre::eyre!( + "you are already following this project", + ))); + } + + sqlx::query!( + " UPDATE mods SET follows = follows + 1 WHERE id = $1 ", - project_id as db_ids::DBProjectId, - ) - .execute(&mut transaction) - .await - .wrap_internal_err("querying database for `project_follow_internal`")?; + project_id as db_ids::DBProjectId, + ) + .execute(&mut transaction) + .await + .wrap_internal_err("querying database for `project_follow_internal`")?; - sqlx::query!( - " - INSERT INTO mod_follows (follower_id, mod_id) - VALUES ($1, $2) - ", - user_id as db_ids::DBUserId, - project_id as db_ids::DBProjectId - ) - .execute(&mut transaction) + transaction + .commit() .await - .wrap_internal_err("querying database for `project_follow_internal`")?; + .wrap_internal_err("committing database transaction")?; - transaction - .commit() - .await - .wrap_internal_err("committing database transaction")?; + db_models::DBProject::clear_cache( + project_id, + project.inner.slug, + None, + &redis, + ) + .await + .wrap_internal_err("clearing cached project data")?; - Ok(HttpResponse::NoContent().body("")) - } else { - Err(ApiError::Request(eyre::eyre!( - "You are already following this project!", - ))) - } + Ok(HttpResponse::NoContent().body("")) } /// Unfollow a project. @@ -3425,7 +3423,7 @@ pub async fn project_unfollow_internal( .1; let string = info.into_inner().0; - let result = db_models::DBProject::get(&string, &**pool, &redis) + let project = db_models::DBProject::get(&string, &**pool, &redis) .await .wrap_api_err("fetching project from database")? .wrap_request_err_with(|| { @@ -3433,65 +3431,58 @@ pub async fn project_unfollow_internal( })?; let user_id: db_ids::DBUserId = user.id.into(); - let project_id = result.inner.id; + let project_id = project.inner.id; - let following = sqlx::query!( + let mut transaction = pool + .begin() + .await + .wrap_internal_err("starting database transaction")?; + + let delete_result = sqlx::query!( " - SELECT EXISTS(SELECT 1 FROM mod_follows mf WHERE mf.follower_id = $1 AND mf.mod_id = $2) - ", + DELETE FROM mod_follows + WHERE follower_id = $1 AND mod_id = $2 + ", user_id as db_ids::DBUserId, project_id as db_ids::DBProjectId ) - .fetch_one(&**pool) - .await.wrap_internal_err("fetching project follow status from database")? - .exists - .unwrap_or(false); - - if following { - let mut transaction = pool - .begin() - .await - .wrap_internal_err("starting database transaction")?; + .execute(&mut transaction) + .await + .wrap_internal_err("querying database for `project_unfollow_internal`")?; - sqlx::query!( - " + if delete_result.rows_affected() == 0 { + return Err(ApiError::Request(eyre::eyre!( + "you are not following this project", + ))); + } + + sqlx::query!( + " UPDATE mods - SET follows = follows - 1 + SET follows = GREATEST(follows - 1, 0) WHERE id = $1 ", - project_id as db_ids::DBProjectId, - ) - .execute(&mut transaction) - .await - .wrap_internal_err( - "querying database for `project_unfollow_internal`", - )?; + project_id as db_ids::DBProjectId, + ) + .execute(&mut transaction) + .await + .wrap_internal_err("querying database for `project_unfollow_internal`")?; - sqlx::query!( - " - DELETE FROM mod_follows - WHERE follower_id = $1 AND mod_id = $2 - ", - user_id as db_ids::DBUserId, - project_id as db_ids::DBProjectId - ) - .execute(&mut transaction) + transaction + .commit() .await - .wrap_internal_err( - "querying database for `project_unfollow_internal`", - )?; + .wrap_internal_err("committing database transaction")?; - transaction - .commit() - .await - .wrap_internal_err("committing database transaction")?; + db_models::DBProject::clear_cache( + project_id, + project.inner.slug, + None, + &redis, + ) + .await + .wrap_internal_err("clearing cached project data")?; - Ok(HttpResponse::NoContent().body("")) - } else { - Err(ApiError::Request(eyre::eyre!( - "You are not following this project!", - ))) - } + Ok(HttpResponse::NoContent().body("")) } /// Get a project's organization. diff --git a/apps/labrinth/tests/project.rs b/apps/labrinth/tests/project.rs index 239ce885f0..11408d70fc 100644 --- a/apps/labrinth/tests/project.rs +++ b/apps/labrinth/tests/project.rs @@ -6,7 +6,9 @@ use common::dummy_data::DUMMY_CATEGORIES; use crate::common::api_common::models::CommonProject; use crate::common::api_common::request_data::ProjectCreationRequestData; -use crate::common::api_common::{ApiProject, ApiTeams, ApiVersion}; +use crate::common::api_common::{ + ApiProject, ApiTeams, ApiVersion, AppendsOptionalPat, +}; use crate::common::dummy_data::{ DummyImage, DummyOrganizationZeta, DummyProjectAlpha, DummyProjectBeta, TestFile, @@ -109,6 +111,99 @@ async fn test_get_project() { .await; } +#[actix_rt::test] +async fn test_project_follow_counter() { + with_test_environment( + Some(8), + |test_env: TestEnvironment| async move { + let project_id = test_env.dummy.project_alpha.project_id.clone(); + + let follow_test_env = test_env.clone(); + let follow_project_id = project_id.clone(); + let follow_requests = (0..4).map(move |_| { + let test_env = follow_test_env.clone(); + let project_id = follow_project_id.clone(); + + async move { + let request = test::TestRequest::post() + .uri(&format!("/v3/project/{project_id}/follow")) + .append_pat(ENEMY_USER_PAT) + .to_request(); + test_env.call(request).await + } + }); + let responses = futures::future::join_all(follow_requests).await; + + assert_eq!( + responses + .iter() + .filter( + |response| response.status() == StatusCode::NO_CONTENT + ) + .count(), + 1 + ); + assert_eq!( + responses + .iter() + .filter( + |response| response.status() == StatusCode::BAD_REQUEST + ) + .count(), + 3 + ); + + let project = test_env + .api + .get_project_deserialized_common(&project_id, ENEMY_USER_PAT) + .await; + assert_eq!(project.followers, 1); + + let unfollow_test_env = test_env.clone(); + let unfollow_project_id = project_id.clone(); + let unfollow_requests = (0..4).map(move |_| { + let test_env = unfollow_test_env.clone(); + let project_id = unfollow_project_id.clone(); + + async move { + let request = test::TestRequest::delete() + .uri(&format!("/v3/project/{project_id}/follow")) + .append_pat(ENEMY_USER_PAT) + .to_request(); + test_env.call(request).await + } + }); + let responses = futures::future::join_all(unfollow_requests).await; + + assert_eq!( + responses + .iter() + .filter( + |response| response.status() == StatusCode::NO_CONTENT + ) + .count(), + 1 + ); + assert_eq!( + responses + .iter() + .filter( + |response| response.status() == StatusCode::BAD_REQUEST + ) + .count(), + 3 + ); + + let project = test_env + .api + .get_project_deserialized_common(&project_id, ENEMY_USER_PAT) + .await; + assert_eq!(project.followers, 0); + }, + ) + .await; +} + #[actix_rt::test] async fn test_add_remove_project() { // Test setup and dummy data From ca1e9790b53202f39206f9e1c0e7e6a851609031 Mon Sep 17 00:00:00 2001 From: aecsocket <43144841+aecsocket@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:58:33 +0100 Subject: [PATCH 2/2] remove test since it's lowkey useless --- apps/labrinth/tests/project.rs | 97 +--------------------------------- 1 file changed, 1 insertion(+), 96 deletions(-) diff --git a/apps/labrinth/tests/project.rs b/apps/labrinth/tests/project.rs index 11408d70fc..239ce885f0 100644 --- a/apps/labrinth/tests/project.rs +++ b/apps/labrinth/tests/project.rs @@ -6,9 +6,7 @@ use common::dummy_data::DUMMY_CATEGORIES; use crate::common::api_common::models::CommonProject; use crate::common::api_common::request_data::ProjectCreationRequestData; -use crate::common::api_common::{ - ApiProject, ApiTeams, ApiVersion, AppendsOptionalPat, -}; +use crate::common::api_common::{ApiProject, ApiTeams, ApiVersion}; use crate::common::dummy_data::{ DummyImage, DummyOrganizationZeta, DummyProjectAlpha, DummyProjectBeta, TestFile, @@ -111,99 +109,6 @@ async fn test_get_project() { .await; } -#[actix_rt::test] -async fn test_project_follow_counter() { - with_test_environment( - Some(8), - |test_env: TestEnvironment| async move { - let project_id = test_env.dummy.project_alpha.project_id.clone(); - - let follow_test_env = test_env.clone(); - let follow_project_id = project_id.clone(); - let follow_requests = (0..4).map(move |_| { - let test_env = follow_test_env.clone(); - let project_id = follow_project_id.clone(); - - async move { - let request = test::TestRequest::post() - .uri(&format!("/v3/project/{project_id}/follow")) - .append_pat(ENEMY_USER_PAT) - .to_request(); - test_env.call(request).await - } - }); - let responses = futures::future::join_all(follow_requests).await; - - assert_eq!( - responses - .iter() - .filter( - |response| response.status() == StatusCode::NO_CONTENT - ) - .count(), - 1 - ); - assert_eq!( - responses - .iter() - .filter( - |response| response.status() == StatusCode::BAD_REQUEST - ) - .count(), - 3 - ); - - let project = test_env - .api - .get_project_deserialized_common(&project_id, ENEMY_USER_PAT) - .await; - assert_eq!(project.followers, 1); - - let unfollow_test_env = test_env.clone(); - let unfollow_project_id = project_id.clone(); - let unfollow_requests = (0..4).map(move |_| { - let test_env = unfollow_test_env.clone(); - let project_id = unfollow_project_id.clone(); - - async move { - let request = test::TestRequest::delete() - .uri(&format!("/v3/project/{project_id}/follow")) - .append_pat(ENEMY_USER_PAT) - .to_request(); - test_env.call(request).await - } - }); - let responses = futures::future::join_all(unfollow_requests).await; - - assert_eq!( - responses - .iter() - .filter( - |response| response.status() == StatusCode::NO_CONTENT - ) - .count(), - 1 - ); - assert_eq!( - responses - .iter() - .filter( - |response| response.status() == StatusCode::BAD_REQUEST - ) - .count(), - 3 - ); - - let project = test_env - .api - .get_project_deserialized_common(&project_id, ENEMY_USER_PAT) - .await; - assert_eq!(project.followers, 0); - }, - ) - .await; -} - #[actix_rt::test] async fn test_add_remove_project() { // Test setup and dummy data