fix: [ENG-1383] Validate & log invalid JSON response (#69)

* fix: don't throw exception on invalid JSON response, return error tuple

Author: jcartwright

lib/postscript/request.ex

@@ -1,5 +1,5 @@
 defmodule Postscript.Request do
-  alias Postscript.{ Config, Helpers, Operation, Response }
+  alias Postscript.{Config, Helpers, Operation, Response}
 
   @type t ::
           %__MODULE__{
@@ -10,20 +10,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)
@@ -49,15 +47,16 @@ defmodule Postscript.Request do
     |> finish(config)
   end
 
-  defp retry(response, _request, %_{ retry: retry }) when is_nil(retry) or retry == false do
+  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 +88,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,20 @@
 defmodule PostscriptTest do
   use ExUnit.Case, async: true
 
-  alias Postscript.{ Http, Operation, Response }
+  alias Postscript.{Http, Operation, Response}
 
-  @ok_resp %{ body: "{\"ok\":true}", headers: [], status_code: 200 }
+  @ok_resp %{body: "{\"ok\":true}", headers: [], status_code: 200}
 
-  @not_ok_resp %{ body: "{\"ok\":false}", headers: [], status_code: 400 }
+  @not_ok_resp %{body: "{\"ok\":false}", headers: [], status_code: 400}
 
   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 +24,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 +38,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 +52,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 +66,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 +105,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 +119,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 +133,39 @@ 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 "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 +175,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)