Various refactoring and typespec improvements

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

lib/web/upload/mime.ex

@@ -11,12 +11,22 @@ defmodule Mobilizon.Web.Upload.MIME do
   @read_bytes 35
 
   @spec file_mime_type(String.t(), String.t()) ::
-          {:ok, content_type :: String.t(), filename :: String.t()} | {:error, any()} | :error
-  @spec file_mime_type(String.t()) :: {:ok, String.t()} | {:error, any()} | :error
+          {:ok, content_type :: String.t(), filename :: String.t()}
+          | {:error, :unknown_mime | :failed_to_fix_extension}
+  @spec file_mime_type(String.t()) :: {:ok, String.t()} | {:error, :unknown_mime}
   def file_mime_type(path, filename) do
-    with {:ok, content_type} <- file_mime_type(path),
-         filename when is_binary(filename) <- fix_extension(filename, content_type) do
-      {:ok, content_type, filename}
+    case file_mime_type(path) do
+      {:ok, content_type} ->
+        case fix_extension(filename, content_type) do
+          {:error, :failed_to_fix_extension} ->
+            {:error, :failed_to_fix_extension}
+
+          filename when is_binary(filename) ->
+            {:ok, content_type, filename}
+        end
+
+      {:error, :unknown_mime} ->
+        {:error, :unknown_mime}
     end
   end
 
@@ -33,12 +43,12 @@ defmodule Mobilizon.Web.Upload.MIME do
     end
   end
 
-  @spec bin_mime_type(binary()) :: {:ok, String.t()} | :error
+  @spec bin_mime_type(binary()) :: {:ok, String.t()} | {:error, :unknown_mime}
   def bin_mime_type(<<head::binary-size(@read_bytes), _::binary>>) do
     {:ok, check_mime_type(head)}
   end
 
-  def bin_mime_type(_), do: :error
+  def bin_mime_type(_), do: {:error, :unknown_mime}
 
   def mime_type(<<_::binary>>), do: {:ok, @default}
 
@@ -67,8 +77,9 @@ defmodule Mobilizon.Web.Upload.MIME do
     end
   end
 
-  defp fix_extension(_, _), do: :error
+  defp fix_extension(_, _), do: {:error, :failed_to_fix_extension}
 
+  @spec check_mime_type(binary) :: String.t()
   defp check_mime_type(<<0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, _::binary>>) do
     "image/png"
   end

lib/web/upload/upload.ex

@@ -79,7 +79,7 @@ defmodule Mobilizon.Web.Upload do
          }
 
   @spec store(source, options :: [option()]) ::
-          {:ok, map()} | {:error, String.t()} | {:error, atom()}
+          {:ok, t()} | {:error, String.t()} | {:error, atom()}
   def store(upload, opts \\ []) do
     opts = get_opts(opts)
 
@@ -90,7 +90,7 @@ defmodule Mobilizon.Web.Upload do
         |> perform_filter_and_put_file(opts)
 
       {:error, error} ->
-        error
+        {:error, error}
     end
   end
 
@@ -169,28 +169,38 @@ defmodule Mobilizon.Web.Upload do
           {Config.get!([:instance, :upload_limit]), nil}
       end
 
