Fix the S3 form to allow the user to enter a custom region (#2380)

lib/livebook/file_system/s3.ex

@@ -29,23 +29,6 @@ defmodule Livebook.FileSystem.S3 do
     field :hub_id, :string
   end
 
-  @doc """
-  Infers region from the given bucket URL.
-  """
-  @spec region_from_uri(String.t()) :: String.t()
-  # TODO: make it private again on Livebook v0.12
-  def 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()
-
-    case Enum.at(splitted_host, 2, "auto") do
-      "s3" -> "us-east-1"
-      "r2" -> "auto"
-      region -> region
-    end
-  end
-
   @doc """
   Returns an `%Ecto.Changeset{}` for tracking file system changes.
   """
@@ -64,20 +47,12 @@ defmodule Livebook.FileSystem.S3 do
       :secret_access_key,
       :hub_id
     ])
-    |> put_region_from_uri()
-    |> validate_required([:bucket_url, :region, :hub_id])
+    |> validate_required([:bucket_url, :hub_id])
     |> Livebook.Utils.validate_url(:bucket_url)
     |> validate_credentials()
     |> put_id()
   end
 
-  defp put_region_from_uri(changeset) do
-    case get_field(changeset, :bucket_url) do
-      nil -> changeset
-      bucket_url -> put_change(changeset, :region, region_from_uri(bucket_url))
-    end
-  end
-
   defp validate_credentials(changeset) do
     case {get_field(changeset, :access_key_id), get_field(changeset, :secret_access_key)} do
       {nil, nil} -> try_environment_credentials(changeset)
@@ -158,6 +133,38 @@ defmodule Livebook.FileSystem.S3 do
       :undefined
     end
   end
+
+  @doc """
+  Infers region from the given bucket URL.
+  """
+  @spec region_from_url(String.t()) :: String.t()
+  def region_from_url(url) do
+    # For many services the API host is of the form *.[region].[rootdomain].com
+    host = URI.parse(url).host || ""
+
+    splitted_host = host |> String.split(".") |> Enum.reverse()
+
+    case Enum.at(splitted_host, 2, "auto") do
+      "s3" -> "us-east-1"
+      "r2" -> "auto"
+      region -> region
+    end
+  end
+
+  @doc """
+  Sets region field on `changeset` based on bucket URL, unless already
+  specified.
+  """
+  @spec maybe_put_region_from_url(Ecto.Changeset.t()) :: Ecto.Changeset.t()
+  def maybe_put_region_from_url(changeset) do
+    case {get_field(changeset, :region), get_field(changeset, :bucket_url)} do
+      {nil, bucket_url} when bucket_url != nil ->
+        put_change(changeset, :region, region_from_url(bucket_url))
+
+      _ ->
+        changeset
+    end
+  end
 end
 
 defimpl Livebook.FileSystem, for: Livebook.FileSystem.S3 do

lib/livebook/migration.ex

@@ -113,7 +113,7 @@ defmodule Livebook.Migration do
             new_fields = %{
               hub_id: Livebook.Hubs.Personal.id(),
               external_id: nil,
-              region: Livebook.FileSystem.S3.region_from_uri(config.bucket_url)
+              region: Livebook.FileSystem.S3.region_from_url(config.bucket_url)
             }
 
             config = Map.merge(new_fields, config)
@@ -142,7 +142,7 @@ defmodule Livebook.Migration do
       new_attrs = %{
         hub_id: Livebook.Hubs.Personal.id(),
         external_id: nil,
-        region: Livebook.FileSystem.S3.region_from_uri(attrs.bucket_url)
+        region: Livebook.FileSystem.S3.region_from_url(attrs.bucket_url)
       }
 
       attrs = Map.merge(new_attrs, attrs)

lib/livebook_web/components/form_components.ex

@@ -532,6 +532,7 @@ defmodule LivebookWeb.FormComponents do
           data-show
           type="button"
           aria-label="show password"
+          tabindex="-1"
           phx-click={
             JS.remove_attribute("type", to: "##{@id} input")
             |> JS.set_attribute({"type", "text"}, to: "##{@id} input")
@@ -546,6 +547,7 @@ defmodule LivebookWeb.FormComponents do
           data-hide
           type="button"
           aria-label="hide password"
+          tabindex="-1"
           phx-click={
             JS.remove_attribute("type", to: "##{@id} input")
             |> JS.set_attribute({"type", "password"}, to: "##{@id} input")

lib/livebook_web/live/hub/file_system_form_component.ex

@@ -58,7 +58,13 @@ defmodule LivebookWeb.Hub.FileSystemFormComponent do
             label="Bucket URL"
             placeholder="https://s3.[region].amazonaws.com/[bucket]"
           />
-          <.text_field field={f[:region]} label="Region (optional)" />
+          <.text_field
+            field={f[:region]}
+            label="Region (optional)"
+            placeholder={
+              FileSystem.S3.region_from_url(Ecto.Changeset.get_field(@changeset, :bucket_url) || "")
+            }
+          />
           <%= if Livebook.Config.aws_credentials?() do %>
             <.password_field field={f[:access_key_id]} label="Access Key ID (optional)" />
             <.password_field field={f[:secret_access_key]} label="Secret Access Key (optional)" />
@@ -102,7 +108,10 @@ defmodule LivebookWeb.Hub.FileSystemFormComponent do
   end
 
   def handle_event("save", %{"file_system" => attrs}, socket) do
-    changeset = FileSystems.change_file_system(socket.assigns.file_system, attrs)
+    changeset =
+      socket.assigns.file_system
+      |> FileSystems.change_file_system(attrs)
+      |> FileSystem.S3.maybe_put_region_from_url()
 
     with {:ok, file_system} <- Ecto.Changeset.apply_action(changeset, :update),
          :ok <- check_file_system_connectivity(file_system),