Add middleware to handle unexpected non-json response for edge case#38
Open
nikushi wants to merge 5 commits intoraszi:masterfrom
Open
Add middleware to handle unexpected non-json response for edge case#38nikushi wants to merge 5 commits intoraszi:masterfrom
nikushi wants to merge 5 commits intoraszi:masterfrom
Conversation
Failures:
1) Mrkt::Authentication#authenticate on a unexpected non json response should raise an Error
Failure/Error: expect { subject }.to raise_error(Mrkt::Errors::Error)
expected Mrkt::Errors::Unknown, got #<NoMethodError: undefined method `fetch' for "something wrong":String> with backtrace:
# ./lib/mrkt/concerns/authentication.rb:34:in `block in authenticate'
# ./lib/mrkt/concerns/authentication.rb:31:in `tap'
# ./lib/mrkt/concerns/authentication.rb:31:in `authenticate'
# ./spec/concerns/authentication_spec.rb:5:in `block (3 levels) in <top (required)>'
# ./spec/concerns/authentication_spec.rb:22:in `block (5 levels) in <top (required)>'
# ./spec/concerns/authentication_spec.rb:22:in `block (4 levels) in <top (required)>'
# ./spec/concerns/authentication_spec.rb:22:in `block (4 levels) in <top (required)>'
This reverts commit 366898c.
raszi
requested changes
Dec 1, 2018
Owner
raszi
left a comment
raszi
left a comment
There was a problem hiding this comment.
Nice PR but I don't really know how we should handle this issue. I believe the client should not expect anything but JSON in the response and it should fail early.
Co-Authored-By: nikushi <deneb.ge@gmail.com>
Author
|
Thanks for reviewing. Do you mean that the middleware I added should not pass 2XX response? Instead it should raise the same error too? I was worried about that point actually. If we choose more strict approach, it should be raised if the middleware see a response type other than json, even if it's status is 2XX or something successful one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi, recently I got
NoMethodErrorexception during Marketo side system trouble. This exception happened onGET /identity/oauth/tokendue to following backtrace.This is a bug of mrkt gem of edge case handling. There is a case that Marketo's server could respond with non json content-type e.g.
text/plain. Although I could not confirm what it's content type was accurately, I believe that the exception was caused by the content type, from the stracktrace we got. You can see the detail in the first commit to reproduce the bug.