-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
Error Handling #275
Error Handling #275
Changes from 4 commits
4a65df4
a9f5b60
6c311cd
ddc80cf
2e66fbe
198cea8
0955b2d
6880a23
0a029c2
ff83299
cb7b3d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
namespace TMDbLib.Objects.Exceptions | ||
{ | ||
public class APIException : TMDbException | ||
{ | ||
public TMDbStatusMessage StatusMessage { get; } | ||
|
||
public APIException(string message, TMDbStatusMessage statusMessage) : base(message) | ||
{ | ||
StatusMessage = statusMessage; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
using System.Net; | ||
|
||
namespace TMDbLib.Objects.Exceptions | ||
{ | ||
public class GeneralHttpException : TMDbException | ||
{ | ||
public HttpStatusCode HttpStatusCode { get; } | ||
|
||
public GeneralHttpException(HttpStatusCode httpStatusCode) | ||
: base("TMDb returned an unexpected HTTP error") | ||
{ | ||
HttpStatusCode = httpStatusCode; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
namespace TMDbLib.Objects.Exceptions | ||
{ | ||
public class NotFoundException : APIException | ||
{ | ||
public NotFoundException(TMDbStatusMessage statusMessage) | ||
: base("The requested item was not found", statusMessage) | ||
{ | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
using System; | ||
|
||
namespace TMDbLib.Objects.Exceptions | ||
{ | ||
public class TMDbException : Exception | ||
{ | ||
public TMDbException(string message) | ||
: base(message) | ||
{ | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
using Newtonsoft.Json; | ||
|
||
namespace TMDbLib.Objects.Exceptions | ||
{ | ||
public class TMDbStatusMessage | ||
{ | ||
[JsonProperty("status_code")] | ||
public int StatusCode { get; set; } | ||
|
||
[JsonProperty("status_message")] | ||
public string StatusMessage { get; set; } | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the object from TMDb? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, an example could be: http://api.themoviedb.org/3/credit/9999999999?api_key=c6b31d1cdad6a56a23f0c913e2482a31 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,8 +42,10 @@ public RestRequest AddParameter(string key, string value, ParameterType type = P | |
{ | ||
case ParameterType.QueryString: | ||
return AddQueryString(key, value); | ||
|
||
case ParameterType.UrlSegment: | ||
return AddUrlSegment(key, value); | ||
|
||
default: | ||
throw new ArgumentOutOfRangeException(nameof(type), type, null); | ||
} | ||
|
@@ -84,63 +86,45 @@ private void AppendQueryString(StringBuilder sb, KeyValuePair<string, string> va | |
AppendQueryString(sb, value.Key, value.Value); | ||
} | ||
|
||
private void CheckResponse(HttpResponseMessage response) | ||
{ | ||
if (response.StatusCode == HttpStatusCode.Unauthorized) | ||
throw new UnauthorizedAccessException("Call to TMDb returned unauthorized. Most likely the provided API key is invalid."); | ||
} | ||
|
||
public async Task<RestResponse> ExecuteDelete(CancellationToken cancellationToken) | ||
{ | ||
HttpResponseMessage resp = await SendInternal(HttpMethod.Delete, cancellationToken).ConfigureAwait(false); | ||
|
||
CheckResponse(resp); | ||
|
||
return new RestResponse(resp); | ||
} | ||
|
||
public async Task<RestResponse<T>> ExecuteDelete<T>(CancellationToken cancellationToken) | ||
{ | ||
HttpResponseMessage resp = await SendInternal(HttpMethod.Delete, cancellationToken).ConfigureAwait(false); | ||
|
||
CheckResponse(resp); | ||
|
||
return new RestResponse<T>(resp, _client); | ||
} | ||
|
||
public async Task<RestResponse> ExecuteGet(CancellationToken cancellationToken) | ||
{ | ||
HttpResponseMessage resp = await SendInternal(HttpMethod.Get, cancellationToken).ConfigureAwait(false); | ||
|
||
CheckResponse(resp); | ||
|
||
return new RestResponse(resp); | ||
} | ||
|
||
public async Task<RestResponse<T>> ExecuteGet<T>(CancellationToken cancellationToken) | ||
{ | ||
HttpResponseMessage resp = await SendInternal(HttpMethod.Get, cancellationToken).ConfigureAwait(false); | ||
|
||
CheckResponse(resp); | ||
|
||
return new RestResponse<T>(resp, _client); | ||
} | ||
|
||
public async Task<RestResponse> ExecutePost(CancellationToken cancellationToken) | ||
{ | ||
HttpResponseMessage resp = await SendInternal(HttpMethod.Post, cancellationToken).ConfigureAwait(false); | ||
|
||
CheckResponse(resp); | ||
|
||
return new RestResponse(resp); | ||
} | ||
|
||
public async Task<RestResponse<T>> ExecutePost<T>(CancellationToken cancellationToken) | ||
{ | ||
HttpResponseMessage resp = await SendInternal(HttpMethod.Post, cancellationToken).ConfigureAwait(false); | ||
|
||
CheckResponse(resp); | ||
|
||
return new RestResponse<T>(resp, _client); | ||
} | ||
|
||
|
@@ -201,13 +185,21 @@ private async Task<HttpResponseMessage> SendInternal(HttpMethod method, Cancella | |
|
||
Debug.Assert(timesToTry >= 1); | ||
|
||
TMDbStatusMessage statusMessage = null; | ||
|
||
do | ||
{ | ||
using (HttpRequestMessage req = PrepRequest(method)) | ||
{ | ||
HttpResponseMessage resp = | ||
await _client.HttpClient.SendAsync(req, cancellationToken).ConfigureAwait(false); | ||
|
||
if (!resp.IsSuccessStatusCode) | ||
{ | ||
statusMessage = | ||
JsonConvert.DeserializeObject<TMDbStatusMessage>(await resp.Content.ReadAsStringAsync()); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will there always be a Status message object in erronous responses? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aha. Just read #270 - we've seen HTML before. Perhaps we should also check the Content-Type on the response. If it is not JSON (application/json), it's an error, and we should throw the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, not necesarily. An issue was reported were the API returned HTML instead of JSON. Not sure how to handle it exactly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check for the Content-Type. I imagine that after checking for 429 responses, we check if the Content Type is json. If it isn't, then it's an error no matter what the status code is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can also move the parsing down to the other if.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Want to throw a specific exception for that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. For response other JSON - no. It's an error, yes, but it should feed back in to the rest of the loop. We just should not even attempt to deserialize a status message (or an object) - if the content type is not json. |
||
if (resp.StatusCode == (HttpStatusCode)429) | ||
{ | ||
// The previous result was a ratelimit, read the Retry-After header and wait the allotted time | ||
|
@@ -227,12 +219,30 @@ private async Task<HttpResponseMessage> SendInternal(HttpMethod method, Cancella | |
return resp; | ||
|
||
if (!resp.IsSuccessStatusCode) | ||
return resp; | ||
{ | ||
switch (resp.StatusCode) | ||
{ | ||
case HttpStatusCode.Unauthorized: | ||
throw new UnauthorizedAccessException("Call to TMDb returned unauthorized. Most likely the provided API key is invalid."); | ||
|
||
case HttpStatusCode.NotFound: | ||
if (_client.ThrowExceptionsOnNotFound) | ||
{ | ||
throw new NotFoundException(statusMessage); | ||
} | ||
else | ||
{ | ||
return null; | ||
} | ||
} | ||
throw new GeneralHttpException(resp.StatusCode); | ||
//return resp; | ||
} | ||
} | ||
} while (timesToTry-- > 0); | ||
|
||
// We never reached a success | ||
throw new RequestLimitExceededException(retryHeader?.Date, retryHeader?.Delta); | ||
throw new RequestLimitExceededException(statusMessage, retryHeader?.Date, retryHeader?.Delta); | ||
} | ||
|
||
public RestRequest SetBody(object obj) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ public RestResponse(HttpResponseMessage response) | |
Response = response; | ||
} | ||
|
||
public bool IsValid { get { return Response != null; } } | ||
|
||
public HttpStatusCode StatusCode => Response.StatusCode; | ||
|
||
public async Task<Stream> GetContent() | ||
|
@@ -53,7 +55,14 @@ public static implicit operator T(RestResponse<T> response) | |
{ | ||
try | ||
{ | ||
return response.GetDataObject().Result; | ||
if (response.IsValid) | ||
{ | ||
return response.GetDataObject().Result; | ||
} | ||
else | ||
{ | ||
return default(T); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, I'd rather fail. IIRC, the RestResponse is never exposed -- so we are in complete control of calls. For this case, it should be "if (!Valid) throw new ..". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment in the PR thread about this line. |
||
} | ||
} | ||
catch (AggregateException ex) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,14 @@ public void TestGetCreditTv() | |
Assert.Equal("", result.Media.Character); | ||
} | ||
|
||
[Fact] | ||
public void TestMissingCredit() | ||
{ | ||
Credit result = Config.Client.GetCreditsAsync("9999999999").Result; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make a single test that tests the exception-throwing part. Testing each "area" of TMDb once for not-found is probably a good idea. That API hasn't really shined in concistency :) |
||
|
||
Assert.Null(result); | ||
} | ||
|
||
[Fact] | ||
public void TestGetCreditEpisode() | ||
{ | ||
|
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 shouldn't check for IsValid in some places.
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.
This is also related to trying to return null.. Response isn't null, as it's a wrapper containing the null value.
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.
Another thing to fix then is that if ExecuteGet always returns non-null - we should never check for null.
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.
Yeah, that's probably a good idea.