Fix File System migration and some touchups (#2235)

lib/livebook/file_system/s3.ex

@@ -24,43 +24,11 @@ defmodule Livebook.FileSystem.S3 do
   end
 
   @doc """
-  Returns a new file system struct.
+  Infers region from the given bucket URL.
-
-  ## Options
-
-    * `:region` - the bucket region. By default the URL is assumed
-      to have the format `*.[region].[rootdomain].com` and the region
-      is inferred from that URL
-
-    * `:external_id` - the external id from Teams.
-
-    * `:hub_id` - the Hub id.
-
-    * `:id` - the file system id.
-
   """
-  @spec new(String.t(), String.t(), String.t(), keyword()) :: t()
+  @spec region_from_uri(String.t()) :: String.t()
-  def new(bucket_url, access_key_id, secret_access_key, opts \\ []) do
+  # TODO: make it private again on Livebook v0.12
-    opts = Keyword.validate!(opts, [:region, :external_id, :hub_id, :id])
+  def region_from_uri(uri) do
-
-    bucket_url = String.trim_trailing(bucket_url, "/")
-    region = opts[:region] || region_from_uri(bucket_url)
-
-    hub_id = Keyword.get(opts, :hub_id, Livebook.Hubs.Personal.id())
-    id = opts[:id] || id(hub_id, bucket_url)
-
-    %__MODULE__{
-      id: id,
-      bucket_url: bucket_url,
-      external_id: opts[:external_id],
-      region: region,
-      access_key_id: access_key_id,
-      secret_access_key: secret_access_key,
-      hub_id: hub_id
-    }
-  end
-
-  defp region_from_uri(uri) do
     # For many services the API host is of the form *.[region].[rootdomain].com
     %{host: host} = URI.parse(uri)
     splitted_host = host |> String.split(".") |> Enum.reverse()
@@ -72,36 +40,6 @@ defmodule Livebook.FileSystem.S3 do
     end
   end
 
-  @doc """
-  Parses file system from a configuration map.
-  """
-  @spec from_config(map()) :: {:ok, t()} | {:error, String.t()}
-  def from_config(config) do
-    case config do
-      %{
-        bucket_url: bucket_url,
-        access_key_id: access_key_id,
-        secret_access_key: secret_access_key
-      } ->
-        file_system =
-          new(bucket_url, access_key_id, secret_access_key,
-            region: config[:region],
-            external_id: config[:external_id]
-          )
-
-        {:ok, file_system}
-
-      _config ->
-        {:error,
-         "S3 configuration is expected to have keys: :bucket_url, :access_key_id and :secret_access_key, but got #{inspect(config)}"}
-    end
-  end
-
-  @spec to_config(t()) :: map()
-  def to_config(%__MODULE__{} = s3) do
-    Map.take(s3, [:bucket_url, :region, :access_key_id, :secret_access_key])
-  end
-
   @doc """
   Returns an `%Ecto.Changeset{}` for tracking file system changes.
   """
@@ -121,7 +59,7 @@ defmodule Livebook.FileSystem.S3 do
       :hub_id
     ])
     |> put_region_from_uri()
-    |> validate_required([:bucket_url, :access_key_id, :secret_access_key])
+    |> validate_required([:bucket_url, :region, :access_key_id, :secret_access_key, :hub_id])
     |> Livebook.Utils.validate_url(:bucket_url)
     |> put_id()
   end
@@ -421,19 +359,31 @@ defimpl Livebook.FileSystem, for: Livebook.FileSystem.S3 do
     })
   end
 
-  def load(_file_system, fields) do
+  def load(file_system, fields) do
-    S3.new(fields.bucket_url, fields.access_key_id, fields.secret_access_key,
+    %{
-      region: fields[:region],
+      file_system
-      external_id: fields[:external_id],
+      | id: fields.id,
-      id: fields[:id],
+        bucket_url: fields.bucket_url,
-      hub_id: fields[:hub_id]
+        external_id: fields.external_id,
-    )
+        region: fields.region,
+        access_key_id: fields.access_key_id,
+        secret_access_key: fields.secret_access_key,
+        hub_id: fields.hub_id
+    }
   end
 
   def dump(file_system) do
     file_system
     |> Map.from_struct()
