Provide shared facade in Storage module (#1463)

lib/livebook/hubs.ex

@@ -4,16 +4,6 @@ defmodule Livebook.Hubs do
   alias Livebook.Storage
   alias Livebook.Hubs.{Fly, Local, Metadata, Provider}
 
-  defmodule NotFoundError do
-    @moduledoc false
-
-    defexception [:id, plug_status: 404]
-
-    def message(%{id: id}) do
-      "could not find a hub matching #{inspect(id)}"
-    end
-  end
-
   @namespace :hubs
 
   @doc """
@@ -21,7 +11,7 @@ defmodule Livebook.Hubs do
   """
   @spec fetch_hubs() :: list(Provider.t())
   def fetch_hubs do
-    for fields <- Storage.current().all(@namespace) do
+    for fields <- Storage.all(@namespace) do
       to_struct(fields)
     end
   end
@@ -39,14 +29,11 @@ defmodule Livebook.Hubs do
   @doc """
   Gets one hub from storage.
 
-  Raises `NotFoundError` if the hub does not exist.
+  Raises `Livebook.Storage.NotFoundError` if the hub does not exist.
   """
   @spec fetch_hub!(String.t()) :: Provider.t()
   def fetch_hub!(id) do
-    case Storage.current().fetch(@namespace, id) do
+    Storage.fetch!(@namespace, id) |> to_struct()
-      :error -> raise NotFoundError, id: id
-      {:ok, fields} -> to_struct(fields)
-    end
   end
 
   @doc """
@@ -54,7 +41,7 @@ defmodule Livebook.Hubs do
   """
   @spec hub_exists?(String.t()) :: boolean()
   def hub_exists?(id) do
-    case Storage.current().fetch(@namespace, id) do
+    case Storage.fetch(@namespace, id) do
       :error -> false
       {:ok, _} -> true
     end
@@ -67,7 +54,7 @@ defmodule Livebook.Hubs do
   def save_hub(struct) do
     attributes = struct |> Map.from_struct() |> Map.to_list()
 
-    with :ok <- Storage.current().insert(@namespace, struct.id, attributes),
+    with :ok <- Storage.insert(@namespace, struct.id, attributes),
          :ok <- broadcast_hubs_change() do
       struct
     end
@@ -75,7 +62,7 @@ defmodule Livebook.Hubs do
 
   @doc false
   def delete_hub(id) do
-    Storage.current().delete(@namespace, id)
+    Storage.delete(@namespace, id)
   end
 
   @doc false

lib/livebook/secrets.ex

@@ -3,24 +3,15 @@ defmodule Livebook.Secrets do
 
   import Ecto.Changeset, only: [apply_action: 2]
 
+  alias Livebook.Storage
   alias Livebook.Secrets.Secret
 
-  defmodule NotFoundError do
-    @moduledoc false
-
-    defexception [:message, plug_status: 404]
-  end
-
-  defp storage() do
-    Livebook.Storage.current()
-  end
-
   @doc """
   Get the secrets list from storage.
   """
   @spec fetch_secrets() :: list(Secret.t())
   def fetch_secrets() do
-    for fields <- storage().all(:secrets) do
+    for fields <- Storage.all(:secrets) do
       struct!(Secret, Map.delete(fields, :id))
     end
     |> Enum.sort()
@@ -32,21 +23,16 @@ defmodule Livebook.Secrets do
   """
   @spec fetch_secret!(String.t()) :: Secret.t()
   def fetch_secret!(id) do
-    case storage().fetch(:secrets, id) do
+    fields = Storage.fetch!(:secrets, id)
-      :error ->
-        raise NotFoundError, "could not find the secret matching #{inspect(id)}"
-
-      {:ok, fields} ->
     struct!(Secret, Map.delete(fields, :id))
   end
-  end
 
   @doc """
   Checks if the secret already exists.
   """
   @spec secret_exists?(String.t()) :: boolean()
   def secret_exists?(id) do
-    storage().fetch(:secrets, id) != :error
+    Storage.fetch(:secrets, id) != :error
   end
 
   @doc """
@@ -65,7 +51,7 @@ defmodule Livebook.Secrets do
   defp save_secret(secret) do
     attributes = secret |> Map.from_struct() |> Map.to_list()
 
-    with :ok <- storage().insert(:secrets, secret.name, attributes),
+    with :ok <- Storage.insert(:secrets, secret.name, attributes),
          :ok <- broadcast_secrets_change({:set_secret, secret}) do
       {:ok, secret}
     end
@@ -78,7 +64,7 @@ defmodule Livebook.Secrets do
   def unset_secret(id) do
     if secret_exists?(id) do
       secret = fetch_secret!(id)
-      storage().delete(:secrets, id)
+      Storage.delete(:secrets, id)
       broadcast_secrets_change({:unset_secret, secret})
     end
 

lib/livebook/settings.ex

@@ -6,14 +6,9 @@ defmodule Livebook.Settings do
   import Ecto.Changeset, only: [apply_action: 2]
 
   alias Livebook.FileSystem
+  alias Livebook.Storage
   alias Livebook.Settings.EnvVar
 
-  defmodule NotFoundError do
-    @moduledoc false
-
-    defexception [:message, plug_status: 404]
-  end
-
   @typedoc """
   An id that is used for filesystem's manipulation, either insertion or removal.
   """
@@ -24,7 +19,7 @@ defmodule Livebook.Settings do
   """
   @spec autosave_path() :: String.t() | nil
   def autosave_path() do
-    case storage().fetch_key(:settings, "global", :autosave_path) do
+    case Storage.fetch_key(:settings, "global", :autosave_path) do
       {:ok, value} -> value
       :error -> default_autosave_path()
     end
@@ -43,7 +38,7 @@ defmodule Livebook.Settings do
   """
   @spec set_autosave_path(String.t()) :: :ok
   def set_autosave_path(autosave_path) do
-    storage().insert(:settings, "global", autosave_path: autosave_path)
+    Storage.insert(:settings, "global", autosave_path: autosave_path)
   end
 
   @doc """
@@ -51,7 +46,7 @@ defmodule Livebook.Settings do
   """
   @spec reset_autosave_path() :: :ok
   def reset_autosave_path() do
-    storage().delete_key(:settings, "global", :autosave_path)
+    Storage.delete_key(:settings, "global", :autosave_path)
   end
 
   @doc """
@@ -62,7 +57,7 @@ defmodule Livebook.Settings do
   @spec file_systems() :: [{file_system_id(), Filesystem.t()}]
   def file_systems() do
     restored_file_systems =
-      storage().all(:filesystem)
+      Storage.all(:filesystem)
       |> Enum.sort_by(&Map.get(&1, :order, System.os_time()))
       |> Enum.map(fn %{id: fs_id} = raw_fs ->
         {fs_id, storage_to_fs(raw_fs)}
@@ -84,7 +79,7 @@ defmodule Livebook.Settings do
     id = Livebook.Utils.random_short_id()
 
     :ok =
-      storage().insert(:filesystem, id, [{:type, "s3"}, {:order, System.os_time()} | attributes])
+      Storage.insert(:filesystem, id, [{:type, "s3"}, {:order, System.os_time()} | attributes])
 
     id
   end
@@ -94,11 +89,7 @@ defmodule Livebook.Settings do
   """
   @spec remove_file_system(file_system_id()) :: :ok
   def remove_file_system(filesystem_id) do
-    storage().delete(:filesystem, filesystem_id)
+    Storage.delete(:filesystem, filesystem_id)
-  end
-
-  defp storage() do
-    Livebook.Storage.current()
   end
 
   defp storage_to_fs(%{type: "s3"} = config) do
@@ -113,7 +104,7 @@ defmodule Livebook.Settings do
   """
   @spec update_check_enabled?() :: boolean()
   def update_check_enabled?() do
-    case storage().fetch_key(:settings, "global", :update_check_enabled) do
+    case Storage.fetch_key(:settings, "global", :update_check_enabled) do
       {:ok, value} -> value
       :error -> true
     end
@@ -124,7 +115,7 @@ defmodule Livebook.Settings do
   """
   @spec set_update_check_enabled(boolean()) :: :ok
   def set_update_check_enabled(enabled) do
-    storage().insert(:settings, "global", update_check_enabled: enabled)
+    Storage.insert(:settings, "global", update_check_enabled: enabled)
   end
 
   @doc """
@@ -132,7 +123,7 @@ defmodule Livebook.Settings do
   """
   @spec fetch_env_vars() :: list(EnvVar.t())
   def fetch_env_vars do
-    for fields <- storage().all(:env_vars) do
+    for fields <- Storage.all(:env_vars) do
       struct!(EnvVar, Map.delete(fields, :id))
     end
   end
@@ -144,22 +135,16 @@ defmodule Livebook.Settings do
   """
   @spec fetch_env_var!(String.t()) :: EnvVar.t()
   def fetch_env_var!(id) do
-    case storage().fetch(:env_vars, id) do
+    fields = Storage.fetch!(:env_vars, id)
-      :error ->
-        raise NotFoundError,
-          message: "could not find an environment variable matching #{inspect(id)}"
-
-      {:ok, fields} ->
     struct!(EnvVar, Map.delete(fields, :id))
   end
-  end
 
   @doc """
   Checks if environment variable already exists.
   """
   @spec env_var_exists?(String.t()) :: boolean()
   def env_var_exists?(id) do
-    storage().fetch(:env_vars, id) != :error
+    Storage.fetch(:env_vars, id) != :error
   end
 
   @doc """
@@ -181,7 +166,7 @@ defmodule Livebook.Settings do
   defp save_env_var(env_var) do
     attributes = env_var |> Map.from_struct() |> Map.to_list()
 
-    with :ok <- storage().insert(:env_vars, env_var.name, attributes),
+    with :ok <- Storage.insert(:env_vars, env_var.name, attributes),
          :ok <- broadcast_env_vars_change({:env_var_set, env_var}) do
       {:ok, env_var}
     end
@@ -197,7 +182,7 @@ defmodule Livebook.Settings do
   def unset_env_var(id) do
     if env_var_exists?(id) do
       env_var = fetch_env_var!(id)
-      storage().delete(:env_vars, id)
+      Storage.delete(:env_vars, id)
       broadcast_env_vars_change({:env_var_unset, env_var})
     end
 

lib/livebook/storage.ex

@@ -9,9 +9,18 @@ defmodule Livebook.Storage do
   @type attribute :: atom()
   @type value :: binary() | nil
   @type timestamp :: non_neg_integer()
-
   @type entity :: %{required(:id) => entity_id(), optional(attribute()) => value()}
 
+  defmodule NotFoundError do
+    @moduledoc false
+
+    defexception [:id, :namespace, plug_status: 404]
+
+    def message(%{namespace: namespace, id: id}) do
+      "could not find entry in \"#{namespace}\" with ID #{inspect(id)}"
+    end
+  end
+
   @doc """
   Returns all values in namespace.
 
@@ -21,6 +30,11 @@ defmodule Livebook.Storage do
   """
   @callback all(namespace()) :: [entity()]
 
+  @doc """
+  Delegate for `c:all/1`.
+  """
+  def all(namespace), do: current().all(namespace)
+
   @doc """
   Returns a map identified by `entity_id` in `namespace`.
 
@@ -30,6 +44,21 @@ defmodule Livebook.Storage do
   """
   @callback fetch(namespace(), entity_id()) :: {:ok, entity()} | :error
 
+  @doc """
+  Delegate for `c:fetch/2`.
+  """
+  def fetch(namespace, id), do: current().fetch(namespace, id)
+
+  @doc """
+  Raising delegate for `c:fetch/2`.
+  """
+  def fetch!(namespace, id) do
+    case current().fetch(namespace, id) do
+      {:ok, entity} -> entity
+      :error -> raise NotFoundError, namespace: namespace, id: id
+    end
+  end
+
   @doc """
   Returns the value for a given `namespace`-`entity_id`-`attribute`.
 
@@ -39,21 +68,41 @@ defmodule Livebook.Storage do
   """
   @callback fetch_key(namespace(), entity_id(), attribute()) :: {:ok, value()} | :error
 
+  @doc """
+  Delegate for `c:fetch_key/3`.
+  """
+  def fetch_key(namespace, id, attribute), do: current().fetch_key(namespace, id, attribute)
+
   @doc """
   Inserts given list of attribute-value paris to a entity belonging to specified namespace.
   """
   @callback insert(namespace(), entity_id(), [{attribute(), value()}]) :: :ok
 
+  @doc """
+  Delegate for `c:insert/3`.
+  """
+  def insert(namespace, id, attributes), do: current().insert(namespace, id, attributes)
+
   @doc """
   Deletes an entity of given id from given namespace.
   """
   @callback delete(namespace(), entity_id()) :: :ok
 
+  @doc """
+  Delegate for `c:delete/2`.
+  """
+  def delete(namespace, id), do: current().delete(namespace, id)
+
   @doc """
   Deletes an attribute from given entity.
   """
   @callback delete_key(namespace(), entity_id(), attribute()) :: :ok
 
+  @doc """
+  Delegate for `c:delete_key/3`.
+  """
+  def delete_key(namespace, id, attribute), do: current().delete_key(namespace, id, attribute)
+
   @spec current() :: module()
   def current(), do: Application.fetch_env!(:livebook, :storage)
 end

test/livebook/hubs_test.exs

@@ -34,8 +34,8 @@ defmodule Livebook.HubsTest do
   end
 
   test "fetch_hub!/1 returns one persisted fly" do
-    assert_raise Hubs.NotFoundError,
+    assert_raise Livebook.Storage.NotFoundError,
-                 ~s/could not find a hub matching "fly-foo"/,
+                 ~s/could not find entry in \"hubs\" with ID "fly-foo"/,
                  fn ->
                    Hubs.fetch_hub!("fly-foo")
                  end

test/livebook/secrets_test.exs

@@ -13,12 +13,12 @@ defmodule Livebook.SecretsTest do
     refute %Secret{name: "FOO", value: "111"} in Secrets.fetch_secrets()
   end
 
-  test "fetch an specif secret" do
+  test "fetch an specific secret" do
     secret = %{name: "FOO", value: "111"}
     Secrets.set_secret(secret)
 
-    assert_raise Secrets.NotFoundError,
+    assert_raise Livebook.Storage.NotFoundError,
-                 ~s(could not find the secret matching "NOT_HERE"),
+                 ~s(could not find entry in \"secrets\" with ID "NOT_HERE"),
                  fn ->
                    Secrets.fetch_secret!("NOT_HERE")
                  end

test/livebook/settings_test.exs

@@ -12,8 +12,8 @@ defmodule Livebook.SettingsTest do
   end
 
   test "fetch_env_var!/1 returns one persisted fly" do
-    assert_raise Settings.NotFoundError,
+    assert_raise Livebook.Storage.NotFoundError,
-                 ~s/could not find an environment variable matching "123456"/,
+                 ~s/could not find entry in \"env_vars\" with ID "123456"/,
                  fn ->
                    Settings.fetch_env_var!("123456")
                  end

test/support/factory.ex

@@ -55,7 +55,7 @@ defmodule Livebook.Factory do
   def insert_env_var(factory_name, attrs \\ %{}) do
     env_var = build(factory_name, attrs)
     attributes = env_var |> Map.from_struct() |> Map.to_list()
-    Livebook.Storage.current().insert(:env_vars, env_var.name, attributes)
+    Livebook.Storage.insert(:env_vars, env_var.name, attributes)
 
     env_var
   end

test/test_helper.exs

@@ -37,7 +37,7 @@ Application.put_env(:livebook, Livebook.Runtime.Embedded,
 )
 
 # Disable autosaving
-Livebook.Storage.current().insert(:settings, "global", autosave_path: nil)
+Livebook.Storage.insert(:settings, "global", autosave_path: nil)
 
 erl_docs_available? = Code.fetch_docs(:gen_server) != {:error, :chunk_not_found}