From c70299d250a4c879dd69e7f94a18cb7f3774dc92 Mon Sep 17 00:00:00 2001 From: Vishnupriya Varadarajan Date: Wed, 10 Jan 2024 15:23:45 +1100 Subject: [PATCH 1/6] add new http_component that would return the body of the response --- lib/koala/api/graph_batch_api.rb | 8 ++++++++ spec/cases/graph_api_batch_spec.rb | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/lib/koala/api/graph_batch_api.rb b/lib/koala/api/graph_batch_api.rb index 4a403ef0..83cf7b41 100644 --- a/lib/koala/api/graph_batch_api.rb +++ b/lib/koala/api/graph_batch_api.rb @@ -55,6 +55,14 @@ def execute(http_options = {}) original_api.graph_call("/", args, "post", http_options) do |response| raise bad_response if response.nil? + #when http_component is set we receive Koala::Http_service response object + # from graph_call.so thiis needs to be parsed + # as generate_results method handles only JSON rsponse + if http_options[:http_component] + response = JSON.load(response.body) + raise bad_response unless response.is_a?(Array) + end + batch_results += generate_results(response, batch) end end diff --git a/spec/cases/graph_api_batch_spec.rb b/spec/cases/graph_api_batch_spec.rb index d28dad25..ba3888ef 100644 --- a/spec/cases/graph_api_batch_spec.rb +++ b/spec/cases/graph_api_batch_spec.rb @@ -392,6 +392,16 @@ expect(result[0]).to eq({"Content-Type" => "text/javascript; charset=UTF-8"}) 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 result[0].status == 203 + expect result[0].body == "{\"id\":\"1234\"}" + expect result[0].headers == {"Content-Type"=>"text/javascript; charset=UTF-8"} + 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, "[]", {})) From 8b2e8194585c63c81c5e8511b7046d68f4b7413c Mon Sep 17 00:00:00 2001 From: Vishnupriya Varadarajan Date: Wed, 10 Jan 2024 16:56:06 +1100 Subject: [PATCH 2/6] returning custom error when the body is nil --- lib/koala/api/graph_batch_api.rb | 8 ++++---- spec/cases/graph_api_batch_spec.rb | 7 +++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/koala/api/graph_batch_api.rb b/lib/koala/api/graph_batch_api.rb index 83cf7b41..8ce20a2e 100644 --- a/lib/koala/api/graph_batch_api.rb +++ b/lib/koala/api/graph_batch_api.rb @@ -53,14 +53,14 @@ 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 thiis needs to be parsed # as generate_results method handles only JSON rsponse if http_options[:http_component] response = JSON.load(response.body) - raise bad_response unless response.is_a?(Array) + raise bad_response("Facebook returned an invalid body") unless response.is_a?(Array) end batch_results += generate_results(response, batch) @@ -89,9 +89,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) diff --git a/spec/cases/graph_api_batch_spec.rb b/spec/cases/graph_api_batch_spec.rb index ba3888ef..96bd5af3 100644 --- a/spec/cases/graph_api_batch_spec.rb +++ b/spec/cases/graph_api_batch_spec.rb @@ -416,6 +416,13 @@ 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\]/) + end context "with error info" do before :each do From 03ce756d3cd266a4ef92d03246ecc6b5488fe479 Mon Sep 17 00:00:00 2001 From: vvaradarajan Date: Thu, 28 Mar 2024 10:37:27 +1100 Subject: [PATCH 3/6] fix unit test --- lib/koala/api/graph_batch_api.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/koala/api/graph_batch_api.rb b/lib/koala/api/graph_batch_api.rb index 8ce20a2e..81c5662d 100644 --- a/lib/koala/api/graph_batch_api.rb +++ b/lib/koala/api/graph_batch_api.rb @@ -146,6 +146,11 @@ 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) else GraphCollection.evaluate(result, original_api) end end From 5652df0b446ad2e9076bb290c0dec38b95468c0f Mon Sep 17 00:00:00 2001 From: Yoann Lecuyer Date: Thu, 2 May 2024 01:00:31 +0200 Subject: [PATCH 4/6] fix typos --- lib/koala/api/graph_batch_api.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/koala/api/graph_batch_api.rb b/lib/koala/api/graph_batch_api.rb index 81c5662d..bd79d17b 100644 --- a/lib/koala/api/graph_batch_api.rb +++ b/lib/koala/api/graph_batch_api.rb @@ -55,9 +55,9 @@ def execute(http_options = {}) original_api.graph_call("/", args, "post", http_options) do |response| 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 thiis needs to be parsed - # as generate_results method handles only JSON rsponse + # 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) From 96e7cf92b74e5290f8ba31974706f5443ff3df3f Mon Sep 17 00:00:00 2001 From: Sai Kambaiyyagari Date: Thu, 2 May 2024 22:15:34 +1000 Subject: [PATCH 5/6] 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 From ef627193ee0051b6ea7c69eda548fadfbd3c2436 Mon Sep 17 00:00:00 2001 From: Sai Kambaiyyagari Date: Wed, 8 May 2024 09:33:55 +1000 Subject: [PATCH 6/6] Update the changelog: Summarize the code change done in the PR --- changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog.md b/changelog.md index 8a22e015..77ac45b5 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: