Issue #50: HTTP errors are ignored and result in empty responses with… #55
Conversation
…s with cache poisoning. WARNING This change is not a drop in replacement as if we get 403 because the token is incorrect, a checked exception is raised. Also note that, if an empty or null token is passed, it is now a fail fast (instead of getting a null pointer exception at the first request)
There was a problem hiding this comment.
@tdltdl thanks for sending your PR. I will continue to bring timely feedback from now on so we can create new package versions after each PR.
Please go over the exception restructuring suggested, with all HTTP exceptions inheriting from HttpErrorException. That includes simplifying all throws clauses to throws HttpErrorException where it makes sense.
Could you also update README.md' s Error section and document the new errors hierarchically?
I'll help you update the documentation for every exception after your next change.
| @@ -374,6 +375,9 @@ public Builder setCache(Cache cache) { | |||
| } | |||
|
|
|||
| public IPinfo build() { | |||
There was a problem hiding this comment.
Shouldn't this have throws IllegalArgumentException?
| * <p>That likely indicates that the token is either missing, incorrect or expired. | ||
| * It is a checked exception so, if you have multiple tokens (example during transition), you can fall back to another token.</p> | ||
| */ | ||
| public class InvalidTokenException extends Exception { |
There was a problem hiding this comment.
Now that we've introduced a proper HttpErrorException class, we should have InvalidTokenException and RateLimitedException also inherit from it, as they are technically exceptions stemming from an HTTP error response.
This helps simplify the design (no need to disambiguate everywhere with throws RateLimitedException, InvalidTokenException ) and it also helps clarify (in code) that they stem from the same source.
| @@ -18,9 +17,9 @@ protected BaseRequest(OkHttpClient client, String token) { | |||
| this.token = token; | |||
| } | |||
|
|
|||
| public abstract T handle() throws RateLimitedException; | |||
| public abstract T handle() throws RateLimitedException, InvalidTokenException; | |||
There was a problem hiding this comment.
This can be simplified to throws HttpErrorException
|
|
||
| public Response handleRequest(Request.Builder request) throws RateLimitedException { | ||
| public Response handleRequest(Request.Builder request) throws RateLimitedException, InvalidTokenException { |
There was a problem hiding this comment.
Similarly, this can be simplified throws HttpErrorException
… cache poisoning.
WARNING This change is not a drop in replacement as if we get 403 because the token is incorrect, a checked exception is raised.
Also note that, if an empty or null token is passed, it is now a fail fast (instead of getting a null pointer exception at the first request)