From: Piotr Juszczyk <74842304+pijuszczyk@users.noreply.github.com> Date: Mon, 10 Jul 2023 15:30:30 +0000 (+0200) Subject: Fix #3501 - Fix aggregation counts for elements removed and deleted (#3543) X-Git-Url: http://these/git/%7B%60%24%7BghostArchiveUrl%7D/static/%7B%60%24%7BwebArchiveUrl%7D/save/%7B%60https:/%24%7Bsubmission.url%7D?a=commitdiff_plain;h=9c2490d4f24671cc2770ba68feaf6d7562b1db6a;p=lemmy.git Fix #3501 - Fix aggregation counts for elements removed and deleted (#3543) Two bugs were found and fixed: - previously elements removal and deletion were counted as two separate disappearances - removing comments did not affect post aggregations --- diff --git a/crates/db_schema/src/aggregates/community_aggregates.rs b/crates/db_schema/src/aggregates/community_aggregates.rs index 2c2eaa78..310178f1 100644 --- a/crates/db_schema/src/aggregates/community_aggregates.rs +++ b/crates/db_schema/src/aggregates/community_aggregates.rs @@ -167,7 +167,7 @@ mod tests { .unwrap(); assert_eq!(2, after_follow_again.subscribers); - // Remove a parent comment (the comment count should also be 0) + // Remove a parent post (the comment count should also be 0) Post::delete(pool, inserted_post.id).await.unwrap(); let after_parent_post_delete = CommunityAggregates::read(pool, inserted_community.id) .await diff --git a/crates/db_schema/src/aggregates/post_aggregates.rs b/crates/db_schema/src/aggregates/post_aggregates.rs index 176f694a..93e6f3f7 100644 --- a/crates/db_schema/src/aggregates/post_aggregates.rs +++ b/crates/db_schema/src/aggregates/post_aggregates.rs @@ -38,7 +38,7 @@ mod tests { use crate::{ aggregates::post_aggregates::PostAggregates, source::{ - comment::{Comment, CommentInsertForm}, + comment::{Comment, CommentInsertForm, CommentUpdateForm}, community::{Community, CommunityInsertForm}, instance::Instance, person::{Person, PersonInsertForm}, @@ -181,4 +181,100 @@ mod tests { Instance::delete(pool, inserted_instance.id).await.unwrap(); } + + #[tokio::test] + #[serial] + async fn test_soft_delete() { + let pool = &build_db_pool_for_tests().await; + + let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()) + .await + .unwrap(); + + let new_person = PersonInsertForm::builder() + .name("thommy_community_agg".into()) + .public_key("pubkey".to_string()) + .instance_id(inserted_instance.id) + .build(); + + let inserted_person = Person::create(pool, &new_person).await.unwrap(); + + let new_community = CommunityInsertForm::builder() + .name("TIL_community_agg".into()) + .title("nada".to_owned()) + .public_key("pubkey".to_string()) + .instance_id(inserted_instance.id) + .build(); + + let inserted_community = Community::create(pool, &new_community).await.unwrap(); + + let new_post = PostInsertForm::builder() + .name("A test post".into()) + .creator_id(inserted_person.id) + .community_id(inserted_community.id) + .build(); + + let inserted_post = Post::create(pool, &new_post).await.unwrap(); + + let comment_form = CommentInsertForm::builder() + .content("A test comment".into()) + .creator_id(inserted_person.id) + .post_id(inserted_post.id) + .build(); + + let inserted_comment = Comment::create(pool, &comment_form, None).await.unwrap(); + + let post_aggregates_before = PostAggregates::read(pool, inserted_post.id).await.unwrap(); + assert_eq!(1, post_aggregates_before.comments); + + Comment::update( + pool, + inserted_comment.id, + &CommentUpdateForm::builder().removed(Some(true)).build(), + ) + .await + .unwrap(); + + let post_aggregates_after_remove = PostAggregates::read(pool, inserted_post.id).await.unwrap(); + assert_eq!(0, post_aggregates_after_remove.comments); + + Comment::update( + pool, + inserted_comment.id, + &CommentUpdateForm::builder().removed(Some(false)).build(), + ) + .await + .unwrap(); + + Comment::update( + pool, + inserted_comment.id, + &CommentUpdateForm::builder().deleted(Some(true)).build(), + ) + .await + .unwrap(); + + let post_aggregates_after_delete = PostAggregates::read(pool, inserted_post.id).await.unwrap(); + assert_eq!(0, post_aggregates_after_delete.comments); + + Comment::update( + pool, + inserted_comment.id, + &CommentUpdateForm::builder().removed(Some(true)).build(), + ) + .await + .unwrap(); + + let post_aggregates_after_delete_remove = + PostAggregates::read(pool, inserted_post.id).await.unwrap(); + assert_eq!(0, post_aggregates_after_delete_remove.comments); + + Comment::delete(pool, inserted_comment.id).await.unwrap(); + Post::delete(pool, inserted_post.id).await.unwrap(); + Person::delete(pool, inserted_person.id).await.unwrap(); + Community::delete(pool, inserted_community.id) + .await + .unwrap(); + Instance::delete(pool, inserted_instance.id).await.unwrap(); + } } diff --git a/crates/db_schema/src/aggregates/site_aggregates.rs b/crates/db_schema/src/aggregates/site_aggregates.rs index 719beb8f..cfc14df4 100644 --- a/crates/db_schema/src/aggregates/site_aggregates.rs +++ b/crates/db_schema/src/aggregates/site_aggregates.rs @@ -19,22 +19,18 @@ mod tests { aggregates::site_aggregates::SiteAggregates, source::{ comment::{Comment, CommentInsertForm}, - community::{Community, CommunityInsertForm}, + community::{Community, CommunityInsertForm, CommunityUpdateForm}, instance::Instance, person::{Person, PersonInsertForm}, post::{Post, PostInsertForm}, site::{Site, SiteInsertForm}, }, traits::Crud, - utils::build_db_pool_for_tests, + utils::{build_db_pool_for_tests, DbPool}, }; use serial_test::serial; - #[tokio::test] - #[serial] - async fn test_crud() { - let pool = &build_db_pool_for_tests().await; - + async fn prepare_site_with_community(pool: &DbPool) -> (Instance, Person, Site, Community) { let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()) .await .unwrap(); @@ -62,6 +58,21 @@ mod tests { .build(); let inserted_community = Community::create(pool, &new_community).await.unwrap(); + ( + inserted_instance, + inserted_person, + inserted_site, + inserted_community, + ) + } + + #[tokio::test] + #[serial] + async fn test_crud() { + let pool = &build_db_pool_for_tests().await; + + let (inserted_instance, inserted_person, inserted_site, inserted_community) = + prepare_site_with_community(pool).await; let new_post = PostInsertForm::builder() .name("A test post".into()) @@ -127,4 +138,64 @@ mod tests { Instance::delete(pool, inserted_instance.id).await.unwrap(); } + + #[tokio::test] + #[serial] + async fn test_soft_delete() { + let pool = &build_db_pool_for_tests().await; + + let (inserted_instance, inserted_person, inserted_site, inserted_community) = + prepare_site_with_community(pool).await; + + let site_aggregates_before = SiteAggregates::read(pool).await.unwrap(); + assert_eq!(1, site_aggregates_before.communities); + + Community::update( + pool, + inserted_community.id, + &CommunityUpdateForm::builder().deleted(Some(true)).build(), + ) + .await + .unwrap(); + + let site_aggregates_after_delete = SiteAggregates::read(pool).await.unwrap(); + assert_eq!(0, site_aggregates_after_delete.communities); + + Community::update( + pool, + inserted_community.id, + &CommunityUpdateForm::builder().deleted(Some(false)).build(), + ) + .await + .unwrap(); + + Community::update( + pool, + inserted_community.id, + &CommunityUpdateForm::builder().removed(Some(true)).build(), + ) + .await + .unwrap(); + + let site_aggregates_after_remove = SiteAggregates::read(pool).await.unwrap(); + assert_eq!(0, site_aggregates_after_remove.communities); + + Community::update( + pool, + inserted_community.id, + &CommunityUpdateForm::builder().deleted(Some(true)).build(), + ) + .await + .unwrap(); + + let site_aggregates_after_remove_delete = SiteAggregates::read(pool).await.unwrap(); + assert_eq!(0, site_aggregates_after_remove_delete.communities); + + Community::delete(pool, inserted_community.id) + .await + .unwrap(); + Site::delete(pool, inserted_site.id).await.unwrap(); + Person::delete(pool, inserted_person.id).await.unwrap(); + Instance::delete(pool, inserted_instance.id).await.unwrap(); + } } diff --git a/migrations/2023-07-08-101154_fix_soft_delete_aggregates/down.sql b/migrations/2023-07-08-101154_fix_soft_delete_aggregates/down.sql new file mode 100644 index 00000000..b9616dee --- /dev/null +++ b/migrations/2023-07-08-101154_fix_soft_delete_aggregates/down.sql @@ -0,0 +1,105 @@ +-- 2023-06-19-120700_no_double_deletion/up.sql +create or replace function was_removed_or_deleted(TG_OP text, OLD record, NEW record) +RETURNS boolean +LANGUAGE plpgsql +as $$ + begin + IF (TG_OP = 'INSERT') THEN + return false; + end if; + + IF (TG_OP = 'DELETE' AND OLD.deleted = 'f' AND OLD.removed = 'f') THEN + return true; + end if; + + return TG_OP = 'UPDATE' AND ( + (OLD.deleted = 'f' AND NEW.deleted = 't') OR + (OLD.removed = 'f' AND NEW.removed = 't') + ); +END $$; + +-- 2022-04-04-183652_update_community_aggregates_on_soft_delete/up.sql +create or replace function was_restored_or_created(TG_OP text, OLD record, NEW record) + RETURNS boolean + LANGUAGE plpgsql +as $$ +begin + IF (TG_OP = 'DELETE') THEN + return false; + end if; + + IF (TG_OP = 'INSERT') THEN + return true; + end if; + + return TG_OP = 'UPDATE' AND ( + (OLD.deleted = 't' AND NEW.deleted = 'f') OR + (OLD.removed = 't' AND NEW.removed = 'f') + ); +END $$; + +-- 2021-08-02-002342_comment_count_fixes/up.sql +create or replace function post_aggregates_comment_deleted() +returns trigger language plpgsql +as $$ +begin + IF NEW.deleted = TRUE THEN + update post_aggregates pa + set comments = comments - 1 + where pa.post_id = NEW.post_id; + ELSE + update post_aggregates pa + set comments = comments + 1 + where pa.post_id = NEW.post_id; + END IF; + return null; +end $$; + +create trigger post_aggregates_comment_set_deleted +after update of deleted on comment +for each row +execute procedure post_aggregates_comment_deleted(); + +create or replace function post_aggregates_comment_count() +returns trigger language plpgsql +as $$ +begin + IF (TG_OP = 'INSERT') THEN + update post_aggregates pa + set comments = comments + 1, + newest_comment_time = NEW.published + where pa.post_id = NEW.post_id; + + -- A 2 day necro-bump limit + update post_aggregates pa + set newest_comment_time_necro = NEW.published + from post p + where pa.post_id = p.id + and pa.post_id = NEW.post_id + -- Fix issue with being able to necro-bump your own post + and NEW.creator_id != p.creator_id + and pa.published > ('now'::timestamp - '2 days'::interval); + + ELSIF (TG_OP = 'DELETE') THEN + -- Join to post because that post may not exist anymore + update post_aggregates pa + set comments = comments - 1 + from post p + where pa.post_id = p.id + and pa.post_id = OLD.post_id; + ELSIF (TG_OP = 'UPDATE') THEN + -- Join to post because that post may not exist anymore + update post_aggregates pa + set comments = comments - 1 + from post p + where pa.post_id = p.id + and pa.post_id = OLD.post_id; + END IF; + return null; +end $$; + +-- 2020-12-10-152350_create_post_aggregates/up.sql +create or replace trigger post_aggregates_comment_count +after insert or delete on comment +for each row +execute procedure post_aggregates_comment_count(); diff --git a/migrations/2023-07-08-101154_fix_soft_delete_aggregates/up.sql b/migrations/2023-07-08-101154_fix_soft_delete_aggregates/up.sql new file mode 100644 index 00000000..abc89d28 --- /dev/null +++ b/migrations/2023-07-08-101154_fix_soft_delete_aggregates/up.sql @@ -0,0 +1,81 @@ +-- Fix for duplicated decrementations when both `deleted` and `removed` fields are set subsequently +create or replace function was_removed_or_deleted(TG_OP text, OLD record, NEW record) +RETURNS boolean +LANGUAGE plpgsql +as $$ + begin + IF (TG_OP = 'INSERT') THEN + return false; + end if; + + IF (TG_OP = 'DELETE' AND OLD.deleted = 'f' AND OLD.removed = 'f') THEN + return true; + end if; + + return TG_OP = 'UPDATE' AND OLD.deleted = 'f' AND OLD.removed = 'f' AND ( + NEW.deleted = 't' OR NEW.removed = 't' + ); +END $$; + +create or replace function was_restored_or_created(TG_OP text, OLD record, NEW record) + RETURNS boolean + LANGUAGE plpgsql +as $$ +begin + IF (TG_OP = 'DELETE') THEN + return false; + end if; + + IF (TG_OP = 'INSERT') THEN + return true; + end if; + + return TG_OP = 'UPDATE' AND NEW.deleted = 'f' AND NEW.removed = 'f' AND ( + OLD.deleted = 't' OR OLD.removed = 't' + ); +END $$; + +-- Fix for post's comment count not updating after setting `removed` to 't' +drop trigger if exists post_aggregates_comment_set_deleted on comment; +drop function post_aggregates_comment_deleted(); + +create or replace function post_aggregates_comment_count() + returns trigger language plpgsql +as $$ +begin + -- Check for post existence - it may not exist anymore + IF TG_OP = 'INSERT' OR EXISTS ( + select 1 from post p where p.id = OLD.post_id + ) THEN + IF (was_restored_or_created(TG_OP, OLD, NEW)) THEN + update post_aggregates pa + set comments = comments + 1 where pa.post_id = NEW.post_id; + ELSIF (was_removed_or_deleted(TG_OP, OLD, NEW)) THEN + update post_aggregates pa + set comments = comments - 1 where pa.post_id = OLD.post_id; + END IF; + END IF; + + IF TG_OP = 'INSERT' THEN + update post_aggregates pa + set newest_comment_time = NEW.published + where pa.post_id = NEW.post_id; + + -- A 2 day necro-bump limit + update post_aggregates pa + set newest_comment_time_necro = NEW.published + from post p + where pa.post_id = p.id + and pa.post_id = NEW.post_id + -- Fix issue with being able to necro-bump your own post + and NEW.creator_id != p.creator_id + and pa.published > ('now'::timestamp - '2 days'::interval); + END IF; + + return null; +end $$; + +create or replace trigger post_aggregates_comment_count + after insert or delete or update of removed, deleted on comment + for each row +execute procedure post_aggregates_comment_count(); \ No newline at end of file