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

@@ -87,7 +87,7 @@ def store(upload, opts \\ []) 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 @@ def store(upload, opts \\ []) 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 @@ def store(upload, opts \\ []) 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 @@ defp tempfile_for_image(data) 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 @@ defp get_url(%__MODULE__{name: name}, {:file, path}) do
     |> Path.join()
   end
 
-  defp get_url(_upload, {:url, url}), do: url
-
   def base_url(), do: Config.get([__MODULE__, :uploader]).base_url()
 end

lib/pleroma/uploaders/local.ex

@@ -30,7 +30,7 @@ def put_file(upload) do
       File.cp!(upload.tempfile, result_file)
     end
 
-    :ok
+    {:ok, upload}
   end
 
   def upload_path do

lib/pleroma/uploaders/s3.ex

@@ -47,7 +47,7 @@ def put_file(%Pleroma.Upload{} = upload) do
 
     case ExAws.request(op) do
       {:ok, _} ->
-        {:ok, {:file, s3_name}}
+        {:ok, Map.put(upload, :path, s3_name)}
 
       error ->
         Logger.error("#{__MODULE__}: #{inspect(error)}")

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 @@ defp handle_callback(uploader, upload) 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})

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",

test/pleroma/upload_test.exs

@@ -37,8 +37,8 @@ def put_file(%{path: path} = _upload, module_name) 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 @@ test "it returns file" 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

test/pleroma/uploaders/local_test.exs

@@ -24,7 +24,7 @@ test "put file to local folder" 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 @@ test "deletes local file" 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)

test/pleroma/uploaders/s3_test.exs

@@ -67,7 +67,7 @@ test "it returns path with bucket namespace when namespace is set" 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