Skip to content

Commit

Permalink
Fix LemmyNet#3501 - Fix aggregation counts for elements removed and d…
Browse files Browse the repository at this point in the history 
…eleted

Two bugs were found and fixed:
- previously elements removal and deletion were counted as two separate disappearances
- removing comments did not affect post aggregations
  • Loading branch information
@pijuszczyk
pijuszczyk committed Jul 8, 2023
1 parent 0c82f4e commit d870438
Show file tree
Hide file tree
Showing 5 changed files with 362 additions and 9 deletions.
2 changes: 1 addition & 1 deletion crates/db_schema/src/aggregates/community_aggregates.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
98 changes: 97 additions & 1 deletion crates/db_schema/src/aggregates/post_aggregates.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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();
}
}
85 changes: 78 additions & 7 deletions crates/db_schema/src/aggregates/site_aggregates.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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();
}
}
105 changes: 105 additions & 0 deletions migrations/2023-07-08-101154_fix_soft_delete_aggregates/down.sql
View file
Original file line number Diff line number Diff line change
@@ -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();

0 comments on commit d870438

Please sign in to comment.