Move building of url to modules

We got a path and there's a "base_url" function. With that the url was
build. But not all url's are nessecarely build like this. Building the
url should be the responsability of the Uploader itself, so it's now
moved to there.

lib/pleroma/upload.ex

@@ -63,10 +63,10 @@ defmodule Pleroma.Upload do
           blurhash: String.t(),
           path: String.t()
         }
-  defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path]
+  defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path, :url]
 
   defp get_description(opts, upload) do
-    case {opts[:description], Pleroma.Config.get([Pleroma.Upload, :default_description])} do
+    case {opts[:description], Config.get([Pleroma.Upload, :default_description])} do
       {description, _} when is_binary(description) -> description
       {_, :filename} -> upload.name
       {_, str} when is_binary(str) -> str
@@ -78,6 +78,7 @@ defmodule Pleroma.Upload do
   @doc "Store a file. If using a `Plug.Upload{}` as the source, be sure to use `Majic.Plug` to ensure its content_type and filename is correct."
   def store(upload, opts \\ []) do
     opts = get_opts(opts)
+    uploader = Config.get([__MODULE__, :uploader])
 
     with {:ok, upload} <- prepare_upload(upload, opts),
          upload = %__MODULE__{upload | path: upload.path || "#{upload.id}/#{upload.name}"},
