From 75264f8f1c5057ef62cf441f31a9a209ccaa9a30 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Tue, 20 Sep 2022 09:31:12 +0200 Subject: [PATCH] Treat non-200 HTTP responses as errors (#2243) * Tag non-200 HTTP responses as errors Co-authored-by: RobertJoonas * Send get_invoices/1 errors to Sentry Co-authored-by: RobertJoonas * Keep Google API module matching non-200 responses Co-authored-by: RobertJoonas * Make sure HTTPClient.Error.t() doesn't appear in the UI * Unify get_invoices/1 signature Co-authored-by: RobertJoonas Co-authored-by: RobertJoonas --- lib/plausible/billing/paddle_api.ex | 35 ++++++++++--------- lib/plausible/google/http.ex | 12 +++---- lib/plausible/http_client.ex | 32 +++++++++++++++-- .../templates/auth/user_settings.html.eex | 2 +- .../site/settings_search_console.html.eex | 2 +- test/plausible/http_client_test.exs | 20 +++++++++++ test/support/paddle_api_mock.ex | 29 +++++++-------- 7 files changed, 91 insertions(+), 41 deletions(-) diff --git a/lib/plausible/billing/paddle_api.ex b/lib/plausible/billing/paddle_api.ex index 92c3df552..824e742c1 100644 --- a/lib/plausible/billing/paddle_api.ex +++ b/lib/plausible/billing/paddle_api.ex @@ -89,6 +89,10 @@ defmodule Plausible.Billing.PaddleApi do end end + @spec get_invoices(Plausible.Billing.Subscription.t()) :: + {:ok, list()} + | {:error, :request_failed} + | {:error, :no_subscription} def get_invoices(nil), do: {:error, :no_subscription} def get_invoices(subscription) do @@ -103,28 +107,25 @@ defmodule Plausible.Billing.PaddleApi do to: Timex.shift(Timex.today(), days: 1) |> Timex.format!("{YYYY}-{0M}-{0D}") } - case HTTPClient.post(invoices_url(), @headers, params) do - {:ok, response} -> - body = Jason.decode!(response.body) + with {:ok, response} <- HTTPClient.post(invoices_url(), @headers, params), + {:ok, body} <- Jason.decode(response.body), + true <- Map.get(body, "success"), + [_ | _] = response <- Map.get(body, "response") do + Enum.sort(response, fn %{"payout_date" => d1}, %{"payout_date" => d2} -> + Date.compare(Date.from_iso8601!(d1), Date.from_iso8601!(d2)) == :gt + end) + |> Enum.take(12) + |> then(&{:ok, &1}) + else + error -> + Sentry.capture_message("Failed to retrieve invoices from Paddle", + extra: %{extra: error} + ) - if body["success"] && body["response"] != [] do - body["response"] |> last_12_invoices() - else - {:error, :request_failed} - end - - {:error, _reason} -> {:error, :request_failed} end end - defp last_12_invoices(invoice_list) do - Enum.sort(invoice_list, fn %{"payout_date" => d1}, %{"payout_date" => d2} -> - Date.compare(Date.from_iso8601!(d1), Date.from_iso8601!(d2)) == :gt - end) - |> Enum.take(12) - end - def checkout_domain() do case Application.get_env(:plausible, :environment) do "dev" -> "https://sandbox-checkout.paddle.com" diff --git a/lib/plausible/google/http.ex b/lib/plausible/google/http.ex index 5bfbfe775..009910a6b 100644 --- a/lib/plausible/google/http.ex +++ b/lib/plausible/google/http.ex @@ -138,7 +138,7 @@ defmodule Plausible.Google.HTTP do {:ok, %Finch.Response{body: body, status: 200}} -> {:ok, Jason.decode!(body)} - {:ok, %Finch.Response{body: body}} -> + {:error, %{reason: %Finch.Response{body: body}}} -> Sentry.capture_message("Error fetching Google view ID", extra: Jason.decode!(body)) {:error, body} @@ -174,16 +174,16 @@ defmodule Plausible.Google.HTTP do {:ok, %Finch.Response{body: body, status: 200}} -> {:ok, Jason.decode!(body)} - {:ok, %Finch.Response{body: body, status: 401}} -> + {:error, %{reason: %Finch.Response{body: body, status: 401}}} -> Sentry.capture_message("Error fetching Google queries", extra: Jason.decode!(body)) {:error, :invalid_credentials} - {:ok, %Finch.Response{body: body, status: 403}} -> + {:error, %{reason: %Finch.Response{body: body, status: 403}}} -> body = Jason.decode!(body) Sentry.capture_message("Error fetching Google queries", extra: body) {:error, get_in(body, ["error", "message"])} - {:ok, %Finch.Response{body: body}} -> + {:error, %{reason: %Finch.Response{body: body}}} -> Sentry.capture_message("Error fetching Google queries", extra: Jason.decode!(body)) {:error, :unknown} @@ -212,7 +212,7 @@ defmodule Plausible.Google.HTTP do {:ok, %Finch.Response{body: body, status: 200}} -> {:ok, Jason.decode!(body)} - {:ok, %Finch.Response{body: body, status: _non_http_200}} -> + {:error, %{reason: %Finch.Response{body: body, status: _non_http_200}}} -> body |> Jason.decode!() |> Map.get("error") @@ -261,7 +261,7 @@ defmodule Plausible.Google.HTTP do {:ok, date} - {:ok, %Finch.Response{body: body}} -> + {:error, %{reason: %Finch.Response{body: body}}} -> Sentry.capture_message("Error fetching Google view ID", extra: Jason.decode!(body)) {:error, body} diff --git a/lib/plausible/http_client.ex b/lib/plausible/http_client.ex index dcc8a9bb1..c65f5331d 100644 --- a/lib/plausible/http_client.ex +++ b/lib/plausible/http_client.ex @@ -9,10 +9,24 @@ defmodule Plausible.HTTPClient do URL encoding is invoked. """ + defmodule Non200Error do + defstruct reason: nil + + @type t :: %__MODULE__{reason: Finch.Response.t()} + + @spec new(Finch.Response.t()) :: t() + def new(%Finch.Response{status: status} = response) + when is_integer(status) and (status < 200 or status >= 300) do + %__MODULE__{reason: response} + end + end + @type url() :: Finch.Request.url() @type headers() :: Finch.Request.headers() @type params() :: Finch.Request.body() | map() - @type response() :: {:ok, Finch.Response.t()} | {:error, Mint.Types.error() | Finch.Error.t()} + @type response() :: + {:ok, Finch.Response.t()} + | {:error, Mint.Types.error() | Finch.Error.t() | Non200Error.t()} @doc """ Make a POST request @@ -35,7 +49,8 @@ defmodule Plausible.HTTPClient do method |> build_request(url, headers, params) - |> do_request + |> do_request() + |> tag_error() end defp build_request(method, url, headers, params) do @@ -69,4 +84,17 @@ defmodule Plausible.HTTPClient do {Jason.encode!(params), [{"content-type", "application/json"} | headers]} end end + + defp tag_error({:ok, %Finch.Response{status: status}} = ok) + when is_integer(status) and status >= 200 and status < 300 do + ok + end + + defp tag_error({:ok, %Finch.Response{status: _} = response}) do + {:error, Non200Error.new(response)} + end + + defp tag_error({:error, _} = error) do + error + end end diff --git a/lib/plausible_web/templates/auth/user_settings.html.eex b/lib/plausible_web/templates/auth/user_settings.html.eex index a8020f035..9d1fdc3a9 100644 --- a/lib/plausible_web/templates/auth/user_settings.html.eex +++ b/lib/plausible_web/templates/auth/user_settings.html.eex @@ -136,7 +136,7 @@

- <% invoice_list -> %> + <% {:ok, invoice_list} when is_list(invoice_list) -> %>

Invoices

diff --git a/lib/plausible_web/templates/site/settings_search_console.html.eex b/lib/plausible_web/templates/site/settings_search_console.html.eex index fb5502ede..f024a1b09 100644 --- a/lib/plausible_web/templates/site/settings_search_console.html.eex +++ b/lib/plausible_web/templates/site/settings_search_console.html.eex @@ -41,7 +41,7 @@ <%= if error == "invalid_grant" do %>

Invalid Grant error returned from Google. See here on how to fix it.

<% else %> -

<%= error %>

+

Something went wrong

<% end %> <% end %> <% else %> diff --git a/test/plausible/http_client_test.exs b/test/plausible/http_client_test.exs index 08b4d3e4e..b38c3dabc 100644 --- a/test/plausible/http_client_test.exs +++ b/test/plausible/http_client_test.exs @@ -102,6 +102,26 @@ defmodule Plausible.HTTPClientTest do HTTPClient.post(bypass_url(bypass, path: "/any"), headers_no_content_type, params) end + test "non-200 responses are tagged as errors", %{bypass: bypass} do + Bypass.expect_once(bypass, "GET", "/get", fn conn -> + Conn.resp(conn, 300, "oops") + end) + + assert {:error, + %HTTPClient.Non200Error{ + reason: %Finch.Response{status: 300, body: "oops"} + }} = HTTPClient.get(bypass_url(bypass, path: "/get")) + + Bypass.expect_once(bypass, "GET", "/get", fn conn -> + Conn.resp(conn, 400, "oops") + end) + + assert {:error, + %HTTPClient.Non200Error{ + reason: %Finch.Response{status: 400, body: "oops"} + }} = HTTPClient.get(bypass_url(bypass, path: "/get")) + end + defp bypass_url(bypass, opts) do port = bypass.port path = Keyword.get(opts, :path, "/") diff --git a/test/support/paddle_api_mock.ex b/test/support/paddle_api_mock.ex index 42ad51a37..e44cec0ae 100644 --- a/test/support/paddle_api_mock.ex +++ b/test/support/paddle_api_mock.ex @@ -34,20 +34,21 @@ defmodule Plausible.PaddleApi.Mock do {:error, :request_failed} _ -> - [ - %{ - "amount" => 11.11, - "currency" => "EUR", - "payout_date" => "2020-12-24", - "receipt_url" => "https://some-receipt-url.com" - }, - %{ - "amount" => 22, - "currency" => "USD", - "payout_date" => "2020-11-24", - "receipt_url" => "https://other-receipt-url.com" - } - ] + {:ok, + [ + %{ + "amount" => 11.11, + "currency" => "EUR", + "payout_date" => "2020-12-24", + "receipt_url" => "https://some-receipt-url.com" + }, + %{ + "amount" => 22, + "currency" => "USD", + "payout_date" => "2020-11-24", + "receipt_url" => "https://other-receipt-url.com" + } + ]} end end end