-    |> Map.take([:id, :bucket_url, :region, :access_key_id, :secret_access_key, :hub_id])
+    |> Map.take([
+      :id,
+      :bucket_url,
+      :region,
+      :access_key_id,
+      :secret_access_key,
+      :hub_id,
+      :external_id
+    ])
   end
 
   def external_metadata(file_system) do

lib/livebook/live_markdown/import.ex

@@ -414,7 +414,7 @@ defmodule Livebook.LiveMarkdown.Import do
       when is_list(file_entry_metadata) ->
         file_system_by_id =
           if Enum.any?(file_entry_metadata, &(&1["type"] == "file")) do
-            for file_system <- Livebook.Settings.file_systems(),
+            for file_system <- Livebook.Hubs.get_file_systems(),
                 do: {file_system.id, file_system},
                 into: %{}
           else

lib/livebook/migration.ex

@@ -64,9 +64,19 @@ defmodule Livebook.Migration do
         id_mapping =
           for config <- configs, into: %{} do
             old_id = config.id
+
+            # Ensure new file system fields
+            new_fields = %{
+              hub_id: Livebook.Hubs.Personal.id(),
+              external_id: nil,
+              region: Livebook.FileSystem.S3.region_from_uri(config.bucket_url)
+            }
+
+            config = Map.merge(new_fields, config)
+
             # At this point S3 is the only file system we store
-            {:ok, file_system} = Livebook.FileSystem.S3.from_config(config)
+            file_system = Livebook.FileSystems.load("s3", config)
-            Livebook.Settings.save_file_system(file_system)
+            Livebook.Hubs.Personal.save_file_system(file_system)
             Livebook.Storage.delete(:filesystem, old_id)
             {old_id, file_system.id}
           end
@@ -92,11 +102,10 @@ defmodule Livebook.Migration do
 
     with {:ok, default_file_system_id} <-
            Livebook.Storage.fetch_key(:settings, "global", :default_file_system_id) do
-      with {:ok, default_file_system} <-
+      Livebook.Hubs.get_file_systems()
-             Livebook.Settings.fetch_file_system(default_file_system_id) do
+      |> Enum.find(&(&1.id == default_file_system_id))
-        default_dir = Livebook.FileSystem.File.new(default_file_system)
+      |> Livebook.FileSystem.File.new()
-        Livebook.Settings.set_default_dir(default_dir)
+      |> Livebook.Settings.set_default_dir()
-      end
 
       Livebook.Storage.delete_key(:settings, "global", :default_file_system_id)
     end

lib/livebook/notebook_manager.ex

@@ -243,8 +243,13 @@ defmodule Livebook.NotebookManager do
         _ -> %{}
       end
 
+    file_systems =
+      Livebook.Storage.all(:file_systems)
+      |> Enum.sort_by(& &1.bucket_url)
+      |> Enum.map(fn fields -> Livebook.FileSystems.load(fields.type, fields) end)
+
     file_system_by_id =
-      for file_system <- Livebook.Settings.file_systems(),
+      for file_system <- file_systems,
           do: {file_system.id, file_system},
           into: %{}
 

lib/livebook/settings.ex

@@ -42,70 +42,6 @@ defmodule Livebook.Settings do
     Storage.delete_key(:settings, "global", :autosave_path)
   end
 
