Merge pull request 'OAuth App: Fix test, optimize code.' (#7692) from gitlab-mr-iid-4315 into develop

Reviewed-on: https://git.pleroma.social/pleroma/pleroma/pulls/7692

changelog.d/optimize-orphaned-apps-cleanup.change

@@ -0,0 +1 @@
+- Optimize orphaned OAuth apps cleanup by deleting records in batches instead of one by one.

lib/pleroma/web/o_auth/app.ex

@@ -8,6 +8,7 @@ defmodule Pleroma.Web.OAuth.App do
   import Ecto.Query
   alias Pleroma.Repo
   alias Pleroma.User
+  alias Pleroma.Web.OAuth.Authorization
   alias Pleroma.Web.OAuth.Token
 
   @type t :: %__MODULE__{}
@@ -191,15 +192,26 @@ defmodule Pleroma.Web.OAuth.App do
 
   @spec remove_orphans(pos_integer()) :: :ok
   def remove_orphans(limit \\ 100) do
-    fifteen_mins_ago = DateTime.add(DateTime.utc_now(), -900, :second)
+    fifteen_mins_ago = NaiveDateTime.add(NaiveDateTime.utc_now(), -900)
 
     Repo.transaction(fn ->
+      app_ids =
+        from(a in __MODULE__,
+          where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago,
+          limit: ^limit,
+          lock: "FOR UPDATE SKIP LOCKED",
+          select: a.id
+        )
+        |> Repo.all()
+
+      # Bulk deletes bypass the schema's association cleanup.
+      from(a in Authorization, where: a.app_id in ^app_ids) |> Repo.delete_all()
+      from(t in Token, where: t.app_id in ^app_ids) |> Repo.delete_all()
+
       from(a in __MODULE__,
-        where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago,
-        limit: ^limit
+        where: a.id in ^app_ids and is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago
       )
-      |> Repo.all()
-      |> Enum.each(&Repo.delete(&1))
+      |> Repo.delete_all()
     end)
 
     :ok

test/pleroma/web/o_auth/app_test.exs

@@ -5,7 +5,11 @@
 defmodule Pleroma.Web.OAuth.AppTest do
   use Pleroma.DataCase, async: true
 
+  alias Pleroma.MFA.Token, as: MFAToken
   alias Pleroma.Web.OAuth.App
+  alias Pleroma.Web.OAuth.Authorization
+  alias Pleroma.Web.OAuth.Token
+  alias Pleroma.Web.Push.Subscription
   import Pleroma.Factory
 
   describe "get_or_make/2" do
@@ -57,29 +61,32 @@ defmodule Pleroma.Web.OAuth.AppTest do
   test "removes orphaned apps" do
     attrs = %{client_name: "Mastodon-Local", redirect_uris: "."}
     {:ok, %App{} = old_app} = App.get_or_make(attrs, ["write"])
+    authorization = insert(:oauth_authorization, app: old_app)
+    token = insert(:oauth_token, app: old_app)
+    mfa_token = insert(:mfa_token, authorization: authorization)
+    push_subscription = insert(:push_subscription, token: token)
 
-    # backdate the old app so it's within the threshold for being cleaned up
-    one_hour_ago = DateTime.add(DateTime.utc_now(), -3600)
+    user = insert(:user)
+    attrs = %{client_name: "OldButValid", redirect_uris: ".", user_id: user.id}
+    {:ok, %App{} = old_owned_app} = App.get_or_make(attrs, ["write"])
+
+    one_hour_ago = NaiveDateTime.add(NaiveDateTime.utc_now(), -3600)
 
     {:ok, _} =
-      "UPDATE apps SET inserted_at = $1, updated_at = $1 WHERE id = $2"
-      |> Pleroma.Repo.query([one_hour_ago, old_app.id])
+      "UPDATE apps SET inserted_at = $1, updated_at = $1 WHERE id IN ($2, $3)"
+      |> Repo.query([one_hour_ago, old_app.id, old_owned_app.id])
 
-    # Create the new app after backdating the old one
     attrs = %{client_name: "PleromaFE", redirect_uris: "."}
-    {:ok, %App{} = app} = App.get_or_make(attrs, ["write"])
-
-    # Ensure the new app has a recent timestamp
-    now = DateTime.utc_now()
-
-    {:ok, _} =
-      "UPDATE apps SET inserted_at = $1, updated_at = $1 WHERE id = $2"
-      |> Pleroma.Repo.query([now, app.id])
+    {:ok, %App{} = recent_app} = App.get_or_make(attrs, ["write"])
 
     App.remove_orphans()
 
-    assert [returned_app] = Pleroma.Repo.all(App)
-    assert returned_app.client_name == "PleromaFE"
-    assert returned_app.id == app.id
+    refute Repo.get(App, old_app.id)
+    refute Repo.get(Authorization, authorization.id)
+    refute Repo.get(Token, token.id)
+    refute Repo.get(MFAToken, mfa_token.id)
+    refute Repo.get(Subscription, push_subscription.id)
+    assert Repo.get(App, recent_app.id)
+    assert Repo.get(App, old_owned_app.id)
   end
 end