From f5f672ae57060018d027038852fbfe971d32cf3b Mon Sep 17 00:00:00 2001 From: MikaAK Date: Mon, 28 Aug 2023 17:22:14 -0700 Subject: [PATCH] fix: make sure we respect caching being enabled/disabled --- coveralls.json | 7 +++++ lib/request_cache.ex | 17 ++++-------- lib/request_cache/middleware.ex | 2 +- lib/request_cache/plug.ex | 33 ++++++++++++++---------- lib/request_cache/resolver_middleware.ex | 14 +++++++++- test/request_cache_absinthe_test.exs | 31 ++++++++++++++++++++++ test/request_cache_plug_test.exs | 31 +++++++++++++++++++++- 7 files changed, 106 insertions(+), 29 deletions(-) create mode 100644 coveralls.json diff --git a/coveralls.json b/coveralls.json new file mode 100644 index 0000000..3153247 --- /dev/null +++ b/coveralls.json @@ -0,0 +1,7 @@ +{ + "skip_files": ["lib/application.ex"], + "custom_stop_words": ["defdelegate"], + "terminal_options": {"file_column_width": 80}, + "coverage_options": {"treat_no_relevant_lines_as_covered": true, "minimum_coverage": 85} +} + diff --git a/lib/request_cache.ex b/lib/request_cache.ex index 5e8148d..29c5555 100644 --- a/lib/request_cache.ex +++ b/lib/request_cache.ex @@ -16,20 +16,13 @@ defmodule RequestCache do end end - if RequestCache.Application.dependency_found?(:absinthe) do - @spec store( - result :: any, - opts_or_ttl :: opts | pos_integer - ) :: {:middleware, module, RequestCache.ResolverMiddleware.opts} - def store(result, ttl) when is_integer(ttl) do - store(result, [ttl: ttl]) - end - - def store(result, opts) when is_list(opts) do + if RequestCache.Application.dependency_found?(:absinthe) and + RequestCache.Application.dependency_found?(:absinthe_plug) do + def store(result, opts_or_ttl) do if RequestCache.Config.enabled?() do - {:middleware, RequestCache.ResolverMiddleware, Keyword.put(opts, :value, result)} + RequestCache.ResolverMiddleware.store_result(result, opts_or_ttl) else - {:ok, result} + result end end diff --git a/lib/request_cache/middleware.ex b/lib/request_cache/middleware.ex index c821d7f..74afe2a 100644 --- a/lib/request_cache/middleware.ex +++ b/lib/request_cache/middleware.ex @@ -27,7 +27,7 @@ if absinthe_loaded? do context: Map.update!( resolution.context, RequestCache.Config.conn_private_key(), - &Keyword.put(&1, :request, opts) + &Keyword.merge(&1, [request: opts, cache_request?: true]) ) } else diff --git a/lib/request_cache/plug.ex b/lib/request_cache/plug.ex index 721edb1..8219b9e 100644 --- a/lib/request_cache/plug.ex +++ b/lib/request_cache/plug.ex @@ -19,6 +19,8 @@ defmodule RequestCache.Plug do @json_regex ~r/^(\[|\{)(.*|\n)*(\]|\})$/ @html_regex ~r//i + def request_cache_header, do: @request_cache_header + @impl Plug def init(opts), do: opts @@ -161,9 +163,21 @@ defmodule RequestCache.Plug do end defp enabled_for_request?(%Plug.Conn{private: private}) do - get_in(private, [conn_private_key(), :enabled?]) - || get_in(private, [:absinthe, :context, conn_private_key(), :enabled?]) - || [] + plug_present? = get_in(private, [conn_private_key(), :enabled?]) || + get_in(private, [:absinthe, :context, conn_private_key(), :enabled?]) + + marked_for_cache? = get_in(private, [conn_private_key(), :cache_request?]) || + get_in(private, [:absinthe, :context, conn_private_key(), :cache_request?]) + + if plug_present? do + Util.verbose_log("[RequestCache.Plug] Plug enabled for request") + end + + if marked_for_cache? do + Util.verbose_log("[RequestCache.Plug] Plug has been marked for cache") + end + + plug_present? && marked_for_cache? end defp conn_request(%Plug.Conn{private: private}) do @@ -191,7 +205,7 @@ defmodule RequestCache.Plug do Util.verbose_log("[RequestCache.Plug] Storing REST request in #{conn_private_key()}") Plug.Conn.put_private(conn, conn_private_key(), - enabled?: true, + cache_request?: true, request: opts ) else @@ -202,16 +216,7 @@ defmodule RequestCache.Plug do end def store_request(conn, ttl) when is_integer(ttl) do - if conn.private[conn_private_key()][:enabled?] do - Plug.Conn.put_private(conn, conn_private_key(), - enabled?: true, - request: [ttl: ttl] - ) - else - Util.log_cache_disabled_message() - - conn - end + store_request(conn, [ttl: ttl]) end defp conn_private_key do diff --git a/lib/request_cache/resolver_middleware.ex b/lib/request_cache/resolver_middleware.ex index 1e2ac53..85b82fa 100644 --- a/lib/request_cache/resolver_middleware.ex +++ b/lib/request_cache/resolver_middleware.ex @@ -18,7 +18,7 @@ if absinthe_loaded? do defp enable_cache_for_resolution(resolution, opts) do if resolution.context[RequestCache.Config.conn_private_key()][:enabled?] do - config = [request: Keyword.delete(opts, :value)] + config = [request: Keyword.delete(opts, :value), cache_request?: true] resolution = %{resolution | state: :resolved, @@ -43,5 +43,17 @@ if absinthe_loaded? do } end end + + @spec store_result( + result :: any, + opts_or_ttl :: opts | pos_integer + ) :: {:middleware, module, RequestCache.ResolverMiddleware.opts} + def store_result(result, ttl) when is_integer(ttl) do + store_result(result, [ttl: ttl]) + end + + def store_result(result, opts) when is_list(opts) do + {:middleware, RequestCache.ResolverMiddleware, Keyword.put(opts, :value, result)} + end end end diff --git a/test/request_cache_absinthe_test.exs b/test/request_cache_absinthe_test.exs index 5fb7029..24d33e4 100644 --- a/test/request_cache_absinthe_test.exs +++ b/test/request_cache_absinthe_test.exs @@ -16,6 +16,14 @@ defmodule RequestCacheAbsintheTest do end end + field :uncached_hello, :string do + resolve fn _, %{context: %{call_pid: pid}} -> + EnsureCalledOnlyOnce.call(pid) + + {:ok, "HelloUncached"} + end + end + field :hello_world, :string do middleware RequestCache.Middleware, ttl: :timer.seconds(100) @@ -66,6 +74,7 @@ defmodule RequestCacheAbsintheTest do @query "query Hello { hello }" @query_2 "query Hello2 { helloWorld }" @query_error "query HelloError { helloError }" + @uncached_query "query HelloUncached { uncachedHello }" setup do {:ok, pid} = EnsureCalledOnlyOnce.start_link() @@ -73,6 +82,26 @@ defmodule RequestCacheAbsintheTest do %{call_pid: pid} end + @tag capture_log: true + test "does not cache queries that don't ask for caching", %{call_pid: pid} do + assert %Plug.Conn{} = :get + |> conn(graphql_url(@uncached_query)) + |> RequestCache.Support.Utils.ensure_default_opts() + |> Absinthe.Plug.put_options(context: %{call_pid: pid}) + |> Router.call([]) + + assert_raise Plug.Conn.WrapperError, fn -> + conn = :get + |> conn(graphql_url(@uncached_query)) + |> RequestCache.Support.Utils.ensure_default_opts() + |> Absinthe.Plug.put_options(context: %{call_pid: pid}) + |> Router.call([]) + + assert [] === get_resp_header(conn, RequestCache.Plug.request_cache_header()) + end + end + + @tag capture_log: true test "allows you to use middleware before a resolver to cache the results of the request", %{call_pid: pid} do conn = :get |> conn(graphql_url(@query_2)) @@ -88,6 +117,7 @@ defmodule RequestCacheAbsintheTest do |> Map.get(:resp_body) end + @tag capture_log: true test "allows you to use &store/2 in a resolver to cache the results of the request", %{call_pid: pid} do conn = :get |> conn(graphql_url(@query)) @@ -103,6 +133,7 @@ defmodule RequestCacheAbsintheTest do |> Map.get(:resp_body) end + @tag capture_log: true test "throws an error when called twice without cache", %{call_pid: pid} do conn = :get |> conn(graphql_url(@query_error)) diff --git a/test/request_cache_plug_test.exs b/test/request_cache_plug_test.exs index 966a9a2..c41997b 100644 --- a/test/request_cache_plug_test.exs +++ b/test/request_cache_plug_test.exs @@ -14,6 +14,12 @@ defmodule RequestCachePlugTest do plug :match plug :dispatch + match "/my_uncached_route" do + EnsureCalledOnlyOnce.call(conn.private[:call_pid]) + + send_resp(conn, 200, Jason.encode!(%{test: Enum.random(1..100_000_000)})) + end + match "/my_route" do EnsureCalledOnlyOnce.call(conn.private[:call_pid]) @@ -96,6 +102,26 @@ defmodule RequestCachePlugTest do %{caller_pid: pid} end + @tag capture_log: true + test "does not cache routes that don't ask for caching", %{caller_pid: pid} do + assert %Plug.Conn{} = :get + |> conn("/my_uncached_route") + |> RequestCache.Support.Utils.ensure_default_opts() + |> put_private(:call_pid, pid) + |> Router.call([]) + + assert_raise Plug.Conn.WrapperError, fn -> + conn = %Plug.Conn{} = :get + |> conn("/my_uncached_route") + |> RequestCache.Support.Utils.ensure_default_opts() + |> put_private(:call_pid, pid) + |> Router.call([]) + + assert [] === get_resp_header(conn, RequestCache.Plug.request_cache_header()) + end + end + + @tag capture_log: true test "stops any plug from running if cache is found", %{caller_pid: pid} do assert %Plug.Conn{} = :get |> conn("/my_route") @@ -110,6 +136,7 @@ defmodule RequestCachePlugTest do |> RouterWithBreakingPlug.call([]) end + @tag capture_log: true test "stops any plug from running if cache using default ttl is found", %{caller_pid: pid} do assert %Plug.Conn{} = :get |> conn("/my_route_default_ttl") @@ -133,6 +160,7 @@ defmodule RequestCachePlugTest do end) =~ "RequestCache requested" end + @tag capture_log: true test "includes proper headers with when served from the cache", %{ caller_pid: pid } do @@ -162,10 +190,11 @@ defmodule RequestCachePlugTest do ] end + @tag capture_log: true test "allows for for custom content-type header and returns it when served from the cache", %{ caller_pid: pid } do - route = "/my_route/:param" + route = "/my_route/cache" assert %Plug.Conn{resp_headers: uncached_headers} = :get |> conn(route)