[bugfix] handle unexpected response bodies #492
Conversation
adsteel
changed the title
|
@jonnyom okay, take 2. My commits are in order, and probably best reviewed that way. Tests pass at every commit, but please feel free to double check that. I think I came up with a better solution that also did not involve a breaking change. Let me know what you think. |
|
|
||
| parsed_body = extract_response_body(response) | ||
|
|
||
| return nil if parsed_body.nil? |
There was a problem hiding this comment.
This line and https://github.com/intercom/intercom-ruby/pull/492/files#diff-e0313851110ec5f90dfd3a2befbc94f3R123 are to prevent any existing tests from breaking, in case users are depending on this behavior.
spec/unit/intercom/request_spec.rb
Outdated
| @@ -7,27 +7,27 @@ | |||
| it 'raises an error when a html error page rendered' do | |||
| response = OpenStruct.new(:code => 500) | |||
| req = Intercom::Request.new('path/', 'GET') | |||
| proc {req.parse_body('<html>something</html>', response)}.must_raise(Intercom::ServerError) | |||
| _(proc {req.parse_body('<html>something</html>', response)}).must_raise(Intercom::ServerError) | |||
There was a problem hiding this comment.
Thanks for this. Do you have happen to have a link to Minitest's documentation that outlines this behaviour?
There was a problem hiding this comment.
Ah, I see you've refactored this later on. Nice! 👍
| @@ -129,19 +129,21 @@ def decode(content_encoding, body) | |||
| end | |||
|
|
|||
| def raise_errors_on_failure(res) | |||
| if res.code.to_i.eql?(404) | |||
| code = res.code.to_i | |||
lib/intercom/request.rb
Outdated
| def self.post(path, form_data) | ||
| new(path, method_with_body(Net::HTTP::Post, path, form_data)) | ||
| end | ||
| private |
There was a problem hiding this comment.
To make it a little clearer, can we inline the private methods please?
|
This looks great, thank you @adsteel One small stylistic change that I think could improve readability in the Request class (specifically inlining private methods), but other than that this looks like a very elegant solution. |
|
@jonnyom I added a commit converting to the inlined private definition. Let me know if there's anything else! |
|
@adsteel this looks great, thanks a lot! If you'd like to just rebase off the master branch (Looks like a readme change) then I think we can merge thiks. |
If the JSON parsing errors, `parsed_body` is `nil` and the following line raises the error.
This will help us better reason about which methods should have tests, and which methods other objects are depending on specific behavior from.
Done as a step toward fixing a bug in `#parse_body` - Moves client test to client_spec.rb - Replaces parse_body tests with higher level tests - Makes parse_body private - Organizes and cleans up the request_spec file generally
`parse_body` was doing three things: - checking for response errors - handling parse errors - actually parsing the response body We get some cleaner code by separating the parsing from the response state checking.
Some responses do not have an expected error code and have non-JSON bodies that fail to parse. The unexpected `nil` caused runtime errors. This fix raises an error on those responses with more information that will allow developers to find a path forward, either by more fixes to this gem or by handling the new error in their own code. I'm giving this version bump a patch level bump because it looks like all existing functionality is preserved. We will now simply get better information instead of `undefined method '[]' for nil:NilClass`. Addresses #491
|
@jonnyom rebased with master. |
Addresses #491