Repository: keathley/finch (elixir)

Issue:
Allow pool configuration urls to be provided as a binary?
Right now we force the user to mentally parse the url in to the `{scheme, host, port}` (not that it's very difficult), but maybe some users would prefer to configure their pools like:
```
pools: %{
  "https://hex.pm" => %{size: 10, count: 2}
}
```
instead of the current:
```
pools: %{
  {:https, "hex.pm", 443} => %{size: 10, count: 2}
}
```

Pull request:
Change pools to be configured using URLs instead of shp tuples
This resolves #7 

This is mostly a straightforward change. The most controversial decision is making `parse_and_normalize_url/1` return `{parsed_uri, shp}`. The reason I did this is to ensure that URLs are always parsed and normalized consistently, when being used to construct an `shp` tuple, but without requiring two parses when `Finch.request/5` is called.

I also raise an `ArgumentError` now when an invalid scheme is passed to `normalize_scheme/1`, so that we can return better errors on pool startup.

Patch:
diff --git a/lib/finch.ex b/lib/finch.ex
index 0254176..1e3adc5 100644
--- a/lib/finch.ex
+++ b/lib/finch.ex
@@ -26,21 +26,22 @@ defmodule Finch do
   Finch.request(MyFinch, :get, "https://hex.pm")
   ```
 
-  When using HTTP/1, Finch will parse the passed in url into a `{scheme, host, port}`
+  When using HTTP/1, Finch will parse the passed in URL into a `{scheme, host, port}`
   tuple, and maintain one or more connection pools for each `{scheme, host, port}` you
   interact with.
 
-  You can also configure a pool size and count to be used for each specific
-  `{scheme, host, port}`s that are known before starting Finch. See `Finch.start_link/1` for
-  configuration options.
+  You can also configure a pool size and count to be used for specific URLs that are
+  known before starting Finch. The passed URLs will be parsed into `{scheme, host, port}`,
+  and the corresponding pools will be started. See `Finch.start_link/1` for configuration
+  options.
 
   ```elixir
   children = [
     {Finch,
-     name: MyConfiguredFinch, 
+     name: MyConfiguredFinch,
      pools: %{
        :default => [size: 10],
-       {:https, "hex.pm", 443} => [size: 32, count: 8, backoff: [initial: 1, max: 30_000]]
+       "https://hex.pm" => [size: 32, count: 8, backoff: [initial: 1, max: 30_000]]
      }}
   ]
   ```
@@ -119,7 +120,8 @@ defmodule Finch do
 
     * `:pools` - Configuration for your pools. You can provide a `:default` catch-all
     configuration for any non specfied {scheme, host, port}, or configuration for any
-    specific {scheme, host, port}. See "Pool Configuration Options" below.
+    URL that will be parsed into a {scheme, host, port}. See "Pool Configuration Options"
+    below.
 
   ### Pool Configuration Options
   #{NimbleOptions.docs(@pool_config_schema)}
@@ -152,21 +154,39 @@ defmodule Finch do
   end
 
   def request(name, method, url, headers \\ [], body \\ nil, opts \\ []) do
-    uri = URI.parse(url)
-
-    req = %{
-      scheme: normalize_scheme(uri.scheme),
-      host: uri.host,
-      port: uri.port,
-      method: build_method(method),
-      path: uri.path || "/",
-      headers: headers,
-      body: body,
-      query: uri.query
-    }
+    with {:ok, uri} <- parse_and_normalize_url(url) do
+      req = %{
+        scheme: uri.scheme,
+        host: uri.host,
+        port: uri.port,
+        method: build_method(method),
+        path: uri.path,
+        headers: headers,
+        body: body,
+        query: uri.query
+      }
+
+      shp = {uri.scheme, uri.host, uri.port}
+
+      pool = PoolManager.get_pool(name, shp)
+      Pool.request(pool, req, opts)
+    end
+  end
 
-    pool = PoolManager.get_pool(name, req.scheme, req.host, req.port)
-    Pool.request(pool, req, opts)
+  defp parse_and_normalize_url(url) when is_binary(url) do
+    with parsed_uri <- URI.parse(url),
+         normalized_path <- parsed_uri.path || "/",
+         {:ok, scheme} <- normalize_scheme(parsed_uri.scheme) do
+      normalized_uri = %{
+        scheme: scheme,
+        host: parsed_uri.host,
+        port: parsed_uri.port,
+        path: normalized_path,
+        query: parsed_uri.query
+      }
+
+      {:ok, normalized_uri}
+    end
   end
 
   defp build_method(method) when method in @methods, do: method
@@ -177,8 +197,14 @@ defmodule Finch do
 
   defp normalize_scheme(scheme) do
     case scheme do
-      "https" -> :https
-      "http" -> :http
+      "https" ->
+        {:ok, :https}
+
+      "http" ->
+        {:ok, :http}
+
+      scheme ->
+        {:error, "invalid scheme #{inspect(scheme)}"}
     end
   end
 
@@ -186,16 +212,36 @@ defmodule Finch do
     {:ok, default} = NimbleOptions.validate([], @pool_config_schema)
 
     Enum.reduce(pools, %{default: valid_opts_to_map(default)}, fn {destination, opts}, acc ->
-      case cast_pool_opts(opts) do
-        {:ok, validated} ->
-          Map.put(acc, destination, validated)
-
+      with {:ok, valid_destination} <- cast_destination(destination),
+           {:ok, valid_pool_opts} <- cast_pool_opts(opts) do
+        Map.put(acc, valid_destination, valid_pool_opts)
+      else
         {:error, reason} ->
           raise ArgumentError, "got invalid configuration for pool #{destination}! #{reason}"
       end
     end)
   end
 
+  defp cast_destination(destination) do
+    case destination do
+      :default ->
+        {:ok, destination}
+
+      url when is_binary(url) ->
+        cast_binary_destination(url)
+
+      _ ->
+        {:error, "invalid destination"}
+    end
+  end
+
+  defp cast_binary_destination(url) when is_binary(url) do
+    with {:ok, uri} <- parse_and_normalize_url(url),
+         shp <- {uri.scheme, uri.host, uri.port} do
+      {:ok, shp}
+    end
+  end
+
   defp cast_pool_opts(opts) do
     with {:ok, valid} <- NimbleOptions.validate(opts, @pool_config_schema) do
       {:ok, valid_opts_to_map(valid)}
diff --git a/lib/finch/pool_manager.ex b/lib/finch/pool_manager.ex
index ad1947c..456b967 100644
--- a/lib/finch/pool_manager.ex
+++ b/lib/finch/pool_manager.ex
@@ -15,9 +15,7 @@ defmodule Finch.PoolManager do
     {:ok, config}
   end
 
-  def get_pool(registry_name, scheme, host, port) do
-    key = {scheme, host, port}
-
+  def get_pool(registry_name, {_scheme, _host, _port} = key) do
     case lookup_pool(registry_name, key) do
       pool when is_pid(pool) ->
         pool


Test patch:
diff --git a/test/finch_test.exs b/test/finch_test.exs
index 9f1a509..aeb103d 100644
--- a/test/finch_test.exs
+++ b/test/finch_test.exs
@@ -47,9 +47,16 @@ defmodule FinchTest do
         end)
 
       assert error.message =~ "got invalid configuration"
+
+      error =
+        assert_raise(ArgumentError, fn ->
+          Finch.start_link(name: MyFinch, pools: %{invalid: [count: 5, size: 5]})
+        end)
+
+      assert error.message =~ "invalid destination"
     end
 
-    test "specific scheme, host, port combos can be configurated independently and pools will be started automatically",
+    test "pools are started based on only the {scheme, host, port} of the URLs",
          %{bypass: bypass} do
       other_bypass = Bypass.open()
       default_bypass = Bypass.open()
@@ -58,8 +65,8 @@ defmodule FinchTest do
         {Finch,
          name: MyFinch,
          pools: %{
-           shp(bypass) => [count: 5, size: 5],
-           shp(other_bypass) => [count: 10, size: 10]
+           endpoint(bypass, "/some-path") => [count: 5, size: 5],
+           endpoint(other_bypass, "/some-other-path") => [count: 10, size: 10]
          }}
       )
 
@@ -70,6 +77,44 @@ defmodule FinchTest do
       assert get_pools(MyFinch, shp(default_bypass)) |> length() == 0
     end
 
+    test "pools with an invalid URL cannot be started" do
+      error =
+        assert_raise(ArgumentError, fn ->
+          Finch.start_link(
+            name: MyFinch,
+            pools: %{
+              "example.com" => [count: 5, size: 5]
+            }
+          )
+        end)
+
+      assert error.message =~ "invalid scheme nil"
+
+      error =
+        assert_raise(ArgumentError, fn ->
+          Finch.start_link(
+            name: MyFinch,
+            pools: %{
+              "example" => [count: 5, size: 5]
+            }
+          )
+        end)
+
+      assert error.message =~ "invalid scheme nil"
+
+      error =
+        assert_raise(ArgumentError, fn ->
+          Finch.start_link(
+            name: MyFinch,
+            pools: %{
+              ":443" => [count: 5, size: 5]
+            }
+          )
+        end)
+
+      assert error.message =~ "invalid scheme nil"
+    end
+
     test "impossible to accidentally start multiple pools when they are dynamically started", %{
       bypass: bypass
     } do
@@ -140,6 +185,14 @@ defmodule FinchTest do
                end)
     end
 
+    test "raises when requesting a URL with an invalid scheme" do
+      start_supervised({Finch, name: MyFinch})
+
+      assert {:error, error} = Finch.request(MyFinch, :get, "ftp://example.com")
+
+      assert error =~ "invalid scheme \"ftp\""
+    end
+
     test "properly handles connection: close", %{bypass: bypass} do
       start_supervised({Finch, name: MyFinch})