From 522b3034aa17ae37b476c4dced47d583f4518cc5 Mon Sep 17 00:00:00 2001
From: Claudio Ortolina <cloud8421@gmail.com>
Date: Thu, 27 Feb 2025 08:58:24 +0000
Subject: [PATCH] Rewrite LastFm.APIImpl using Req

---
 lib/last_fm/api_impl.ex | 261 +++++++++++++++++++++-------------------
 1 file changed, 134 insertions(+), 127 deletions(-)

diff --git a/lib/last_fm/api_impl.ex b/lib/last_fm/api_impl.ex
index 7695b69b..47127f0f 100644
--- a/lib/last_fm/api_impl.ex
+++ b/lib/last_fm/api_impl.ex
@@ -5,164 +5,171 @@ defmodule LastFm.APIImpl do
 
   alias LastFm.{Artist, Track}
 
-  @base_url "https://ws.audioscrobbler.com/2.0/"
+  defmodule ErrorResponse do
+    defstruct [:error, :message]
 
-  # Experimental: metrics show some long running requests
-  # that end up hitting timeouts (at default values),
-  # so we make them shorter to leverage retries
-  @request_opts [
-    pool_timeout: 1000,
-    receive_timeout: 1000,
-    request_timeout: 2500
-  ]
+    def new(error_code, message) do
+      %__MODULE__{error: map_error(error_code), message: message}
+    end
+
+    defp map_error(2), do: :invalid_service
+    defp map_error(3), do: :invalid_method
+    defp map_error(4), do: :authentication_failed
+    defp map_error(5), do: :invalid_format
+    defp map_error(6), do: :invalid_parameters
+    defp map_error(7), do: :invalid_resource
+    defp map_error(8), do: :operation_failed
+    defp map_error(9), do: :invalid_session_key
+    defp map_error(10), do: :invalid_api_key
+    defp map_error(11), do: :service_offline
+    defp map_error(13), do: :invalid_method_signature
+    defp map_error(16), do: :transient_error
+    defp map_error(26), do: :suspended_api_key
+    defp map_error(29), do: :rate_limit_exceeded
+  end
 
   @impl true
   def get_recent_tracks(config) do
-    options = [
-      method: "user.getrecenttracks",
-      user: config.user,
-      api_key: config.api_key,
-      format: "json",
-      limit: 50
-    ]
+    params =
+      config
+      |> base_params()
+      |> Keyword.merge(method: "user.getrecenttracks", limit: 50)
 
-    url = @base_url <> "?" <> URI.encode_query(options)
-
-    Logger.debug("Fetching data from #{sanitize_url(url, config.api_key)}")
-
-    case json_get(url, config.user_agent) do
-      {:ok, response} ->
-        {:ok,
-         response
-         |> get_in(["recenttracks", "track"])
-         |> Track.from_api_response()}
-
-      other ->
-        msg =
-          "Failed to fetch data from #{sanitize_url(url, config.api_key)}, reason: #{inspect(other)}"
-
-        Logger.error(msg)
-        {:error, msg}
-    end
+    config
+    |> new_request()
+    |> Req.merge(url: "/", params: params)
+    |> Req.Request.append_response_steps(parse_tracks: &parse_tracks/1)
+    |> get_request()
   end
 
   @impl true
-  def get_artist_info({:musicbrainz_id, artist_mbid}, config) do
-    do_get_artist_info([mbid: artist_mbid], config)
-  end
+  def get_artist_info(id_or_name_option, config) do
+    params =
+      config
+      |> base_params()
+      |> Keyword.merge(method: "artist.getInfo")
+      |> put_musicbrainz_id_or_name(id_or_name_option)
 
-  def get_artist_info({:name, artist_name}, config) do
-    do_get_artist_info([artist: artist_name], config)
-  end
-
-  defp do_get_artist_info(options, config) do
-    base_options = [
-      method: "artist.getInfo",
-      api_key: config.api_key,
-      user: config.user,
-      format: "json",
-      limit: 50
-    ]
-
-    options = Keyword.merge(base_options, options)
-
-    url = @base_url <> "?" <> URI.encode_query(options)
-
-    Logger.debug("Fetching data from #{sanitize_url(url, config.api_key)}")
-
-    case json_get(url, config.user_agent) do
-      {:ok, response} ->
-        {:ok,
-         response
-         |> Map.get("artist")
-         |> Artist.from_api_response()}
-
-      error ->
-        msg =
-          "Failed to fetch data from #{sanitize_url(url, config.api_key)}, reason: #{inspect(error)}"
-
-        Logger.error(msg)
-        error
-    end
+    config
+    |> new_request()
+    |> Req.merge(url: "/", params: params)
+    |> Req.Request.append_response_steps(parse_tracks: &parse_artist/1)
+    |> get_request()
   end
 
   @impl true
-  def get_similar_artists({:name, artist_name}, config) do
-    do_get_similar_artists([artist: artist_name], config)
+  def get_similar_artists(id_or_name_option, config) do
+    params =
+      config
+      |> base_params()
+      |> Keyword.merge(limit: 100, method: "artist.getSimilar")
+      |> put_musicbrainz_id_or_name(id_or_name_option)
+
+    config
+    |> new_request()
+    |> Req.merge(url: "/", params: params)
+    |> Req.Request.append_response_steps(parse_tracks: &parse_similar_artists/1)
+    |> get_request()
   end
 
