From 038a6fb7cbd671c8396d8df2351d6e21adbe5f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Thu, 17 Aug 2023 21:59:08 +0200 Subject: [PATCH] Split files into references/attachments and support renaming --- assets/css/js_interop.css | 4 + lib/livebook/runtime.ex | 6 + lib/livebook/runtime/attached.ex | 4 + lib/livebook/runtime/elixir_standalone.ex | 4 + lib/livebook/runtime/embedded.ex | 4 + .../runtime/erl_dist/runtime_server.ex | 22 ++ lib/livebook/session.ex | 57 +++- lib/livebook/session/data.ex | 46 +++ lib/livebook_web/live/session_helpers.ex | 24 ++ lib/livebook_web/live/session_live.ex | 24 ++ .../add_file_entry_file_component.ex | 3 +- .../add_file_entry_upload_component.ex | 3 +- .../add_file_entry_url_component.ex | 7 +- .../live/session_live/files_list_component.ex | 314 ++++++++++++------ .../rename_file_entry_component.ex | 88 +++++ .../session_live/secrets_list_component.ex | 5 +- lib/livebook_web/router.ex | 1 + test/livebook/session/data_test.exs | 56 ++++ test/livebook/session_test.exs | 58 +++- test/livebook_web/live/session_live_test.exs | 28 +- test/support/noop_runtime.ex | 2 + 21 files changed, 642 insertions(+), 118 deletions(-) create mode 100644 lib/livebook_web/live/session_live/rename_file_entry_component.ex diff --git a/assets/css/js_interop.css b/assets/css/js_interop.css index 891896175d1..f0494ef339d 100644 --- a/assets/css/js_interop.css +++ b/assets/css/js_interop.css @@ -81,6 +81,10 @@ solely client-side operations. @apply hidden; } +[data-el-session][data-js-dragging="external"] [data-el-files-add-button] { + @apply hidden; +} + [data-el-cell][data-js-focused] { @apply border-blue-300 border-opacity-100; } diff --git a/lib/livebook/runtime.ex b/lib/livebook/runtime.ex index 7f39282d3b6..a3e18d8b36f 100644 --- a/lib/livebook/runtime.ex +++ b/lib/livebook/runtime.ex @@ -553,6 +553,12 @@ defprotocol Livebook.Runtime do @spec transfer_file(t(), String.t(), String.t(), (path :: String.t() | nil -> any())) :: :ok def transfer_file(runtime, path, file_id, callback) + @doc """ + Updates the id by which the file is referenced. + """ + @spec relabel_file(t(), String.t(), String.t()) :: :ok + def relabel_file(runtime, file_id, new_file_id) + @doc """ Cleans up resources allocated with `transfer_file/4`, if any. """ diff --git a/lib/livebook/runtime/attached.ex b/lib/livebook/runtime/attached.ex index b5c9bb69e69..6af742208bf 100644 --- a/lib/livebook/runtime/attached.ex +++ b/lib/livebook/runtime/attached.ex @@ -141,6 +141,10 @@ defimpl Livebook.Runtime, for: Livebook.Runtime.Attached do RuntimeServer.read_file(runtime.server_pid, path) end + def relabel_file(runtime, file_id, new_file_id) do + RuntimeServer.relabel_file(runtime.server_pid, file_id, new_file_id) + end + def transfer_file(runtime, path, file_id, callback) do RuntimeServer.transfer_file(runtime.server_pid, path, file_id, callback) end diff --git a/lib/livebook/runtime/elixir_standalone.ex b/lib/livebook/runtime/elixir_standalone.ex index c2d438cbb13..4bd7dac83be 100644 --- a/lib/livebook/runtime/elixir_standalone.ex +++ b/lib/livebook/runtime/elixir_standalone.ex @@ -141,6 +141,10 @@ defimpl Livebook.Runtime, for: Livebook.Runtime.ElixirStandalone do RuntimeServer.transfer_file(runtime.server_pid, path, file_id, callback) end + def relabel_file(runtime, file_id, new_file_id) do + RuntimeServer.relabel_file(runtime.server_pid, file_id, new_file_id) + end + def revoke_file(runtime, file_id) do RuntimeServer.revoke_file(runtime.server_pid, file_id) end diff --git a/lib/livebook/runtime/embedded.ex b/lib/livebook/runtime/embedded.ex index 28252d49937..8f02c5078f6 100644 --- a/lib/livebook/runtime/embedded.ex +++ b/lib/livebook/runtime/embedded.ex @@ -107,6 +107,10 @@ defimpl Livebook.Runtime, for: Livebook.Runtime.Embedded do RuntimeServer.transfer_file(runtime.server_pid, path, file_id, callback) end + def relabel_file(runtime, file_id, new_file_id) do + RuntimeServer.relabel_file(runtime.server_pid, file_id, new_file_id) + end + def revoke_file(runtime, file_id) do RuntimeServer.revoke_file(runtime.server_pid, file_id) end diff --git a/lib/livebook/runtime/erl_dist/runtime_server.ex b/lib/livebook/runtime/erl_dist/runtime_server.ex index 3cd92b560dc..296923895fc 100644 --- a/lib/livebook/runtime/erl_dist/runtime_server.ex +++ b/lib/livebook/runtime/erl_dist/runtime_server.ex @@ -199,6 +199,20 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do :ok end + @doc """ + Changes the file id set by `transfer_file/4`, if any. + + See `Livebook.Runtime` for more details. + """ + @spec relabel_file(pid(), String.t(), String.t()) :: :ok + def relabel_file(pid, file_id, new_file_id) do + unless same_host?(pid) do + GenServer.cast(pid, {:relabel_file, file_id, new_file_id}) + end + + :ok + end + @doc """ Removes the file created by `transfer_file/4`, if any. @@ -614,6 +628,14 @@ defmodule Livebook.Runtime.ErlDist.RuntimeServer do {:noreply, state} end + def handle_cast({:relabel_file, file_id, new_file_id}, state) do + path = file_path(state, file_id) + new_path = file_path(state, new_file_id) + File.rename(path, new_path) + + {:noreply, state} + end + def handle_cast({:revoke_file, file_id}, state) do target_path = file_path(state, file_id) File.rm(target_path) diff --git a/lib/livebook/session.ex b/lib/livebook/session.ex index fae60e8288c..865f0310405 100644 --- a/lib/livebook/session.ex +++ b/lib/livebook/session.ex @@ -615,6 +615,14 @@ defmodule Livebook.Session do GenServer.cast(pid, {:add_file_entries, self(), file_entries}) end + @doc """ + Sends a file entry rename request to the server. + """ + @spec rename_file_entry(pid(), String.t(), String.t()) :: :ok + def rename_file_entry(pid, name, new_name) do + GenServer.cast(pid, {:rename_file_entry, self(), name, new_name}) + end + @doc """ Sends a file entry deletion request to the server. """ @@ -1380,6 +1388,12 @@ defmodule Livebook.Session do {:noreply, handle_operation(state, operation)} end + def handle_cast({:rename_file_entry, client_pid, name, new_name}, state) do + client_id = client_id(state, client_pid) + operation = {:rename_file_entry, client_id, name, new_name} + {:noreply, handle_operation(state, operation)} + end + def handle_cast({:delete_file_entry, client_pid, name}, state) do client_id = client_id(state, client_pid) operation = {:delete_file_entry, client_id, name} @@ -2136,14 +2150,26 @@ defmodule Livebook.Session do notify_update(state) end - defp after_operation(state, prev_state, {:add_file_entries, _client_id, _file_entries}) do + defp after_operation(state, prev_state, {:add_file_entries, _client_id, file_entries}) do + names = for entry <- file_entries, do: entry.name, into: MapSet.new() + replaced_names = - Enum.map( - prev_state.data.notebook.file_entries -- state.data.notebook.file_entries, - & &1.name - ) + for file_entry <- prev_state.data.notebook.file_entries, + file_entry.name in names, + do: file_entry.name + + cleanup_file_entries(state, replaced_names) + state + end + + defp after_operation(state, prev_state, {:rename_file_entry, _client_id, name, new_name}) do + replaced_names = + for file_entry <- prev_state.data.notebook.file_entries, + file_entry.name == new_name, + do: file_entry.name cleanup_file_entries(state, replaced_names) + remap_file_entry(state, name, new_name) state end @@ -2650,6 +2676,27 @@ defmodule Livebook.Session do end end + defp remap_file_entry(state, name, new_name) do + cache_file = file_entry_cache_file(state.session_id, name) + new_cache_file = file_entry_cache_file(state.session_id, new_name) + FileSystem.File.rename(cache_file, new_cache_file) + + file_entry = Enum.find(state.data.notebook.file_entries, &(&1.name == new_name)) + + if file_entry.type == :attachment do + files_dir = files_dir_from_state(state) + file = FileSystem.File.resolve(files_dir, name) + new_file = FileSystem.File.resolve(files_dir, new_name) + FileSystem.File.rename(file, new_file) + end + + if Runtime.connected?(state.data.runtime) do + file_id = file_entry_file_id(name) + new_file_id = file_entry_file_id(new_name) + Runtime.relabel_file(state.data.runtime, file_id, new_file_id) + end + end + defp before_close(state) do maybe_save_notebook_sync(state) broadcast_message(state.session_id, :session_closed) diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex index 47e9e70f61c..5c91ebaf8b8 100644 --- a/lib/livebook/session/data.ex +++ b/lib/livebook/session/data.ex @@ -221,6 +221,7 @@ defmodule Livebook.Session.Data do | {:set_notebook_hub, client_id(), String.t()} | {:sync_hub_secrets, client_id()} | {:add_file_entries, client_id(), list(Notebook.file_entry())} + | {:rename_file_entry, client_id(), name :: String.t(), new_name :: String.t()} | {:delete_file_entry, client_id(), String.t()} | {:allow_file_entry, client_id(), String.t()} | {:set_app_settings, client_id(), AppSettings.t()} @@ -913,6 +914,19 @@ defmodule Livebook.Session.Data do |> wrap_ok() end + def apply_operation(data, {:rename_file_entry, _client_id, name, new_name}) do + with {:ok, file_entry} <- fetch_file_entry(data.notebook, name), + true <- name != new_name do + data + |> with_actions() + |> rename_file_entry(file_entry, new_name) + |> set_dirty() + |> wrap_ok() + else + _ -> :error + end + end + def apply_operation(data, {:delete_file_entry, _client_id, name}) do with {:ok, file_entry} <- fetch_file_entry(data.notebook, name) do data @@ -1712,6 +1726,38 @@ defmodule Livebook.Session.Data do |> set!(notebook: %{data.notebook | file_entries: file_entries ++ kept_file_entries}) end + defp rename_file_entry({data, _} = data_actions, file_entry, new_name) do + file_entries = + for entry <- data.notebook.file_entries, entry.name != new_name do + if entry == file_entry do + %{entry | name: new_name} + else + entry + end + end + + quarantine_file_entry_names = + MapSet.delete(data.notebook.quarantine_file_entry_names, new_name) + + quarantine_file_entry_names = + if MapSet.member?(quarantine_file_entry_names, file_entry.name) do + quarantine_file_entry_names + |> MapSet.delete(file_entry.name) + |> MapSet.put(new_name) + else + quarantine_file_entry_names + end + + data_actions + |> set!( + notebook: %{ + data.notebook + | file_entries: file_entries, + quarantine_file_entry_names: quarantine_file_entry_names + } + ) + end + defp delete_file_entry({data, _} = data_actions, file_entry) do file_entries = data.notebook.file_entries -- [file_entry] diff --git a/lib/livebook_web/live/session_helpers.ex b/lib/livebook_web/live/session_helpers.ex index 19360472561..1eb9dbc2a91 100644 --- a/lib/livebook_web/live/session_helpers.ex +++ b/lib/livebook_web/live/session_helpers.ex @@ -248,4 +248,28 @@ defmodule LivebookWeb.SessionHelpers do confirm_icon: "close-circle-line" ) end + + @doc """ + Converts the given arbitrary name to a file entry name. + + The returned name is either valid or empty. + """ + @spec sanitize_file_entry_name(String.t()) :: String.t() | nil + def sanitize_file_entry_name(client_name) do + client_name + |> String.replace(~r/[^\s\w-.]/u, "") + |> String.trim() + |> String.replace(~r/\s+/u, "_") + |> case do + "" -> + "" + + name -> + if String.contains?(name, ".") do + name + else + name <> ".bin" + end + end + end end diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index 7202468dcd9..f673ef55e0a 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -491,6 +491,21 @@ defmodule LivebookWeb.SessionLive do <.add_file_entry_content session={@session} file_entries={@data_view.file_entries} tab={@tab} /> + <.modal + :if={@live_action == :rename_file_entry} + id="rename-file-entry-modal" + show + width={:big} + patch={@self_path} + > + <.live_component + module={LivebookWeb.SessionLive.RenameFileEntryComponent} + id="rename-file-entry" + session={@session} + file_entry={@renaming_file_entry} + /> + + <.modal :if={@live_action == :shortcuts} id="shortcuts-modal" @@ -989,6 +1004,15 @@ defmodule LivebookWeb.SessionLive do {:noreply, redirect_to_self(socket)} end + def handle_params(%{"name" => name}, _url, socket) + when socket.assigns.live_action == :rename_file_entry do + if file_entry = find_file_entry(socket, name) do + {:noreply, assign(socket, renaming_file_entry: file_entry)} + else + {:noreply, redirect_to_self(socket)} + end + end + def handle_params(%{"path_parts" => path_parts}, requested_url, socket) when socket.assigns.live_action == :catch_all do path_parts = diff --git a/lib/livebook_web/live/session_live/add_file_entry_file_component.ex b/lib/livebook_web/live/session_live/add_file_entry_file_component.ex index 3afdb831083..51d6a265b4a 100644 --- a/lib/livebook_web/live/session_live/add_file_entry_file_component.ex +++ b/lib/livebook_web/live/session_live/add_file_entry_file_component.ex @@ -30,6 +30,7 @@ defmodule LivebookWeb.SessionLive.AddFileEntryFileComponent do def update(%{event: {:set_file, file, info}}, socket) do file_info = %{exists: info.exists} name = if FileSystem.File.dir?(file), do: "", else: FileSystem.File.name(file) + name = LivebookWeb.SessionHelpers.sanitize_file_entry_name(name) changeset = changeset(Map.put(socket.assigns.changeset.params, "name", name)) {:ok, assign(socket, file: file, file_info: file_info, changeset: changeset)} end @@ -84,7 +85,7 @@ defmodule LivebookWeb.SessionLive.AddFileEntryFileComponent do field={f[:copy]} options={[ {"false", "Store only file location"}, - {"true", "Copy file contents to the notebook files directory"} + {"true", "Save file as an attachment in the notebook files directory"} ]} /> diff --git a/lib/livebook_web/live/session_live/add_file_entry_upload_component.ex b/lib/livebook_web/live/session_live/add_file_entry_upload_component.ex index 3fb2440b568..bb57e01fcd9 100644 --- a/lib/livebook_web/live/session_live/add_file_entry_upload_component.ex +++ b/lib/livebook_web/live/session_live/add_file_entry_upload_component.ex @@ -95,7 +95,8 @@ defmodule LivebookWeb.SessionLive.AddFileEntryUploadComponent do |> JS.dispatch("blur", to: "#add-file-entry-form-name") ) - {%{data | "name" => entry.client_name}, socket} + name = LivebookWeb.SessionHelpers.sanitize_file_entry_name(entry.client_name) + {%{data | "name" => name}, socket} _ -> {data, socket} diff --git a/lib/livebook_web/live/session_live/add_file_entry_url_component.ex b/lib/livebook_web/live/session_live/add_file_entry_url_component.ex index 4e12de7540a..3f1a79672d1 100644 --- a/lib/livebook_web/live/session_live/add_file_entry_url_component.ex +++ b/lib/livebook_web/live/session_live/add_file_entry_url_component.ex @@ -75,7 +75,7 @@ defmodule LivebookWeb.SessionLive.AddFileEntryUrlComponent do field={f[:copy]} options={[ {"false", "Store only URL location"}, - {"true", "Download URL contents to the notebook files directory"} + {"true", "Download file as an attachment to the notebook files directory"} ]} /> @@ -104,7 +104,10 @@ defmodule LivebookWeb.SessionLive.AddFileEntryUrlComponent do end def handle_event("url_blur", %{"value" => url}, socket) do - name = Livebook.Utils.url_basename(url) + name = + url + |> Livebook.Utils.url_basename() + |> LivebookWeb.SessionHelpers.sanitize_file_entry_name() socket = if socket.assigns.changeset.params["name"] == "" and name != "" do diff --git a/lib/livebook_web/live/session_live/files_list_component.ex b/lib/livebook_web/live/session_live/files_list_component.ex index 1fe4baa808b..8009219640d 100644 --- a/lib/livebook_web/live/session_live/files_list_component.ex +++ b/lib/livebook_web/live/session_live/files_list_component.ex @@ -24,19 +24,25 @@ defmodule LivebookWeb.SessionLive.FilesListComponent do end def update(assigns, socket) do - {:ok, assign(socket, assigns)} + socket = assign(socket, assigns) + + {attachment_file_entries, reference_file_entries} = + Enum.split_with(assigns.file_entries, &(&1.type == :attachment)) + + {:ok, + assign(socket, + attachment_file_entries: attachment_file_entries, + reference_file_entries: reference_file_entries + )} end @impl true def render(assigns) do ~H"""
-
-

