-
Notifications
You must be signed in to change notification settings - Fork 131
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
Catch IOExceptions from response body InputStream #486
Conversation
Could be JsonIOException, which is a RuntimeException, so catching all Exceptions
Should the potential exceptions from these lines be wrapped as well? Auth0.Android/auth0/src/main/java/com/auth0/android/request/internal/BaseRequest.kt Line 106 in 5f20b76
(For fromJsonResponse maybe it would be the responsibility of the implementors of that method instead to catch the exceptions, so this might be outside the scope of this pull request.)
Auth0.Android/auth0/src/main/java/com/auth0/android/request/internal/BaseRequest.kt Line 110 in 5f20b76
|
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.
Thanks for opening this. Looks good, left two change requests.
auth0/src/test/java/com/auth0/android/request/internal/BaseRequestTest.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Luciano Balmaceda <[email protected]>
@lbalmaceda as @Marcono1234 points out, this same exception or an |
@jeffdgr8 Please let me know whether you will be progressing this PR or if we should pick it up from our side - thanks. |
@lbalmaceda yes, sorry, had a busy last week. I'll take a look at this this evening. |
Fixed 5c035ef |
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.
Thanks for this!
Changes
Handle exception from
resultAdapter.fromJson(reader)
call.References
Closes #483
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors (*that were completing successfully before, 16 failed before and after changes)