From 0dda0c263b78e7fdefab5d7245937f7f5193ec1d Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Wed, 21 Feb 2024 11:51:28 -0500 Subject: [PATCH 1/5] Create a simple cache This will prevent the need from retrieving the value for each and every call. This may not be overly important for env vars that are currently implemented. However, this could become a big cost saver for when you are retrieving your values from an external service. We also renamed the MyConfig.validate!/0 function to MyConfig.load!/0. You are now required to call `MyConfig.load!/0` before you try using your configuration as that is what will actually grab the values and store them into the cache. --- README.md | 2 +- lib/cache.ex | 22 ++++++ lib/cache/ets.ex | 40 ++++++++++ lib/provider.ex | 67 +++++++--------- lib/provider/application.ex | 15 ++++ mix.exs | 1 + test/cache/ets_test.exs | 33 ++++++++ test/provider_test.exs | 148 +++++++----------------------------- 8 files changed, 166 insertions(+), 162 deletions(-) create mode 100644 lib/cache.ex create mode 100644 lib/cache/ets.ex create mode 100644 lib/provider/application.ex create mode 100644 test/cache/ets_test.exs diff --git a/README.md b/README.md index e70e184..3d9f075 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ defmodule MySystem.Application do use Application def start(_type, _args) do - MySystem.Config.validate!() + MySystem.Config.load!() # ... end diff --git a/lib/cache.ex b/lib/cache.ex new file mode 100644 index 0000000..9877cb8 --- /dev/null +++ b/lib/cache.ex @@ -0,0 +1,22 @@ +defmodule Provider.Cache do + @moduledoc """ + Defines a behaviour for cache implementations to follow + """ + + @callback set(mod :: module(), key :: atom(), value :: term()) :: :ok + @callback get(mod :: module(), key :: atom()) :: {:ok, term()} | {:error, :not_found} + + @spec set(module(), atom(), term()) :: :ok + def set(mod, key, value) do + impl().set(mod, key, value) + end + + @spec get(module(), atom()) :: {:ok, term()} | {:error, :not_found} + def get(mod, key) do + impl().get(mod, key) + end + + defp impl do + Application.get_env(:provider, :cache) || Provider.Cache.ETS + end +end diff --git a/lib/cache/ets.ex b/lib/cache/ets.ex new file mode 100644 index 0000000..668dc6b --- /dev/null +++ b/lib/cache/ets.ex @@ -0,0 +1,40 @@ +defmodule Provider.Cache.ETS do + @moduledoc """ + An ets based cache implementation + """ + @behaviour Provider.Cache + + use GenServer + + @spec start_link(Keyword.t()) :: GenServer.server() + def start_link(_opts) do + GenServer.start_link(__MODULE__, :ok, name: __MODULE__) + end + + @impl Provider.Cache + def set(module, key, value) do + GenServer.call(__MODULE__, {:set, module, key, value}) + end + + @impl Provider.Cache + def get(module, key) do + case :ets.lookup(__MODULE__, {module, key}) do + [{{^module, ^key}, value}] -> {:ok, value} + [] -> {:error, :not_found} + end + end + + @impl GenServer + def init(:ok) do + state = :ets.new(__MODULE__, [:named_table]) + + {:ok, state} + end + + @impl GenServer + def handle_call({:set, module, key, value}, _from, state) do + :ets.insert(state, {{module, key}, value}) + + {:reply, :ok, state} + end +end diff --git a/lib/provider.ex b/lib/provider.ex index 65510ac..a7f4d8b 100644 --- a/lib/provider.ex +++ b/lib/provider.ex @@ -21,8 +21,7 @@ defmodule Provider do This will generate the following functions in the module: - - `fetch_all` - retrieves values of all parameters - - `validate!` - validates that all parameters are correctly provided + - `load!` - validates that all parameters are correctly provided and stores them in the cache - `db_host`, `db_name`, `db_pool_size`, ... - getter of each declared parameter ## Describing params @@ -130,22 +129,6 @@ defmodule Provider do end end - @doc "Retrieves a single parameter." - @spec fetch_one(source, param_name, param_spec) :: {:ok, value} | {:error, [String.t()]} - def fetch_one(source, param_name, param_spec) do - with {:ok, map} <- fetch_all(source, %{param_name => param_spec}), - do: {:ok, Map.fetch!(map, param_name)} - end - - @doc "Retrieves a single param, raising if the value is not available." - @spec fetch_one!(source, param_name, param_spec) :: value - def fetch_one!(source, param, param_spec) do - case fetch_one(source, param, param_spec) do - {:ok, value} -> value - {:error, errors} -> raise Enum.join(errors, ", ") - end - end - # ------------------------------------------------------------------------ # Private # ------------------------------------------------------------------------ @@ -199,22 +182,20 @@ defmodule Provider do |> Keyword.fetch!(:params) |> Enum.map(fn {name, spec} -> {name, quote(do: %{unquote_splicing(spec)})} end) - @doc "Retrieves all parameters." - @spec fetch_all :: {:ok, %{unquote_splicing(typespecs)}} | {:error, [String.t()]} - def fetch_all do - Provider.fetch_all( - unquote(Keyword.fetch!(spec, :source)), - - # quoted_params is itself a keyword list, so we need to convert it into a map - %{unquote_splicing(quoted_params)} - ) - end - - @doc "Validates all parameters, raising if some values are missing or invalid." - @spec validate!() :: :ok - def validate! do - with {:error, errors} <- fetch_all() do - raise "Following OS env var errors were found:\n#{Enum.join(Enum.sort(errors), "\n")}" + @doc "Loads and validates all parameters, raising if some values are missing or invalid." + @spec load!() :: :ok + def load! do + case Provider.fetch_all( + unquote(Keyword.fetch!(spec, :source)), + %{ + unquote_splicing(quoted_params) + } + ) do + {:ok, values} -> + Enum.each(values, fn {k, v} -> Provider.Cache.set(__MODULE__, k, v) end) + + {:error, errors} -> + raise "Following OS env var errors were found:\n#{Enum.join(Enum.sort(errors), "\n")}" end :ok @@ -229,11 +210,19 @@ defmodule Provider do # bug in credo spec check # credo:disable-for-next-line Credo.Check.Readability.Specs def unquote(param_name)() do - Provider.fetch_one!( - unquote(Keyword.fetch!(spec, :source)), - unquote(param_name), - unquote(param_spec) - ) + case Provider.Cache.get(__MODULE__, unquote(param_name)) do + {:ok, value} -> + value + + {:error, :not_found} -> + raise "#{unquote(Keyword.fetch!(spec, :source)).display_name(unquote(param_name))} is missing" + end + + # Provider.fetch_one!( + # unquote(Keyword.fetch!(spec, :source)), + # unquote(param_name), + # unquote(param_spec) + # ) end end ) diff --git a/lib/provider/application.ex b/lib/provider/application.ex new file mode 100644 index 0000000..5b1767b --- /dev/null +++ b/lib/provider/application.ex @@ -0,0 +1,15 @@ +defmodule Provider.Application do + @moduledoc false + + use Application + + @impl true + def start(_type, _args) do + children = [ + Provider.Cache.ETS + ] + + opts = [strategy: :one_for_one, name: Provider.Supervisor] + Supervisor.start_link(children, opts) + end +end diff --git a/mix.exs b/mix.exs index 7cdb46e..3d93ac2 100644 --- a/mix.exs +++ b/mix.exs @@ -21,6 +21,7 @@ defmodule Provider.MixProject do def application do [ + mod: {Provider.Application, []}, extra_applications: [:logger] ] end diff --git a/test/cache/ets_test.exs b/test/cache/ets_test.exs new file mode 100644 index 0000000..e474586 --- /dev/null +++ b/test/cache/ets_test.exs @@ -0,0 +1,33 @@ +defmodule Provider.Cache.ETSTest do + use ExUnit.Case, async: false + + alias Provider.Cache.ETS + + describe "get/2" do + test "returns an error tuple if not value is found" do + assert {:error, :not_found} = ETS.get(__MODULE__, :key_not_found) + end + end + + describe "set/3" do + test "retrieves the value after it has been set" do + assert :ok == ETS.set(__MODULE__, :set_success_1, "value") + assert :ok == ETS.set(__MODULE__, :set_success_2, 42) + assert :ok == ETS.set(__MODULE__, :set_success_3, true) + assert :ok == ETS.set(__MODULE__, :set_success_4, 3.14) + + assert {:ok, "value"} = ETS.get(__MODULE__, :set_success_1) + assert {:ok, 42} = ETS.get(__MODULE__, :set_success_2) + assert {:ok, true} = ETS.get(__MODULE__, :set_success_3) + assert {:ok, 3.14} = ETS.get(__MODULE__, :set_success_4) + end + + test "overwrites a given key" do + assert :ok == ETS.set(__MODULE__, :set_success_1, "value1") + assert {:ok, "value1"} = ETS.get(__MODULE__, :set_success_1) + + assert :ok == ETS.set(__MODULE__, :set_success_1, "value2") + assert {:ok, "value2"} = ETS.get(__MODULE__, :set_success_1) + end + end +end diff --git a/test/provider_test.exs b/test/provider_test.exs index d5656be..f00c2e7 100644 --- a/test/provider_test.exs +++ b/test/provider_test.exs @@ -1,99 +1,11 @@ defmodule ProviderTest do use ExUnit.Case, async: true alias Provider + alias ProviderTest.ProcDictCache alias ProviderTest.TestModule - describe "fetch_one" do - test "returns correct value" do - param = param_spec() - System.put_env(param.os_env_name, "some value") - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, "some value"} - end - - test "returns default value if OS env is not set" do - param = param_spec(default: "default value") - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == - {:ok, "default value"} - end - - test "ignores default value and returns OS env value if it's available" do - param = param_spec(default: "default value") - System.put_env(param.os_env_name, "os env value") - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == - {:ok, "os env value"} - end - - test "converts to integer" do - param = param_spec(type: :integer, default: 123) - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, 123} - - System.put_env(param.os_env_name, "456") - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, 456} - end - - test "converts to float" do - param = param_spec(type: :float, default: 3.14) - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, 3.14} - - System.put_env(param.os_env_name, "2.72") - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, 2.72} - end - - test "converts to boolean" do - param = param_spec(type: :boolean, default: true) - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, true} - - System.put_env(param.os_env_name, "false") - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == {:ok, false} - end - - test "reports error on missing value" do - param = param_spec() - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == - {:error, [error(param, "is missing")]} - end - - test "empty string is treated as a missing value" do - param = param_spec() - System.put_env(param.os_env_name, "") - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == - {:error, [error(param, "is missing")]} - end - - for type <- ~w/integer float boolean/a do - test "reports error on #{type} conversion" do - param = param_spec(type: unquote(type), default: 123) - System.put_env(param.os_env_name, "invalid value") - - assert Provider.fetch_one(Provider.SystemEnv, param.name, param.opts) == - {:error, [error(param, "is invalid")]} - end - end - end - - describe "fetch_one!" do - test "returns correct value" do - param = param_spec() - System.put_env(param.os_env_name, "some value") - assert Provider.fetch_one!(Provider.SystemEnv, param.name, param.opts) == "some value" - end - - test "returns default value if OS env is not set" do - param = param_spec() - - assert_raise( - RuntimeError, - "#{param.os_env_name} is missing", - fn -> Provider.fetch_one!(Provider.SystemEnv, param.name, param.opts) end - ) - end + setup_all do + Application.put_env(:provider, :cache, ProcDictCache) end describe "fetch_all" do @@ -130,45 +42,18 @@ defmodule ProviderTest do Enum.each(1..7, &System.delete_env("OPT_#{&1}")) end - test "fetch_all/0 succeeds for correct data" do - System.put_env("OPT_1", "qux") - System.put_env("OPT_2", "42") - System.put_env("OPT_6", "false") - System.put_env("OPT_7", "3.14") - - assert TestModule.fetch_all() == - {:ok, - %{ - opt_1: "qux", - opt_2: 42, - opt_3: "foo", - opt_4: "bar", - opt_5: "baz", - opt_6: false, - opt_7: 3.14 - }} - end - - test "fetch_all/0 returns errors for invalid data" do - assert TestModule.fetch_all() == - { - :error, - ["OPT_1 is missing", "OPT_2 is missing", "OPT_6 is missing", "OPT_7 is missing"] - } - end - - test "validate!/0 succeeds for correct data" do + test "load!/0 succeeds for correct data" do System.put_env("OPT_1", "some data") System.put_env("OPT_2", "42") System.put_env("OPT_6", "false") System.put_env("OPT_7", "3.14") - assert TestModule.validate!() == :ok + assert TestModule.load!() == :ok end - test "validate!/0 raises on error" do + test "load!/0 raises on error" do System.put_env("OPT_2", "foobar") - error = assert_raise RuntimeError, fn -> TestModule.validate!() end + error = assert_raise RuntimeError, fn -> TestModule.load!() end assert error.message =~ "OPT_1 is missing" assert error.message =~ "OPT_2 is invalid" assert error.message =~ "OPT_6 is missing" @@ -181,6 +66,8 @@ defmodule ProviderTest do System.put_env("OPT_6", "false") System.put_env("OPT_7", "3.14") + TestModule.load!() + assert TestModule.opt_1() == "some data" assert TestModule.opt_2() == 42 assert TestModule.opt_3() == "foo" @@ -251,4 +138,21 @@ defmodule ProviderTest do defp bar, do: "bar" end + + defmodule ProcDictCache do + @behaviour Provider.Cache + + @impl true + def set(mod, key, val) do + Process.put({mod, key}, val) + end + + @impl true + def get(mod, key) do + case Process.get({mod, key}, :undefined) do + :undefined -> {:error, :not_found} + v -> {:ok, v} + end + end + end end From a76bd351a5b149f0c2984e55c791fdf42bee2fa1 Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Tue, 4 Feb 2025 15:44:02 -0500 Subject: [PATCH 2/5] Remove cache polymorphism The polymorphism did not really add anything to the implementation and made it messier than it needed to be. This change also turned the config module into a GenServer that will start a named ETS table (the cache) and load all of the data from the source into the cache for quick retrieval. Currently, the only purpose of the GenServer is to own the ETS table. --- README.md | 4 +++- lib/cache.ex | 22 -------------------- lib/cache/ets.ex | 40 ------------------------------------- lib/provider.ex | 34 ++++++++++++++++++++++--------- lib/provider/application.ex | 15 -------------- lib/provider/cache.ex | 14 +++++++++++++ mix.exs | 1 - test/cache/ets_test.exs | 33 ------------------------------ test/provider_test.exs | 27 ++++++------------------- 9 files changed, 48 insertions(+), 142 deletions(-) delete mode 100644 lib/cache.ex delete mode 100644 lib/cache/ets.ex delete mode 100644 lib/provider/application.ex create mode 100644 lib/provider/cache.ex delete mode 100644 test/cache/ets_test.exs diff --git a/README.md b/README.md index 3d9f075..1e82af4 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,9 @@ defmodule MySystem.Application do use Application def start(_type, _args) do - MySystem.Config.load!() + children = [ + MySystem.Config + ] # ... end diff --git a/lib/cache.ex b/lib/cache.ex deleted file mode 100644 index 9877cb8..0000000 --- a/lib/cache.ex +++ /dev/null @@ -1,22 +0,0 @@ -defmodule Provider.Cache do - @moduledoc """ - Defines a behaviour for cache implementations to follow - """ - - @callback set(mod :: module(), key :: atom(), value :: term()) :: :ok - @callback get(mod :: module(), key :: atom()) :: {:ok, term()} | {:error, :not_found} - - @spec set(module(), atom(), term()) :: :ok - def set(mod, key, value) do - impl().set(mod, key, value) - end - - @spec get(module(), atom()) :: {:ok, term()} | {:error, :not_found} - def get(mod, key) do - impl().get(mod, key) - end - - defp impl do - Application.get_env(:provider, :cache) || Provider.Cache.ETS - end -end diff --git a/lib/cache/ets.ex b/lib/cache/ets.ex deleted file mode 100644 index 668dc6b..0000000 --- a/lib/cache/ets.ex +++ /dev/null @@ -1,40 +0,0 @@ -defmodule Provider.Cache.ETS do - @moduledoc """ - An ets based cache implementation - """ - @behaviour Provider.Cache - - use GenServer - - @spec start_link(Keyword.t()) :: GenServer.server() - def start_link(_opts) do - GenServer.start_link(__MODULE__, :ok, name: __MODULE__) - end - - @impl Provider.Cache - def set(module, key, value) do - GenServer.call(__MODULE__, {:set, module, key, value}) - end - - @impl Provider.Cache - def get(module, key) do - case :ets.lookup(__MODULE__, {module, key}) do - [{{^module, ^key}, value}] -> {:ok, value} - [] -> {:error, :not_found} - end - end - - @impl GenServer - def init(:ok) do - state = :ets.new(__MODULE__, [:named_table]) - - {:ok, state} - end - - @impl GenServer - def handle_call({:set, module, key, value}, _from, state) do - :ets.insert(state, {{module, key}, value}) - - {:reply, :ok, state} - end -end diff --git a/lib/provider.ex b/lib/provider.ex index a7f4d8b..c6b76c6 100644 --- a/lib/provider.ex +++ b/lib/provider.ex @@ -182,6 +182,28 @@ defmodule Provider do |> Keyword.fetch!(:params) |> Enum.map(fn {name, spec} -> {name, quote(do: %{unquote_splicing(spec)})} end) + use GenServer + + def start_link do + GenServer.start_link(__MODULE__, :ok, name: __MODULE__) + end + + @impl GenServer + def init(:ok) do + Provider.Cache.new(__MODULE__) + + load!() + + {:ok, nil} + end + + def child_spec(_arg) do + %{ + id: __MODULE__, + start: {__MODULE__, :start_link, []} + } + end + @doc "Loads and validates all parameters, raising if some values are missing or invalid." @spec load!() :: :ok def load! do @@ -192,13 +214,13 @@ defmodule Provider do } ) do {:ok, values} -> - Enum.each(values, fn {k, v} -> Provider.Cache.set(__MODULE__, k, v) end) + Provider.Cache.set(__MODULE__, Enum.to_list(values)) + + :ok {:error, errors} -> raise "Following OS env var errors were found:\n#{Enum.join(Enum.sort(errors), "\n")}" end - - :ok end # Generate getter for each param. @@ -217,12 +239,6 @@ defmodule Provider do {:error, :not_found} -> raise "#{unquote(Keyword.fetch!(spec, :source)).display_name(unquote(param_name))} is missing" end - - # Provider.fetch_one!( - # unquote(Keyword.fetch!(spec, :source)), - # unquote(param_name), - # unquote(param_spec) - # ) end end ) diff --git a/lib/provider/application.ex b/lib/provider/application.ex deleted file mode 100644 index 5b1767b..0000000 --- a/lib/provider/application.ex +++ /dev/null @@ -1,15 +0,0 @@ -defmodule Provider.Application do - @moduledoc false - - use Application - - @impl true - def start(_type, _args) do - children = [ - Provider.Cache.ETS - ] - - opts = [strategy: :one_for_one, name: Provider.Supervisor] - Supervisor.start_link(children, opts) - end -end diff --git a/lib/provider/cache.ex b/lib/provider/cache.ex new file mode 100644 index 0000000..2e83d5f --- /dev/null +++ b/lib/provider/cache.ex @@ -0,0 +1,14 @@ +defmodule Provider.Cache do + def new(name) do + :ets.new(name, [:named_table, {:read_concurrency, true}, :public]) + end + + def set(table, objects), do: :ets.insert(table, objects) + + def get(table, key) do + case :ets.lookup(table, key) do + [{^key, value}] -> {:ok, value} + [] -> {:error, :not_found} + end + end +end diff --git a/mix.exs b/mix.exs index 3d93ac2..7cdb46e 100644 --- a/mix.exs +++ b/mix.exs @@ -21,7 +21,6 @@ defmodule Provider.MixProject do def application do [ - mod: {Provider.Application, []}, extra_applications: [:logger] ] end diff --git a/test/cache/ets_test.exs b/test/cache/ets_test.exs deleted file mode 100644 index e474586..0000000 --- a/test/cache/ets_test.exs +++ /dev/null @@ -1,33 +0,0 @@ -defmodule Provider.Cache.ETSTest do - use ExUnit.Case, async: false - - alias Provider.Cache.ETS - - describe "get/2" do - test "returns an error tuple if not value is found" do - assert {:error, :not_found} = ETS.get(__MODULE__, :key_not_found) - end - end - - describe "set/3" do - test "retrieves the value after it has been set" do - assert :ok == ETS.set(__MODULE__, :set_success_1, "value") - assert :ok == ETS.set(__MODULE__, :set_success_2, 42) - assert :ok == ETS.set(__MODULE__, :set_success_3, true) - assert :ok == ETS.set(__MODULE__, :set_success_4, 3.14) - - assert {:ok, "value"} = ETS.get(__MODULE__, :set_success_1) - assert {:ok, 42} = ETS.get(__MODULE__, :set_success_2) - assert {:ok, true} = ETS.get(__MODULE__, :set_success_3) - assert {:ok, 3.14} = ETS.get(__MODULE__, :set_success_4) - end - - test "overwrites a given key" do - assert :ok == ETS.set(__MODULE__, :set_success_1, "value1") - assert {:ok, "value1"} = ETS.get(__MODULE__, :set_success_1) - - assert :ok == ETS.set(__MODULE__, :set_success_1, "value2") - assert {:ok, "value2"} = ETS.get(__MODULE__, :set_success_1) - end - end -end diff --git a/test/provider_test.exs b/test/provider_test.exs index f00c2e7..cb98489 100644 --- a/test/provider_test.exs +++ b/test/provider_test.exs @@ -48,7 +48,11 @@ defmodule ProviderTest do System.put_env("OPT_6", "false") System.put_env("OPT_7", "3.14") + {:ok, pid} = TestModule.start_link() + assert TestModule.load!() == :ok + + GenServer.stop(pid) end test "load!/0 raises on error" do @@ -66,7 +70,7 @@ defmodule ProviderTest do System.put_env("OPT_6", "false") System.put_env("OPT_7", "3.14") - TestModule.load!() + {:ok, pid} = TestModule.start_link() assert TestModule.opt_1() == "some data" assert TestModule.opt_2() == 42 @@ -75,10 +79,8 @@ defmodule ProviderTest do assert TestModule.opt_5() == "baz" assert TestModule.opt_6() == false assert TestModule.opt_7() == 3.14 - end - test "access function raises for on error" do - assert_raise RuntimeError, "OPT_1 is missing", fn -> TestModule.opt_1() end + GenServer.stop(pid) end test "template/0 generates config template" do @@ -138,21 +140,4 @@ defmodule ProviderTest do defp bar, do: "bar" end - - defmodule ProcDictCache do - @behaviour Provider.Cache - - @impl true - def set(mod, key, val) do - Process.put({mod, key}, val) - end - - @impl true - def get(mod, key) do - case Process.get({mod, key}, :undefined) do - :undefined -> {:error, :not_found} - v -> {:ok, v} - end - end - end end From 01f674d71ff11323d3b24cbe8e8e101b8bdea176 Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Wed, 5 Feb 2025 06:20:19 -0500 Subject: [PATCH 3/5] Use start_supervised! function in tests This is going to be more reliable that manually starting and stopping the GenServer. Mainly because if a test fails, the call to stop the GenServer will not be made, resulting in the server not stopping and potentially making other tests fail when they should not. --- lib/provider.ex | 17 ++++------------- test/provider_test.exs | 12 ++++++------ 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/lib/provider.ex b/lib/provider.ex index c6b76c6..e9b09fe 100644 --- a/lib/provider.ex +++ b/lib/provider.ex @@ -184,26 +184,17 @@ defmodule Provider do use GenServer - def start_link do - GenServer.start_link(__MODULE__, :ok, name: __MODULE__) + def start_link(arg) do + GenServer.start_link(__MODULE__, arg, name: __MODULE__) end @impl GenServer - def init(:ok) do + def init(_arg) do Provider.Cache.new(__MODULE__) - load!() - {:ok, nil} end - def child_spec(_arg) do - %{ - id: __MODULE__, - start: {__MODULE__, :start_link, []} - } - end - @doc "Loads and validates all parameters, raising if some values are missing or invalid." @spec load!() :: :ok def load! do @@ -214,7 +205,7 @@ defmodule Provider do } ) do {:ok, values} -> - Provider.Cache.set(__MODULE__, Enum.to_list(values)) + Provider.Cache.set(__MODULE__, Map.to_list(values)) :ok diff --git a/test/provider_test.exs b/test/provider_test.exs index cb98489..d092bff 100644 --- a/test/provider_test.exs +++ b/test/provider_test.exs @@ -48,11 +48,9 @@ defmodule ProviderTest do System.put_env("OPT_6", "false") System.put_env("OPT_7", "3.14") - {:ok, pid} = TestModule.start_link() + start_config_server!() assert TestModule.load!() == :ok - - GenServer.stop(pid) end test "load!/0 raises on error" do @@ -70,7 +68,7 @@ defmodule ProviderTest do System.put_env("OPT_6", "false") System.put_env("OPT_7", "3.14") - {:ok, pid} = TestModule.start_link() + start_config_server!() assert TestModule.opt_1() == "some data" assert TestModule.opt_2() == 42 @@ -79,8 +77,6 @@ defmodule ProviderTest do assert TestModule.opt_5() == "baz" assert TestModule.opt_6() == false assert TestModule.opt_7() == 3.14 - - GenServer.stop(pid) end test "template/0 generates config template" do @@ -119,6 +115,10 @@ defmodule ProviderTest do defp error(param, message), do: "#{param.os_env_name} #{message}" + defp start_config_server! do + start_supervised!(TestModule, restart: :temporary) + end + defmodule TestModule do baz = "baz" From 35b8a15384fff74e2da819142b6e50d47bb5068e Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Wed, 5 Feb 2025 11:06:10 -0500 Subject: [PATCH 4/5] Fix credo warnings When writing the spec for Provider.Cache.get/2, I realized that the error case is not something that should ever happen anymore. One of two things should happen when calling that function. Either, * The user forgot to start the GenServer, in which case ets will complains that the table does not exist; or * The user did start the GenServer, in which case the ets table exists and it will be initialized with the data from the configured source. And if you happen to not give a default to a given param and forget to configure it, the GenServer will fail to start because of it. --- lib/provider.ex | 9 ++------- lib/provider/cache.ex | 25 +++++++++++++++++++------ 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/provider.ex b/lib/provider.ex index e9b09fe..7f078a5 100644 --- a/lib/provider.ex +++ b/lib/provider.ex @@ -184,6 +184,7 @@ defmodule Provider do use GenServer + @spec start_link(term()) :: GenServer.on_start() def start_link(arg) do GenServer.start_link(__MODULE__, arg, name: __MODULE__) end @@ -223,13 +224,7 @@ defmodule Provider do # bug in credo spec check # credo:disable-for-next-line Credo.Check.Readability.Specs def unquote(param_name)() do - case Provider.Cache.get(__MODULE__, unquote(param_name)) do - {:ok, value} -> - value - - {:error, :not_found} -> - raise "#{unquote(Keyword.fetch!(spec, :source)).display_name(unquote(param_name))} is missing" - end + Provider.Cache.get(__MODULE__, unquote(param_name)) end end ) diff --git a/lib/provider/cache.ex b/lib/provider/cache.ex index 2e83d5f..c4c25b5 100644 --- a/lib/provider/cache.ex +++ b/lib/provider/cache.ex @@ -1,14 +1,27 @@ defmodule Provider.Cache do - def new(name) do + @moduledoc """ + This module will act as a cache to store the values in. This will prevent us + from needing to query the source on every call which could be expensive. + """ + + @spec new(atom()) :: :ok + def(new(name)) do :ets.new(name, [:named_table, {:read_concurrency, true}, :public]) + + :ok end - def set(table, objects), do: :ets.insert(table, objects) + @spec set(atom(), Keyword.list()) :: :ok + def set(table, objects) do + :ets.insert(table, objects) + :ok + end + + @spec get(atom(), atom()) :: term() def get(table, key) do - case :ets.lookup(table, key) do - [{^key, value}] -> {:ok, value} - [] -> {:error, :not_found} - end + [{^key, value}] = :ets.lookup(table, key) + + value end end From 4faf5db46a5930a71089ffaa6f27990b658418dd Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Wed, 5 Feb 2025 11:41:38 -0500 Subject: [PATCH 5/5] Clean up the cache interface --- lib/provider.ex | 7 ++++--- lib/provider/cache.ex | 27 --------------------------- 2 files changed, 4 insertions(+), 30 deletions(-) delete mode 100644 lib/provider/cache.ex diff --git a/lib/provider.ex b/lib/provider.ex index 7f078a5..425469a 100644 --- a/lib/provider.ex +++ b/lib/provider.ex @@ -191,7 +191,7 @@ defmodule Provider do @impl GenServer def init(_arg) do - Provider.Cache.new(__MODULE__) + :ets.new(__MODULE__, [:named_table, {:read_concurrency, true}, :public]) load!() {:ok, nil} end @@ -206,7 +206,7 @@ defmodule Provider do } ) do {:ok, values} -> - Provider.Cache.set(__MODULE__, Map.to_list(values)) + :ets.insert(__MODULE__, Map.to_list(values)) :ok @@ -224,7 +224,8 @@ defmodule Provider do # bug in credo spec check # credo:disable-for-next-line Credo.Check.Readability.Specs def unquote(param_name)() do - Provider.Cache.get(__MODULE__, unquote(param_name)) + [{unquote(param_name), value}] = :ets.lookup(__MODULE__, unquote(param_name)) + value end end ) diff --git a/lib/provider/cache.ex b/lib/provider/cache.ex deleted file mode 100644 index c4c25b5..0000000 --- a/lib/provider/cache.ex +++ /dev/null @@ -1,27 +0,0 @@ -defmodule Provider.Cache do - @moduledoc """ - This module will act as a cache to store the values in. This will prevent us - from needing to query the source on every call which could be expensive. - """ - - @spec new(atom()) :: :ok - def(new(name)) do - :ets.new(name, [:named_table, {:read_concurrency, true}, :public]) - - :ok - end - - @spec set(atom(), Keyword.list()) :: :ok - def set(table, objects) do - :ets.insert(table, objects) - - :ok - end - - @spec get(atom(), atom()) :: term() - def get(table, key) do - [{^key, value}] = :ets.lookup(table, key) - - value - end -end