Page MenuHomePhorge
Files F85825256

No OneTemporary
Actions

Authored By
Unknown
Size
9 KB
Referenced Files
None
Subscribers
None

View Options

diff --git a/changelog.d/optimize-orphaned-apps-cleanup.change b/changelog.d/optimize-orphaned-apps-cleanup.change
new file mode 100644
index 000000000..e86b75524
--- /dev/null
+++ b/changelog.d/optimize-orphaned-apps-cleanup.change
@@ -0,0 +1 @@
+- Optimize orphaned OAuth apps cleanup by using a single delete query instead of deleting records one by one.
diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex
index 7661c2566..30a9d9134 100644
--- a/lib/pleroma/web/o_auth/app.ex
+++ b/lib/pleroma/web/o_auth/app.ex
@@ -1,184 +1,188 @@
# Pleroma: A lightweight social networking server
# Copyright © 2017-2022 Pleroma Authors <https://pleroma.social/>
# SPDX-License-Identifier: AGPL-3.0-only
defmodule Pleroma.Web.OAuth.App do
use Ecto.Schema
import Ecto.Changeset
import Ecto.Query
alias Pleroma.Repo
alias Pleroma.User
alias Pleroma.Web.OAuth.Token
@type t :: %__MODULE__{}
schema "apps" do
field(:client_name, :string)
field(:redirect_uris, :string)
field(:scopes, {:array, :string}, default: [])
field(:website, :string)
field(:client_id, :string)
field(:client_secret, :string)
field(:trusted, :boolean, default: false)
belongs_to(:user, User, type: FlakeId.Ecto.CompatType)
has_many(:oauth_authorizations, Pleroma.Web.OAuth.Authorization, on_delete: :delete_all)
has_many(:oauth_tokens, Pleroma.Web.OAuth.Token, on_delete: :delete_all)
timestamps()
end
@spec changeset(t(), map()) :: Ecto.Changeset.t()
def changeset(struct, params) do
cast(struct, params, [:client_name, :redirect_uris, :scopes, :website, :trusted, :user_id])
end
@spec register_changeset(t(), map()) :: Ecto.Changeset.t()
def register_changeset(struct, params \\ %{}) do
changeset =
struct
|> changeset(params)
|> validate_required([:client_name, :redirect_uris, :scopes])
if changeset.valid? do
changeset
|> put_change(
:client_id,
:crypto.strong_rand_bytes(32) |> Base.url_encode64(padding: false)
)
|> put_change(
:client_secret,
:crypto.strong_rand_bytes(32) |> Base.url_encode64(padding: false)
)
else
changeset
end
end
@spec create(map()) :: {:ok, t()} | {:error, Ecto.Changeset.t()}
def create(params) do
%__MODULE__{}
|> register_changeset(params)
|> Repo.insert()
end
@spec update(pos_integer(), map()) :: {:ok, t()} | {:error, Ecto.Changeset.t()} | nil
def update(id, params) do
with %__MODULE__{} = app <- Repo.get(__MODULE__, id) do
app
|> changeset(params)
|> Repo.update()
end
end
@doc """
Gets app by attrs or create new with attrs.
And updates the scopes if need.
"""
@spec get_or_make(map(), list(String.t())) :: {:ok, t()} | {:error, Ecto.Changeset.t()}
def get_or_make(attrs, scopes) do
with %__MODULE__{} = app <- Repo.get_by(__MODULE__, attrs) do
update_scopes(app, scopes)
else
_e ->
%__MODULE__{}
|> register_changeset(Map.put(attrs, :scopes, scopes))
|> Repo.insert()
end
end
defp update_scopes(%__MODULE__{} = app, []), do: {:ok, app}
defp update_scopes(%__MODULE__{scopes: scopes} = app, scopes), do: {:ok, app}
defp update_scopes(%__MODULE__{} = app, scopes) do
app
|> change(%{scopes: scopes})
|> Repo.update()
end
@spec search(map()) :: {:ok, [t()], non_neg_integer()}
def search(params) do
query = from(a in __MODULE__)
query =
if params[:client_name] do
from(a in query, where: a.client_name == ^params[:client_name])
else
query
end
query =
if params[:client_id] do
from(a in query, where: a.client_id == ^params[:client_id])
else
query
end
query =
if Map.has_key?(params, :trusted) do
from(a in query, where: a.trusted == ^params[:trusted])
else
query
end
query =
from(u in query,
limit: ^params[:page_size],
offset: ^((params[:page] - 1) * params[:page_size])
)
count = Repo.aggregate(__MODULE__, :count, :id)
{:ok, Repo.all(query), count}
end
@spec get_user_apps(User.t()) :: {:ok, [t()], non_neg_integer()}
def get_user_apps(%User{id: user_id}) do
from(a in __MODULE__, where: a.user_id == ^user_id)
|> Repo.all()
end
@spec destroy(pos_integer()) :: {:ok, t()} | {:error, Ecto.Changeset.t()}
def destroy(id) do
with %__MODULE__{} = app <- Repo.get(__MODULE__, id) do
Repo.delete(app)
end
end
@spec errors(Ecto.Changeset.t()) :: map()
def errors(changeset) do
Enum.reduce(changeset.errors, %{}, fn
{:client_name, {error, _}}, acc ->
Map.put(acc, :name, error)
{key, {error, _}}, acc ->
Map.put(acc, key, error)
end)
end
@spec maybe_update_owner(Token.t()) :: :ok
def maybe_update_owner(%Token{app_id: app_id, user_id: user_id}) when not is_nil(user_id) do
__MODULE__.update(app_id, %{user_id: user_id})
:ok
end
def maybe_update_owner(_), do: :ok
@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 ->
+ # First get the IDs of apps to delete
+ app_ids =
from(a in __MODULE__,
where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago,
- limit: ^limit
+ limit: ^limit,
+ select: a.id
)
|> Repo.all()
- |> Enum.each(&Repo.delete(&1))
- end)
+
+ # Then delete those specific apps
+ from(a in __MODULE__, where: a.id in ^app_ids)
+ |> Repo.delete_all()
:ok
end
end
diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs
index 44219cf90..871b10ac3 100644
--- a/test/pleroma/web/o_auth/app_test.exs
+++ b/test/pleroma/web/o_auth/app_test.exs
@@ -1,73 +1,103 @@
# Pleroma: A lightweight social networking server
# Copyright © 2017-2022 Pleroma Authors <https://pleroma.social/>
# SPDX-License-Identifier: AGPL-3.0-only
defmodule Pleroma.Web.OAuth.AppTest do
use Pleroma.DataCase, async: true
alias Pleroma.Web.OAuth.App
import Pleroma.Factory
describe "get_or_make/2" do
test "gets exist app" do
attrs = %{client_name: "Mastodon-Local", redirect_uris: "."}
app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]}))
{:ok, %App{} = exist_app} = App.get_or_make(attrs, [])
assert exist_app == app
end
test "make app" do
attrs = %{client_name: "Mastodon-Local", redirect_uris: "."}
{:ok, %App{} = app} = App.get_or_make(attrs, ["write"])
assert app.scopes == ["write"]
end
test "gets exist app and updates scopes" do
attrs = %{client_name: "Mastodon-Local", redirect_uris: "."}
app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]}))
{:ok, %App{} = exist_app} = App.get_or_make(attrs, ["read", "write", "follow", "push"])
assert exist_app.id == app.id
assert exist_app.scopes == ["read", "write", "follow", "push"]
end
test "has unique client_id" do
insert(:oauth_app, client_name: "", redirect_uris: "", client_id: "boop")
error =
catch_error(insert(:oauth_app, client_name: "", redirect_uris: "", client_id: "boop"))
assert %Ecto.ConstraintError{} = error
assert error.constraint == "apps_client_id_index"
assert error.type == :unique
end
end
test "get_user_apps/1" do
user = insert(:user)
apps = [
insert(:oauth_app, user_id: user.id),
insert(:oauth_app, user_id: user.id),
insert(:oauth_app, user_id: user.id)
]
assert Enum.sort(App.get_user_apps(user)) == Enum.sort(apps)
end
test "removes orphaned apps" do
+ # Create an orphaned app (no user_id)
attrs = %{client_name: "Mastodon-Local", redirect_uris: "."}
{:ok, %App{} = old_app} = App.get_or_make(attrs, ["write"])
- attrs = %{client_name: "PleromaFE", redirect_uris: "."}
- {:ok, %App{} = app} = App.get_or_make(attrs, ["write"])
+ # Create a non-orphaned app with a user
+ user = insert(:user)
+ attrs = %{client_name: "PleromaFE", redirect_uris: ".", user_id: user.id}
+ {:ok, %App{} = kept_app} = App.get_or_make(attrs, ["write"])
+
+ # Create an old but non-orphaned app
+ attrs = %{client_name: "OldButValid", redirect_uris: ".", user_id: user.id}
+ {:ok, %App{} = old_kept_app} = App.get_or_make(attrs, ["write"])
+
+ # backdate both old apps so they're within the threshold for being cleaned up
+ # 1 hour ago
+ past_time = NaiveDateTime.add(NaiveDateTime.utc_now(), -3600)
- # backdate the old app so it's within the threshold for being cleaned up
{:ok, _} =
- "UPDATE apps SET inserted_at = now() - interval '1 hour' WHERE id = #{old_app.id}"
- |> Pleroma.Repo.query()
+ Repo.query(
+ "UPDATE apps SET inserted_at = $1 WHERE id IN ($2, $3)",
+ [past_time, old_app.id, old_kept_app.id]
+ )
+
+ # Ensure the updates were applied
+ updated_app = Repo.get(App, old_app.id)
+ updated_kept_app = Repo.get(App, old_kept_app.id)
+
+ assert NaiveDateTime.compare(
+ updated_app.inserted_at,
+ NaiveDateTime.add(NaiveDateTime.utc_now(), -900)
+ ) == :lt
+
+ assert NaiveDateTime.compare(
+ updated_kept_app.inserted_at,
+ NaiveDateTime.add(NaiveDateTime.utc_now(), -900)
+ ) == :lt
App.remove_orphans()
- assert [app] == Pleroma.Repo.all(App)
+ # Verify the orphaned app was removed
+ assert is_nil(Repo.get(App, old_app.id))
+ # Verify both non-orphaned apps still exist, regardless of age
+ assert Repo.get(App, kept_app.id)
+ assert Repo.get(App, old_kept_app.id)
end
end

File Metadata

Mime Type
text/x-diff
Expires
Thu, Oct 29, 11:42 PM (1 d, 11 h)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
1798880
Default Alt Text
(9 KB)

Event Timeline