- Files -

- <.files_info_icon /> -
+

+ Files +

- Add to files + Add file
-
-
- <%= if file_entry.name in @quarantine_file_entry_names do %> - - <% else %> -
- <.remix_icon icon={file_entry_icon(file_entry.type)} class="text-lg align-middle mr-2" /> - <%= file_entry.name %> -
- <% end %> - <%= if file_entry.name in @transferring_file_entry_names do %> - <.spinner class="mr-[3px]" /> - <% else %> - <.menu id={"file-entry-#{idx}-menu"} position={:bottom_right}> - <:toggle> - - - <.menu_item> - - - <.menu_item disabled={not Livebook.Session.file_entry_cacheable?(@session, file_entry)}> - - - <.menu_item> - - <.remix_icon icon="download-2-line" /> - Download - - - <.menu_item variant={:danger}> - - - - <% end %> -
-
-
+
<.link class="w-full flex items-center justify-center p-8 py-1 space-x-2 text-sm font-medium text-gray-500 border border-gray-400 border-dashed rounded-xl hover:bg-gray-100" role="button" patch={~p"/sessions/#{@session.id}/add-file/storage"} > <.remix_icon icon="add-line" class="text-lg align-center" /> - New file + Add file
+
+
+

+ References +

+ <.references_info_icon /> +
+ <.file_entries + file_entries={@reference_file_entries} + quarantine_file_entry_names={@quarantine_file_entry_names} + transferring_file_entry_names={@transferring_file_entry_names} + session={@session} + myself={@myself} + /> +
+
+
+

