From 2edb0944ba9bf4c0694a5c8b1e3e89dd86794271 Mon Sep 17 00:00:00 2001 From: ilja Date: Sun, 26 Mar 2023 13:52:22 +0200 Subject: [PATCH] Return the complete `upload` from Pleroma.Uploaders.put_file/2 To move more logic to the Uploader modules, it's better to keep the same `upload` and manipulate that while going through the upload pipeline, so that's how it's now done. Since we expect the `upload`, I removed the option for returning just `:ok` from the uploaders. It was possible to bypass the logic in the `get_url` function, but that was unused. Since it makes even less sense when returning `upload`, I removed that In a later stage I want to return the url from the uploader module, which should obsolete this function. I saw a warning from `Pleroma.Upload.Filter.MogrifyTest` which I now fixed. --- lib/pleroma/upload.ex | 12 ++++-------- lib/pleroma/uploaders/local.ex | 2 +- lib/pleroma/uploaders/s3.ex | 2 +- lib/pleroma/uploaders/uploader.ex | 21 +++++++++------------ test/pleroma/upload/filter/mogrify_test.exs | 2 +- test/pleroma/upload_test.exs | 6 +++--- test/pleroma/uploaders/local_test.exs | 6 +++--- test/pleroma/uploaders/s3_test.exs | 2 +- 8 files changed, 23 insertions(+), 30 deletions(-) diff --git a/lib/pleroma/upload.ex b/lib/pleroma/upload.ex index 173ef5e3b..e2dc182db 100644 --- a/lib/pleroma/upload.ex +++ b/lib/pleroma/upload.ex @@ -87,7 +87,7 @@ defmodule Pleroma.Upload do {_, true} <- {:description_limit, String.length(description) <= Config.get([:instance, :description_limit])}, - {:ok, file} <- Pleroma.Uploaders.Uploader.put_file(uploader, upload) do + {:ok, upload} <- Pleroma.Uploaders.Uploader.put_file(uploader, upload) do {:ok, %{ "id" => Utils.generate_object_id(), @@ -97,7 +97,7 @@ defmodule Pleroma.Upload do %{ "type" => "Link", "mediaType" => upload.content_type, - "href" => get_url(upload, file) + "href" => get_url(upload.name, upload.path) } |> Maps.put_if_present("width", upload.width) |> Maps.put_if_present("height", upload.height) @@ -110,9 +110,7 @@ defmodule Pleroma.Upload do {:error, :description_too_long} {:error, error} -> - Logger.error( - "#{__MODULE__} store (using #{inspect(uploader)}) failed: #{inspect(error)}" - ) + Logger.error("#{__MODULE__} store (using #{inspect(uploader)}) failed: #{inspect(error)}") {:error, error} end @@ -214,7 +212,7 @@ defmodule Pleroma.Upload do tmp_path end - defp get_url(%__MODULE__{name: name}, {:file, path}) do + defp get_url(name, path) do base_url = base_url() path = @@ -229,7 +227,5 @@ defmodule Pleroma.Upload do |> Path.join() end - defp get_url(_upload, {:url, url}), do: url - def base_url(), do: Config.get([__MODULE__, :uploader]).base_url() end diff --git a/lib/pleroma/uploaders/local.ex b/lib/pleroma/uploaders/local.ex index c4e0d912d..d0202f076 100644 --- a/lib/pleroma/uploaders/local.ex +++ b/lib/pleroma/uploaders/local.ex @@ -30,7 +30,7 @@ defmodule Pleroma.Uploaders.Local do File.cp!(upload.tempfile, result_file) end - :ok + {:ok, upload} end def upload_path do diff --git a/lib/pleroma/uploaders/s3.ex b/lib/pleroma/uploaders/s3.ex index de5a7892f..e714f5719 100644 --- a/lib/pleroma/uploaders/s3.ex +++ b/lib/pleroma/uploaders/s3.ex @@ -47,7 +47,7 @@ defmodule Pleroma.Uploaders.S3 do case ExAws.request(op) do {:ok, _} -> - {:ok, {:file, s3_name}} + {:ok, Map.put(upload, :path, s3_name)} error -> Logger.error("#{__MODULE__}: #{inspect(error)}") diff --git a/lib/pleroma/uploaders/uploader.ex b/lib/pleroma/uploaders/uploader.ex index 96545a4a4..15840a049 100644 --- a/lib/pleroma/uploaders/uploader.ex +++ b/lib/pleroma/uploaders/uploader.ex @@ -34,26 +34,23 @@ defmodule Pleroma.Uploaders.Uploader do * `:wait_callback` will wait for an http post request at `/api/pleroma/upload_callback/:upload_path` and call the uploader's `http_callback/3` method. """ - @type file_spec :: {:file | :url, String.t()} @callback put_file(upload :: struct()) :: - :ok | {:ok, file_spec()} | {:error, String.t()} | :wait_callback + :ok | {:ok, Pleroma.Upload} | {:error, String.t()} | :wait_callback @callback delete_file(file :: String.t()) :: :ok | {:error, String.t()} @callback base_url() :: String.t() - @callback http_callback(Plug.Conn.t(), Map.t()) :: - {:ok, Plug.Conn.t()} - | {:ok, Plug.Conn.t(), file_spec()} + @callback http_callback(Plug.Conn.t(), Map.t(), Pleroma.Upload) :: + {:ok, Plug.Conn.t(), Pleroma.Upload} | {:error, Plug.Conn.t(), String.t()} - @optional_callbacks http_callback: 2 + @optional_callbacks http_callback: 3 - @spec put_file(module(), upload :: struct()) :: {:ok, file_spec()} | {:error, String.t()} + @spec put_file(module(), upload :: struct()) :: {:ok, Pleroma.Upload} | {:error, String.t()} def put_file(uploader, upload) do case uploader.put_file(upload) do - :ok -> {:ok, {:file, upload.path}} + {:ok, %Pleroma.Upload{}} = ok -> ok :wait_callback -> handle_callback(uploader, upload) - {:ok, _} = ok -> ok {:error, _} = error -> error end end @@ -63,10 +60,10 @@ defmodule Pleroma.Uploaders.Uploader do receive do {__MODULE__, pid, conn, params} -> - case uploader.http_callback(conn, params) do - {:ok, conn, ok} -> + case uploader.http_callback(conn, params, upload) do + {:ok, conn, upload} -> send(pid, {__MODULE__, conn}) - {:ok, ok} + {:ok, upload} {:error, conn, error} -> send(pid, {__MODULE__, conn}) diff --git a/test/pleroma/upload/filter/mogrify_test.exs b/test/pleroma/upload/filter/mogrify_test.exs index d62cd83b4..96e3db93a 100644 --- a/test/pleroma/upload/filter/mogrify_test.exs +++ b/test/pleroma/upload/filter/mogrify_test.exs @@ -9,7 +9,7 @@ defmodule Pleroma.Upload.Filter.MogrifyTest do alias Pleroma.Upload.Filter test "apply mogrify filter" do - clear_config(Filter.Mogrify, args: [{"tint", "40"}]) + clear_config([Filter.Mogrify, :args], [{"tint", "40"}]) File.cp!( "test/fixtures/image.jpg", diff --git a/test/pleroma/upload_test.exs b/test/pleroma/upload_test.exs index 165e77a57..5ae689c9d 100644 --- a/test/pleroma/upload_test.exs +++ b/test/pleroma/upload_test.exs @@ -37,8 +37,8 @@ defmodule Pleroma.UploadTest do describe "Tried storing a file when http callback response success result" do defmodule TestUploaderSuccess do - def http_callback(conn, _params), - do: {:ok, conn, {:file, "post-process-file.jpg"}} + def http_callback(conn, _params, upload), + do: {:ok, conn, Map.put(upload, :path, "post-process-file.jpg")} def put_file(upload), do: TestUploaderBase.put_file(upload, __MODULE__) @@ -74,7 +74,7 @@ defmodule Pleroma.UploadTest do describe "Tried storing a file when http callback response error" do defmodule TestUploaderError do - def http_callback(conn, _params), do: {:error, conn, "Errors"} + def http_callback(conn, _params, _upload), do: {:error, conn, "Errors"} def put_file(upload), do: TestUploaderBase.put_file(upload, __MODULE__) end diff --git a/test/pleroma/uploaders/local_test.exs b/test/pleroma/uploaders/local_test.exs index 0a5952f50..5d405dac5 100644 --- a/test/pleroma/uploaders/local_test.exs +++ b/test/pleroma/uploaders/local_test.exs @@ -24,7 +24,7 @@ defmodule Pleroma.Uploaders.LocalTest do tempfile: Path.absname("test/fixtures/image_tmp.jpg") } - assert Local.put_file(file) == :ok + assert Local.put_file(file) == {:ok, file} assert Path.join([Local.upload_path(), file_path]) |> File.exists?() @@ -43,8 +43,8 @@ defmodule Pleroma.Uploaders.LocalTest do tempfile: Path.absname("test/fixtures/image_tmp.jpg") } - :ok = Local.put_file(file) - local_path = Path.join([Local.upload_path(), file_path]) + {:ok, file} = Local.put_file(file) + local_path = Path.join([Local.upload_path(), file.path]) assert File.exists?(local_path) Local.delete_file(file_path) diff --git a/test/pleroma/uploaders/s3_test.exs b/test/pleroma/uploaders/s3_test.exs index 2711e2c8d..4cc6d186d 100644 --- a/test/pleroma/uploaders/s3_test.exs +++ b/test/pleroma/uploaders/s3_test.exs @@ -67,7 +67,7 @@ defmodule Pleroma.Uploaders.S3Test do test "save file", %{file_upload: file_upload} do with_mock ExAws, request: fn _ -> {:ok, :ok} end do - assert S3.put_file(file_upload) == {:ok, {:file, "test_folder/image-tet.jpg"}} + assert S3.put_file(file_upload) == {:ok, file_upload} end end