+    activity_type = Keyword.get(opts, :activity_type, activity_type)
+    size_limit = Keyword.get(opts, :size_limit, size_limit)
+    uploader = Keyword.get(opts, :uploader, Config.get([__MODULE__, :uploader]))
+    filters = Keyword.get(opts, :filters, Config.get([__MODULE__, :filters]))
+    description = Keyword.get(opts, :description)
+
+    allow_list_mime_types =
+      Keyword.get(
+        opts,
+        :allow_list_mime_types,
+        Config.get([__MODULE__, :allow_list_mime_types])
+      )
+
+    base_url =
+      Keyword.get(
+        opts,
+        :base_url,
+        Config.get([__MODULE__, :base_url], Endpoint.url())
+      )
+
     %{
-      activity_type: Keyword.get(opts, :activity_type, activity_type),
-      size_limit: Keyword.get(opts, :size_limit, size_limit),
-      uploader: Keyword.get(opts, :uploader, Config.get([__MODULE__, :uploader])),
-      filters: Keyword.get(opts, :filters, Config.get([__MODULE__, :filters])),
-      description: Keyword.get(opts, :description),
-      allow_list_mime_types:
-        Keyword.get(
-          opts,
-          :allow_list_mime_types,
-          Config.get([__MODULE__, :allow_list_mime_types])
-        ),
-      base_url:
-        Keyword.get(
-          opts,
-          :base_url,
-          Config.get([__MODULE__, :base_url], Endpoint.url())
-        )
+      activity_type: activity_type,
+      size_limit: size_limit,
+      uploader: uploader,
+      filters: filters,
+      description: description,
+      allow_list_mime_types: allow_list_mime_types,
+      base_url: base_url
     }
   end
 
-  @spec prepare_upload(t(), internal_options()) :: {:ok, t()}
+  @spec prepare_upload(t(), internal_options()) :: {:ok, t()} | {:error, atom()}
   defp prepare_upload(%Plug.Upload{} = file, opts) do
     with {:ok, size} <- check_file_size(file.path, opts.size_limit),
          {:ok, content_type, name} <- MIME.file_mime_type(file.path, file.filename),
@@ -203,10 +213,14 @@ defmodule Mobilizon.Web.Upload do
          content_type: content_type,
          size: size
        }}
+    else
+      {:error, err} ->
+        {:error, err}
     end
   end
 
-  @spec prepare_upload(%{body: String.t(), name: String.t()}, internal_options()) :: {:ok, t()}
+  @spec prepare_upload(%{body: String.t(), name: String.t()}, internal_options()) ::
+          {:ok, t()} | {:error, :mime_type_not_allowed}
   defp prepare_upload(%{body: body, name: name} = _file, opts) do
     with :ok <- check_binary_size(body, opts.size_limit),
          tmp_path <- tempfile_for_image(body),
@@ -270,7 +284,7 @@ defmodule Mobilizon.Web.Upload do
 
   defp url_from_spec(_upload, _base_url, {:url, url}), do: url
 
-  @spec check_allowed_mime_type(String.t(), List.t()) :: :ok | {:error, :atom}
+  @spec check_allowed_mime_type(String.t(), List.t()) :: :ok | {:error, :mime_type_not_allowed}
   defp check_allowed_mime_type(content_type, allow_list_mime_types) do
     if Enum.any?(allow_list_mime_types, &(&1 == content_type)),
       do: :ok,

lib/web/upload/uploader/local.ex

@@ -21,8 +21,8 @@ defmodule Mobilizon.Web.Upload.Uploader.Local do
   @impl true
   @spec put_file(Upload.t()) ::
           :ok | {:ok, {:file, String.t()}} | {:error, :tempfile_no_longer_exists}
-  def put_file(%Upload{path: path, tempfile: tempfile}) do
-    {path, file} = local_path(path)
+  def put_file(%Upload{path: initial_path, tempfile: tempfile}) do
+    {path, file} = local_path(initial_path)
     result_file = Path.join(path, file)
 
     if File.exists?(result_file) do
@@ -31,23 +31,11 @@ defmodule Mobilizon.Web.Upload.Uploader.Local do
     else
       if File.exists?(tempfile) do
         File.cp!(tempfile, result_file)
-        {:ok, {:file, result_file}}
+        {:ok, {:file, initial_path}}
       else
         {:error, :tempfile_no_longer_exists}
       end
     end
-
-    with {:result_exists, false} <- {:result_exists, File.exists?(result_file)},
-         {:temp_file_exists, true} <- {:temp_file_exists, File.exists?(tempfile)} do
-      File.cp!(tempfile, result_file)
-    else
-      {:result_exists, _} ->
-        # If the resulting file already exists, it's because of the Dedupe filter
-        :ok
-
-      {:temp_file_exists, _} ->
-        {:error, "Temporary file no longer exists"}
-    end
   end
 
   @impl true