+ Attachments +

+ <.attachments_info_icon /> +
+ <.file_entries + file_entries={@attachment_file_entries} + quarantine_file_entry_names={@quarantine_file_entry_names} + transferring_file_entry_names={@transferring_file_entry_names} + session={@session} + myself={@myself} + /> +
+
+ """ + end + + defp file_entries(%{file_entries: []} = assigns) do + ~H""" +
+ No files yet
""" end - defp files_info_icon(assigns) do + defp file_entries(assigns) do + ~H""" +
+
+ <%= if file_entry.name in @quarantine_file_entry_names do %> + + <% else %> +
+ <.remix_icon + icon={file_entry_icon(file_entry, @session)} + class="text-lg align-middle mr-2" + /> + <%= file_entry.name %> +
+ <% end %> + <%= if file_entry.name in @transferring_file_entry_names do %> + <.spinner class="mr-[3px]" /> + <% else %> + <.menu id={"file-entry-#{file_entry.type}-#{idx}-menu"} position={:bottom_right}> + <:toggle> + + + <.menu_item> + + + <.menu_item> + + <.remix_icon icon="download-2-line" /> + Download + + + <.menu_item> + + + <.menu_item disabled={not Livebook.Session.file_entry_cacheable?(@session, file_entry)}> + + + <.menu_item> + <.link + role="menuitem" + patch={~p"/sessions/#{@session.id}/rename-file/#{file_entry.name}"} + > + <.remix_icon icon="edit-line" /> + Rename + + + <.menu_item variant={:danger}> + + + + <% end %> +
+
+ """ + end + + defp references_info_icon(assigns) do ~H""" - <.remix_icon icon="question-line" class="text-xl leading-none" /> + <.remix_icon icon="question-line" class="leading-none text-gray-700" /> """ end - defp file_entry_icon(:attachment), do: "file-3-line" - defp file_entry_icon(:file), do: "share-forward-line" - defp file_entry_icon(:url), do: "global-line" + defp attachments_info_icon(assigns) do + ~H""" + + <.remix_icon icon="question-line" class="leading-none text-gray-700" /> + + """ + end + + defp file_entry_icon(%{type: :url}, _session), do: "links-line" + + defp file_entry_icon(%{type: :attachment}, session) do + if FileSystem.File.local?(session.files_dir), do: "file-3-line", else: "cloud-line" + end + + defp file_entry_icon(%{type: :file, file: file}, _session) do + if FileSystem.File.local?(file), do: "file-3-line", else: "cloud-line" + end + + defp file_entry_location(%{type: :url, url: url}, _session), do: url + + defp file_entry_location(%{type: :attachment, name: name}, session) do + file = FileSystem.File.resolve(session.files_dir, name) + file.path + end + + defp file_entry_location(%{type: :file, file: file}, _session) do + file.path + end @impl true def handle_event("delete_file_entry", %{"name" => name}, socket) do diff --git a/lib/livebook_web/live/session_live/rename_file_entry_component.ex b/lib/livebook_web/live/session_live/rename_file_entry_component.ex new file mode 100644 index 00000000000..287f1e02c90 --- /dev/null +++ b/lib/livebook_web/live/session_live/rename_file_entry_component.ex @@ -0,0 +1,88 @@ +defmodule LivebookWeb.SessionLive.RenameFileEntryComponent do + use LivebookWeb, :live_component + + import Ecto.Changeset + + @impl true + def update(assigns, socket) do + socket = + socket + |> assign(assigns) + |> assign_new(:changeset, fn -> changeset(assigns.file_entry) end) + + {:ok, socket} + end + + defp changeset(file_entry, attrs \\ %{}) do + data = %{name: file_entry.name} + types = %{name: :string} + + cast({data, types}, attrs, [:name]) + |> validate_required([:name]) + |> Livebook.Notebook.validate_file_entry_name(:name) + end + + @impl true + def render(assigns) do + ~H""" +
+

