fix: validate & log invalid JSON response

Author: jcartwright

lib/postscript/request.ex

@@ -1,5 +1,7 @@
 defmodule Postscript.Request do
-  alias Postscript.{ Config, Helpers, Operation, Response }
+  require Logger
+
+  alias Postscript.{Config, Helpers, Operation, Response}
 
   @type t ::
           %__MODULE__{
@@ -10,20 +12,18 @@ defmodule Postscript.Request do
             url: String.t()
           }
 
-  defstruct [
-    body: nil,
-    headers: [],
-    method: nil,
-    private: %{},
-    url: nil
-  ]
+  defstruct body: nil,
+            headers: [],
+            method: nil,
+            private: %{},
+            url: nil
 
   @spec new(Operation.t(), Config.t()) :: t
   def new(operation, config) do
     body = Helpers.Body.encode!(operation, config)
 
     headers = []
-    headers = headers ++ [{ "content-type", "application/json" }]
+    headers = headers ++ [{"content-type", "application/json"}]
     headers = headers ++ config.http_headers
 
     url = Helpers.Url.to_string(operation, config)
@@ -45,19 +45,47 @@ defmodule Postscript.Request do
 
     request
     |> config.http_client.send(config.http_client_opts)
+    |> maybe_validate_json_decode(config)
     |> retry(request, config)
     |> finish(config)
   end
 
-  defp retry(response, _request, %_{ retry: retry }) when is_nil(retry) or retry == false do
+  defp maybe_validate_json_decode({:ok, %{body: body, headers: headers}} = response, config) do
+    headers
+    |> Enum.reduce(%{}, fn {k, v}, acc -> Map.put(acc, String.downcase(k), v) end)
+    |> case do
+      %{"content-type" => "application/json"} ->
+        case config.json_codec.decode(body) do
+          {:ok, _decoded} ->
+            response
+
+          {:error, decode_error} ->
+            Logger.warning([
+              inspect(__MODULE__),
+              " received an invalid JSON response ",
+              inspect(body)
+            ])
+
+            {:error, decode_error}
+        end
+
+      _otherwise ->
+        response
+    end
+  end
+
+  defp maybe_validate_json_decode(response, _config), do: response
+
+  defp retry(response, _request, %_{retry: retry}) when is_nil(retry) or retry == false do
     response
   end
 
-  defp retry({ :ok, %{ status_code: status_code } } = response, request, config) when status_code >= 500 do
+  defp retry({:ok, %{status_code: status_code}} = response, request, config)
+       when status_code >= 500 do
     do_retry(response, request, config)
   end
 
-  defp retry({ :error, _ } = response, request, config) do
+  defp retry({:error, _} = response, request, config) do
     do_retry(response, request, config)
   end
 
@@ -89,10 +117,12 @@ defmodule Postscript.Request do
 
   defp finish(response, config) do
     case response do
-      { :ok, %{ status_code: status_code } = response } when status_code >= 400 ->
-        { :error, Response.new(response, config) }
-      { :ok, %{ status_code: status_code } = response } when status_code >= 200 ->
-        { :ok, Response.new(response, config) }
+      {:ok, %{status_code: status_code} = response} when status_code >= 400 ->
+        {:error, Response.new(response, config)}
+
+      {:ok, %{status_code: status_code} = response} when status_code >= 200 ->
+        {:ok, Response.new(response, config)}
+
       otherwise ->
         otherwise
     end

lib/postscript/response.ex

@@ -1,5 +1,5 @@
 defmodule Postscript.Response do
