Changed filesystems config manipulation to use ids (#1017)

* Changed filesystems mainpulation to use ids

* Adjusted to review

* Added filesystem ordering

* Apply suggestions from code review

* Format

Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>

lib/livebook/settings.ex

@@ -5,6 +5,11 @@ defmodule Livebook.Settings do
 
   alias Livebook.FileSystem
 
+  @typedoc """
+  An id that is used for filesystem's manipulation, either insertion or removal.
+  """
+  @type file_system_id :: :local | String.t()
+
   @doc """
   Returns the autosave path.
 
@@ -19,38 +24,49 @@ defmodule Livebook.Settings do
   end
 
   @doc """
-  Returns all known filesystems.
+  Returns all known filesystems with their associated ids.
+
+  In case of the local filesystem the id resolves to `:local` atom.
   """
-  @spec file_systems() :: list(FileSystem.t())
+  @spec file_systems() :: [{file_system_id(), Filesystem.t()}]
   def file_systems() do
-    [Livebook.Config.local_filesystem() | Enum.map(storage().all(:filesystem), &storage_to_fs/1)]
+    restored_file_systems =
+      storage().all(:filesystem)
+      |> Enum.sort_by(&Map.get(&1, :order, System.monotonic_time()))
+      |> Enum.map(fn %{id: fs_id} = raw_fs ->
+        {fs_id, storage_to_fs(raw_fs)}
+      end)
+
+    [{:local, Livebook.Config.local_filesystem()} | restored_file_systems]
   end
 
   @doc """
-  Appends a new file system to the configured ones.
-
-  TODO: Refactor to receive settings submission parameters.
+  Saves a new file system to the configured ones.
   """
-  @spec append_file_system(FileSystem.t()) :: :ok
-  def append_file_system(%FileSystem.S3{} = file_system) do
+  @spec save_filesystem(FileSystem.t()) :: file_system_id()
+  def save_filesystem(%FileSystem.S3{} = file_system) do
     attributes =
       file_system
       |> FileSystem.S3.to_config()
       |> Map.to_list()
 
-    storage().insert(:filesystem, generate_filesystem_id(), [{:type, "s3"} | attributes])
+    id = Livebook.Utils.random_short_id()
+
+    :ok =
+      storage().insert(:filesystem, id, [
+        {:type, "s3"},
+        {:order, System.monotonic_time()} | attributes
+      ])
+
+    id
   end
 
   @doc """
   Removes the given file system from the configured ones.
-
-  TODO: Refactor to receive the filesystem id.
   """
-  @spec remove_file_system(FileSystem.t()) :: :ok
-  def remove_file_system(file_system) do
-    storage().all(:filesystem)
-    |> Enum.find(&(storage_to_fs(&1) == file_system))
-    |> then(fn %{id: id} -> storage().delete(:filesystem, id) end)
+  @spec remove_file_system(file_system_id()) :: :ok
+  def remove_file_system(filesystem_id) do
+    storage().delete(:filesystem, filesystem_id)
   end
 
   defp storage() do
@@ -63,8 +79,4 @@ defmodule Livebook.Settings do
       {:error, message} -> raise ArgumentError, "invalid S3 filesystem: #{message}"
     end
   end
-
-  defp generate_filesystem_id() do
-    :crypto.strong_rand_bytes(6) |> Base.url_encode64()
-  end
 end

lib/livebook_web/live/file_select_component.ex

@@ -200,7 +200,7 @@ defmodule LivebookWeb.FileSelectComponent do
         </button>
       </:toggle>
       <:content>
-        <%= for {file_system, index} <- @file_systems |> Enum.with_index() do %>
+        <%= for {file_system_id, file_system} <- @file_systems do %>
           <%= if file_system == @file.file_system do %>
             <button class="menu-item text-gray-900" role="menuitem">
               <.file_system_icon file_system={file_system} />
@@ -211,7 +211,7 @@ defmodule LivebookWeb.FileSelectComponent do
               role="menuitem"
               phx-target={@myself}
               phx-click="set_file_system"
-              phx-value-index={index}>
+              phx-value-id={file_system_id}>
               <.file_system_icon file_system={file_system} />
               <span class="font-medium"><%= file_system_label(file_system) %></span>
             </button>
@@ -331,9 +331,12 @@ defmodule LivebookWeb.FileSelectComponent do
   end
 
   @impl true
-  def handle_event("set_file_system", %{"index" => index}, socket) do
-    index = String.to_integer(index)
-    file_system = Enum.at(socket.assigns.file_systems, index)
+  def handle_event("set_file_system", %{"id" => file_system_id}, socket) do
+    {^file_system_id, file_system} =
+      Enum.find(socket.assigns.file_systems, fn {id, _file_system} ->
+        id == file_system_id
+      end)
+
     file = FileSystem.File.new(file_system)
 
     send(self(), {:set_file, file, %{exists: true}})
@@ -342,7 +345,10 @@ defmodule LivebookWeb.FileSelectComponent do
   end
 
   def handle_event("set_path", %{"path" => path}, socket) do
-    file = FileSystem.File.new(socket.assigns.file.file_system) |> FileSystem.File.resolve(path)
+    file =
+      socket.assigns.file.file_system
+      |> FileSystem.File.new()
+      |> FileSystem.File.resolve(path)
 
     info =
       socket.assigns.file_infos

lib/livebook_web/live/settings_live.ex

@@ -134,17 +134,16 @@ defmodule LivebookWeb.SettingsLive do
         <.live_component module={LivebookWeb.SettingsLive.RemoveFileSystemComponent}
           id="detach-file-system"
           return_to={Routes.settings_path(@socket, :page)}
-          file_system={@file_system} />
+          file_system_id={@file_system_id}
+        />
       </.modal>
     <% end %>
     """
   end
 
   @impl true
-  def handle_params(%{"file_system_index" => index}, _url, socket) do
-    index = String.to_integer(index)
-    file_system = Enum.at(socket.assigns.file_systems, index)
-    {:noreply, assign(socket, :file_system, file_system)}
+  def handle_params(%{"file_system_id" => file_system_id}, _url, socket) do
+    {:noreply, assign(socket, file_system_id: file_system_id)}
   end
 
   def handle_params(_params, _url, socket), do: {:noreply, socket}

lib/livebook_web/live/settings_live/add_file_system_component.ex

@@ -82,7 +82,7 @@ defmodule LivebookWeb.SettingsLive.AddFileSystemComponent do
 
     case FileSystem.File.list(default_dir) do
       {:ok, _} ->
-        Livebook.Settings.append_file_system(file_system)
+        Livebook.Settings.save_filesystem(file_system)
         send(self(), {:file_systems_updated, Livebook.Settings.file_systems()})
         {:noreply, push_patch(socket, to: socket.assigns.return_to)}
 

lib/livebook_web/live/settings_live/file_systems_component.ex

@@ -8,14 +8,14 @@ defmodule LivebookWeb.SettingsLive.FileSystemsComponent do
     ~H"""
     <div class="flex flex-col space-y-4">
       <div class="flex flex-col space-y-4">
-        <%= for {file_system, index} <- Enum.with_index(@file_systems) do %>
+        <%= for {file_system_id, file_system} <- @file_systems do %>
           <div class="flex items-center justify-between border border-gray-200 rounded-lg p-4">
             <div class="flex items-center space-x-12">
               <.file_system_info file_system={file_system} />
             </div>
             <%= unless is_struct(file_system, FileSystem.Local) do %>
               <%= live_patch "Detach",
-                    to: Routes.settings_path(@socket, :detach_file_system, index),
+                    to: Routes.settings_path(@socket, :detach_file_system, file_system_id),
                     class: "button-base button-outlined-red" %>
             <% end %>
           </div>

lib/livebook_web/live/settings_live/remove_file_system_component.ex

@@ -26,7 +26,7 @@ defmodule LivebookWeb.SettingsLive.RemoveFileSystemComponent do
 
   @impl true
   def handle_event("detach", %{}, socket) do
-    Livebook.Settings.remove_file_system(socket.assigns.file_system)
+    Livebook.Settings.remove_file_system(socket.assigns.file_system_id)
     send(self(), {:file_systems_updated, Livebook.Settings.file_systems()})
     {:noreply, push_patch(socket, to: socket.assigns.return_to)}
   end

lib/livebook_web/router.ex

@@ -52,7 +52,7 @@ defmodule LivebookWeb.Router do
       live "/settings", SettingsLive, :page
       live "/settings/user-profile", SettingsLive, :user
       live "/settings/add-file-system", SettingsLive, :add_file_system
-      live "/settings/detach-file-system/:file_system_index", SettingsLive, :detach_file_system
+      live "/settings/detach-file-system/:file_system_id", SettingsLive, :detach_file_system
 
       live "/explore", ExploreLive, :page
       live "/explore/user-profile", ExploreLive, :user