-  @doc """
-  Returns all known file systems.
-  """
-  @spec file_systems() :: list(FileSystem.t())
-  def file_systems() do
-    restored_file_systems =
-      Storage.all(:file_systems)
-      |> Enum.sort_by(&Map.get(&1, :order, System.os_time()))
-      |> Enum.map(&storage_to_fs/1)
-
-    [Livebook.Config.local_file_system() | restored_file_systems]
-  end
-
-  @doc """
-  Finds a file system by id.
-  """
-  @spec fetch_file_system(FileSystem.id()) :: {:ok, FileSystem.t()}
-  def fetch_file_system(file_system_id) do
-    local_file_system = Livebook.Config.local_file_system()
-
-    if file_system_id == local_file_system.id do
-      {:ok, local_file_system}
-    else
-      with {:ok, config} <- Storage.fetch(:file_systems, file_system_id) do
-        {:ok, storage_to_fs(config)}
-      end
-    end
-  end
-
-  @doc """
-  Saves a new file system to the configured ones.
-  """
-  @spec save_file_system(FileSystem.t()) :: :ok
-  def save_file_system(%FileSystem.S3{} = file_system) do
-    attributes =
-      file_system
-      |> FileSystem.S3.to_config()
-      |> Map.to_list()
-
-    attrs = [{:type, "s3"}, {:order, System.os_time()} | attributes]
-    :ok = Storage.insert(:file_systems, file_system.id, attrs)
-  end
-
-  @doc """
-  Removes the given file system from the configured ones.
-  """
-  @spec remove_file_system(FileSystem.id()) :: :ok
-  def remove_file_system(file_system_id) do
-    if default_dir().file_system.id == file_system_id do
-      Storage.delete_key(:settings, "global", :default_dir)
-    end
-
-    Livebook.NotebookManager.remove_file_system(file_system_id)
-
-    Storage.delete(:file_systems, file_system_id)
-  end
-
-  defp storage_to_fs(%{type: "s3"} = config) do
-    case FileSystem.S3.from_config(config) do
-      {:ok, fs} -> fs
-      {:error, message} -> raise ArgumentError, "invalid S3 configuration: #{message}"
-    end
-  end
-
   @doc """
   Returns whether the update check is enabled.
   """
@@ -246,9 +182,10 @@ defmodule Livebook.Settings do
   @spec default_dir() :: FileSystem.File.t()
   def default_dir() do
     with {:ok, %{file_system_id: file_system_id, path: path}} <-
-           Storage.fetch_key(:settings, "global", :default_dir),
+           Storage.fetch_key(:settings, "global", :default_dir) do
-         {:ok, file_system} <- fetch_file_system(file_system_id) do
+      Livebook.Hubs.get_file_systems()
-      FileSystem.File.new(file_system, path)
+      |> Enum.find(&(&1.id == file_system_id))
+      |> FileSystem.File.new(path)
     else
       _ -> FileSystem.File.new(Livebook.Config.local_file_system())
     end

test/livebook/file_system/file_test.exs

@@ -1,6 +1,8 @@
 defmodule Livebook.FileSystem.FileTest do
   use ExUnit.Case, async: true
 
+  import Livebook.Factory
+  import Livebook.HubHelpers
   import Livebook.TestHelpers
 
   alias Livebook.FileSystem
@@ -271,7 +273,7 @@ defmodule Livebook.FileSystem.FileTest do
     test "supports regular files from different file systems via stream read and write",
          %{tmp_dir: tmp_dir} do
       bypass = Bypass.open()
