diff --git a/lib/routes/api/v1.ex b/lib/routes/api/v1.ex index 0306831..d567d54 100644 --- a/lib/routes/api/v1.ex +++ b/lib/routes/api/v1.ex @@ -182,8 +182,8 @@ defmodule Api.V1 do end @spec determine_svg_url(map()) :: binary | nil - defp determine_svg_url(%{svg_url: svg_url}), do: svg_url - defp determine_svg_url(%{url: url}), do: url + defp determine_svg_url(%{svg_url: svg_url}) when is_binary(svg_url), do: svg_url + defp determine_svg_url(%{url: url}) when is_binary(url), do: url defp determine_svg_url(_params), do: nil @spec validate_svg_content(map(), binary()) :: String.t() | {:error, {integer(), String.t()}} @@ -336,7 +336,7 @@ defmodule Api.V1 do conn |> set_headers() |> send_resp(200, svg) {:error, {code, msg}} -> - Logger.error("Error generating SVG: #{inspect({code, msg})}") + Logger.warning("Error generating response: #{inspect({code, msg})}") conn |> json_response(code, %{status: code, message: msg}) @@ -364,7 +364,7 @@ defmodule Api.V1 do conn |> handle_res_type(params.res, params, recent_track) else {:error, res} -> - Logger.notice("Error: #{res.reason}") + Logger.warning("Failed to fetch valid upstream response: #{res.reason}") case res.code do 404 -> diff --git a/lib/utils.ex b/lib/utils.ex index b78c520..1dec495 100644 --- a/lib/utils.ex +++ b/lib/utils.ex @@ -63,70 +63,126 @@ defmodule Toru.Utils do "http://ws.audioscrobbler.com/2.0/?method=user.getrecenttracks&user=#{username |> URI.encode_www_form()}&api_key=#{Toru.Env.get!(:lfm_token)}&format=json&limit=2" end - @spec fetch_res(String.t()) :: + @spec fetch_res(String.t(), atom()) :: {:error, %{:code => integer(), :reason => String.t()}} | {:ok, map()} - def fetch_res(url) do - with {:ok, value} <- Cache.get(url) do - {:ok, value} - else - _ -> - http_client = Application.get_env(:toru, :http_client, Toru.DefaultHTTPClient) - - case http_client.get(url) do - {:ok, %HTTPoison.Response{status_code: 200, body: body}} -> - value = Poison.decode!(body) - Cache.put(url, value, 30) + def fetch_res(url, cache \\ :cache) do + try do + case cache do + :cache -> + with {:ok, value} <- Cache.get(url) do {:ok, value} + else + _ -> make_http_request(url) + end - {:ok, %HTTPoison.Response{status_code: 404}} -> - {:error, %{:code => 404, :reason => "User not found"}} - - {:ok, %HTTPoison.Response{status_code: 400}} -> - {:error, %{:code => 400, :reason => "Invalid request"}} - - {:ok, %HTTPoison.Response{status_code: 403}} -> - {:error, %{:code => 403, :reason => "Invalid API key"}} - - {:ok, %HTTPoison.Response{status_code: 429}} -> - {:error, %{:code => 429, :reason => "Rate limit exceeded"}} - - {:error, %HTTPoison.Error{reason: reason}} -> - {:error, %{:code => 500, :reason => reason}} - - _ -> - {:error, %{:code => 500, :reason => "Unknown error"}} - end + :no_cache -> + make_http_request(url) + end + rescue + _ -> + {:error, %{:code => 500, :reason => "Unknown error fetching data"}} end end - def fetch_res(url, :no_cache) do + defp make_http_request(url) do http_client = Application.get_env(:toru, :http_client, Toru.DefaultHTTPClient) case http_client.get(url) do {:ok, %HTTPoison.Response{status_code: 200, body: body}} -> value = Poison.decode!(body) + Cache.put(url, value, 30) {:ok, value} - {:ok, %HTTPoison.Response{status_code: 404}} -> - {:error, %{:code => 404, :reason => "User not found"}} - - {:ok, %HTTPoison.Response{status_code: 400}} -> - {:error, %{:code => 400, :reason => "Invalid request"}} - - {:ok, %HTTPoison.Response{status_code: 403}} -> - {:error, %{:code => 403, :reason => "Invalid API key"}} - - {:ok, %HTTPoison.Response{status_code: 429}} -> - {:error, %{:code => 429, :reason => "Rate limit exceeded"}} + {:ok, %HTTPoison.Response{status_code: code}} when code in [400, 403, 404, 429] -> + {:error, %{:code => code, :reason => get_error_reason(code)}} {:error, %HTTPoison.Error{reason: reason}} -> {:error, %{:code => 500, :reason => reason}} _ -> - {:error, %{:code => 500, :reason => "Unknown error"}} + {:error, %{:code => 500, :reason => "Unknown upstream error"}} end end + defp get_error_reason(400), do: "Invalid request" + defp get_error_reason(403), do: "Invalid API key" + defp get_error_reason(404), do: "User not found" + defp get_error_reason(429), do: "Rate limit exceeded" + + # @spec fetch_res(String.t()) :: + # {:error, %{:code => integer(), :reason => String.t()}} | {:ok, map()} + # def fetch_res(url) do + # try do + # with {:ok, value} <- Cache.get(url) do + # {:ok, value} + # else + # _ -> + # http_client = Application.get_env(:toru, :http_client, Toru.DefaultHTTPClient) + + # case http_client.get(url) do + # {:ok, %HTTPoison.Response{status_code: 200, body: body}} -> + # value = Poison.decode!(body) + # Cache.put(url, value, 30) + # {:ok, value} + + # {:ok, %HTTPoison.Response{status_code: 404}} -> + # {:error, %{:code => 404, :reason => "User not found"}} + + # {:ok, %HTTPoison.Response{status_code: 400}} -> + # {:error, %{:code => 400, :reason => "Invalid request"}} + + # {:ok, %HTTPoison.Response{status_code: 403}} -> + # {:error, %{:code => 403, :reason => "Invalid API key"}} + + # {:ok, %HTTPoison.Response{status_code: 429}} -> + # {:error, %{:code => 429, :reason => "Rate limit exceeded"}} + + # {:error, %HTTPoison.Error{reason: reason}} -> + # {:error, %{:code => 500, :reason => reason}} + + # _ -> + # {:error, %{:code => 500, :reason => "Unknown error"}} + # end + # end + # rescue + # _ -> + # {:error, %{:code => 500, :reason => "Unknown error fetching data"}} + # end + # end + + # def fetch_res(url, :no_cache) do + # try do + # http_client = Application.get_env(:toru, :http_client, Toru.DefaultHTTPClient) + + # case http_client.get(url) do + # {:ok, %HTTPoison.Response{status_code: 200, body: body}} -> + # value = Poison.decode!(body) + # {:ok, value} + + # {:ok, %HTTPoison.Response{status_code: 404}} -> + # {:error, %{:code => 404, :reason => "User not found"}} + + # {:ok, %HTTPoison.Response{status_code: 400}} -> + # {:error, %{:code => 400, :reason => "Invalid request"}} + + # {:ok, %HTTPoison.Response{status_code: 403}} -> + # {:error, %{:code => 403, :reason => "Invalid API key"}} + + # {:ok, %HTTPoison.Response{status_code: 429}} -> + # {:error, %{:code => 429, :reason => "Rate limit exceeded"}} + + # {:error, %HTTPoison.Error{reason: reason}} -> + # {:error, %{:code => 500, :reason => reason}} + + # _ -> + # {:error, %{:code => 500, :reason => "Unknown error"}} + # end + # rescue + # _ -> + # {:error, %{:code => 500, :reason => "Unknown error fetching data"}} + # end + # end + @spec playing_indicator(boolean()) :: String.t() def playing_indicator(playing) do case playing do diff --git a/test/toru_test.exs b/test/toru_test.exs index 00270a1..5b58e02 100644 --- a/test/toru_test.exs +++ b/test/toru_test.exs @@ -2,6 +2,7 @@ defmodule ToruApplicationTest do use ExUnit.Case use Plug.Test + import ExUnit.CaptureLog import Mox doctest Toru.Application @@ -60,19 +61,20 @@ defmodule ToruApplicationTest do body = conn.resp_body - # Check for strings within the SVG for now assert String.contains?(body, "Far Out") assert String.contains?(body, "Stay With Me - Extended Mix") end - test "Filling in provided (valid) SVG" do + test "Filling in provided SVG - via 'svg_url' param" do mock_lfm_response = %HTTPoison.Response{ body: Poison.encode!(stub_json()), status_code: 200 } mock_svg_response = %HTTPoison.Response{ - body: "${artist}", + body: """ + ${artist} + """, status_code: 200, headers: [{"content-type", "image/svg+xml"}] } @@ -105,17 +107,21 @@ defmodule ToruApplicationTest do body = conn.resp_body assert String.contains?(body, "Far Out") + assert String.contains?(body, "data-testattr=\"true\"") end - test "Filling in provided (invalid) svg" do + test "Filling in provided SVG - via 'url' param" do mock_lfm_response = %HTTPoison.Response{ body: Poison.encode!(stub_json()), status_code: 200 } mock_svg_response = %HTTPoison.Response{ - body: "", - status_code: 200 + body: """ + ${artist} + """, + status_code: 200, + headers: [{"content-type", "image/svg+xml"}] } Toru.MockHTTPClient @@ -137,18 +143,73 @@ defmodule ToruApplicationTest do ) conn = - conn(:get, "/api/v1/kiosion?svg_url=https://example.com") + conn(:get, "/api/v1/kiosion?url=https://example.com") |> Toru.Router.call(@opts) + assert conn.state == :sent + assert conn.status == 200 + + body = conn.resp_body + + assert String.contains?(body, "Far Out") + assert String.contains?(body, "data-testattr=\"true\"") + end + + test "Filling in provided svg - invalid asset" do + mock_lfm_response = %HTTPoison.Response{ + body: Poison.encode!(stub_json()), + status_code: 200 + } + + mock_svg_response = %HTTPoison.Response{ + body: "", + status_code: 200 + } + + Toru.MockHTTPClient + |> Mox.expect( + :get, + 3, + fn url -> + cond do + String.contains?(url, "format=json") -> + {:ok, mock_lfm_response} + + String.contains?(url, "example.com") -> + {:ok, mock_svg_response} + + true -> + {:ok, %HTTPoison.Response{body: "", status_code: 200}} + end + end + ) + + log_output = + capture_log(fn -> + conn = + conn(:get, "/api/v1/kiosion?svg_url=https://example.com") + |> Toru.Router.call(@opts) + + Process.put(:temp_conn, conn) + end) + + conn = Process.get(:temp_conn) + Process.delete(:temp_conn) + assert conn.state == :sent assert conn.status == 415 body = Poison.decode!(conn.resp_body) + assert Regex.match?( + ~r/\[warning\].*Provided SVG resource is not of type image\/svg\+xml/, + log_output + ) + assert body["message"] == "Provided SVG resource is not of type image/svg+xml" end - test "GET json res from API" do + test "GET JSON response" do mock_response = %HTTPoison.Response{ body: Poison.encode!(stub_json()), status_code: 200 @@ -195,6 +256,52 @@ defmodule ToruApplicationTest do assert body == expected end + test "GET JSON response - invalid upstream data" do + mock_invalid_json_response = %HTTPoison.Response{ + body: "{ invalid: JSON }", + status_code: 200 + } + + Toru.MockHTTPClient + |> Mox.expect( + :get, + 1, + fn url -> + if String.contains?(url, "format=json") do + {:ok, mock_invalid_json_response} + else + {:ok, %HTTPoison.Response{body: "", status_code: 200}} + end + end + ) + + log_output = + capture_log(fn -> + conn = + conn(:get, "/api/v1/kiosion?res=json") + |> Toru.Router.call(@opts) + + Process.put(:temp_conn, conn) + end) + + conn = Process.get(:temp_conn) + Process.delete(:temp_conn) + + assert conn.state == :sent + assert conn.status == 500 + + expected_error = %{ + "status" => 500, + "message" => "Internal Error", + "detail" => "Unknown error fetching data" + } + + body = Poison.decode!(conn.resp_body) + + assert body == expected_error + assert Regex.match?(~r/\[warning\].*Failed to fetch valid upstream response/, log_output) + end + defp stub_json do %{ :recenttracks => %{