+ Rename file +

+ <.form + :let={f} + for={@changeset} + as={:data} + id="rename-file-entry-form" + phx-change="validate" + phx-submit="submit" + phx-target={@myself} + > + <.text_field + field={f[:name]} + label="Name" + id="add-file-entry-form-name" + autocomplete="off" + phx-debounce="200" + autofocus + /> +
+ + <.link patch={~p"/sessions/#{@session.id}"} class="button-base button-outlined-gray"> + Cancel + +
+ +
+ """ + end + + @impl true + def handle_event("validate", %{"data" => data}, socket) do + changeset = + socket.assigns.file_entry + |> changeset(data) + |> Map.replace!(:action, :validate) + + {:noreply, assign(socket, changeset: changeset)} + end + + def handle_event("submit", %{"data" => data}, socket) do + %{file_entry: file_entry} = socket.assigns + + file_entry + |> changeset(data) + |> apply_action(:insert) + |> case do + {:ok, data} -> + Livebook.Session.rename_file_entry(socket.assigns.session.pid, file_entry.name, data.name) + {:noreply, push_patch(socket, to: ~p"/sessions/#{socket.assigns.session.id}")} + + {:error, changeset} -> + {:noreply, assign(socket, changeset: changeset)} + end + end +end diff --git a/lib/livebook_web/live/session_live/secrets_list_component.ex b/lib/livebook_web/live/session_live/secrets_list_component.ex index e367ba3b179..0e9e6ee7b62 100644 --- a/lib/livebook_web/live/session_live/secrets_list_component.ex +++ b/lib/livebook_web/live/session_live/secrets_list_component.ex @@ -15,7 +15,10 @@ defmodule LivebookWeb.SessionLive.SecretsListComponent do
Available only to this session
-
+
<.session_secret :for={ secret <- @secrets |> Session.Data.session_secrets(@hub.id) |> Enum.sort_by(& &1.name) diff --git a/lib/livebook_web/router.ex b/lib/livebook_web/router.ex index 178114dcd85..a0c621788e5 100644 --- a/lib/livebook_web/router.ex +++ b/lib/livebook_web/router.ex @@ -87,6 +87,7 @@ defmodule LivebookWeb.Router do live "/sessions/:id/settings/file", SessionLive, :file_settings live "/sessions/:id/settings/app", SessionLive, :app_settings live "/sessions/:id/add-file/:tab", SessionLive, :add_file_entry + live "/sessions/:id/rename-file/:name", SessionLive, :rename_file_entry live "/sessions/:id/bin", SessionLive, :bin get "/sessions/:id/download/export/:format", SessionController, :download_source live "/sessions/:id/export/:tab", SessionLive, :export diff --git a/test/livebook/session/data_test.exs b/test/livebook/session/data_test.exs index 22ef128541b..a68c12337f5 100644 --- a/test/livebook/session/data_test.exs +++ b/test/livebook/session/data_test.exs @@ -3917,6 +3917,62 @@ defmodule Livebook.Session.DataTest do end end + describe "apply_operation/2 given :rename_file_entry" do + test "returns an error if no file entry with the given name exists" do + data = Data.new() + operation = {:rename_file_entry, @cid, "image.jpg", "image2.jpg"} + assert :error = Data.apply_operation(data, operation) + end + + test "replaces file entry name" do + file_entry = %{type: :attachment, name: "image.jpg"} + + data = + data_after_operations!([ + {:add_file_entries, @cid, [file_entry]} + ]) + + operation = {:rename_file_entry, @cid, "image.jpg", "image2.jpg"} + + assert {:ok, %{notebook: %{file_entries: [%{type: :attachment, name: "image2.jpg"}]}}, []} = + Data.apply_operation(data, operation) + end + + test "replaces existing file entry with the same name" do + file_entry1 = %{type: :attachment, name: "image.jpg"} + file_entry2 = %{type: :attachment, name: "image2.jpg"} + + data = + data_after_operations!([ + {:add_file_entries, @cid, [file_entry1, file_entry2]} + ]) + + operation = {:rename_file_entry, @cid, "image.jpg", "image2.jpg"} + + assert {:ok, %{notebook: %{file_entries: [%{type: :attachment, name: "image2.jpg"}]}}, []} = + Data.apply_operation(data, operation) + end + + test "updates matching file entry name in quarantine" do + file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/image.jpg")) + + file_entry = %{type: :file, name: "image.jpg", file: file} + + notebook = %{ + Notebook.new() + | file_entries: [file_entry], + quarantine_file_entry_names: MapSet.new(["image.jpg"]) + } + + data = Data.new(notebook: notebook) + + operation = {:rename_file_entry, @cid, "image.jpg", "image2.jpg"} + + assert {:ok, %{notebook: notebook}, []} = Data.apply_operation(data, operation) + assert notebook.quarantine_file_entry_names == MapSet.new(["image2.jpg"]) + end + end + describe "apply_operation/2 given :delete_file_entry" do test "returns an error if no file entry with the given name exists" do data = Data.new() diff --git a/test/livebook/session_test.exs b/test/livebook/session_test.exs index 81557525823..770e90a59d3 100644 --- a/test/livebook/session_test.exs +++ b/test/livebook/session_test.exs @@ -1716,6 +1716,33 @@ defmodule Livebook.SessionTest do assert_receive {:runtime_file_entry_spec_reply, {:ok, %{type: :url, url: ^url}}} end + test "renaming file entry renames the cached file" do + bypass = Bypass.open() + url = "http://localhost:#{bypass.port}/image.jpg" + + Bypass.expect_once(bypass, "GET", "/image.jpg", fn conn -> + Plug.Conn.resp(conn, 200, "content") + end) + + session = start_session() + + Session.add_file_entries(session.pid, [%{type: :url, name: "image.jpg", url: url}]) + + runtime = connected_noop_runtime(self()) + Session.set_runtime(session.pid, runtime) + send(session.pid, {:runtime_file_entry_path_request, self(), "image.jpg"}) + + assert_receive {:runtime_file_entry_path_reply, {:ok, path}} + + Session.rename_file_entry(session.pid, "image.jpg", "image2.jpg") + + send(session.pid, {:runtime_file_entry_path_request, self(), "image2.jpg"}) + assert_receive {:runtime_file_entry_path_reply, {:ok, path2}} + + refute File.exists?(path) + assert File.exists?(path2) + end + test "removing file entry removes the cached file" do bypass = Bypass.open() url = "http://localhost:#{bypass.port}/image.jpg" @@ -1740,7 +1767,7 @@ defmodule Livebook.SessionTest do refute File.exists?(path) end - test "replacing file entry removes the cached file" do + test "replacing file entry via add removes the cached file" do bypass = Bypass.open() url = "http://localhost:#{bypass.port}/image.jpg" @@ -1764,6 +1791,35 @@ defmodule Livebook.SessionTest do refute File.exists?(path) end + test "replacing file entry via rename removes the cached file" do + bypass = Bypass.open() + url = "http://localhost:#{bypass.port}/image.jpg" + + Bypass.expect_once(bypass, "GET", "/image.jpg", fn conn -> + Plug.Conn.resp(conn, 200, "content") + end) + + session = start_session() + + Session.add_file_entries(session.pid, [%{type: :url, name: "image.jpg", url: url}]) + + %{files_dir: files_dir} = Session.get_by_pid(session.pid) + image_file = FileSystem.File.resolve(files_dir, "image2.jpg") + :ok = FileSystem.File.write(image_file, "") + Session.add_file_entries(session.pid, [%{type: :attachment, name: "image2.jpg"}]) + + runtime = connected_noop_runtime(self()) + Session.set_runtime(session.pid, runtime) + send(session.pid, {:runtime_file_entry_path_request, self(), "image.jpg"}) + + assert_receive {:runtime_file_entry_path_reply, {:ok, path}} + + Session.rename_file_entry(session.pid, "image2.jpg", "image.jpg") + wait_for_session_update(session.pid) + + refute File.exists?(path) + end + test "clear_file_entry_cache/2 removes the cached file" do bypass = Bypass.open() url = "http://localhost:#{bypass.port}/image.jpg" diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs index ddabb079139..064c3fa923c 100644 --- a/test/livebook_web/live/session_live_test.exs +++ b/test/livebook_web/live/session_live_test.exs @@ -1867,6 +1867,32 @@ defmodule LivebookWeb.SessionLiveTest do } = Session.get_data(session.pid) end + test "renaming :attachment file entry", %{conn: conn, session: session} do + %{files_dir: files_dir} = session + image_file = FileSystem.File.resolve(files_dir, "image.jpg") + :ok = FileSystem.File.write(image_file, "") + Session.add_file_entries(session.pid, [%{type: :attachment, name: "image.jpg"}]) + + {:ok, view, _} = live(conn, ~p"/sessions/#{session.id}") + + assert view |> element(~s/[data-el-files-list]/) |> render() =~ "image.jpg" + + view + |> element(~s/[data-el-files-list] menu a/, "Rename") + |> render_click() + + view + |> element(~s/#rename-file-entry-form/) + |> render_submit(%{"data" => %{"name" => "image2.jpg"}}) + + assert %{notebook: %{file_entries: [%{type: :attachment, name: "image2.jpg"}]}} = + Session.get_data(session.pid) + + page = view |> element(~s/[data-el-files-list]/) |> render() + assert page =~ "image2.jpg" + refute page =~ "image.jpg" + end + test "deleting :attachment file entry and removing the file from the file system", %{conn: conn, session: session} do %{files_dir: files_dir} = session @@ -1929,7 +1955,7 @@ defmodule LivebookWeb.SessionLiveTest do assert view |> element(~s/[data-el-files-list]/) |> render() =~ "image.jpg" view - |> element(~s/[data-el-files-list] menu button/, "Copy to files") + |> element(~s/[data-el-files-list] menu button/, "Move to attachments") |> render_click() assert_receive {:operation, diff --git a/test/support/noop_runtime.ex b/test/support/noop_runtime.ex index dc0a8c91989..84600fb5b20 100644 --- a/test/support/noop_runtime.ex +++ b/test/support/noop_runtime.ex @@ -38,6 +38,8 @@ defmodule Livebook.Runtime.NoopRuntime do :ok end + def relabel_file(_runtime, _file_id, _new_file_id), do: :ok + def revoke_file(runtime, file_id) do trace(runtime, :revoke_file, [file_id]) :ok