New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new http_component that would return the body of the response and custom error messages. #685
base: master
Are you sure you want to change the base?
Conversation
lib/koala/api/graph_batch_api.rb
Outdated
# 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you handle JSONError and add a spec for it. If fb sends a messed up body we will end up with a json error instead of a koala error.
And from experience, fb loves to send html error pages or null instead of well formed json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have added the code change to handle the JSON error and added test coverage as well.
spec/cases/graph_api_batch_spec.rb
Outdated
@@ -406,6 +416,13 @@ | |||
Koala::Facebook::API.new("foo").batch {|batch_api| batch_api.get_object('me') } | |||
}.to raise_exception(Koala::Facebook::BadFacebookResponse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}.to raise_exception(Koala::Facebook::BadFacebookResponse) | |
}.to raise_exception(Koala::Facebook::BadFacebookResponse, /"Facebook returned an empty body"/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have updated the test to check for the error message as well.
lib/koala/api/graph_batch_api.rb
Outdated
# 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we need to add a check for :response because if you set http_component = header or status it will fail see https://github.com/pri1012/koala/blob/c3279139fab69d25d017cb9b83d1de9e18fd5018/lib/koala/api.rb#L57
spec/cases/graph_api_batch_spec.rb
Outdated
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') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test where http_component is status, and another one with headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. We have added tests for http_component :status
and :headers
as well.
lib/koala/api/graph_batch_api.rb
Outdated
@@ -138,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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's return result which is defined above. And also you are mixing code and status in this initialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @ylecuyer. We have use result
defined above.
bf514d6
to
d9cd078
Compare
d9cd078
to
f93bddf
Compare
f93bddf
to
96e7cf9
Compare
Description:
This PR includes changes which will help use the latest koala version when http_component is set
Changes included:
Parsing of response: when http_component is set, we receive Koala::Http_service response instead of a hash and generate_results method handles only JSON response. This affects all batch request.
Custom Error message when the body is nil