-      s3_fs = FileSystem.S3.new("http://localhost:#{bypass.port}/mybucket", "key", "secret")
+      s3_fs = build_bypass_file_system(bypass)
       local_fs = FileSystem.Local.new()
 
       create_tree!(tmp_dir,
@@ -295,7 +297,7 @@ defmodule Livebook.FileSystem.FileTest do
     test "supports directories from different file systems via stream read and write",
          %{tmp_dir: tmp_dir} do
       bypass = Bypass.open()
-      s3_fs = FileSystem.S3.new("http://localhost:#{bypass.port}/mybucket", "key", "secret")
+      s3_fs = build_bypass_file_system(bypass)
       local_fs = FileSystem.Local.new()
 
       create_tree!(tmp_dir,
@@ -329,7 +331,7 @@ defmodule Livebook.FileSystem.FileTest do
     @tag :tmp_dir
     test "returns an error when files from different file systems are given and the destination file exists",
          %{tmp_dir: tmp_dir} do
-      s3_fs = FileSystem.S3.new("https://example.com/mybucket", "key", "secret")
+      s3_fs = build(:fs_s3, bucket_url: "https://example.com/mybucket")
       local_fs = FileSystem.Local.new()
 
       create_tree!(tmp_dir,
@@ -349,7 +351,7 @@ defmodule Livebook.FileSystem.FileTest do
     test "supports regular files from different file systems via explicit read, write, delete",
          %{tmp_dir: tmp_dir} do
       bypass = Bypass.open()
-      s3_fs = FileSystem.S3.new("http://localhost:#{bypass.port}/mybucket", "key", "secret")
+      s3_fs = build_bypass_file_system(bypass)
       local_fs = FileSystem.Local.new()
 
       create_tree!(tmp_dir,

test/livebook/file_system/s3_test.exs

@@ -11,25 +11,6 @@ defmodule Livebook.FileSystem.S3Test do
     {:ok, bypass: bypass, file_system: file_system}
   end
 
-  describe "new/3" do
-    test "trims trailing slash in bucket URL" do
-      assert %{bucket_url: "https://example.com/mybucket"} =
-               S3.new("https://example.com/mybucket/", "key", "secret")
-    end
-
-    test "determines region based on the URL by default" do
-      assert %{region: "eu-central-1"} =
-               S3.new("https://s3.eu-central-1.amazonaws.com/mybucket", "key", "secret")
-    end
-
-    test "accepts explicit region as an option" do
-      assert %{region: "auto"} =
-               S3.new("https://s3.eu-central-1.amazonaws.com/mybucket", "key", "secret",
-                 region: "auto"
-               )
-    end
-  end
-
   describe "FileSystem.default_path/1" do
     test "returns the root path" do
       file_system = build(:fs_s3, bucket_url: "https://example.com/mybucket", region: "auto")
@@ -988,66 +969,55 @@ defmodule Livebook.FileSystem.S3Test do
   end
 
   describe "FileSystem.load/2" do
-    test "loads region and external id from fields map" do
+    test "loads from atom keys" do
+      bucket_url = "https://mybucket.s3.amazonaws.com"
+      hash = :crypto.hash(:sha256, bucket_url)
+      encrypted_hash = Base.url_encode64(hash, padding: false)
+
       fields = %{
-        bucket_url: "https://mybucket.s3.amazonaws.com",
+        id: "s3-#{encrypted_hash}",
+        bucket_url: bucket_url,
         region: "us-east-1",
         external_id: "123456789",
         access_key_id: "key",
-        secret_access_key: "secret"
+        secret_access_key: "secret",
+        hub_id: "personal-hub"
       }
 
-      hash = :crypto.hash(:sha256, fields.bucket_url)
-      id = "s3-#{Base.url_encode64(hash, padding: false)}"
-
       assert FileSystem.load(%S3{}, fields) == %S3{
-               id: id,
+               id: fields.id,
                bucket_url: fields.bucket_url,
                external_id: fields.external_id,
                region: fields.region,
                access_key_id: fields.access_key_id,
-               secret_access_key: fields.secret_access_key
+               secret_access_key: fields.secret_access_key,
-             }
+               hub_id: fields.hub_id
-    end
-
-    test "loads region from bucket url" do
-      fields = %{
-        bucket_url: "https://mybucket.s3.us-east-1.amazonaws.com",
-        access_key_id: "key",
-        secret_access_key: "secret"
-      }
-
-      hash = :crypto.hash(:sha256, fields.bucket_url)
-
-      assert FileSystem.load(%S3{}, fields) == %S3{
-               id: "s3-#{Base.url_encode64(hash, padding: false)}",
-               bucket_url: fields.bucket_url,
-               external_id: nil,
-               region: "us-east-1",
-               access_key_id: fields.access_key_id,
-               secret_access_key: fields.secret_access_key
              }
     end
 
     test "loads from string keys" do
+      bucket_url = "https://mybucket.s3.amazonaws.com"
+      hash = :crypto.hash(:sha256, bucket_url)
+      encrypted_hash = Base.url_encode64(hash, padding: false)
+
       fields = %{
-        "bucket_url" => "https://mybucket.s3.amazonaws.com",
+        "id" => "s3-#{encrypted_hash}",
+        "bucket_url" => bucket_url,
         "region" => "us-east-1",
         "external_id" => "123456789",
         "access_key_id" => "key",
-        "secret_access_key" => "secret"
+        "secret_access_key" => "secret",
+        "hub_id" => "personal-hub"
       }
 
-      hash = :crypto.hash(:sha256, fields["bucket_url"])
-      id = "s3-#{Base.url_encode64(hash, padding: false)}"
-
       assert FileSystem.load(%S3{}, fields) == %S3{
-               id: id,
+               id: fields["id"],
                bucket_url: fields["bucket_url"],
                external_id: fields["external_id"],
                region: fields["region"],
                access_key_id: fields["access_key_id"],
-               secret_access_key: fields["secret_access_key"]
+               secret_access_key: fields["secret_access_key"],
+               hub_id: fields["hub_id"]
              }
     end
   end
@@ -1062,7 +1032,8 @@ defmodule Livebook.FileSystem.S3Test do
                region: "us-east-1",
                access_key_id: "key",
                secret_access_key: "secret",
-               hub_id: "personal-hub"
+               hub_id: "personal-hub",
+               external_id: nil
              }
     end
   end

test/livebook/session_test.exs

@@ -1,6 +1,7 @@
 defmodule Livebook.SessionTest do
   use ExUnit.Case, async: true
 
+  import Livebook.HubHelpers
   import Livebook.TestHelpers
 
   alias Livebook.{Session, Delta, Runtime, Utils, Notebook, FileSystem, Apps, App}
@@ -1653,8 +1654,8 @@ defmodule Livebook.SessionTest do
 
     test "when remote :file replies with the cached path" do
       bypass = Bypass.open()
-      bucket_url = "http://localhost:#{bypass.port}/mybucket"
+      s3_fs = build_bypass_file_system(bypass)
-      s3_fs = FileSystem.S3.new(bucket_url, "key", "secret")
+      bucket_url = s3_fs.bucket_url
 
       Bypass.expect_once(bypass, "GET", "/mybucket/image.jpg", fn conn ->
         Plug.Conn.resp(conn, 200, "content")

test/livebook_teams/web/hub/edit_live_test.exs

@@ -373,7 +373,7 @@ defmodule LivebookWeb.Integration.Hub.EditLiveTest do
   end
 
   defp expect_s3_listing(bypass) do
-    Bypass.expect_once(bypass, "GET", "/", fn conn ->
+    Bypass.expect_once(bypass, "GET", "/mybucket", fn conn ->
       conn
       |> Plug.Conn.put_resp_content_type("application/xml")
       |> Plug.Conn.resp(200, """

test/livebook_web/live/hub/edit_live_test.exs

@@ -276,7 +276,7 @@ defmodule LivebookWeb.Hub.EditLiveTest do
   end
 
   defp expect_s3_listing(bypass) do
-    Bypass.expect_once(bypass, "GET", "/", fn conn ->
+    Bypass.expect_once(bypass, "GET", "/mybucket", fn conn ->
       conn
       |> Plug.Conn.put_resp_content_type("application/xml")
       |> Plug.Conn.resp(200, """

test/support/hub_helpers.ex

@@ -105,8 +105,8 @@ defmodule Livebook.HubHelpers do
     erpc_call(node, :create_file_system, [[org_key: org_key]])
   end
 
-  def build_bypass_file_system(bypass, hub_id \\ nil) do
+  def build_bypass_file_system(bypass, hub_id \\ Livebook.Hubs.Personal.id()) do
-    bucket_url = "http://localhost:#{bypass.port}"
+    bucket_url = "http://localhost:#{bypass.port}/mybucket"
 
     file_system =
       build(:fs_s3,