From e8fa477793e1395664f79d572800f11994cdd38d Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 13 Jul 2019 19:17:57 +0300 Subject: [PATCH 1/9] Refactor Follows/Followers counter syncronization - Actually sync counters in the database instead of info cache (which got overriden after user update was finished anyway) - Add following count field to user info - Set hide_followers/hide_follows for remote users based on http status codes for the first collection page --- config/test.exs | 3 +- lib/pleroma/object/fetcher.ex | 6 ++- lib/pleroma/user.ex | 4 +- lib/pleroma/user/info.ex | 13 ++++- lib/pleroma/web/activity_pub/activity_pub.ex | 53 ++++++++++++++++++- .../web/activity_pub/transmogrifier.ex | 27 ---------- test/web/activity_pub/transmogrifier_test.exs | 28 ---------- 7 files changed, 73 insertions(+), 61 deletions(-) diff --git a/config/test.exs b/config/test.exs index 96ecf3592..28eea3b00 100644 --- a/config/test.exs +++ b/config/test.exs @@ -29,7 +29,8 @@ email: "admin@example.com", notify_email: "noreply@example.com", skip_thread_containment: false, - federating: false + federating: false, + external_user_synchronization: false # Configure your database config :pleroma, Pleroma.Repo, diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 101c21f96..bc3e7e5bc 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -76,7 +76,7 @@ def fetch_object_from_id!(id, options \\ []) do end end - def fetch_and_contain_remote_object_from_id(id) do + def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do Logger.info("Fetching object #{id} via AP") with true <- String.starts_with?(id, "http"), @@ -96,4 +96,8 @@ def fetch_and_contain_remote_object_from_id(id) do {:error, e} end end + + def fetch_and_contain_remote_object_from_id(_id) do + {:error, "id must be a string"} + end end diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index e5a6c2529..c252e8bff 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -114,7 +114,9 @@ def ap_following(%User{} = user), do: "#{ap_id(user)}/following" def user_info(%User{} = user, args \\ %{}) do following_count = - if args[:following_count], do: args[:following_count], else: following_count(user) + if args[:following_count], + do: args[:following_count], + else: user.info.following_count || following_count(user) follower_count = if args[:follower_count], do: args[:follower_count], else: user.info.follower_count diff --git a/lib/pleroma/user/info.ex b/lib/pleroma/user/info.ex index 08e43ff0f..2d8395b73 100644 --- a/lib/pleroma/user/info.ex +++ b/lib/pleroma/user/info.ex @@ -16,6 +16,7 @@ defmodule Pleroma.User.Info do field(:source_data, :map, default: %{}) field(:note_count, :integer, default: 0) field(:follower_count, :integer, default: 0) + field(:following_count, :integer, default: nil) field(:locked, :boolean, default: false) field(:confirmation_pending, :boolean, default: false) field(:confirmation_token, :string, default: nil) @@ -195,7 +196,11 @@ def remote_user_creation(info, params) do :uri, :hub, :topic, - :salmon + :salmon, + :hide_followers, + :hide_follows, + :follower_count, + :following_count ]) end @@ -206,7 +211,11 @@ def user_upgrade(info, params) do :source_data, :banner, :locked, - :magic_key + :magic_key, + :follower_count, + :following_count, + :hide_follows, + :hide_followers ]) end diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index a3174a787..0a22fe223 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1013,6 +1013,56 @@ defp object_to_user_data(data) do {:ok, user_data} end + defp maybe_update_follow_information(data) do + with {:enabled, true} <- + {:enabled, Pleroma.Config.get([:instance, :external_user_synchronization])}, + {:ok, following_data} <- + Fetcher.fetch_and_contain_remote_object_from_id(data.following_address), + following_count <- following_data["totalItems"], + hide_follows <- collection_private?(following_data), + {:ok, followers_data} <- + Fetcher.fetch_and_contain_remote_object_from_id(data.follower_address), + followers_count <- followers_data["totalItems"], + hide_followers <- collection_private?(followers_data) do + info = %{ + "hide_follows" => hide_follows, + "follower_count" => followers_count, + "following_count" => following_count, + "hide_followers" => hide_followers + } + + info = Map.merge(data.info, info) + Map.put(data, :info, info) + else + {:enabled, false} -> + data + + e -> + Logger.error( + "Follower/Following counter update for #{data.ap_id} failed.\n" <> inspect(e) + ) + + data + end + end + + defp collection_private?(data) do + if is_map(data["first"]) and + data["first"]["type"] in ["CollectionPage", "OrderedCollectionPage"] do + false + else + with {:ok, _data} <- Fetcher.fetch_and_contain_remote_object_from_id(data["first"]) do + false + else + {:error, {:ok, %{status: code}}} when code in [401, 403] -> + true + + _e -> + false + end + end + end + def user_data_from_user_object(data) do with {:ok, data} <- MRF.filter(data), {:ok, data} <- object_to_user_data(data) do @@ -1024,7 +1074,8 @@ def user_data_from_user_object(data) do def fetch_and_prepare_user_from_ap_id(ap_id) do with {:ok, data} <- Fetcher.fetch_and_contain_remote_object_from_id(ap_id), - {:ok, data} <- user_data_from_user_object(data) do + {:ok, data} <- user_data_from_user_object(data), + data <- maybe_update_follow_information(data) do {:ok, data} else e -> Logger.error("Could not decode user at fetch #{ap_id}, #{inspect(e)}") diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index d14490bb5..e34fe6611 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -1087,10 +1087,6 @@ def upgrade_user_from_ap_id(ap_id) do PleromaJobQueue.enqueue(:transmogrifier, __MODULE__, [:user_upgrade, user]) end - if Pleroma.Config.get([:instance, :external_user_synchronization]) do - update_following_followers_counters(user) - end - {:ok, user} else %User{} = user -> {:ok, user} @@ -1123,27 +1119,4 @@ def maybe_fix_user_object(data) do data |> maybe_fix_user_url end - - def update_following_followers_counters(user) do - info = %{} - - following = fetch_counter(user.following_address) - info = if following, do: Map.put(info, :following_count, following), else: info - - followers = fetch_counter(user.follower_address) - info = if followers, do: Map.put(info, :follower_count, followers), else: info - - User.set_info_cache(user, info) - end - - defp fetch_counter(url) do - with {:ok, %{body: body, status: code}} when code in 200..299 <- - Pleroma.HTTP.get( - url, - [{:Accept, "application/activity+json"}] - ), - {:ok, data} <- Jason.decode(body) do - data["totalItems"] - end - end end diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index b896a532b..6d05138fb 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -1359,32 +1359,4 @@ test "removes recipient's follower collection from cc", %{user: user} do refute recipient.follower_address in fixed_object["to"] end end - - test "update_following_followers_counters/1" do - user1 = - insert(:user, - local: false, - follower_address: "http://localhost:4001/users/masto_closed/followers", - following_address: "http://localhost:4001/users/masto_closed/following" - ) - - user2 = - insert(:user, - local: false, - follower_address: "http://localhost:4001/users/fuser2/followers", - following_address: "http://localhost:4001/users/fuser2/following" - ) - - Transmogrifier.update_following_followers_counters(user1) - Transmogrifier.update_following_followers_counters(user2) - - %{follower_count: followers, following_count: following} = User.get_cached_user_info(user1) - assert followers == 437 - assert following == 152 - - %{follower_count: followers, following_count: following} = User.get_cached_user_info(user2) - - assert followers == 527 - assert following == 267 - end end From e5b850a99115859ceb028c3891f59d5e6ffd5d56 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 13 Jul 2019 23:56:10 +0300 Subject: [PATCH 2/9] Refactor fetching follow information to a separate function --- lib/pleroma/user/info.ex | 1 + lib/pleroma/web/activity_pub/activity_pub.ex | 51 +++++++++++++------- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/lib/pleroma/user/info.ex b/lib/pleroma/user/info.ex index 2d8395b73..67e8801ea 100644 --- a/lib/pleroma/user/info.ex +++ b/lib/pleroma/user/info.ex @@ -16,6 +16,7 @@ defmodule Pleroma.User.Info do field(:source_data, :map, default: %{}) field(:note_count, :integer, default: 0) field(:follower_count, :integer, default: 0) + # Should be filled in only for remote users field(:following_count, :integer, default: nil) field(:locked, :boolean, default: false) field(:confirmation_pending, :boolean, default: false) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 0a22fe223..eadd335ca 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1013,17 +1013,15 @@ defp object_to_user_data(data) do {:ok, user_data} end - defp maybe_update_follow_information(data) do - with {:enabled, true} <- - {:enabled, Pleroma.Config.get([:instance, :external_user_synchronization])}, - {:ok, following_data} <- - Fetcher.fetch_and_contain_remote_object_from_id(data.following_address), - following_count <- following_data["totalItems"], - hide_follows <- collection_private?(following_data), + def fetch_follow_information_for_user(user) do + with {:ok, following_data} <- + Fetcher.fetch_and_contain_remote_object_from_id(user.following_address), + following_count when is_integer(following_count) <- following_data["totalItems"], + {:ok, hide_follows} <- collection_private(following_data), {:ok, followers_data} <- - Fetcher.fetch_and_contain_remote_object_from_id(data.follower_address), - followers_count <- followers_data["totalItems"], - hide_followers <- collection_private?(followers_data) do + Fetcher.fetch_and_contain_remote_object_from_id(user.follower_address), + followers_count when is_integer(followers_count) <- followers_data["totalItems"], + {:ok, hide_followers} <- collection_private(followers_data) do info = %{ "hide_follows" => hide_follows, "follower_count" => followers_count, @@ -1031,8 +1029,22 @@ defp maybe_update_follow_information(data) do "hide_followers" => hide_followers } - info = Map.merge(data.info, info) - Map.put(data, :info, info) + info = Map.merge(user.info, info) + {:ok, Map.put(user, :info, info)} + else + {:error, _} = e -> + e + + e -> + {:error, e} + end + end + + defp maybe_update_follow_information(data) do + with {:enabled, true} <- + {:enabled, Pleroma.Config.get([:instance, :external_user_synchronization])}, + {:ok, data} <- fetch_follow_information_for_user(data) do + data else {:enabled, false} -> data @@ -1046,19 +1058,22 @@ defp maybe_update_follow_information(data) do end end - defp collection_private?(data) do + defp collection_private(data) do if is_map(data["first"]) and data["first"]["type"] in ["CollectionPage", "OrderedCollectionPage"] do - false + {:ok, false} else with {:ok, _data} <- Fetcher.fetch_and_contain_remote_object_from_id(data["first"]) do - false + {:ok, false} else {:error, {:ok, %{status: code}}} when code in [401, 403] -> - true + {:ok, true} - _e -> - false + {:error, _} = e -> + e + + e -> + {:error, e} end end end From d06d1b751d44802c5c3701f916ae2ce7d3c3be56 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sun, 14 Jul 2019 00:21:35 +0300 Subject: [PATCH 3/9] Use atoms when updating user info --- lib/pleroma/web/activity_pub/activity_pub.ex | 16 ++++++++-------- lib/pleroma/web/activity_pub/transmogrifier.ex | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index eadd335ca..df4155d21 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -986,10 +986,10 @@ defp object_to_user_data(data) do user_data = %{ ap_id: data["id"], info: %{ - "ap_enabled" => true, - "source_data" => data, - "banner" => banner, - "locked" => locked + ap_enabled: true, + source_data: data, + banner: banner, + locked: locked }, avatar: avatar, name: data["name"], @@ -1023,10 +1023,10 @@ def fetch_follow_information_for_user(user) do followers_count when is_integer(followers_count) <- followers_data["totalItems"], {:ok, hide_followers} <- collection_private(followers_data) do info = %{ - "hide_follows" => hide_follows, - "follower_count" => followers_count, - "following_count" => following_count, - "hide_followers" => hide_followers + hide_follows: hide_follows, + follower_count: followers_count, + following_count: following_count, + hide_followers: hide_followers } info = Map.merge(user.info, info) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index e34fe6611..10b362908 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -609,13 +609,13 @@ def handle_incoming( with %User{ap_id: ^actor_id} = actor <- User.get_cached_by_ap_id(object["id"]) do {:ok, new_user_data} = ActivityPub.user_data_from_user_object(object) - banner = new_user_data[:info]["banner"] - locked = new_user_data[:info]["locked"] || false + banner = new_user_data[:info][:banner] + locked = new_user_data[:info][:locked] || false update_data = new_user_data |> Map.take([:name, :bio, :avatar]) - |> Map.put(:info, %{"banner" => banner, "locked" => locked}) + |> Map.put(:info, %{banner: banner, locked: locked}) actor |> User.upgrade_changeset(update_data) From 183da33e005c8a8e8472350a3b6b36ff6f82d67d Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sun, 14 Jul 2019 00:56:02 +0300 Subject: [PATCH 4/9] Add tests for fetch_follow_information_for_user and check object type when fetching the page --- lib/pleroma/web/activity_pub/activity_pub.ex | 3 +- test/web/activity_pub/activity_pub_test.exs | 81 ++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index df4155d21..c821ba45f 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1063,7 +1063,8 @@ defp collection_private(data) do data["first"]["type"] in ["CollectionPage", "OrderedCollectionPage"] do {:ok, false} else - with {:ok, _data} <- Fetcher.fetch_and_contain_remote_object_from_id(data["first"]) do + with {:ok, %{"type" => type}} when type in ["CollectionPage", "OrderedCollectionPage"] <- + Fetcher.fetch_and_contain_remote_object_from_id(data["first"]) do {:ok, false} else {:error, {:ok, %{status: code}}} when code in [401, 403] -> diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 59d56f3a7..448ffbf54 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1222,4 +1222,85 @@ test "fetches only public posts for other users" do assert result.id == activity.id end end + + describe "fetch_follow_information_for_user" do + test "syncronizes following/followers counters" do + user = + insert(:user, + local: false, + follower_address: "http://localhost:4001/users/fuser2/followers", + following_address: "http://localhost:4001/users/fuser2/following" + ) + + {:ok, user} = ActivityPub.fetch_follow_information_for_user(user) + assert user.info.follower_count == 527 + assert user.info.following_count == 267 + end + + test "detects hidden followers" do + mock(fn env -> + case env.url do + "http://localhost:4001/users/masto_closed/followers?page=1" -> + %Tesla.Env{status: 403, body: ""} + + "http://localhost:4001/users/masto_closed/following?page=1" -> + %Tesla.Env{ + status: 200, + body: + Jason.encode!(%{ + "id" => "http://localhost:4001/users/masto_closed/following?page=1", + "type" => "OrderedCollectionPage" + }) + } + + _ -> + apply(HttpRequestMock, :request, [env]) + end + end) + + user = + insert(:user, + local: false, + follower_address: "http://localhost:4001/users/masto_closed/followers", + following_address: "http://localhost:4001/users/masto_closed/following" + ) + + {:ok, user} = ActivityPub.fetch_follow_information_for_user(user) + assert user.info.hide_followers == true + assert user.info.hide_follows == false + end + + test "detects hidden follows" do + mock(fn env -> + case env.url do + "http://localhost:4001/users/masto_closed/following?page=1" -> + %Tesla.Env{status: 403, body: ""} + + "http://localhost:4001/users/masto_closed/followers?page=1" -> + %Tesla.Env{ + status: 200, + body: + Jason.encode!(%{ + "id" => "http://localhost:4001/users/masto_closed/followers?page=1", + "type" => "OrderedCollectionPage" + }) + } + + _ -> + apply(HttpRequestMock, :request, [env]) + end + end) + + user = + insert(:user, + local: false, + follower_address: "http://localhost:4001/users/masto_closed/followers", + following_address: "http://localhost:4001/users/masto_closed/following" + ) + + {:ok, user} = ActivityPub.fetch_follow_information_for_user(user) + assert user.info.hide_followers == false + assert user.info.hide_follows == true + end + end end From 0c2dcb4c69ed340d02a4b20a4f341f1d9aaaba38 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sun, 14 Jul 2019 01:58:39 +0300 Subject: [PATCH 5/9] Add follow information refetching after following/unfollowing --- lib/pleroma/user.ex | 91 +++++++++++++++----- lib/pleroma/user/info.ex | 10 +++ lib/pleroma/web/activity_pub/activity_pub.ex | 21 +++-- test/web/activity_pub/activity_pub_test.exs | 18 ++-- 4 files changed, 98 insertions(+), 42 deletions(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index c252e8bff..2e9b01205 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -406,6 +406,8 @@ def follow(%User{} = follower, %User{info: info} = followed) do {1, [follower]} = Repo.update_all(q, []) + follower = maybe_update_following_count(follower) + {:ok, _} = update_follower_count(followed) set_cache(follower) @@ -425,6 +427,8 @@ def unfollow(%User{} = follower, %User{} = followed) do {1, [follower]} = Repo.update_all(q, []) + follower = maybe_update_following_count(follower) + {:ok, followed} = update_follower_count(followed) set_cache(follower) @@ -698,32 +702,75 @@ def update_note_count(%User{} = user) do |> update_and_set_cache() end - def update_follower_count(%User{} = user) do - follower_count_query = - User.Query.build(%{followers: user, deactivated: false}) - |> select([u], %{count: count(u.id)}) + def maybe_fetch_follow_information(user) do + with {:ok, user} <- fetch_follow_information(user) do + user + else + e -> + Logger.error( + "Follower/Following counter update for #{user.ap_id} failed.\n" <> inspect(e) + ) - User - |> where(id: ^user.id) - |> join(:inner, [u], s in subquery(follower_count_query)) - |> update([u, s], - set: [ - info: - fragment( - "jsonb_set(?, '{follower_count}', ?::varchar::jsonb, true)", - u.info, - s.count - ) - ] - ) - |> select([u], u) - |> Repo.update_all([]) - |> case do - {1, [user]} -> set_cache(user) - _ -> {:error, user} + user end end + def fetch_follow_information(user) do + with {:ok, info} <- ActivityPub.fetch_follow_information_for_user(user) do + info_cng = User.Info.follow_information_update(user.info, info) + + changeset = + user + |> change() + |> put_embed(:info, info_cng) + + update_and_set_cache(changeset) + else + {:error, _} = e -> e + e -> {:error, e} + end + end + + def update_follower_count(%User{} = user) do + unless user.local == false and Pleroma.Config.get([:instance, :external_user_synchronization]) do + follower_count_query = + User.Query.build(%{followers: user, deactivated: false}) + |> select([u], %{count: count(u.id)}) + + User + |> where(id: ^user.id) + |> join(:inner, [u], s in subquery(follower_count_query)) + |> update([u, s], + set: [ + info: + fragment( + "jsonb_set(?, '{follower_count}', ?::varchar::jsonb, true)", + u.info, + s.count + ) + ] + ) + |> select([u], u) + |> Repo.update_all([]) + |> case do + {1, [user]} -> set_cache(user) + _ -> {:error, user} + end + else + {:ok, maybe_fetch_follow_information(user)} + end + end + + def maybe_update_following_count(%User{local: false} = user) do + if Pleroma.Config.get([:instance, :external_user_synchronization]) do + {:ok, maybe_fetch_follow_information(user)} + else + user + end + end + + def maybe_update_following_count(user), do: user + def remove_duplicated_following(%User{following: following} = user) do uniq_following = Enum.uniq(following) diff --git a/lib/pleroma/user/info.ex b/lib/pleroma/user/info.ex index 67e8801ea..4cc3f2f2c 100644 --- a/lib/pleroma/user/info.ex +++ b/lib/pleroma/user/info.ex @@ -330,4 +330,14 @@ def remove_reblog_mute(info, ap_id) do cast(info, params, [:muted_reblogs]) end + + def follow_information_update(info, params) do + info + |> cast(params, [ + :hide_followers, + :hide_follows, + :follower_count, + :following_count + ]) + end end diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index c821ba45f..2dd9dbf7f 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1022,15 +1022,13 @@ def fetch_follow_information_for_user(user) do Fetcher.fetch_and_contain_remote_object_from_id(user.follower_address), followers_count when is_integer(followers_count) <- followers_data["totalItems"], {:ok, hide_followers} <- collection_private(followers_data) do - info = %{ - hide_follows: hide_follows, - follower_count: followers_count, - following_count: following_count, - hide_followers: hide_followers - } - - info = Map.merge(user.info, info) - {:ok, Map.put(user, :info, info)} + {:ok, + %{ + hide_follows: hide_follows, + follower_count: followers_count, + following_count: following_count, + hide_followers: hide_followers + }} else {:error, _} = e -> e @@ -1043,8 +1041,9 @@ def fetch_follow_information_for_user(user) do defp maybe_update_follow_information(data) do with {:enabled, true} <- {:enabled, Pleroma.Config.get([:instance, :external_user_synchronization])}, - {:ok, data} <- fetch_follow_information_for_user(data) do - data + {:ok, info} <- fetch_follow_information_for_user(data) do + info = Map.merge(data.info, info) + Map.put(data, :info, info) else {:enabled, false} -> data diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 448ffbf54..24d8493fe 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1232,9 +1232,9 @@ test "syncronizes following/followers counters" do following_address: "http://localhost:4001/users/fuser2/following" ) - {:ok, user} = ActivityPub.fetch_follow_information_for_user(user) - assert user.info.follower_count == 527 - assert user.info.following_count == 267 + {:ok, info} = ActivityPub.fetch_follow_information_for_user(user) + assert info.follower_count == 527 + assert info.following_count == 267 end test "detects hidden followers" do @@ -1265,9 +1265,9 @@ test "detects hidden followers" do following_address: "http://localhost:4001/users/masto_closed/following" ) - {:ok, user} = ActivityPub.fetch_follow_information_for_user(user) - assert user.info.hide_followers == true - assert user.info.hide_follows == false + {:ok, info} = ActivityPub.fetch_follow_information_for_user(user) + assert info.hide_followers == true + assert info.hide_follows == false end test "detects hidden follows" do @@ -1298,9 +1298,9 @@ test "detects hidden follows" do following_address: "http://localhost:4001/users/masto_closed/following" ) - {:ok, user} = ActivityPub.fetch_follow_information_for_user(user) - assert user.info.hide_followers == false - assert user.info.hide_follows == true + {:ok, info} = ActivityPub.fetch_follow_information_for_user(user) + assert info.hide_followers == false + assert info.hide_follows == true end end end From d4ee76ab6355db0bed59b5126fe04d3399561798 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 20 Jul 2019 18:52:41 +0000 Subject: [PATCH 6/9] Apply suggestion to lib/pleroma/user.ex --- lib/pleroma/user.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 2e9b01205..956ec6240 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -708,7 +708,7 @@ def maybe_fetch_follow_information(user) do else e -> Logger.error( - "Follower/Following counter update for #{user.ap_id} failed.\n" <> inspect(e) + "Follower/Following counter update for #{user.ap_id} failed.\n#{inspect(e)}" ) user From c3ecaea64dd377b586e3b2a5316e90884ec78fe6 Mon Sep 17 00:00:00 2001 From: rinpatch Date: Sat, 20 Jul 2019 18:53:00 +0000 Subject: [PATCH 7/9] Apply suggestion to lib/pleroma/object/fetcher.ex --- lib/pleroma/object/fetcher.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index bc3e7e5bc..1e60d0082 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -97,7 +97,8 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do end end - def fetch_and_contain_remote_object_from_id(_id) do + def fetch_and_contain_remote_object_from_id(%{"id" => id), do: fetch_and_contain_remote_object_from_id(id) + def fetch_and_contain_remote_object_from_id(_id), do: {:error, "id must be a string"} {:error, "id must be a string"} end end From f42719506c539a4058c52d3a6e4a828948ac74ce Mon Sep 17 00:00:00 2001 From: rinpatch Date: Wed, 31 Jul 2019 14:20:34 +0300 Subject: [PATCH 8/9] Fix credo issues --- lib/pleroma/user.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index fd1c0a544..7acf1e53c 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -741,7 +741,7 @@ def fetch_follow_information(user) do end def update_follower_count(%User{} = user) do - unless user.local == false and Pleroma.Config.get([:instance, :external_user_synchronization]) do + unless !user.local and Pleroma.Config.get([:instance, :external_user_synchronization]) do follower_count_query = User.Query.build(%{followers: user, deactivated: false}) |> select([u], %{count: count(u.id)}) From 301ea0dc0466371032f44f3e936d1b951ed9784c Mon Sep 17 00:00:00 2001 From: rinpatch Date: Wed, 31 Jul 2019 19:37:55 +0300 Subject: [PATCH 9/9] Add tests for counters being updated on follow --- lib/pleroma/user.ex | 2 +- .../masto_closed_followers_page.json | 1 + .../masto_closed_following_page.json | 1 + test/support/http_request_mock.ex | 16 ++++ test/user_test.exs | 74 +++++++++++++++++++ test/web/activity_pub/activity_pub_test.exs | 20 ----- 6 files changed, 93 insertions(+), 21 deletions(-) create mode 100644 test/fixtures/users_mock/masto_closed_followers_page.json create mode 100644 test/fixtures/users_mock/masto_closed_following_page.json diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 7acf1e53c..69835f3dd 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -741,7 +741,7 @@ def fetch_follow_information(user) do end def update_follower_count(%User{} = user) do - unless !user.local and Pleroma.Config.get([:instance, :external_user_synchronization]) do + if user.local or !Pleroma.Config.get([:instance, :external_user_synchronization]) do follower_count_query = User.Query.build(%{followers: user, deactivated: false}) |> select([u], %{count: count(u.id)}) diff --git a/test/fixtures/users_mock/masto_closed_followers_page.json b/test/fixtures/users_mock/masto_closed_followers_page.json new file mode 100644 index 000000000..04ab0c4d3 --- /dev/null +++ b/test/fixtures/users_mock/masto_closed_followers_page.json @@ -0,0 +1 @@ +{"@context":"https://www.w3.org/ns/activitystreams","id":"http://localhost:4001/users/masto_closed/followers?page=1","type":"OrderedCollectionPage","totalItems":437,"next":"http://localhost:4001/users/masto_closed/followers?page=2","partOf":"http://localhost:4001/users/masto_closed/followers","orderedItems":["https://testing.uguu.ltd/users/rin","https://patch.cx/users/rin","https://letsalllovela.in/users/xoxo","https://pleroma.site/users/crushv","https://aria.company/users/boris","https://kawen.space/users/crushv","https://freespeech.host/users/cvcvcv","https://pleroma.site/users/picpub","https://pixelfed.social/users/nosleep","https://boopsnoot.gq/users/5c1896d162f7d337f90492a3","https://pikachu.rocks/users/waifu","https://royal.crablettesare.life/users/crablettes"]} diff --git a/test/fixtures/users_mock/masto_closed_following_page.json b/test/fixtures/users_mock/masto_closed_following_page.json new file mode 100644 index 000000000..8d8324699 --- /dev/null +++ b/test/fixtures/users_mock/masto_closed_following_page.json @@ -0,0 +1 @@ +{"@context":"https://www.w3.org/ns/activitystreams","id":"http://localhost:4001/users/masto_closed/following?page=1","type":"OrderedCollectionPage","totalItems":152,"next":"http://localhost:4001/users/masto_closed/following?page=2","partOf":"http://localhost:4001/users/masto_closed/following","orderedItems":["https://testing.uguu.ltd/users/rin","https://patch.cx/users/rin","https://letsalllovela.in/users/xoxo","https://pleroma.site/users/crushv","https://aria.company/users/boris","https://kawen.space/users/crushv","https://freespeech.host/users/cvcvcv","https://pleroma.site/users/picpub","https://pixelfed.social/users/nosleep","https://boopsnoot.gq/users/5c1896d162f7d337f90492a3","https://pikachu.rocks/users/waifu","https://royal.crablettesare.life/users/crablettes"]} diff --git a/test/support/http_request_mock.ex b/test/support/http_request_mock.ex index 2ed5f5042..bdfe43b28 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -792,6 +792,14 @@ def get("http://localhost:4001/users/masto_closed/followers", _, _, _) do }} end + def get("http://localhost:4001/users/masto_closed/followers?page=1", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/users_mock/masto_closed_followers_page.json") + }} + end + def get("http://localhost:4001/users/masto_closed/following", _, _, _) do {:ok, %Tesla.Env{ @@ -800,6 +808,14 @@ def get("http://localhost:4001/users/masto_closed/following", _, _, _) do }} end + def get("http://localhost:4001/users/masto_closed/following?page=1", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/users_mock/masto_closed_following_page.json") + }} + end + def get("http://localhost:4001/users/fuser2/followers", _, _, _) do {:ok, %Tesla.Env{ diff --git a/test/user_test.exs b/test/user_test.exs index 556df45fd..7ec241c25 100644 --- a/test/user_test.exs +++ b/test/user_test.exs @@ -1393,4 +1393,78 @@ test "performs update cache if user updated" do assert %User{bio: "test-bio"} = User.get_cached_by_ap_id(user.ap_id) end end + + describe "following/followers synchronization" do + setup do + sync = Pleroma.Config.get([:instance, :external_user_synchronization]) + on_exit(fn -> Pleroma.Config.put([:instance, :external_user_synchronization], sync) end) + end + + test "updates the counters normally on following/getting a follow when disabled" do + Pleroma.Config.put([:instance, :external_user_synchronization], false) + user = insert(:user) + + other_user = + insert(:user, + local: false, + follower_address: "http://localhost:4001/users/masto_closed/followers", + following_address: "http://localhost:4001/users/masto_closed/following", + info: %{ap_enabled: true} + ) + + assert User.user_info(other_user).following_count == 0 + assert User.user_info(other_user).follower_count == 0 + + {:ok, user} = Pleroma.User.follow(user, other_user) + other_user = Pleroma.User.get_by_id(other_user.id) + + assert User.user_info(user).following_count == 1 + assert User.user_info(other_user).follower_count == 1 + end + + test "syncronizes the counters with the remote instance for the followed when enabled" do + Pleroma.Config.put([:instance, :external_user_synchronization], false) + + user = insert(:user) + + other_user = + insert(:user, + local: false, + follower_address: "http://localhost:4001/users/masto_closed/followers", + following_address: "http://localhost:4001/users/masto_closed/following", + info: %{ap_enabled: true} + ) + + assert User.user_info(other_user).following_count == 0 + assert User.user_info(other_user).follower_count == 0 + + Pleroma.Config.put([:instance, :external_user_synchronization], true) + {:ok, _user} = User.follow(user, other_user) + other_user = User.get_by_id(other_user.id) + + assert User.user_info(other_user).follower_count == 437 + end + + test "syncronizes the counters with the remote instance for the follower when enabled" do + Pleroma.Config.put([:instance, :external_user_synchronization], false) + + user = insert(:user) + + other_user = + insert(:user, + local: false, + follower_address: "http://localhost:4001/users/masto_closed/followers", + following_address: "http://localhost:4001/users/masto_closed/following", + info: %{ap_enabled: true} + ) + + assert User.user_info(other_user).following_count == 0 + assert User.user_info(other_user).follower_count == 0 + + Pleroma.Config.put([:instance, :external_user_synchronization], true) + {:ok, other_user} = User.follow(other_user, user) + + assert User.user_info(other_user).following_count == 152 + end + end end diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 853c93ab5..3d9a678dd 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1149,16 +1149,6 @@ test "detects hidden followers" do "http://localhost:4001/users/masto_closed/followers?page=1" -> %Tesla.Env{status: 403, body: ""} - "http://localhost:4001/users/masto_closed/following?page=1" -> - %Tesla.Env{ - status: 200, - body: - Jason.encode!(%{ - "id" => "http://localhost:4001/users/masto_closed/following?page=1", - "type" => "OrderedCollectionPage" - }) - } - _ -> apply(HttpRequestMock, :request, [env]) end @@ -1182,16 +1172,6 @@ test "detects hidden follows" do "http://localhost:4001/users/masto_closed/following?page=1" -> %Tesla.Env{status: 403, body: ""} - "http://localhost:4001/users/masto_closed/followers?page=1" -> - %Tesla.Env{ - status: 200, - body: - Jason.encode!(%{ - "id" => "http://localhost:4001/users/masto_closed/followers?page=1", - "type" => "OrderedCollectionPage" - }) - } - _ -> apply(HttpRequestMock, :request, [env]) end