-  alias Postscript.{ Config, Http }
+  alias Postscript.{Config, Http}
 
   @type t ::
           %__MODULE__{
@@ -15,7 +15,11 @@ defmodule Postscript.Response do
     body =
       response
       |> Map.get(:body)
-      |> config.json_codec.decode!()
+      |> config.json_codec.decode()
+      |> case do
+        {:ok, json} -> json
+        {:error, _decode_error} -> Map.get(response, :body)
+      end
 
     %__MODULE__{}
     |> Map.put(:body, body)

test/postscript_test.exs

@@ -1,20 +1,28 @@
 defmodule PostscriptTest do
   use ExUnit.Case, async: true
 
-  alias Postscript.{ Http, Operation, Response }
+  import ExUnit.CaptureLog, only: [with_log: 2]
 
-  @ok_resp %{ body: "{\"ok\":true}", headers: [], status_code: 200 }
+  alias Postscript.{Http, Operation, Response}
 
-  @not_ok_resp %{ body: "{\"ok\":false}", headers: [], status_code: 400 }
+  @ok_resp %{body: "{\"ok\":true}", headers: [], status_code: 200}
+
+  @not_ok_resp %{body: "{\"ok\":false}", headers: [], status_code: 400}
+
+  @not_json_resp %{
+    body: "not json",
+    headers: [{"content-type", "application/json"}],
+    status_code: 200
+  }
 
   test "sends the proper HTTP method" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :get, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :get, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -24,11 +32,11 @@ defmodule PostscriptTest do
   test "uses the proper URL for GET requests" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :get, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :get, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -38,11 +46,11 @@ defmodule PostscriptTest do
   test "uses the proper URL for DELETE requests" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :delete, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :delete, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -52,11 +60,11 @@ defmodule PostscriptTest do
   test "uses the proper URL for non-GET requests" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -66,31 +74,36 @@ defmodule PostscriptTest do
   test "sends the proper HTTP headers" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
     operation = %Operation{}
     operation = Map.put(operation, :method, :get)
-    operation = Map.put(operation, :params, [hello: "world"])
+    operation = Map.put(operation, :params, hello: "world")
     operation = Map.put(operation, :path, "/fake")
 
-    Postscript.request(operation, api_key: "thisisfake", http_client: Http.Mock, http_headers: [{ "x-custom-header", "true" }], shop_token: "thisisfake")
-
-    assert { "content-type", "application/json" } in Http.Mock.get_request_headers()
-    assert { "authorization", "Bearer thisisfake" } in Http.Mock.get_request_headers()
-    assert { "x-custom-header", "true" } in Http.Mock.get_request_headers()
-    assert { "x-postscript-shop-token", "thisisfake" } in Http.Mock.get_request_headers()
+    Postscript.request(operation,
+      api_key: "thisisfake",
+      http_client: Http.Mock,
+      http_headers: [{"x-custom-header", "true"}],
+      shop_token: "thisisfake"
+    )
+
+    assert {"content-type", "application/json"} in Http.Mock.get_request_headers()
+    assert {"authorization", "Bearer thisisfake"} in Http.Mock.get_request_headers()
+    assert {"x-custom-header", "true"} in Http.Mock.get_request_headers()
+    assert {"x-postscript-shop-token", "thisisfake"} in Http.Mock.get_request_headers()
   end
 
   test "sends the proper body for GET requests" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :get, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :get, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -100,11 +113,11 @@ defmodule PostscriptTest do
   test "sends the proper body for DELETE requests" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :delete, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :delete, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -114,11 +127,11 @@ defmodule PostscriptTest do
   test "sends the proper body for non-GET requests" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock)
 
@@ -128,39 +141,59 @@ defmodule PostscriptTest do
   test "returns :ok when the request is successful" do
     Http.Mock.start_link()
 
-    response = { :ok, @ok_resp }
+    response = {:ok, @ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     result = Postscript.request(operation, http_client: Http.Mock)
 
-    assert { :ok, %Response{} } = result
+    assert {:ok, %Response{}} = result
   end
 
   test "returns :error when the request is not successful" do
     Http.Mock.start_link()
 
-    response = { :ok, @not_ok_resp }
+    response = {:ok, @not_ok_resp}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     result = Postscript.request(operation, http_client: Http.Mock)
 
-    assert { :error, %Response{} } = result
+    assert {:error, %Response{}} = result
+  end
+
+  test "logs warning and returns :error when response is not valid JSON" do
+    Http.Mock.start_link()
+
+    response = {:ok, @not_json_resp}
+
+    Http.Mock.put_response(response)
+
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
+
+    assert {result, log} =
+             with_log([level: :warning], fn ->
+               Postscript.request(operation, http_client: Http.Mock)
+             end)
+
+    assert {:error, _error} = result
+
+    assert log =~ "invalid JSON response"
+    assert log =~ "\"not json\""
   end
 
   test "passes the response through when unrecognized" do
     Http.Mock.start_link()
 
-    response = { :error, :timeout }
+    response = {:error, :timeout}
 
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     result = Postscript.request(operation, http_client: Http.Mock)
 
@@ -170,30 +203,30 @@ defmodule PostscriptTest do
   test "retries failed requests" do
     Http.Mock.start_link()
 
-    response_1 = { :error, :timeout }
-    response_2 = { :ok, @ok_resp }
+    response_1 = {:error, :timeout}
+    response_2 = {:ok, @ok_resp}
 
     Http.Mock.put_response(response_1)
     Http.Mock.put_response(response_2)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     result = Postscript.request(operation, http_client: Http.Mock, retry: Postscript.Retry.Linear)
 
-    assert { :ok, %Response{} } = result
+    assert {:ok, %Response{}} = result
   end
 
   test "retries up to max attempts" do
     Http.Mock.start_link()
 
-    response = { :error, :timeout }
+    response = {:error, :timeout}
 
     Http.Mock.put_response(response)
     Http.Mock.put_response(response)
     Http.Mock.put_response(response)
     Http.Mock.put_response(response)
 
-    operation = %Operation{ method: :post, params: [hello: "world"], path: "/fake" }
+    operation = %Operation{method: :post, params: [hello: "world"], path: "/fake"}
 
     Postscript.request(operation, http_client: Http.Mock, retry: Postscript.Retry.Linear)