Delete files when updating parent identities

Closes #127

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

lib/mobilizon/actors/actor.ex

@@ -70,8 +70,8 @@ defmodule Mobilizon.Actors.Actor do
     many_to_many(:memberships, Actor, join_through: Member)
     belongs_to(:user, User)
     has_many(:feed_tokens, FeedToken, foreign_key: :actor_id)
-    embeds_one(:avatar, File)
+    embeds_one(:avatar, File, on_replace: :update)
-    embeds_one(:banner, File)
+    embeds_one(:banner, File, on_replace: :update)
 
     timestamps()
   end

lib/mobilizon/actors/actors.ex

@@ -9,6 +9,8 @@ defmodule Mobilizon.Actors do
   alias Mobilizon.Repo
 
   alias Mobilizon.Actors.{Actor, Bot, Member, Follower}
+  alias Mobilizon.Media.File
+  alias Ecto.Multi
 
   alias Mobilizon.Service.ActivityPub
   require Logger
@@ -119,9 +121,24 @@ defmodule Mobilizon.Actors do
   def update_actor(%Actor{} = actor, attrs) do
     actor
     |> Actor.changeset(attrs)
+    |> delete_files_if_media_changed()
     |> Repo.update()
   end
 
+  defp delete_files_if_media_changed(%Ecto.Changeset{changes: changes} = changeset) do
+    Enum.each([:avatar, :banner], fn key ->
+      if Map.has_key?(changes, key) do
+        with %Ecto.Changeset{changes: %{url: new_url}} <- changes[key],
+             old_url <- Map.from_struct(changeset.data) |> Map.get(key) |> Map.get(:url),
+             false <- new_url == old_url do
+          MobilizonWeb.Upload.remove(old_url)
+        end
+      end
+    end)
+
+    changeset
+  end
+
   @doc """
   Deletes a Actor.
 
@@ -135,6 +152,26 @@ defmodule Mobilizon.Actors do
 
   """
   @spec delete_actor(Actor.t()) :: {:ok, Actor.t()} | {:error, Ecto.Changeset.t()}
+  def delete_actor(%Actor{domain: nil} = actor) do
+    case Multi.new()
+         |> Multi.delete(:actor, actor)
+         |> Multi.run(:remove_banner, fn _repo,
+                                         %{actor: %Actor{banner: %File{url: url}}} = _picture ->
+           MobilizonWeb.Upload.remove(url)
+         end)
+         |> Multi.run(:remove_avatar, fn _repo,
+                                         %{actor: %Actor{avatar: %File{url: url}}} = _picture ->
+           MobilizonWeb.Upload.remove(url)
+         end)
+         |> Repo.transaction() do
+      {:ok, %{actor: %Actor{} = actor}} ->
+        {:ok, actor}
+
+      {:error, remove, error, _} when remove in [:remove_banner, :remove_avatar] ->
+        {:error, error}
+    end
+  end
+
   def delete_actor(%Actor{} = actor) do
     Repo.delete(actor)
   end

lib/mobilizon/media.ex

@@ -7,6 +7,8 @@ defmodule Mobilizon.Media do
   alias Mobilizon.Repo
 
   alias Mobilizon.Media.Picture
+  alias Mobilizon.Media.File
+  alias Ecto.Multi
 
   @doc false
   def data() do
@@ -97,7 +99,15 @@ defmodule Mobilizon.Media do
 
   """
   def delete_picture(%Picture{} = picture) do
-    Repo.delete(picture)
+    case Multi.new()
+         |> Multi.delete(:picture, picture)
+         |> Multi.run(:remove, fn _repo, %{picture: %Picture{file: %File{url: url}}} = _picture ->
+           MobilizonWeb.Upload.remove(url)
+         end)
+         |> Repo.transaction() do
+      {:ok, %{picture: %Picture{} = picture}} -> {:ok, picture}
+      {:error, :remove, error, _} -> {:error, error}
+    end
   end
 
   @doc """

lib/mobilizon_web/upload.ex

@@ -88,6 +88,16 @@ defmodule MobilizonWeb.Upload do
     end
   end
 
