Ensure correct Faraday JSON parsing/response body for invalid respons…

…e header (#110)

See my investigation here:
#100 (comment)

In the Faraday 1 to Faraday 2 process, the ensure JSON response
middleware was rewritten from scratch, which caused a change in how
permissive the middleware was to response Content-Type headers which did
not properly indicate a JSON response.

In Faraday 1.x, the JSON middleware would attempt a JSON parse of the
body, regardless of the presence or value of a Content-Type header. This
meant that Graphlient would have raised an exception (or re-raised one
caught from Faraday) if the response body was not valid JSON.

In Faraday 2.x, the new JSON middleware will only attempt to parse the
response body if it determines that the Content-Type response header
indicates a JSON body. By default it uses a regex on the header value
(/\bjson$/ at the time of writing). Crucially, if the header is missing,
or does not match the regex, instead of raising an exception,
`Faraday::Response#body` returns a String, rather than the
previously-assumed Hash or Array.

This change restores the parsing logic which is tolerant to incorrect or
missing Content-Type header values. Response bodies which are not valid
JSON will raise an exception. Any unforseen situation where `#body`
returns a type that is not String, Hash, or Array, will raise a separate
exception detailing that this is unexpected behavior and a bug should be
filed.

Resolves #100

CHANGELOG.md

@@ -1,5 +1,6 @@
 ### (Next)
 * Your contribution here.
+* [#110](https://github.com/ashkan18/graphlient/pull/110): Ensure correct Faraday JSON response body parsing with invalid response header - [@taylorthurlow](https://github.com/taylorthurlow).
 * [#107](https://github.com/ashkan18/graphlient/pull/107): Pass in initialized schema as an option - [@kbaum](https://github.com/kbaum).
 * [#106](https://github.com/ashkan18/graphlient/pull/106): Add 3.2 to the list of ruby ci versions - [@igor-drozdov](https://github.com/igor-drozdov).
 * [#102](https://github.com/ashkan18/graphlient/pull/102): Update ci to test latest jruby - [@ashkan18](https://github.com/ashkan18).

lib/graphlient/adapters/http/faraday_adapter.rb

@@ -1,4 +1,5 @@
 require 'faraday'
+require 'json'
 
 module Graphlient
   module Adapters
@@ -13,7 +14,8 @@ def execute(document:, operation_name:, variables:, context:)
               variables: variables
             }.to_json
           end
-          response.body
+
+          parse_body(response.body)
         rescue Faraday::ConnectionFailed => e
           raise Graphlient::Errors::ConnectionFailedError, e
         rescue Faraday::TimeoutError => e
@@ -39,6 +41,38 @@ def connection
             end
           end
         end
+
+        private
+
+        # Faraday 2.x's JSON response middleware will only parse a JSON
+        # response body into a Hash (or Array) object if the response headers
+        # match a specific content type regex. See Faraday's response JSON
+        # middleware definition for specifics on what the datatype of the
+        # response body will be. This method will handle the situation where
+        # the response header is not set appropriately, but contains JSON
+        # anyways. If the body cannot be parsed as JSON, an exception will be
+        # raised.
+        def parse_body(body)
+          case body
+          when Hash, Array
+            body
+          when String
+            begin
+              JSON.parse(body)
+            rescue JSON::ParserError
+              raise Graphlient::Errors::ServerError, 'Failed to parse response body as JSON'
+            end
+          else
+            inner_exception = StandardError.new <<~ERR.strip.tr("\n", ' ')
+              Unexpected response body type '#{body.class}'. Graphlient doesn't
+              know how to handle a response body of this type, but Faraday is
+              returning it. Please open an issue, particularly if the response
+              body does actually contain valid JSON.
+            ERR
+
+            raise Graphlient::Errors::ClientError, inner_exception
+          end
+        end
       end
     end
   end

spec/graphlient/adapters/http/faraday_adapter_spec.rb

@@ -75,6 +75,61 @@
     end
   end
 
+  context 'a non-error response without an appropriate JSON header' do
+    let(:url) { 'http://example.com/graphql' }
+    let(:headers) { { 'Content-Type' => 'application/notjson' } }
+    let(:client) { Graphlient::Client.new(url) }
+    let(:response_headers) { { 'Content-Type' => 'application/notjson' } }
+
+    context 'when the response body is valid JSON' do
+      before do
+        stub_request(:post, url).to_return(
+          status: 200,
+          headers: response_headers,
+          body: DummySchema.execute(GraphQL::Introspection::INTROSPECTION_QUERY).to_json
+        )
+      end
+
+      it 'retrieves schema' do
+        expect(client.schema).to be_a Graphlient::Schema
+      end
+    end
+
+    context 'when the response body is not valid JSON' do
+      before do
+        stub_request(:post, url).to_return(
+          status: 200,
+          headers: response_headers,
+          body: ''
+        )
+      end
+
+      it 'raises Graphlient::Errors::ServerError' do
+        expect { client.schema }.to raise_error(Graphlient::Errors::ServerError) { |error|
+          expect(error.message).to include('Failed to parse response body as JSON')
+        }
+      end
+    end
+
+    context 'when the Faraday response body object is not a type we expect from Faraday' do
+      before do
+        stub_request(:post, url).to_return(
+          status: 200,
+          headers: response_headers
+        )
+      end
+
+      it 'raises Graphlient::Errors::ClientError' do
+        mock_response = double('response', body: nil)
+        allow(client.http.connection).to receive(:post).and_return(mock_response)
+
+        expect { client.schema }.to raise_error(Graphlient::Errors::ClientError) { |error|
+          expect(error.message).to include "Unexpected response body type 'NilClass'"
+        }
+      end
+    end
+  end
+
   context 'Failed to open TCP connection error' do
     let(:url) { 'http://example.com/graphql' }
     let(:client) { Graphlient::Client.new(url) }