From 7137a83562fa77f9022097da66a65b4c0a10dd3c Mon Sep 17 00:00:00 2001
From: Aske <aske.doerge@gmail.com>
Date: Wed, 27 Mar 2024 20:25:17 +0100
Subject: [PATCH] Fix defaults.

---
 lib/aws/client.ex        | 26 ++++++++++++++++++++------
 test/aws/client_test.exs | 31 +++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/lib/aws/client.ex b/lib/aws/client.ex
index 6418ade3..4d7049fb 100644
--- a/lib/aws/client.ex
+++ b/lib/aws/client.ex
@@ -195,6 +195,8 @@ defmodule AWS.Client do
   end
 
   @doc """
+  Makes a HTTP request using the specified client.
+
   `opts` may contain the keyword `:enable_retries?` that enables request retries on known
   errors such as 500s.
   The keyword `:retry_opts` can further supply the arguments: `:max_retries`, `:base_sleep_time` &
@@ -203,14 +205,28 @@ defmodule AWS.Client do
   `retry_opts: [max_retries: 10, base_sleep_time: 5, cap_sleep_time: 5_000]`
 
   See "FullJitter" at: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/
+
+  ### Examples
+
+      iex> AWS.Client.request(client, :post, url, payload, headers, options)
+      {:ok, %{status_code: 200, body: body}}
+
+      iex> AWS.Client.request(client, :post, url, payload, headers, enable_retries?: true)
+      {:ok, %{status_code: 200, body: body}}
   """
   def request(client, method, url, body, headers, opts \\ []) do
+    # Pop off all retry-related options from opts, so they aren't passed to the HTTP client.
     {enable_retries?, opts} = Keyword.pop(opts, :enable_retries?, false)
     {retry_num, opts} = Keyword.pop(opts, :retry_num, 0)
-    {retry_opts, opts} = Keyword.pop(opts, :retry_opts, nil)
+    {retry_opts, opts} = Keyword.pop(opts, :retry_opts, [])
+    # Defaults for retry_opts
+    retry_opts =
+      Keyword.merge([max_retries: 10, base_sleep_time: 5, cap_sleep_time: 5_000], retry_opts)
 
+    # HTTP Client options
     {mod, options} = Map.fetch!(client, :http_client)
     options = Keyword.merge(options, opts)
+
     resp = apply(mod, :request, [method, url, body, headers, options])
 
     retriable?(resp)
@@ -237,18 +253,16 @@ defmodule AWS.Client do
     end
   end
 
-  def should_retry?(_retry_num, nil), do: false
-
   def should_retry?(retry_num, retry_opts) do
-    max_retries = Keyword.get(retry_opts, :max_retries, 10)
+    max_retries = Keyword.fetch!(retry_opts, :max_retries)
 
     if retry_num >= max_retries do
       # The max-limit of retries has been reached. Give up.
       false
     else
       # Sleep and retry
-      base_sleep_time = Keyword.get(retry_opts, :base_sleep_time, 5)
-      cap_sleep_time = Keyword.get(retry_opts, :cap_sleep_time, 5_000)
+      base_sleep_time = Keyword.fetch!(retry_opts, :base_sleep_time)
+      cap_sleep_time = Keyword.fetch!(retry_opts, :cap_sleep_time)
 
       # This equivalent to "FullJitter" in https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/
       max_sleep_time =
diff --git a/test/aws/client_test.exs b/test/aws/client_test.exs
index 28848324..7a45d89a 100644
--- a/test/aws/client_test.exs
+++ b/test/aws/client_test.exs
@@ -75,7 +75,7 @@ defmodule AWS.ClientTest do
       Bypass.pass(bypass)
     end
 
-    test "retries failed requests, when retry_opts is given", %{client: client, bypass: bypass} do
+    test "retries failed requests", %{client: client, bypass: bypass} do
       # Start the Agent
       TestRetryCounter.start_link(0)
 
@@ -86,16 +86,39 @@ defmodule AWS.ClientTest do
         Plug.Conn.resp(conn, 500, "")
       end)
 
+      assert {:ok, %{status_code: 500}} =
+               AWS.Client.request(client, :get, "#{url(bypass)}timeout", "", [],
+                 # Enable retries
+                 enable_retries?: true
+               )
+
+      # Assert 1 request + 10 (default max_retries value) retries was made
+      assert TestRetryCounter.value() == 1 + 10
+    end
+
+    test "retries failed requests with retry_opts", %{client: client, bypass: bypass} do
+      # Start the Agent
+      TestRetryCounter.start_link(0)
+
+      Bypass.expect(bypass, "GET", "/timeout", fn conn ->
+        # Increase Agent counter
+        TestRetryCounter.increment()
+        # Return 500 so we force a retry from AWS.Client
+        Plug.Conn.resp(conn, 500, "")
+      end)
+
+      max_retries = 2
+
       assert {:ok, %{status_code: 500}} =
                AWS.Client.request(client, :get, "#{url(bypass)}timeout", "", [],
                  # Enable retries
                  enable_retries?: true,
                  # Set retry  options
-                 retry_opts: [max_retries: 3, base_sleep_time: 10, cap_sleep_time: 1000]
+                 retry_opts: [max_retries: max_retries, base_sleep_time: 10, cap_sleep_time: 1000]
                )
 
-      # Assert 1 request + 3 retries was made
-      assert TestRetryCounter.value() == 1 + 3
+      # Assert 1st request + N retries was made
+      assert TestRetryCounter.value() == 1 + max_retries
     end
   end