+  def remove(url, opts \\ []) do
+    with opts <- get_opts(opts),
+         %URI{path: "/media/" <> path, host: host} <- URI.parse(url),
+         true <- host == MobilizonWeb.Endpoint.host() do
+      MobilizonWeb.Uploaders.Uploader.remove_file(opts.uploader, path)
+    else
+      %URI{} = _uri -> {:error, "URL doesn't match pattern"}
+    end
+  end
+
   def char_unescaped?(char) do
     URI.char_unreserved?(char) or char == ?/
   end

lib/mobilizon_web/uploaders/local.ex

@@ -14,18 +14,8 @@ defmodule MobilizonWeb.Uploaders.Local do
   end
 
   def put_file(upload) do
-    {local_path, file} =
+    {path, file} = local_path(upload.path)
-      case Enum.reverse(String.split(upload.path, "/", trim: true)) do
+    result_file = Path.join(path, file)
-        [file] ->
-          {upload_path(), file}
-
-        [file | folders] ->
-          path = Path.join([upload_path()] ++ Enum.reverse(folders))
-          File.mkdir_p!(path)
-          {path, file}
-      end
-
-    result_file = Path.join(local_path, file)
 
     unless File.exists?(result_file) do
       File.cp!(upload.tempfile, result_file)
@@ -34,6 +24,40 @@ defmodule MobilizonWeb.Uploaders.Local do
     :ok
   end
 
+  def remove_file(path) do
+    with {path, file} <- local_path(path),
+         full_path <- Path.join(path, file),
+         true <- File.exists?(full_path),
+         :ok <- File.rm(full_path),
+         :ok <- remove_folder(path) do
+      {:ok, path}
+    else
+      false -> {:error, "File #{path} doesn't exist"}
+    end
+  end
+
+  defp remove_folder(path) do
+    with {:subfolder, true} <- {:subfolder, path != upload_path()},
+         {:empty_folder, {:ok, [] = _files}} <- {:empty_folder, File.ls(path)} do
+      File.rmdir(path)
+    else
+      {:subfolder, _} -> :ok
+      {:empty_folder, _} -> {:error, "Error: Folder is not empty"}
+    end
+  end
+
+  defp local_path(path) do
+    case Enum.reverse(String.split(path, "/", trim: true)) do
+      [file] ->
+        {upload_path(), file}
+
+      [file | folders] ->
+        path = Path.join([upload_path()] ++ Enum.reverse(folders))
+        File.mkdir_p!(path)
+        {path, file}
+    end
+  end
+
   def upload_path do
     Mobilizon.CommonConfig.get!([__MODULE__, :uploads])
   end

lib/mobilizon_web/uploaders/uploader.ex

@@ -35,6 +35,8 @@ defmodule MobilizonWeb.Uploaders.Uploader do
   @callback put_file(MobilizonWeb.Upload.t()) ::
               :ok | {:ok, file_spec()} | {:error, String.t()} | :wait_callback
 
+  @callback remove_file(file_spec()) :: :ok | {:ok, file_spec()} | {:error, String.t()}
+
   @callback http_callback(Plug.Conn.t(), Map.t()) ::
               {:ok, Plug.Conn.t()}
               | {:ok, Plug.Conn.t(), file_spec()}
@@ -42,7 +44,6 @@ defmodule MobilizonWeb.Uploaders.Uploader do
   @optional_callbacks http_callback: 2
 
   @spec put_file(module(), MobilizonWeb.Upload.t()) :: {:ok, file_spec()} | {:error, String.t()}
-
   def put_file(uploader, upload) do
     case uploader.put_file(upload) do
       :ok -> {:ok, {:file, upload.path}}
@@ -52,6 +53,10 @@ defmodule MobilizonWeb.Uploaders.Uploader do
     end
   end
 
+  def remove_file(uploader, path) do
+    uploader.remove_file(path)
+  end
+
   defp handle_callback(uploader, upload) do
     :global.register_name({__MODULE__, upload.path}, self())
 

test/mobilizon/actors/actors_test.exs

@@ -4,7 +4,7 @@ defmodule Mobilizon.ActorsTest do
   alias Mobilizon.Actors
   alias Mobilizon.Actors.{Actor, Member, Follower, Bot}
   alias Mobilizon.Users
-  alias Mobilizon.Media.File
+  alias Mobilizon.Media.File, as: FileModel
   import Mobilizon.Factory
   use ExVCR.Mock, adapter: ExVCR.Adapter.Hackney
 
