From ee740d41940823f992e3dca74f11ddcb7d915939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Sat, 10 Jul 2021 21:49:50 +0200 Subject: [PATCH] Resolve links for imported notebooks (#445) * Resolve links for imported notebooks * Clarify error messages --- lib/livebook/content_loader.ex | 15 +++ lib/livebook/session.ex | 17 ++- lib/livebook/session/data.ex | 3 + lib/livebook/utils.ex | 57 +++++++-- lib/livebook_web/live/home_live.ex | 40 +++++-- .../home_live/import_content_component.ex | 2 +- .../live/home_live/import_url_component.ex | 4 +- lib/livebook_web/live/session_live.ex | 106 ++++++++++++----- test/livebook/content_loader_test.exs | 22 +++- test/livebook_web/live/session_live_test.exs | 111 +++++++++++++++++- 10 files changed, 317 insertions(+), 60 deletions(-) diff --git a/lib/livebook/content_loader.ex b/lib/livebook/content_loader.ex index 001583bc7..284d62a3d 100644 --- a/lib/livebook/content_loader.ex +++ b/lib/livebook/content_loader.ex @@ -55,8 +55,23 @@ defmodule Livebook.ContentLoader do @doc """ Loads binary content from the given URl and validates if its plain text. + + Supports local file:// URLs and remote http(s):// URLs. """ @spec fetch_content(String.t()) :: {:ok, String.t()} | {:error, String.t()} + def fetch_content(url) + + def fetch_content("file://" <> path) do + case File.read(path) do + {:ok, content} -> + {:ok, content} + + {:error, error} -> + message = :file.format_error(error) + {:error, "failed to read #{path}, reason: #{message}"} + end + end + def fetch_content(url) do case :httpc.request(:get, {url, []}, http_opts(), body_format: :binary) do {:ok, {{_, 200, _}, headers, body}} -> diff --git a/lib/livebook/session.ex b/lib/livebook/session.ex index 4d8baf9be..b480e071d 100644 --- a/lib/livebook/session.ex +++ b/lib/livebook/session.ex @@ -22,7 +22,7 @@ defmodule Livebook.Session do @type state :: %{ session_id: id(), data: Data.t(), - runtime_monitor_ref: reference() + runtime_monitor_ref: reference() | nil } @type summary :: %{ @@ -30,7 +30,8 @@ defmodule Livebook.Session do pid: pid(), notebook_name: String.t(), path: String.t() | nil, - images_dir: String.t() + images_dir: String.t(), + origin_url: String.t() | nil } @typedoc """ @@ -52,6 +53,9 @@ defmodule Livebook.Session do * `:notebook` - the initial `Notebook` structure (e.g. imported from a file) + * `:origin_url` - location from where the notebook was obtained, + can be a local file:// URL or a remote http(s):// URL + * `:path` - the file to which the notebook should be saved * `:copy_images_from` - a directory path to copy notebook images from @@ -322,10 +326,12 @@ defmodule Livebook.Session do end defp init_data(opts) do - notebook = Keyword.get(opts, :notebook) - path = Keyword.get(opts, :path) + notebook = opts[:notebook] + path = opts[:path] + origin_url = opts[:origin_url] data = if(notebook, do: Data.new(notebook), else: Data.new()) + data = %{data | origin_url: origin_url} if path do case FileGuard.lock(path, self()) do @@ -579,7 +585,8 @@ defmodule Livebook.Session do pid: self(), notebook_name: state.data.notebook.name, path: state.data.path, - images_dir: images_dir_from_state(state) + images_dir: images_dir_from_state(state), + origin_url: state.data.origin_url } end diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex index 505126dc7..6d7fc69a8 100644 --- a/lib/livebook/session/data.ex +++ b/lib/livebook/session/data.ex @@ -16,6 +16,7 @@ defmodule Livebook.Session.Data do defstruct [ :notebook, + :origin_url, :path, :dirty, :section_infos, @@ -32,6 +33,7 @@ defmodule Livebook.Session.Data do @type t :: %__MODULE__{ notebook: Notebook.t(), + origin_url: String.t() | nil, path: nil | String.t(), dirty: boolean(), section_infos: %{Section.id() => section_info()}, @@ -126,6 +128,7 @@ defmodule Livebook.Session.Data do def new(notebook \\ Notebook.new()) do %__MODULE__{ notebook: notebook, + origin_url: nil, path: nil, dirty: false, section_infos: initial_section_infos(notebook), diff --git a/lib/livebook/utils.ex b/lib/livebook/utils.ex index bafcce8f9..d79ee77f3 100644 --- a/lib/livebook/utils.ex +++ b/lib/livebook/utils.ex @@ -121,18 +121,59 @@ defmodule Livebook.Utils do ## Examples - iex> Livebook.Utils.valid_hex_color?("#111111") - true + iex> Livebook.Utils.valid_hex_color?("#111111") + true - iex> Livebook.Utils.valid_hex_color?("#ABC123") - true + iex> Livebook.Utils.valid_hex_color?("#ABC123") + true - iex> Livebook.Utils.valid_hex_color?("ABCDEF") - false + iex> Livebook.Utils.valid_hex_color?("ABCDEF") + false - iex> Livebook.Utils.valid_hex_color?("#111") - false + iex> Livebook.Utils.valid_hex_color?("#111") + false """ @spec valid_hex_color?(String.t()) :: boolean() def valid_hex_color?(hex_color), do: hex_color =~ ~r/^#[0-9a-fA-F]{6}$/ + + @doc """ + Changes the first letter in the given string to upper case. + + ## Examples + + iex> Livebook.Utils.upcase_first("sippin tea") + "Sippin tea" + + iex> Livebook.Utils.upcase_first("short URL") + "Short URL" + + iex> Livebook.Utils.upcase_first("") + "" + """ + @spec upcase_first(String.t()) :: String.t() + def upcase_first(string) do + {first, rest} = String.split_at(string, 1) + String.upcase(first) <> rest + end + + @doc """ + Expands a relative path in terms of the given URL. + + ## Examples + + iex> Livebook.Utils.expand_url("file:///home/user/lib/file.ex", "../root.ex") + "file:///home/user/root.ex" + + iex> Livebook.Utils.expand_url("https://example.com/lib/file.ex?token=supersecret", "../root.ex") + "https://example.com/root.ex?token=supersecret" + """ + @spec expand_url(String.t(), String.t()) :: String.t() + def expand_url(url, relative_path) do + url + |> URI.parse() + |> Map.update!(:path, fn path -> + path |> Path.dirname() |> Path.join(relative_path) |> Path.expand() + end) + |> URI.to_string() + end end diff --git a/lib/livebook_web/live/home_live.ex b/lib/livebook_web/live/home_live.ex index 3421be622..8d18ccbc5 100644 --- a/lib/livebook_web/live/home_live.ex +++ b/lib/livebook_web/live/home_live.ex @@ -229,24 +229,47 @@ defmodule LivebookWeb.HomeLive do end def handle_event("fork", %{}, socket) do - {notebook, messages} = import_notebook(socket.assigns.path) + path = socket.assigns.path + {notebook, messages} = import_notebook(path) socket = put_import_flash_messages(socket, messages) notebook = Notebook.forked(notebook) - images_dir = Session.images_dir_for_notebook(socket.assigns.path) - {:noreply, create_session(socket, notebook: notebook, copy_images_from: images_dir)} + images_dir = Session.images_dir_for_notebook(path) + + {:noreply, + create_session(socket, + notebook: notebook, + copy_images_from: images_dir, + origin_url: "file://" <> path + )} end def handle_event("open", %{}, socket) do - {notebook, messages} = import_notebook(socket.assigns.path) + path = socket.assigns.path + {notebook, messages} = import_notebook(path) socket = put_import_flash_messages(socket, messages) - {:noreply, create_session(socket, notebook: notebook, path: socket.assigns.path)} + + {:noreply, + create_session(socket, notebook: notebook, path: path, origin_url: "file://" <> path)} end def handle_event("fork_session", %{"id" => session_id}, socket) do data = Session.get_data(session_id) notebook = Notebook.forked(data.notebook) %{images_dir: images_dir} = Session.get_summary(session_id) - {:noreply, create_session(socket, notebook: notebook, copy_images_from: images_dir)} + + origin_url = + if data.path do + "file://" <> data.path + else + data.origin_url + end + + {:noreply, + create_session(socket, + notebook: notebook, + copy_images_from: images_dir, + origin_url: origin_url + )} end @impl true @@ -261,10 +284,11 @@ defmodule LivebookWeb.HomeLive do {:noreply, assign(socket, session_summaries: session_summaries)} end - def handle_info({:import_content, content}, socket) do + def handle_info({:import_content, content, session_opts}, socket) do {notebook, messages} = Livebook.LiveMarkdown.Import.notebook_from_markdown(content) socket = put_import_flash_messages(socket, messages) - {:noreply, create_session(socket, notebook: notebook)} + session_opts = Keyword.merge(session_opts, notebook: notebook) + {:noreply, create_session(socket, session_opts)} end def handle_info( diff --git a/lib/livebook_web/live/home_live/import_content_component.ex b/lib/livebook_web/live/home_live/import_content_component.ex index d474b03cf..d489191f5 100644 --- a/lib/livebook_web/live/home_live/import_content_component.ex +++ b/lib/livebook_web/live/home_live/import_content_component.ex @@ -37,7 +37,7 @@ defmodule LivebookWeb.HomeLive.ImportContentComponent do end def handle_event("import", %{"data" => %{"content" => content}}, socket) do - send(self(), {:import_content, content}) + send(self(), {:import_content, content, []}) {:noreply, socket} end diff --git a/lib/livebook_web/live/home_live/import_url_component.ex b/lib/livebook_web/live/home_live/import_url_component.ex index fef2a82a2..ac7e1eed0 100644 --- a/lib/livebook_web/live/home_live/import_url_component.ex +++ b/lib/livebook_web/live/home_live/import_url_component.ex @@ -50,11 +50,11 @@ defmodule LivebookWeb.HomeLive.ImportUrlComponent do |> ContentLoader.fetch_content() |> case do {:ok, content} -> - send(self(), {:import_content, content}) + send(self(), {:import_content, content, [origin_url: url]}) {:noreply, socket} {:error, message} -> - {:noreply, assign(socket, error_message: String.capitalize(message))} + {:noreply, assign(socket, error_message: Utils.upcase_first(message))} end end end diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index ec1493dee..591228e82 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -733,58 +733,104 @@ defmodule LivebookWeb.SessionLive do true -> socket - |> push_patch(to: Routes.session_path(socket, :page, socket.assigns.session_id)) |> put_flash( :error, "Got unrecognised session path: #{path}\nIf you want to link another notebook, make sure to include the .livemd extension" ) + |> redirect_to_self() end end defp handle_relative_notebook_path(socket, relative_path) do - case socket.private.data.path do + resolution_url = location(socket.private.data) + + case resolution_url do nil -> socket |> put_flash( :info, - "Cannot resolve notebook path #{relative_path}, because the current notebook has no file" + "Cannot resolve notebook path #{relative_path}, because the current notebook has no location" ) - |> push_patch(to: Routes.session_path(socket, :page, socket.assigns.session_id)) + |> redirect_to_self() - path -> - target_path = path |> Path.dirname() |> Path.join(relative_path) |> Path.expand() - maybe_open_notebook(socket, target_path) + url -> + target_url = Livebook.Utils.expand_url(url, relative_path) + + case session_id_by_url(target_url) do + {:ok, session_id} -> + push_redirect(socket, to: Routes.session_path(socket, :page, session_id)) + + {:error, :none} -> + open_notebook(socket, target_url) + + {:error, :many} -> + socket + |> put_flash( + :error, + "Cannot navigate, because multiple sessions were found for #{target_url}" + ) + |> redirect_to_self() + end end end - defp maybe_open_notebook(socket, path) do - if session_id = session_id_by_path(path) do - push_redirect(socket, to: Routes.session_path(socket, :page, session_id)) + defp location(data) + defp location(%{path: path}) when is_binary(path), do: "file://" <> path + defp location(%{origin_url: origin_url}), do: origin_url + + defp open_notebook(socket, url) do + url + |> Livebook.ContentLoader.rewrite_url() + |> Livebook.ContentLoader.fetch_content() + |> case do + {:ok, content} -> + {notebook, messages} = Livebook.LiveMarkdown.Import.notebook_from_markdown(content) + + # If the current session has no path, fork the notebook + fork? = socket.private.data.path == nil + {path, notebook} = path_and_notebook(fork?, url, notebook) + + socket + |> put_import_flash_messages(messages) + |> create_session(notebook: notebook, origin_url: url, path: path) + + {:error, message} -> + socket + |> put_flash(:error, "Cannot navigate, " <> message) + |> redirect_to_self() + end + end + + defp path_and_notebook(fork?, url, notebook) + defp path_and_notebook(false, "file://" <> path, notebook), do: {path, notebook} + defp path_and_notebook(true, "file://" <> _path, notebook), do: {nil, Notebook.forked(notebook)} + defp path_and_notebook(_fork?, _url, notebook), do: {nil, notebook} + + defp session_id_by_url(url) do + session_summaries = SessionSupervisor.get_session_summaries() + + session_with_path = + Enum.find(session_summaries, fn summary -> + summary.path && "file://" <> summary.path == url + end) + + # A session associated with the given path takes + # precedence over sessions originating from this path + if session_with_path do + {:ok, session_with_path.session_id} else - case File.read(path) do - {:ok, content} -> - {notebook, messages} = LiveMarkdown.Import.notebook_from_markdown(content) - - socket - |> put_import_flash_messages(messages) - |> create_session(notebook: notebook, path: path) - - {:error, error} -> - message = :file.format_error(error) - - socket - |> put_flash(:error, "Failed to open #{path}, reason: #{message}") - |> push_patch(to: Routes.session_path(socket, :page, socket.assigns.session_id)) + session_summaries + |> Enum.filter(fn summary -> summary.origin_url == url end) + |> case do + [summary] -> {:ok, summary.session_id} + [] -> {:error, :none} + _ -> {:error, :many} end end end - defp session_id_by_path(path) do - session_summaries = SessionSupervisor.get_session_summaries() - - Enum.find_value(session_summaries, fn summary -> - summary.path == path && summary.session_id - end) + defp redirect_to_self(socket) do + push_patch(socket, to: Routes.session_path(socket, :page, socket.assigns.session_id)) end defp after_operation(socket, _prev_socket, {:client_join, client_pid, user}) do diff --git a/test/livebook/content_loader_test.exs b/test/livebook/content_loader_test.exs index b5a94d7f1..36e8c71fe 100644 --- a/test/livebook/content_loader_test.exs +++ b/test/livebook/content_loader_test.exs @@ -25,7 +25,27 @@ defmodule Livebook.ContentLoaderTest do end end - describe "fetch_content/1" do + describe "fetch_content/1 with local file URL" do + @tag :tmp_dir + test "returns an error then the file cannot be read", %{tmp_dir: tmp_dir} do + path = Path.join(tmp_dir, "nonexistent.livemd") + url = "file://" <> path + + assert ContentLoader.fetch_content(url) == + {:error, "failed to read #{path}, reason: no such file or directory"} + end + + @tag :tmp_dir + test "returns file contents when read successfully", %{tmp_dir: tmp_dir} do + path = Path.join(tmp_dir, "notebook.livemd") + File.write!(path, "# My notebook") + url = "file://" <> path + + assert ContentLoader.fetch_content(url) == {:ok, "# My notebook"} + end + end + + describe "fetch_content/1 with remote URL" do setup do bypass = Bypass.open() {:ok, bypass: bypass} diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs index c84962d42..1196a0ccc 100644 --- a/test/livebook_web/live/session_live_test.exs +++ b/test/livebook_web/live/session_live_test.exs @@ -417,7 +417,7 @@ defmodule LivebookWeb.SessionLiveTest do assert render(view) =~ "Got unrecognised session path: document.pdf" end - test "renders an info message when the session has no associated path", + test "renders an info message when the session has neither original url nor path", %{conn: conn, session_id: session_id} do session_path = "/sessions/#{session_id}" @@ -427,7 +427,7 @@ defmodule LivebookWeb.SessionLiveTest do {:ok, view, _} = follow_redirect(result, conn) assert render(view) =~ - "Cannot resolve notebook path notebook.livemd, because the current notebook has no file" + "Cannot resolve notebook path notebook.livemd, because the current notebook has no location" end @tag :tmp_dir @@ -447,7 +447,7 @@ defmodule LivebookWeb.SessionLiveTest do {:ok, view, _} = follow_redirect(result, conn) assert render(view) =~ - "Failed to open #{notebook_path}, reason: no such file or directory" + "Cannot navigate, failed to read #{notebook_path}, reason: no such file or directory" end @tag :tmp_dir @@ -461,12 +461,37 @@ defmodule LivebookWeb.SessionLiveTest do File.write!(notebook_path, "# Sibling notebook") - assert {:error, {:live_redirect, %{to: _session_path}}} = + assert {:error, {:live_redirect, %{to: new_session_path}}} = result = live(conn, "/sessions/#{session_id}/notebook.livemd") {:ok, view, _} = follow_redirect(result, conn) - assert render(view) =~ "Sibling notebook" + + "/sessions/" <> session_id = new_session_path + data = Session.get_data(session_id) + assert data.path == notebook_path + end + + @tag :tmp_dir + test "if the current session has no path, forks the relative notebook", + %{conn: conn, tmp_dir: tmp_dir} do + index_path = Path.join(tmp_dir, "index.livemd") + notebook_path = Path.join(tmp_dir, "notebook.livemd") + + {:ok, session_id} = SessionSupervisor.create_session(origin_url: "file://" <> index_path) + + File.write!(notebook_path, "# Sibling notebook") + + assert {:error, {:live_redirect, %{to: new_session_path}}} = + result = live(conn, "/sessions/#{session_id}/notebook.livemd") + + {:ok, view, _} = follow_redirect(result, conn) + assert render(view) =~ "Sibling notebook - fork" + + "/sessions/" <> session_id = new_session_path + data = Session.get_data(session_id) + assert data.path == nil + assert data.origin_url == "file://" <> notebook_path end @tag :tmp_dir @@ -526,6 +551,80 @@ defmodule LivebookWeb.SessionLiveTest do assert render(view) =~ "Parent notebook" end + + test "resolves remote URLs", %{conn: conn} do + bypass = Bypass.open() + + Bypass.expect_once(bypass, "GET", "/notebook.livemd", fn conn -> + conn + |> Plug.Conn.put_resp_content_type("text/plain") + |> Plug.Conn.resp(200, "# My notebook") + end) + + index_url = url(bypass.port) <> "/index.livemd" + {:ok, session_id} = SessionSupervisor.create_session(origin_url: index_url) + + {:ok, view, _} = + conn + |> live("/sessions/#{session_id}/notebook.livemd") + |> follow_redirect(conn) + + assert render(view) =~ "My notebook" + end + + test "renders an error message if relative remote notebook cannot be loaded", %{conn: conn} do + bypass = Bypass.open() + + Bypass.expect_once(bypass, "GET", "/notebook.livemd", fn conn -> + Plug.Conn.resp(conn, 500, "Error") + end) + + index_url = url(bypass.port) <> "/index.livemd" + + {:ok, session_id} = SessionSupervisor.create_session(origin_url: index_url) + + session_path = "/sessions/#{session_id}" + + assert {:error, {:live_redirect, %{to: ^session_path}}} = + result = live(conn, "/sessions/#{session_id}/notebook.livemd") + + {:ok, view, _} = follow_redirect(result, conn) + assert render(view) =~ "Cannot navigate, failed to download notebook from the given URL" + end + + test "if the remote notebook is already imported, redirects to the session", + %{conn: conn, test: test} do + index_url = "http://example.com/#{test}/index.livemd" + notebook_url = "http://example.com/#{test}/notebook.livemd" + + {:ok, index_session_id} = SessionSupervisor.create_session(origin_url: index_url) + {:ok, notebook_session_id} = SessionSupervisor.create_session(origin_url: notebook_url) + + notebook_session_path = "/sessions/#{notebook_session_id}" + + assert {:error, {:live_redirect, %{to: ^notebook_session_path}}} = + live(conn, "/sessions/#{index_session_id}/notebook.livemd") + end + + test "renders an error message if there are already multiple session imported from the relative URL", + %{conn: conn, test: test} do + index_url = "http://example.com/#{test}/index.livemd" + notebook_url = "http://example.com/#{test}/notebook.livemd" + + {:ok, index_session_id} = SessionSupervisor.create_session(origin_url: index_url) + {:ok, _notebook_session_id1} = SessionSupervisor.create_session(origin_url: notebook_url) + {:ok, _notebook_session_id2} = SessionSupervisor.create_session(origin_url: notebook_url) + + index_session_path = "/sessions/#{index_session_id}" + + assert {:error, {:live_redirect, %{to: ^index_session_path}}} = + result = live(conn, "/sessions/#{index_session_id}/notebook.livemd") + + {:ok, view, _} = follow_redirect(result, conn) + + assert render(view) =~ + "Cannot navigate, because multiple sessions were found for #{notebook_url}" + end end # Helpers @@ -572,4 +671,6 @@ defmodule LivebookWeb.SessionLiveTest do {:ok, user} = User.new() |> User.change(%{"name" => name}) user end + + defp url(port), do: "http://localhost:#{port}" end