-  def get_similar_artists({:musicbrainz_id, artist_mbid}, config) do
-    do_get_similar_artists([mbid: artist_mbid], config)
+  defp put_musicbrainz_id_or_name(params, {:musicbrainz_id, musicbrainz_id}) do
+    Keyword.put(params, :mbid, musicbrainz_id)
   end
 
-  defp do_get_similar_artists(options, config) do
-    base_options = [
-      method: "artist.getSimilar",
+  defp put_musicbrainz_id_or_name(params, {:name, name}) do
+    Keyword.put(params, :artist, name)
+  end
+
+  defp base_params(config) do
+    [
+      user: config.user,
       api_key: config.api_key,
-      format: "json",
-      limit: 100
+      format: "json"
     ]
+  end
 
-    options = Keyword.merge(base_options, options)
+  defp new_request(config) do
+    Req.new(
+      base_url: "https://ws.audioscrobbler.com/2.0/",
+      # Experimental: metrics show some long running requests
+      # that end up hitting timeouts (at default values),
+      # so we make them shorter to leverage retries
+      max_retries: 1,
+      pool_timeout: 1000,
+      receive_timeout: 1000,
+      connect_options: [
+        timeout: 2500
+      ],
+      user_agent: config.user_agent
+    )
+    |> Req.Request.put_private(:api_key, config.api_key)
+    |> Req.Request.append_request_steps(log_attempt: &log_attempt/1)
+    |> Req.Request.append_response_steps(parse_error: &parse_error/1)
+  end
 
-    url = @base_url <> "?" <> URI.encode_query(options)
+  defp get_request(request) do
+    case Req.get(request) do
+      {:ok, %{body: %ErrorResponse{} = error_response}} ->
+        {:error, error_response.error}
 
-    Logger.debug("Fetching data from #{sanitize_url(url, config.api_key)}")
-
-    case json_get(url, config.user_agent) do
       {:ok, response} ->
-        {:ok,
-         response
-         |> get_in(["similarartists", "artist"])
-         |> Enum.map(&Artist.from_api_response/1)}
-
-      error ->
-        msg =
-          "Failed to fetch data from #{sanitize_url(url, config.api_key)}, reason: #{inspect(error)}"
-
-        Logger.error(msg)
-        error
+        {:ok, response.body}
     end
   end
 
-  defp json_get(url, user_agent) do
-    req =
-      Finch.build(:get, url, [
-        {"User-Agent", user_agent}
-      ])
+  defp log_attempt(request) do
+    url = URI.to_string(request.url)
+    api_key = Req.Request.get_private(request, :api_key)
+    Logger.debug("Fetching data from #{sanitize_url(url, api_key)}")
+    request
+  end
 
-    case Finch.request(req, LastFm.Finch, @request_opts) do
-      {:ok, response} when response.status == 200 ->
-        JSON.decode(response.body)
-        |> identify_body()
+  defp parse_error({request, response}) do
+    case response.body do
+      %{"error" => error_number, "message" => message} ->
+        error = ErrorResponse.new(error_number, message)
 
-      other ->
-        other
+        Logger.error(fn ->
+          url = URI.to_string(request.url)
+          api_key = Req.Request.get_private(request, :api_key)
+          "Failed to fetch data from #{sanitize_url(url, api_key)}, reason: #{message}."
+        end)
+
+        Req.Request.halt(request, Map.put(response, :body, error))
+
+      _other ->
+        {request, response}
     end
   end
 
+  defp parse_tracks({request, response}) do
+    tracks =
+      response.body
+      |> get_in(["recenttracks", "track"])
+      |> Track.from_api_response()
+
+    {request, Map.put(response, :body, tracks)}
+  end
+
+  defp parse_artist({request, response}) do
+    artist =
+      response.body
+      |> Map.get("artist")
+      |> Artist.from_api_response()
+
+    {request, Map.put(response, :body, artist)}
+  end
+
+  defp parse_similar_artists({request, response}) do
+    artists =
+      response.body
+      |> get_in(["similarartists", "artist"])
+      |> Enum.map(&Artist.from_api_response/1)
+
+    {request, Map.put(response, :body, artists)}
+  end
+
   defp sanitize_url(url, api_key) do
     String.replace(url, api_key, "<redacted_api_key>")
   end
-
-  defp identify_body({:ok, %{"error" => error_number, "message" => _message}}) do
-    {:error, map_error(error_number)}
-  end
-
-  defp identify_body(other), do: other
-
-  defp map_error(2), do: :invalid_service
-  defp map_error(3), do: :invalid_method
-  defp map_error(4), do: :authentication_failed
-  defp map_error(5), do: :invalid_format
-  defp map_error(6), do: :invalid_parameters
-  defp map_error(7), do: :invalid_resource
-  defp map_error(8), do: :operation_failed
-  defp map_error(9), do: :invalid_session_key
-  defp map_error(10), do: :invalid_api_key
-  defp map_error(11), do: :service_offline
-  defp map_error(13), do: :invalid_method_signature
-  defp map_error(16), do: :transient_error
-  defp map_error(26), do: :suspended_api_key
-  defp map_error(29), do: :rate_limit_exceeded
 end