@@ -97,14 +97,14 @@ defmodule Mobilizon.ActorsTest do
            id: actor_id,
            preferred_username: preferred_username,
            domain: domain,
-           avatar: %File{name: picture_name} = _picture
+           avatar: %FileModel{name: picture_name} = _picture
          } = _actor} = Actors.get_or_fetch_by_url(@remote_account_url)
 
         assert picture_name == "avatar"
 
         %Actor{
           id: actor_found_id,
-          avatar: %File{name: picture_name} = _picture
+          avatar: %FileModel{name: picture_name} = _picture
         } = Actors.get_actor_by_name("#{preferred_username}@#{domain}")
 
         assert actor_found_id == actor_id
@@ -235,7 +235,8 @@ defmodule Mobilizon.ActorsTest do
 
     test "test get_public_key_for_url/1 with remote actor and bad key" do
       use_cassette "actors/remote_actor_mastodon_tcit_actor_deleted" do
-        assert Actor.get_public_key_for_url(@remote_account_url) == {:error, :actor_fetch_error}
+        assert Actor.get_public_key_for_url(@remote_account_url) ==
+                 {:error, :actor_fetch_error}
       end
     end
 
@@ -257,8 +258,15 @@ defmodule Mobilizon.ActorsTest do
       assert {:error, %Ecto.Changeset{}} = Actors.create_actor(@invalid_attrs)
     end
 
-    test "update_actor/2 with valid data updates the actor", %{actor: actor} do
+    test "update_actor/2 with valid data updates the actor", %{
-      assert {:ok, actor} = Actors.update_actor(actor, @update_attrs)
+      actor: %Actor{} = actor
+    } do
+      assert {:ok, actor} =
+               Actors.update_actor(
+                 actor,
+                 @update_attrs
+               )
+
       assert %Actor{} = actor
       assert actor.summary == "some updated description"
       assert actor.name == "some updated name"
@@ -268,15 +276,85 @@ defmodule Mobilizon.ActorsTest do
       assert actor.preferred_username == "some updated username"
     end
 
+    test "update_actor/2 with valid data updates the actor and it's media files", %{
+      actor: %Actor{avatar: %{url: avatar_url}, banner: %{url: banner_url}} = actor
+    } do
+      %URI{path: "/media/" <> avatar_path} = URI.parse(avatar_url)
+      %URI{path: "/media/" <> banner_path} = URI.parse(banner_url)
+
+      assert File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> avatar_path
+             )
+
+      assert File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> banner_path
+             )
+
+      file = %Plug.Upload{
+        content_type: "image/jpg",
+        path: Path.absname("test/fixtures/image.jpg"),
+        filename: "image.jpg"
+      }
+
+      {:ok, data} = MobilizonWeb.Upload.store(file)
+      url = hd(data["url"])["href"]
+
+      assert {:ok, actor} =
+               Actors.update_actor(
+                 actor,
+                 Map.put(@update_attrs, :avatar, %{name: file.filename, url: url})
+               )
+
+      assert %Actor{} = actor
+      assert actor.summary == "some updated description"
+      assert actor.name == "some updated name"
+      assert actor.domain == "some updated domain"
+      assert actor.keys == "some updated keys"
+      refute actor.suspended
+      assert actor.preferred_username == "some updated username"
+
+      refute File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> avatar_path
+             )
+
+      assert File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> banner_path
+             )
+    end
+
     test "update_actor/2 with invalid data returns error changeset", %{actor: actor} do
       assert {:error, %Ecto.Changeset{}} = Actors.update_actor(actor, @invalid_attrs)
       actor_fetched = Actors.get_actor!(actor.id)
       assert actor = actor_fetched
     end
 
-    test "delete_actor/1 deletes the actor", %{actor: actor} do
+    test "delete_actor/1 deletes the actor", %{
+      actor: %Actor{avatar: %{url: avatar_url}, banner: %{url: banner_url}, id: actor_id} = actor
+    } do
+      %URI{path: "/media/" <> avatar_path} = URI.parse(avatar_url)
+      %URI{path: "/media/" <> banner_path} = URI.parse(banner_url)
+
+      assert File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> avatar_path
+             )
+
+      assert File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> banner_path
+             )
+
       assert {:ok, %Actor{}} = Actors.delete_actor(actor)