@@ -85,8 +86,8 @@ defmodule Pleroma.Upload do
          description = get_description(opts, upload),
          {_, true} <-
            {:description_limit,
-            String.length(description) <= Pleroma.Config.get([:instance, :description_limit])},
-         {:ok, url_spec} <- Pleroma.Uploaders.Uploader.put_file(opts.uploader, upload) do
+            String.length(description) <= Config.get([:instance, :description_limit])},
+         {:ok, upload} <- Pleroma.Uploaders.Uploader.put_file(uploader, upload) do
       {:ok,
        %{
          "id" => Utils.generate_object_id(),
@@ -96,7 +97,7 @@ defmodule Pleroma.Upload do
            %{
              "type" => "Link",
              "mediaType" => upload.content_type,
-             "href" => url_from_spec(upload, opts.base_url, url_spec)
+             "href" => url_with_query_params(upload)
            }
            |> Maps.put_if_present("width", upload.width)
            |> Maps.put_if_present("height", upload.height)
@@ -109,9 +110,7 @@ defmodule Pleroma.Upload do
         {:error, :description_too_long}
 
       {:error, error} ->
-        Logger.error(
-          "#{__MODULE__} store (using #{inspect(opts.uploader)}) failed: #{inspect(error)}"
-        )
+        Logger.error("#{__MODULE__} store (using #{inspect(uploader)}) failed: #{inspect(error)}")
 
         {:error, error}
     end
@@ -125,25 +124,23 @@ defmodule Pleroma.Upload do
     {size_limit, activity_type} =
       case Keyword.get(opts, :type) do
         :banner ->
-          {Pleroma.Config.get!([:instance, :banner_upload_limit]), "Image"}
+          {Config.get!([:instance, :banner_upload_limit]), "Image"}
 
         :avatar ->
-          {Pleroma.Config.get!([:instance, :avatar_upload_limit]), "Image"}
+          {Config.get!([:instance, :avatar_upload_limit]), "Image"}
 
         :background ->
-          {Pleroma.Config.get!([:instance, :background_upload_limit]), "Image"}
+          {Config.get!([:instance, :background_upload_limit]), "Image"}
 
         _ ->
-          {Pleroma.Config.get!([:instance, :upload_limit]), "Document"}
+          {Config.get!([:instance, :upload_limit]), "Document"}
       end
 
     %{
       activity_type: Keyword.get(opts, :activity_type, activity_type),
       size_limit: Keyword.get(opts, :size_limit, size_limit),
-      uploader: Keyword.get(opts, :uploader, Pleroma.Config.get([__MODULE__, :uploader])),
-      filters: Keyword.get(opts, :filters, Pleroma.Config.get([__MODULE__, :filters])),
-      description: Keyword.get(opts, :description),
-      base_url: base_url()
+      filters: Keyword.get(opts, :filters, Config.get([__MODULE__, :filters])),
+      description: Keyword.get(opts, :description)
     }
   end
 
@@ -215,55 +212,14 @@ defmodule Pleroma.Upload do
     tmp_path
   end
 
-  defp url_from_spec(%__MODULE__{name: name}, base_url, {:file, path}) do
-    path =
-      URI.encode(path, &char_unescaped?/1) <>
-        if Pleroma.Config.get([__MODULE__, :link_name], false) do
-          "?name=#{URI.encode(name, &char_unescaped?/1)}"
-        else
-          ""
-        end
-
-    [base_url, path]
-    |> Path.join()
+  defp url_with_query_params(%__MODULE__{url: url, name: name}) do
+    url <>
+      if Config.get([__MODULE__, :link_name], false) do
+        "?name=#{URI.encode(name, &char_unescaped?/1)}"
+      else
+        ""
+      end
   end
 
-  defp url_from_spec(_upload, _base_url, {:url, url}), do: url
-
-  def base_url do
-    uploader = Config.get([Pleroma.Upload, :uploader])
-    upload_base_url = Config.get([Pleroma.Upload, :base_url])
-    public_endpoint = Config.get([uploader, :public_endpoint])
-
-    case uploader do
-      Pleroma.Uploaders.Local ->
-        upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/"
-
-      Pleroma.Uploaders.S3 ->
-        bucket = Config.get([Pleroma.Uploaders.S3, :bucket])
-        truncated_namespace = Config.get([Pleroma.Uploaders.S3, :truncated_namespace])
-        namespace = Config.get([Pleroma.Uploaders.S3, :bucket_namespace])
-
-        bucket_with_namespace =
-          cond do
-            !is_nil(truncated_namespace) ->
-              truncated_namespace
-
-            !is_nil(namespace) ->
-              namespace <> ":" <> bucket
-
-            true ->
-              bucket
-          end
-
-        if public_endpoint do
-          Path.join([public_endpoint, bucket_with_namespace])
-        else
-          Path.join([upload_base_url, bucket_with_namespace])
-        end
-
-      _ ->
-        public_endpoint || upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/"
-    end
-  end
+  def base_url(), do: Config.get([__MODULE__, :uploader]).base_url()
 end

lib/pleroma/uploaders/local.ex

@@ -4,6 +4,7 @@
 
 defmodule Pleroma.Uploaders.Local do
   @behaviour Pleroma.Uploaders.Uploader
+  alias Pleroma.Config
 
   @impl true
   def get_file(_) do
@@ -29,11 +30,13 @@ defmodule Pleroma.Uploaders.Local do
       File.cp!(upload.tempfile, result_file)
     end
 
-    :ok
+    url = [base_url(), URI.encode(upload.path, &Pleroma.Upload.char_unescaped?/1)] |> Path.join()
+
+    {:ok, upload |> Map.put(:url, url)}
   end
 
   def upload_path do
-    Pleroma.Config.get!([__MODULE__, :uploads])
+    Config.get!([__MODULE__, :uploads])
   end
 
   @impl true
@@ -46,4 +49,9 @@ defmodule Pleroma.Uploaders.Local do
       {:error, posix_error} -> {:error, to_string(posix_error)}
     end
   end
+
+  @impl true
+  def base_url() do
+    Config.get([Pleroma.Upload, :base_url]) || Pleroma.Web.Endpoint.url() <> "/media/"
+  end
 end

lib/pleroma/uploaders/s3.ex

@@ -47,7 +47,8 @@ defmodule Pleroma.Uploaders.S3 do
 
     case ExAws.request(op) do
       {:ok, _} ->
-        {:ok, {:file, s3_name}}
+        {:ok,
+         upload |> Map.put(:path, s3_name) |> Map.put(:url, Path.join([base_url(), s3_name]))}
 
       error ->
         Logger.error("#{__MODULE__}: #{inspect(error)}")
@@ -67,6 +68,33 @@ defmodule Pleroma.Uploaders.S3 do
     end
   end
 
+  @impl true
+  def base_url() do
+    upload_base_url = Config.get([Pleroma.Upload, :base_url])
+    public_endpoint = Config.get([Pleroma.Uploaders.S3, :public_endpoint])
+    bucket = Config.get([Pleroma.Uploaders.S3, :bucket])
+    truncated_namespace = Config.get([Pleroma.Uploaders.S3, :truncated_namespace])
+    namespace = Config.get([Pleroma.Uploaders.S3, :bucket_namespace])
+
+    bucket_with_namespace =
+      cond do
+        !is_nil(truncated_namespace) ->
+          truncated_namespace
+
+        !is_nil(namespace) ->
+          namespace <> ":" <> bucket
+
+        true ->
+          bucket
+      end
+
+    if public_endpoint do
+      Path.join([public_endpoint, bucket_with_namespace])
+    else
+      Path.join([upload_base_url, bucket_with_namespace])
+    end
+  end
+
   @regex Regex.compile!("[^0-9a-zA-Z!.*/'()_-]")
   def strict_encode(name) do
     String.replace(name, @regex, "-")

lib/pleroma/uploaders/uploader.ex

@@ -34,24 +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.t()} | {:error, String.t()} | :wait_callback
 
   @callback delete_file(file :: String.t()) :: :ok | {:error, String.t()}
 
-  @callback http_callback(Plug.Conn.t(), Map.t()) ::
-              {:ok, Plug.Conn.t()}
-              | {:ok, Plug.Conn.t(), file_spec()}
-              | {:error, Plug.Conn.t(), String.t()}
-  @optional_callbacks http_callback: 2
+  @callback base_url() :: String.t()
 
-  @spec put_file(module(), upload :: struct()) :: {:ok, file_spec()} | {:error, String.t()}
+  @callback http_callback(Plug.Conn.t(), Map.t(), Pleroma.Upload.t()) ::
+              {:ok, Plug.Conn.t(), Pleroma.Upload.t()}
+              | {:error, Plug.Conn.t(), String.t()}
+  @optional_callbacks http_callback: 3
+
+  @spec put_file(module(), upload :: struct()) :: {:ok, Pleroma.Upload.t()} | {: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
@@ -61,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})

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,10 +37,17 @@ 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.merge(upload, %{
+             path: "post-process-file.jpg",
+             url: "http://localhost:4001/media/post-process-file.jpg"
+           })}
 
       def put_file(upload), do: TestUploaderBase.put_file(upload, __MODULE__)
+
+      def base_url(), do: "http://localhost:4001/media/"
     end
 
     setup do: [uploader: TestUploaderSuccess]
@@ -72,7 +79,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

test/pleroma/uploaders/local_test.exs

@@ -21,10 +21,11 @@ defmodule Pleroma.Uploaders.LocalTest do
         name: "image.jpg",
         content_type: "image/jpeg",
         path: file_path,
-        tempfile: Path.absname("test/fixtures/image_tmp.jpg")
+        tempfile: Path.absname("test/fixtures/image_tmp.jpg"),
+        url: "http://localhost:4001/media/local_upload/files/image.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 +44,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)

test/pleroma/uploaders/s3_test.exs

@@ -59,7 +59,8 @@ defmodule Pleroma.Uploaders.S3Test do
         name: "image-tet.jpg",
         content_type: "image/jpeg",
         path: "test_folder/image-tet.jpg",
-        tempfile: Path.absname("test/instance_static/add/shortcode.png")
+        tempfile: Path.absname("test/instance_static/add/shortcode.png"),
+        url: "https://s3.amazonaws.com/test_bucket/test_folder/image-tet.jpg"
       }
 
       [file_upload: file_upload]
@@ -67,7 +68,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