From 8670403a67388979306dae5907eb24811c3e8021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0smail=20Karsl=C4=B1?= <17887754+ismailkarsli@users.noreply.github.com> Date: Wed, 24 Jan 2024 18:22:05 +0300 Subject: [PATCH] Add local_subscribers field to CommunityAggregates. Fixes #4144 (#4166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add upload timeout to PictrsConfig * Bad space 🤔 * Update PictrsConfig upload timeout to include units. * Add local_subscribers field to CommunityAggregates struct and schema * sql format * local_subscribers test * fix local_subscribers test * Revert "fix local_subscribers test" This reverts commit 4bbac5ce4afe101b2db4b9f099ca772c5ce2932b. * Revert "local_subscribers test" This reverts commit 735107e1f7554dfac6e474104eb87047675f11a5. * Create trigger for local_subscribers * Rename variable * re-trigger ci * re-trigger ci * Add local_subscribers count to follow.spec.ts * Rename local_subscribers to subscribers_local * Add subscribers_local to community_aggregates * added subscribers_local to the aggregate tests * Check if person exists on community_follower trigger * Delete community follows before deleting person * Update lemmy-js-client in api_tests * Refactor local_subscriber migration * fix format * Move migration files date to now * Fix test to wait for aggregates to federate * re-trigger ci --------- Co-authored-by: Dessalines --- api_tests/package.json | 2 +- api_tests/src/follow.spec.ts | 16 +++- api_tests/yarn.lock | 8 +- .../src/aggregates/community_aggregates.rs | 5 ++ crates/db_schema/src/aggregates/structs.rs | 1 + crates/db_schema/src/schema.rs | 1 + .../down.sql | 42 ++++++++++ .../up.sql | 81 +++++++++++++++++++ 8 files changed, 150 insertions(+), 6 deletions(-) create mode 100644 migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/down.sql create mode 100644 migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/up.sql diff --git a/api_tests/package.json b/api_tests/package.json index 4d3e57d21..94cb2df9c 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -27,7 +27,7 @@ "eslint": "^8.55.0", "eslint-plugin-prettier": "^5.0.1", "jest": "^29.5.0", - "lemmy-js-client": "0.19.0", + "lemmy-js-client": "0.19.2-alpha.2", "prettier": "^3.1.1", "ts-jest": "^29.1.0", "typescript": "^5.3.3" diff --git a/api_tests/src/follow.spec.ts b/api_tests/src/follow.spec.ts index 314b45eaf..0187e3ee1 100644 --- a/api_tests/src/follow.spec.ts +++ b/api_tests/src/follow.spec.ts @@ -24,21 +24,32 @@ test("Follow local community", async () => { let community = (await resolveBetaCommunity(user)).community!; expect(community.counts.subscribers).toBe(1); + expect(community.counts.subscribers_local).toBe(1); let follow = await followCommunity(user, true, community.community.id); // Make sure the follow response went through expect(follow.community_view.community.local).toBe(true); expect(follow.community_view.subscribed).toBe("Subscribed"); expect(follow.community_view.counts.subscribers).toBe(2); + expect(follow.community_view.counts.subscribers_local).toBe(2); // Test an unfollow let unfollow = await followCommunity(user, false, community.community.id); expect(unfollow.community_view.subscribed).toBe("NotSubscribed"); expect(unfollow.community_view.counts.subscribers).toBe(1); + expect(unfollow.community_view.counts.subscribers_local).toBe(1); }); test("Follow federated community", async () => { - let betaCommunity = (await resolveBetaCommunity(alpha)).community; + // It takes about 1 second for the community aggregates to federate + let betaCommunity = ( + await waitUntil( + () => resolveBetaCommunity(alpha), + c => + c.community?.counts.subscribers === 1 && + c.community.counts.subscribers_local === 0, + ) + ).community; if (!betaCommunity) { throw "Missing beta community"; } @@ -55,10 +66,12 @@ test("Follow federated community", async () => { expect(betaCommunity?.community.local).toBe(false); expect(betaCommunity?.community.name).toBe("main"); expect(betaCommunity?.subscribed).toBe("Subscribed"); + expect(betaCommunity?.counts.subscribers_local).toBe(1); // check that unfollow was federated let communityOnBeta1 = await resolveBetaCommunity(beta); expect(communityOnBeta1.community?.counts.subscribers).toBe(2); + expect(communityOnBeta1.community?.counts.subscribers_local).toBe(1); // Check it from local let site = await getSite(alpha); @@ -83,4 +96,5 @@ test("Follow federated community", async () => { // check that unfollow was federated let communityOnBeta2 = await resolveBetaCommunity(beta); expect(communityOnBeta2.community?.counts.subscribers).toBe(1); + expect(communityOnBeta2.community?.counts.subscribers_local).toBe(1); }); diff --git a/api_tests/yarn.lock b/api_tests/yarn.lock index d2cf2ab26..6769952cc 100644 --- a/api_tests/yarn.lock +++ b/api_tests/yarn.lock @@ -2286,10 +2286,10 @@ kleur@^3.0.3: resolved "https://registry.yarnpkg.com/kleur/-/kleur-3.0.3.tgz#a79c9ecc86ee1ce3fa6206d1216c501f147fc07e" integrity sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w== -lemmy-js-client@0.19.0: - version "0.19.0" - resolved "https://registry.yarnpkg.com/lemmy-js-client/-/lemmy-js-client-0.19.0.tgz#50098183264fa176784857f45665b06994b31e18" - integrity sha512-h+E8wC9RKjlToWw9+kuGFAzk4Fiaf61KqAwzvoCDAfj2L1r+YNt5EDMOggGCoRx5PlqLuIVr7BNEU46KxJfmHA== +lemmy-js-client@0.19.2-alpha.2: + version "0.19.2-alpha.2" + resolved "https://registry.yarnpkg.com/lemmy-js-client/-/lemmy-js-client-0.19.2-alpha.2.tgz#09956df6392fa7df437343d1f1576b6297537113" + integrity sha512-/RztLo4EIDQeEN51awYJfx8JcNCHecOPrM14sSJ6/qLOOxQTPFsDrd7a2WplHpj7Wf8xci2UNfW26PmnVMOPaQ== dependencies: cross-fetch "^3.1.5" form-data "^4.0.0" diff --git a/crates/db_schema/src/aggregates/community_aggregates.rs b/crates/db_schema/src/aggregates/community_aggregates.rs index f4202738d..334688b97 100644 --- a/crates/db_schema/src/aggregates/community_aggregates.rs +++ b/crates/db_schema/src/aggregates/community_aggregates.rs @@ -156,6 +156,7 @@ mod tests { .unwrap(); assert_eq!(2, community_aggregates_before_delete.subscribers); + assert_eq!(2, community_aggregates_before_delete.subscribers_local); assert_eq!(1, community_aggregates_before_delete.posts); assert_eq!(2, community_aggregates_before_delete.comments); @@ -164,6 +165,7 @@ mod tests { .await .unwrap(); assert_eq!(1, another_community_aggs.subscribers); + assert_eq!(1, another_community_aggs.subscribers_local); assert_eq!(0, another_community_aggs.posts); assert_eq!(0, another_community_aggs.comments); @@ -175,6 +177,7 @@ mod tests { .await .unwrap(); assert_eq!(1, after_unfollow.subscribers); + assert_eq!(1, after_unfollow.subscribers_local); // Follow again just for the later tests CommunityFollower::follow(pool, &second_person_follow) @@ -184,6 +187,7 @@ mod tests { .await .unwrap(); assert_eq!(2, after_follow_again.subscribers); + assert_eq!(2, after_follow_again.subscribers_local); // Remove a parent post (the comment count should also be 0) Post::delete(pool, inserted_post.id).await.unwrap(); @@ -201,6 +205,7 @@ mod tests { .await .unwrap(); assert_eq!(1, after_person_delete.subscribers); + assert_eq!(1, after_person_delete.subscribers_local); // This should delete all the associated rows, and fire triggers let person_num_deleted = Person::delete(pool, inserted_person.id).await.unwrap(); diff --git a/crates/db_schema/src/aggregates/structs.rs b/crates/db_schema/src/aggregates/structs.rs index 0020ecab0..7ca9429f6 100644 --- a/crates/db_schema/src/aggregates/structs.rs +++ b/crates/db_schema/src/aggregates/structs.rs @@ -66,6 +66,7 @@ pub struct CommunityAggregates { pub users_active_half_year: i64, #[serde(skip)] pub hot_rank: f64, + pub subscribers_local: i64, } #[derive(PartialEq, Eq, Debug, Serialize, Deserialize, Clone, Default)] diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index 88d468a6f..daea8ded1 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -198,6 +198,7 @@ diesel::table! { users_active_month -> Int8, users_active_half_year -> Int8, hot_rank -> Float8, + subscribers_local -> Int8, } } diff --git a/migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/down.sql b/migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/down.sql new file mode 100644 index 000000000..43f92d461 --- /dev/null +++ b/migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/down.sql @@ -0,0 +1,42 @@ +ALTER TABLE community_aggregates + DROP COLUMN subscribers_local; + +-- old function from migrations/2023-10-02-145002_community_followers_count_federated/up.sql +-- The subscriber count should only be updated for local communities. For remote +-- communities it is read over federation from the origin instance. +CREATE OR REPLACE FUNCTION community_aggregates_subscriber_count () + RETURNS TRIGGER + LANGUAGE plpgsql + AS $$ +BEGIN + IF (TG_OP = 'INSERT') THEN + UPDATE + community_aggregates + SET + subscribers = subscribers + 1 + FROM + community + WHERE + community.id = community_id + AND community.local + AND community_id = NEW.community_id; + ELSIF (TG_OP = 'DELETE') THEN + UPDATE + community_aggregates + SET + subscribers = subscribers - 1 + FROM + community + WHERE + community.id = community_id + AND community.local + AND community_id = OLD.community_id; + END IF; + RETURN NULL; +END +$$; + +DROP TRIGGER IF EXISTS delete_follow_before_person ON person; + +DROP FUNCTION IF EXISTS delete_follow_before_person; + diff --git a/migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/up.sql b/migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/up.sql new file mode 100644 index 000000000..2ed68ea58 --- /dev/null +++ b/migrations/2024-01-05-213000_community_aggregates_add_local_subscribers/up.sql @@ -0,0 +1,81 @@ +-- Couldn't find a way to put subscribers_local right after subscribers except recreating the table. +ALTER TABLE community_aggregates + ADD COLUMN subscribers_local bigint NOT NULL DEFAULT 0; + +-- update initial value +-- update by counting local persons who follow communities. +WITH follower_counts AS ( + SELECT + community_id, + count(*) AS local_sub_count + FROM + community_follower cf + JOIN person p ON p.id = cf.person_id + WHERE + p.local = TRUE + GROUP BY + community_id) +UPDATE + community_aggregates ca +SET + subscribers_local = local_sub_count +FROM + follower_counts +WHERE + ca.community_id = follower_counts.community_id; + +-- subscribers should be updated only when a local community is followed by a local or remote person +-- subscribers_local should be updated only when a local person follows a local or remote community +CREATE OR REPLACE FUNCTION community_aggregates_subscriber_count () + RETURNS TRIGGER + LANGUAGE plpgsql + AS $$ +BEGIN + IF (TG_OP = 'INSERT') THEN + UPDATE + community_aggregates ca + SET + subscribers = subscribers + community.local::int, + subscribers_local = subscribers_local + person.local::int + FROM + community + LEFT JOIN person ON person.id = NEW.person_id + WHERE + community.id = NEW.community_id + AND community.id = ca.community_id + AND person.local IS NOT NULL; + ELSIF (TG_OP = 'DELETE') THEN + UPDATE + community_aggregates ca + SET + subscribers = subscribers - community.local::int, + subscribers_local = subscribers_local - person.local::int + FROM + community + LEFT JOIN person ON person.id = OLD.person_id + WHERE + community.id = OLD.community_id + AND community.id = ca.community_id + AND person.local IS NOT NULL; + END IF; + RETURN NULL; +END +$$; + +-- to be able to join person on the trigger above, we need to run it before the person is deleted: https://github.com/LemmyNet/lemmy/pull/4166#issuecomment-1874095856 +CREATE FUNCTION delete_follow_before_person () + RETURNS TRIGGER + LANGUAGE plpgsql + AS $$ +BEGIN + DELETE FROM community_follower AS c + WHERE c.person_id = OLD.id; + RETURN OLD; +END; +$$; + +CREATE TRIGGER delete_follow_before_person + BEFORE DELETE ON person + FOR EACH ROW + EXECUTE FUNCTION delete_follow_before_person (); +