-      assert_raise Ecto.NoResultsError, fn -> Actors.get_actor!(actor.id) end
+      assert_raise Ecto.NoResultsError, fn -> Actors.get_actor!(actor_id) end
+
+      refute File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> banner_path
+             )
     end
 
     test "change_actor/1 returns a actor changeset", %{actor: actor} do

test/mobilizon/media/media_test.exs

@@ -5,6 +5,7 @@ defmodule Mobilizon.MediaTest do
   import Mobilizon.Factory
 
   describe "media" do
+    setup [:ensure_local_uploader]
     alias Mobilizon.Media.Picture
 
     @valid_attrs %{
@@ -43,8 +44,21 @@ defmodule Mobilizon.MediaTest do
 
     test "delete_picture/1 deletes the picture" do
       picture = insert(:picture)
+
+      %URI{path: "/media/" <> path} = URI.parse(picture.file.url)
+
+      assert File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> path
+             )
+
       assert {:ok, %Picture{}} = Media.delete_picture(picture)
       assert_raise Ecto.NoResultsError, fn -> Media.get_picture!(picture.id) end
+
+      refute File.exists?(
+               Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+                 "/" <> path
+             )
     end
 
     test "change_picture/1 returns a picture changeset" do

test/mobilizon_web/upload_test.exs

@@ -173,5 +173,48 @@ defmodule Mobilizon.UploadTest do
       assert Path.basename(attachment_url["href"]) ==
                "%3A%3F%23%5B%5D%40%21%24%26%5C%27%28%29%2A%2B%2C%3B%3D.jpg"
     end
+
+    test "upload and delete successfully a file" do
+      {path, url} = upload()
+      assert File.exists?(path)
+      assert {:ok, _} = Upload.remove(url)
+      refute File.exists?(path)
+      path = path |> Path.split() |> Enum.reverse() |> tl |> Enum.reverse() |> Path.join()
+      refute File.exists?(path)
+    end
+
+    test "delete a not existing file" do
+      file =
+        Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <>
+          "/not_existing/definitely.jpg"
+
+      refute File.exists?(file)
+
+      assert {:error, "File not_existing/definitely.jpg doesn't exist"} =
+               Upload.remove("https://mobilizon.test/media/not_existing/definitely.jpg")
+    end
+  end
+
+  defp upload() do
+    File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg")
+
+    file = %Plug.Upload{
+      content_type: "image/jpg",
+      path: Path.absname("test/fixtures/image_tmp.jpg"),
+      filename: "image.jpg"
+    }
+
+    {:ok, data} = Upload.store(file)
+
+    assert %{
+             "url" => [%{"href" => url, "mediaType" => "image/jpeg"}],
+             "size" => 13_227,
+             "type" => "Image"
+           } = data
+
+    assert String.starts_with?(url, MobilizonWeb.Endpoint.url() <> "/media/")
+
+    %URI{path: "/media/" <> path} = URI.parse(url)
+    {Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> "/" <> path, url}
   end
 end

test/support/factory.ex

@@ -7,6 +7,7 @@ defmodule Mobilizon.Factory do
   alias Mobilizon.Actors.Actor
   alias MobilizonWeb.Router.Helpers, as: Routes
   alias MobilizonWeb.Endpoint
+  alias MobilizonWeb.Upload
 
   def user_factory do
     %Mobilizon.Users.User{
@@ -171,9 +172,25 @@ defmodule Mobilizon.Factory do
   end
 
   def file_factory do
+    File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg")
+
+    file = %Plug.Upload{
+      content_type: "image/jpg",
+      path: Path.absname("test/fixtures/image_tmp.jpg"),
+      filename: "image.jpg"
+    }
+
+    {:ok, data} = Upload.store(file)
+
+    %{
+      "url" => [%{"href" => url, "mediaType" => "image/jpeg"}],
+      "size" => 13_227,
+      "type" => "Image"
+    } = data
+
     %Mobilizon.Media.File{
       name: "My Picture",
-      url: MobilizonWeb.Endpoint.url() <> "/uploads/something",
+      url: url,
       content_type: "image/png",
       size: 13_120
     }