From 643a4c8d4c86f3a3e85ebddc2d894356559aed9f Mon Sep 17 00:00:00 2001 From: Vishnupriya Varadarajan Date: Thu, 27 Jun 2024 18:57:42 +1000 Subject: [PATCH] Better: Handles the invalid JSON response from Facebook when the request's http_options[:http_component] is set to ':response'. #685 --- changelog.md | 1 + lib/koala/api/graph_batch_api.rb | 29 ++++++++--- spec/cases/graph_api_batch_spec.rb | 79 +++++++++++++++++++++++++----- 3 files changed, 90 insertions(+), 19 deletions(-) diff --git a/changelog.md b/changelog.md index 1868d01e..11ebe977 100644 --- a/changelog.md +++ b/changelog.md @@ -8,6 +8,7 @@ New features: Updated features: * Add fbtrace_id, x-fb-rev, x-fb-debug to error messages and error class ([#668](https://github.com/arsduo/koala/pull/686)) + * Handles the invalid JSON response from Facebook when the request's http_options[:http_component] is set to ':response' ([#689](https://github.com/arsduo/koala/pull/689)) Removed features: diff --git a/lib/koala/api/graph_batch_api.rb b/lib/koala/api/graph_batch_api.rb index 4a403ef0..6631a151 100644 --- a/lib/koala/api/graph_batch_api.rb +++ b/lib/koala/api/graph_batch_api.rb @@ -53,7 +53,16 @@ def execute(http_options = {}) end original_api.graph_call("/", args, "post", http_options) do |response| - raise bad_response if response.nil? + raise bad_response('Facebook returned an empty body') if response.nil? + + # when http_component is set we receive Koala::Http_service response object + # from graph_call so this needs to be parsed + # as generate_results method handles only JSON response + if http_options[:http_component] && http_options[:http_component] == :response + response = json_body(response.body) + + raise bad_response('Facebook returned an invalid body') unless response.is_a?(Array) + end batch_results += generate_results(response, batch) end @@ -81,9 +90,9 @@ def generate_results(response, batch) end end - def bad_response + def bad_response(message) # Facebook sometimes reportedly returns an empty body at times - BadFacebookResponse.new(200, "", "Facebook returned an empty body") + BadFacebookResponse.new(200, '', message) end def result_from_response(response, options) @@ -123,14 +132,17 @@ def batch_args(calls_for_batch) JSON.dump calls end - def json_body(response) - # quirks_mode is needed because Facebook sometimes returns a raw true or false value -- - # in Ruby 2.4 we can drop that. - JSON.parse(response.fetch("body"), quirks_mode: true) + def json_body(body) + return if body.nil? + + JSON.parse(body) + rescue JSON::ParserError => e + Koala::Utils.logger.error("#{e.class}: #{e.message} while parsing #{body}") + nil end def desired_component(component:, response:, headers:) - result = Koala::HTTPService::Response.new(response['status'], response['body'], headers) + result = Koala::HTTPService::Response.new(response['code'], response['body'], headers) # Get the HTTP component they want case component @@ -138,6 +150,7 @@ def desired_component(component:, response:, headers:) # facebook returns the headers as an array of k/v pairs, but we want a regular hash when :headers then headers # (see note in regular api method about JSON parsing) + when :response then result else GraphCollection.evaluate(result, original_api) end end diff --git a/spec/cases/graph_api_batch_spec.rb b/spec/cases/graph_api_batch_spec.rb index d28dad25..f01f5d40 100644 --- a/spec/cases/graph_api_batch_spec.rb +++ b/spec/cases/graph_api_batch_spec.rb @@ -383,28 +383,85 @@ end end - describe "processing the request" do - it "returns the result headers as a hash if http_component is headers" do + describe 'processing the request' do + let(:response_status) { 203 } + let(:response_body) { '{\"id\":\"1234\"}'.gsub('\\', '') } + let(:response_headers) { { 'Content-Type' => 'text/javascript; charset=UTF-8' } } + + it 'returns the result headers as a hash if http_component is headers' do allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '[{"code":203,"headers":[{"name":"Content-Type","value":"text/javascript; charset=UTF-8"}],"body":"{\"id\":\"1234\"}"}]', {})) result = @api.batch do |batch_api| batch_api.get_object(KoalaTest.user1, {}, :http_component => :headers) end - expect(result[0]).to eq({"Content-Type" => "text/javascript; charset=UTF-8"}) + expect(response_headers).to eq(result[0]) + end + + it 'returns the complete response if http_component is response' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '[{"code":203,"headers":[{"name":"Content-Type","value":"text/javascript; charset=UTF-8"}],"body":"{\"id\":\"1234\"}"}]', {})) + result = @api.batch do |batch_api| + batch_api.get_object(KoalaTest.user1, {}, :http_component => :response) + end + + expect(response_status).to eq(result[0].status) + expect(response_body).to eq(result[0].body) + expect(response_headers).to eq(result[0].headers) end - describe "if it errors" do - it "raises an APIError if the response is not 200" do - allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(500, "[]", {})) + describe 'if it errors' do + it 'raises an APIError if the response is not 200' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(500, '[]', {})) expect { - Koala::Facebook::API.new("foo").batch {|batch_api| batch_api.get_object('me') } + Koala::Facebook::API.new('foo').batch { |batch_api| batch_api.get_object('me') } }.to raise_exception(Koala::Facebook::APIError) end - it "raises a BadFacebookResponse if the body is empty" do - allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, "", {})) + it 'raises a BadFacebookResponse if the body is empty' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '', {})) expect { - Koala::Facebook::API.new("foo").batch {|batch_api| batch_api.get_object('me') } - }.to raise_exception(Koala::Facebook::BadFacebookResponse) + Koala::Facebook::API.new('foo').batch { |batch_api| batch_api.get_object('me') } + }.to raise_exception(Koala::Facebook::BadFacebookResponse, /Facebook returned an empty body \[HTTP 200\]/) + end + + describe 'handle invalid body errors' do + describe 'with http_component set to :response' do + it 'raises a BadFacebookResponse if the body is non-empty, non-array' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '200', {})) + expect { + Koala::Facebook::API.new('foo').batch(http_component: :response) do |batch_api| + batch_api.get_object('me') + end + }.to raise_exception(Koala::Facebook::BadFacebookResponse, /Facebook returned an invalid body \[HTTP 200\]/) + end + + it 'raises a BadFacebookResponse if the body is invalid JSON' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '{"\"id\":\1234\"}"}', {})) + expect { + Koala::Facebook::API.new('foo').batch(http_component: :response) do |batch_api| + batch_api.get_object('me') + end + }.to raise_exception(Koala::Facebook::BadFacebookResponse, /Facebook returned an invalid body \[HTTP 200\]/) + end + end + + %i[headers status].each do |component| + describe "with http_component set to #{component}" do + it 'should not raise a BadFacebookResponse if the body is non-empty, non-array' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '200', {})) + expect { + Koala::Facebook::API.new('foo').batch(http_component: component) { |batch_api| batch_api.get_object('me') } + }.not_to raise_exception(Koala::Facebook::BadFacebookResponse, /Facebook returned an invalid body \[HTTP 200\]/) + end + + it 'should not raise a BadFacebookResponse if the body is invalid JSON' do + allow(Koala).to receive(:make_request).and_return(Koala::HTTPService::Response.new(200, '{"\"id\":\1234\"}"}', {})) + expect { + Koala::Facebook::API.new('foo').batch(http_component: component) do |batch_api| + batch_api.get_object('me') + end + }.not_to raise_exception(Koala::Facebook::BadFacebookResponse, /Facebook returned an invalid body \[HTTP 200\]/) + end + end + end end context "with error info" do