Make region and bucket as optional and always set the endpoint (#2)

* Move `:bucket` from S3 entry to config

* Parse config from bucket URL

* Revert "Parse config from bucket URL"

This reverts commit eb4f39f.

* Make region and bucket as optionals, and set endpoint

These are changes necessary to support the usage when only the bucket
url is known.

* Better document the endpoint field for S3

Also make that attribute required in the typespecs.

* Fix parsing of s3:// URL to consider empty bucket

This is going to treat the endpoint differently depending on
bucket name. It is also going to make `:bucket` nil if we can
build a virtual-host style endpoint.

The rules are more or less the following:

* Endpoint is given
  * And bucket is an empty string (s3:///path): keep bucket nil
  * Otherwise keep both endpoint and bucket in config
* Endpoint is nil
  * Bucket has dots: keep the endpoint _without_ the bucket, fill in bucket field
  * Bucket does not have dots: keep the endpoint with bucket, bucket is nil

lib/fss/s3.ex

@@ -10,6 +10,7 @@ defmodule FSS.S3 do
 
     defstruct [
       :access_key_id,
+      :bucket,
       :region,
       :secret_access_key,
       :endpoint,
@@ -22,18 +23,40 @@ defmodule FSS.S3 do
     The attributes are:
 
     * `:access_key_id` - This attribute is required.
-    * `:region` - This attribute is required.
+
     * `:secret_access_key` - This attribute is required.
-    * `:endpoint` - This attribute is optional. If specified, then `:region` is ignored.
-      This attribute is useful for when you are using a service that is compatible with
-      the AWS S3 API.
+
+    * `:bucket` - A valid bucket name. This attribute is optional,
+      but if is not provided, then the `:endpoint` must include the bucket name
+      either in the host, as a virtual host, or in the path. In other words:
+      if the bucket is not given, then `:endpoint` must be configured.
+
+      This attribute is going to be set to `nil` if the endpoint was
+      not provided, unless the bucket name contain dots in it.
+
+    * `:region` - This attribute is optional. It's normally required when working
+      with the official AWS S3 API.
+
+    * `:endpoint` - If specified, then `:region` is ignored.
+      This attribute is required to be configured if you are using a service that
+      is compatible with the AWS S3 API.
+
+      In case only a "bucket URL" - without discrimination of the bucket name - is provided
+      then the `:bucket` attribute can be nil just like the `:region`.
+
+      In case the endpoint is not provided, we compute a valid one for the AWS S3 API.
+      This endpoint is going to follow the virtual-host style most of the time, with
+      the only exception being when the bucket name has dots. In that case we build
+      the AWS S3 endpoint without the bucket name in it.
+
     * `:token` - This attribute is optional.
     """
     @type t :: %__MODULE__{
             access_key_id: String.t(),
-            region: String.t(),
             secret_access_key: String.t(),
-            endpoint: String.t() | nil,
+            endpoint: String.t(),
+            bucket: String.t() | nil,
+            region: String.t() | nil,
             token: String.t() | nil
           }
   end
@@ -43,19 +66,17 @@ defmodule FSS.S3 do
     Represents the S3 resource itself.
     """
 
-    defstruct [:bucket, :key, :config]
+    defstruct [:key, :config]
 
     @typedoc """
     The entry struct for S3.
 
     The attributes are:
 
-    * `:bucket` - A valid bucket name. This attribute is required.
     * `:key` - A valid key for the resource. This attribute is required.
     * `:config` - A valid S3 config from the type `Config.t()`. This attribute is required.
     """
     @type t :: %__MODULE__{
-            bucket: String.t(),
             key: String.t(),
             config: Config.t()
           }
@@ -77,6 +98,13 @@ defmodule FSS.S3 do
       - `AWS_SECRET_ACCESS_KEY`
       - `AWS_REGION` or `AWS_DEFAULT_REGION`
       - `AWS_SESSION_TOKEN`
+
+      In case the endpoint is not provided, we compute a valid one for the AWS S3 API,
+      That is going to follow the path style. The endpoint is not going to include the
+      `:bucket` in it, being necessary to do that when using this FSS entry.
+
+      See https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-bucket-intro.html
+      for more details.
   """
   @spec parse(String.t(), Keyword.t()) :: {:ok, Entry.t()} | {:error, Exception.t()}
   def parse(url, opts \\ []) do
@@ -86,26 +114,38 @@ defmodule FSS.S3 do
 
     case uri do
       %{scheme: "s3", host: bucket, path: "/" <> key} when is_binary(bucket) ->
+        bucket = if bucket != "", do: bucket
+
         config =
           opts
           |> Keyword.fetch!(:config)
-          |> case do
-            nil ->
-              config_from_system_env()
-
-            %Config{} = config ->
-              config
-
-            config when is_list(config) or is_map(config) ->
-              struct!(config_from_system_env(), config)
-
-            other ->
-              raise ArgumentError,
-                    "expect configuration to be a %FSS.S3.Config{} struct, a keyword list or a map. Instead got #{inspect(other)}"
-          end
+          |> normalize_config!()
           |> validate_config!()
+          |> then(fn %Config{} = config ->
+            config = %Config{config | bucket: bucket}
+
+            if is_nil(config.endpoint) and not is_nil(bucket) do
+              s3_host_suffix = "s3." <> config.region <> ".amazonaws.com"
+
+              # We consume the bucket name in the endpoint if there is no dots in it.
+              {endpoint, bucket} =
+                if String.contains?(bucket, ".") do
+                  {"https://" <> s3_host_suffix, bucket}
+                else
+                  {"https://" <> bucket <> "." <> s3_host_suffix, nil}
+                end
+
+              %Config{config | endpoint: endpoint, bucket: bucket}
+            else
+              config
+            end
+          end)
 
-        {:ok, %Entry{bucket: bucket, key: key, config: config}}
+        if is_nil(config.endpoint) do
+          {:error, ArgumentError.exception("endpoint is required when bucket is nil")}
+        else
+          {:ok, %Entry{key: key, config: config}}
+        end
 
       _ ->
         {:error,
@@ -124,6 +164,18 @@ defmodule FSS.S3 do
     config
   end
 
+  defp normalize_config!(nil), do: config_from_system_env()
+  defp normalize_config!(%Config{} = config), do: config
+
+  defp normalize_config!(config) when is_list(config) or is_map(config) do
+    struct!(config_from_system_env(), config)
+  end
+
+  defp normalize_config!(other) do
+    raise ArgumentError,
+          "expect configuration to be a %FSS.S3.Config{} struct, a keyword list or a map. Instead got #{inspect(other)}"
+  end
+
   defp check!(config, key, env) do
     if Map.fetch!(config, key) in ["", nil] do
       raise ArgumentError,

test/fss/s3_test.exs

@@ -17,15 +17,57 @@ defmodule FSS.S3Test do
     end
 
     test "parses a s3:// style uri", %{config: config} do
-      assert {:ok, %Entry{bucket: "my-bucket", key: "my-file.png", config: %Config{} = config}} =
+      assert {:ok, %Entry{key: "my-file.png", config: %Config{} = config}} =
                S3.parse("s3://my-bucket/my-file.png", config: config)
 
-      assert is_nil(config.endpoint)
+      assert config.endpoint == "https://my-bucket.s3.us-west-2.amazonaws.com"
+      assert config.bucket == nil
       assert config.secret_access_key == "my-secret"
       assert config.access_key_id == "my-access"
       assert config.region == "us-west-2"
     end
 
+    test "parses a s3:// style uri with bucket name containing dots", %{config: config} do
+      assert {:ok, %Entry{key: "my-file.png", config: %Config{} = config}} =
+               S3.parse("s3://my.bucket.with.dots/my-file.png", config: config)
+
+      assert config.endpoint == "https://s3.us-west-2.amazonaws.com"
+      assert config.bucket == "my.bucket.with.dots"
+      assert config.secret_access_key == "my-secret"
+      assert config.access_key_id == "my-access"
+      assert config.region == "us-west-2"
+    end
+
+    test "parses a s3:// style uri without bucket name but passing endpoint", %{config: config} do
+      assert {:ok, %Entry{key: "my-file.png", config: %Config{} = config}} =
+               S3.parse("s3:///my-file.png",
+                 config: %{
+                   config
+                   | endpoint: "https://my-custom-endpoint.example.com/my-custom-bucket"
+                 }
+               )
+
+      assert config.endpoint == "https://my-custom-endpoint.example.com/my-custom-bucket"
+      assert config.bucket == nil
+      assert config.secret_access_key == "my-secret"
+      assert config.access_key_id == "my-access"
+      assert config.region == "us-west-2"
+    end
+
+    test "does not parse a s3:// style uri without bucket and without an endpoint", %{
+      config: config
+    } do
+      assert {:error, error} =
+               S3.parse("s3:///my-file.png",
+                 config: %{
+                   config
+                   | endpoint: nil
+                 }
+               )
+
+      assert error == ArgumentError.exception("endpoint is required when bucket is nil")
+    end
+
     test "accepts a config as a keyword list" do
       assert {:ok, %Entry{config: %Config{} = config}} =
                S3.parse("s3://my-bucket/my-file.png",
@@ -38,6 +80,7 @@ defmodule FSS.S3Test do
                )
 
       assert config.endpoint == "localhost"
+      assert config.bucket == "my-bucket"
       assert config.secret_access_key == "my-secret-1"
       assert config.access_key_id == "my-access-key-1"
       assert config.region == "eu-east-1"
@@ -50,11 +93,14 @@ defmodule FSS.S3Test do
                    endpoint: "localhost",
                    secret_access_key: "my-secret-1",
                    access_key_id: "my-access-key-1",
+                   # We always ignore bucket from config.
+                   bucket: "random-name",
                    region: "eu-east-1"
                  }
                )
 
       assert config.endpoint == "localhost"
+      assert config.bucket == "my-bucket"
       assert config.secret_access_key == "my-secret-1"
       assert config.access_key_id == "my-access-key-1"
       assert config.region == "eu-east-1"