From 423afee98374bc146addd2e383e46830aa3b197d Mon Sep 17 00:00:00 2001 From: Taylor Thurlow Date: Sat, 6 Jan 2024 11:07:06 -0800 Subject: [PATCH] Ensure correct Faraday JSON parsing/response body for invalid response header (#110) See my investigation here: https://github.com/ashkan18/graphlient/issues/100#issuecomment-1877886369 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 + .../adapters/http/faraday_adapter.rb | 36 +++++++++++- .../adapters/http/faraday_adapter_spec.rb | 55 +++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b71e412..7476b49 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/lib/graphlient/adapters/http/faraday_adapter.rb b/lib/graphlient/adapters/http/faraday_adapter.rb index f9e0b29..54a2559 100644 --- a/lib/graphlient/adapters/http/faraday_adapter.rb +++ b/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 diff --git a/spec/graphlient/adapters/http/faraday_adapter_spec.rb b/spec/graphlient/adapters/http/faraday_adapter_spec.rb index a960419..05ecb19 100644 --- a/spec/graphlient/adapters/http/faraday_adapter_spec.rb +++ b/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) }