From 96e7cf92b74e5290f8ba31974706f5443ff3df3f Mon Sep 17 00:00:00 2001 From: Sai Kambaiyyagari Date: Thu, 2 May 2024 22:15:34 +1000 Subject: [PATCH] Incorporate the review comments. --- lib/koala/api/graph_batch_api.rb | 30 +++++------ spec/cases/graph_api_batch_spec.rb | 86 ++++++++++++++++++++++-------- 2 files changed, 78 insertions(+), 38 deletions(-) diff --git a/lib/koala/api/graph_batch_api.rb b/lib/koala/api/graph_batch_api.rb index bd79d17b..6631a151 100644 --- a/lib/koala/api/graph_batch_api.rb +++ b/lib/koala/api/graph_batch_api.rb @@ -53,14 +53,15 @@ def execute(http_options = {}) end original_api.graph_call("/", args, "post", http_options) do |response| - raise bad_response("Facebook returned an empty body") 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] - response = JSON.load(response.body) - raise bad_response("Facebook returned an invalid body") unless response.is_a?(Array) + 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) @@ -91,7 +92,7 @@ def generate_results(response, batch) def bad_response(message) # Facebook sometimes reportedly returns an empty body at times - BadFacebookResponse.new(200, "", message) + BadFacebookResponse.new(200, '', message) end def result_from_response(response, options) @@ -131,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 @@ -146,11 +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 - Koala::HTTPService::Response.new( - response["code"].to_i, - response["body"], - headers) + 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 96bd5af3..f01f5d40 100644 --- a/spec/cases/graph_api_batch_spec.rb +++ b/spec/cases/graph_api_batch_spec.rb @@ -383,45 +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 + 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 result[0].status == 203 - expect result[0].body == "{\"id\":\"1234\"}" - expect result[0].headers == {"Content-Type"=>"text/javascript; charset=UTF-8"} + + 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) - end - - 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) {|batch_api| batch_api.get_object('me') } - }.to raise_exception(Koala::Facebook::BadFacebookResponse, /Facebook returned an invalid body \[HTTP 200\]/) + 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