Error Handling #275

FrederikBolding · 2018-06-20T12:41:45Z

So far I've added exceptions for errors occurring in the RestClient layer. I've also added some tweaks for handling nulls in case of not founds, as well as a property to determine whether or not to throw exceptions when items aren't found.

I'm not done testing yet and would like some feedback on how to handle nulls as well as any other errors that I haven't considered yet.

In relation to issues: #182 #194 #235 #270

… tweaks for handling nulls in case of not founds

LordMike · 2018-06-20T19:33:57Z

TMDbLib/Objects/Exceptions/NotFoundException.cs

+
+namespace TMDbLib.Objects.Exceptions
+{
+    public class NotFoundException : TMDbHttpException


We should probably not name our exceptions "Http". Sure. If something is http specific, go for it - but for the higher level stuff, like "not found", the inheritance tree should not include "Http" (or any other transport protocol).

LordMike · 2018-06-20T19:36:34Z

TMDbLib/Objects/Exceptions/NotFoundException.cs

+{
+    public class NotFoundException : TMDbHttpException
+    {
+        public NotFoundException(HttpStatusCode httpStatusCode, TMDbStatusMessage statusMessage)


In principle, this exceptionshould not know anything about any HttpStatusCode.

TMDbStatusMessage can stay - but we may encounter a scenario in which we don't have data for it. I'll try listing some exception types on the issue.

LordMike · 2018-06-20T19:40:02Z

TMDbLib/Objects/Exceptions/TMDbStatusMessage.cs

+
+        [JsonProperty("status_message")]
+        public string StatusMessage { get; set; }
+    }


This is the object from TMDb?

Yep, an example could be: http://api.themoviedb.org/3/credit/9999999999?api_key=c6b31d1cdad6a56a23f0c913e2482a31

LordMike · 2018-06-20T19:42:18Z

TMDbLib/Rest/RestRequest.cs

        {
            if (response.StatusCode == HttpStatusCode.Unauthorized)
                throw new UnauthorizedAccessException("Call to TMDb returned unauthorized. Most likely the provided API key is invalid.");
-        }
+        }**/


Remove unecessary code.

LordMike · 2018-06-20T19:43:28Z

TMDbLib/Rest/RestRequest.cs

+                        statusMessage =
+                            JsonConvert.DeserializeObject<TMDbStatusMessage>(await resp.Content.ReadAsStringAsync());
+                    }
+


Will there always be a Status message object in erronous responses?

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 GeneralException -- regardless if it's a known response, such as 404.

Hmm, not necesarily. An issue was reported were the API returned HTML instead of JSON. Not sure how to handle it exactly.

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.

You can also move the parsing down to the other if..

Want to throw a specific exception for that?

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.

LordMike · 2018-06-20T19:53:03Z

TMDbLib/Objects/Exceptions/UnhandledException.cs

+
+namespace TMDbLib.Objects.Exceptions
+{
+    public class UnhandledException : TMDbHttpException


Let's name this "GeneralException".

Also. As this is explicitly a protocol issue, we could name this "GeneralHttpException", and include the HttpStatusCode here.

LordMike · 2018-06-20T19:55:18Z

TMDbLib/Rest/RestResponse.cs

+                }
+                else
+                {
+                    return default(T);


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 ..".

See my comment in the PR thread about this line.

LordMike · 2018-06-20T19:55:29Z

TMDbLib/Rest/RestResponse.cs

@@ -17,6 +17,8 @@ public RestResponse(HttpResponseMessage response)
            Response = response;
        }

+        public bool Valid { get { return Response != null; } }


Should probably be named "IsValid"

LordMike · 2018-06-20T19:56:38Z

TMDbLibTests/ClientCreditTests.cs

+        [Fact]
+        public void TestMissingCredit()
+        {
+            Credit result = Config.Client.GetCreditsAsync("9999999999").Result;


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 :)

LordMike · 2018-06-20T20:05:15Z

Looks really good.

Nulls

For nulls, I'd prefer that if the ThrowOnNotFound option was set, we threw errors on api calls (which this PR does, iirc). When it's not set, we return nulls and generally try as much as possible to not throw errors.. Perhaps we should go through all "throw new" statements...

Internally. There was one place in RestResponse where you returned default. This should not happen. Calling "GetObject" or anything like it is an error, if the IsValid is false.

Perhaps ThrowOnNotFound should be ThrowOnApiErrors, or even ThrowExceptions. We probably shouldn't constrain ourselves to "Not Found" in particular.

Exceptions

As for exceptions, I imagine we can do something like this:

TMDbException -- Our base, just inherits from Exception
- APIException -- Our base for all things API (higher level stuff); Includes the TMDb Status Message object
  - NotFoundException
  - APIRequestRateLimitException -- One could argue that this doesn't fit in API, but should be in protocol
- ProtocolException -- Our base for all things protocol
  - GeneralHttpException -- An exception with HTTP Status Code, for all the other things in http

Another more flat approach:

TMDbException -- Our base, just inherits from Exception
- APIException -- Our base for all things API (higher level stuff); Includes the TMDb Status Message object
  - NotFoundException
  - APIRequestRateLimitException
- GeneralHttpException -- An exception with HTTP Status Code, for all the other things in http

FrederikBolding · 2018-06-20T20:12:01Z

The problem with returning nulls directly from SendInternal is that there is quite a lot of code that still runs to deserialize and transform the result into an object. I wasn't exactly sure how to handle it, making sure that the actual result returned is null (hence the default(T)).

Furthermore, wouldn't we like to distinguish between throwing errors on "not founds" and general errors?

LordMike · 2018-06-20T20:19:55Z

Throwing not founds, generals etc. happens all the way down in RestResponse. So that's already "handled".

What we need to figure out is what happens when we don't throw errors.

I now remember all the touchup happening afterwards. Jibbers. That code shouldn't run if there is no object to run on. Having a default(T) and then patching in something afterwards is bad -- then we'll end with something with nothing of value, except and identifier stating that this (empty) object represents something something of "this" id.

Encapsulating this patchning to an action would equally be bad, as it's more complex and less forgiving .. (something like client.DoRequestInternal<T>(uri, Action<T> postProcessAction) -- postProcess will only be called if no errors happened).

Hrmm.. We could change the internal request, to a Try* pattern. Something like this.

bool client.TryDoRequest<T>(uri, ..., out T result)

This will force the consumers (us, internally) to remember that this can "not-fail" but still "fail".

LordMike · 2018-06-20T20:26:17Z

Arh crap. The calls are async, so out-parameters are a no-go.

The only option then is to encapsulate the response in a container which indicates success.. Which, surprise(!), we already have - it now has a success indicator called "IsValid".

... So we're left with this.

        public async Task<TvEpisode> GetTvEpisodeAsync(int tvShowId, int seasonNumber, int episodeNumber, TvEpisodeMethods extraMethods = TvEpisodeMethods.Undefined, string language = null, CancellationToken cancellationToken = default(CancellationToken))
        {
     ..
            RestResponse<TvEpisode> resp = await req.ExecuteGet<TvEpisode>(cancellationToken).ConfigureAwait(false);

            if (!resp.IsValid) <<----- We add this check
                // No data
                return null;
        
            TvEpisode item = await resp.GetDataObject().ConfigureAwait(false);

            // No data to patch up so return
            if (item == null)
                return null;

            // Patch up data, so that the end user won't notice that we share objects between request-types.
            if (item.Videos != null)
                item.Videos.Id = item.Id ?? 0;

     ..

            return item;
        }

Which, as you correctly point out, is cleaner by just returning null... Crap.. -.-

LordMike · 2018-06-20T20:38:48Z

TMDbLib/Client/TMDbClientMovies.cs

@@ -101,7 +101,7 @@ public partial class TMDbClient
            RestResponse<Movie> response = await req.ExecuteGet<Movie>(cancellationToken).ConfigureAwait(false);

            // No data to patch up so return
-            if (response == null) return null;
+            if (response == null || !response.IsValid) return null;


We shouldn't check for IsValid in some places.

This is also related to trying to return null.. Response isn't null, as it's a wrapper containing the null value.

Another thing to fix then is that if ExecuteGet always returns non-null - we should never check for null.

Yeah, that's probably a good idea.

LordMike · 2018-06-20T20:41:43Z

TMDbLib/Rest/RestRequest.cs

+                        statusMessage =
+                            JsonConvert.DeserializeObject<TMDbStatusMessage>(await resp.Content.ReadAsStringAsync());
+                    }
+


You can also move the parsing down to the other if..

…l exception in case of weird results

…where needed

FrederikBolding · 2018-06-23T10:44:15Z

I think I might have covered most of the cases where we should return null now. Any further comments, changes etc?

LordMike · 2018-06-23T12:37:03Z

I'm looking through the code now in VS with R# - I'm doing a number of its suggested changes. I'll also take a look at the SendInternal method.. :)

LordMike · 2018-06-23T12:46:19Z

Wow. The test suite needs a serious round of cleanup.

LordMike · 2018-06-23T12:52:18Z

@FrederikBolding I've tried reordering the operations a bit in the SendInternal. I've tried combining the error handling to one place, while returning quickly for successes.

Likewise, I now only consider responses with JSON as being successfull, as iirc we never return streams of stuff (The images are links to elsewhere).

How does this look?

FrederikBolding · 2018-06-23T13:07:02Z

Looks good! 😄 - With regards to testing I've attempted to clean up and fix a bunch of tests in #253

FrederikBolding added 2 commits June 20, 2018 14:37

Added exceptions for errors occurring in RestClient layer, Added some…

4a65df4

… tweaks for handling nulls in case of not founds

Reverted accidental change to TestMoviesExtrasNone

a9f5b60

LordMike requested changes Jun 20, 2018

View reviewed changes

FrederikBolding added 2 commits June 20, 2018 22:28

Renamed Valid property, Changed exception structure, Removed unused code

6c311cd

Renamed exception file, Removed unused imports

ddc80cf

LordMike requested changes Jun 20, 2018

View reviewed changes

FrederikBolding added 4 commits June 21, 2018 00:29

Moved status message parsing into existing if statement, Added genera…

2e66fbe

…l exception in case of weird results

Added correct null responses for tv + tests

198cea8

Added tests for multiple other cases of missing items, Returned null …

0955b2d

…where needed

Tweaked RestRequest error handling slightly

6880a23

Cleanups

0a029c2

Reorder operations in SendInternal

ff83299

Rename ThrowExceptions option

cb7b3d1

LordMike approved these changes Jun 23, 2018

View reviewed changes

LordMike merged commit d396d83 into jellyfin:master Jun 23, 2018

This was referenced Jun 23, 2018

Handling of not existing movies/tvshow/season/episodes #182

Closed

Throw errors when TMDb fails #194

Closed

GetMovieAsync returns empty object #235

Closed

GetMoviesAsunc issue #270

Closed

FrederikBolding changed the title ~~Error Handling (WIP)~~ Error Handling Jun 24, 2018

FrederikBolding deleted the error-handling branch June 24, 2018 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error Handling #275

Error Handling #275

FrederikBolding commented Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

FrederikBolding Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018 •

edited

Loading

FrederikBolding Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

FrederikBolding Jun 20, 2018

LordMike Jun 22, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

FrederikBolding Jun 20, 2018

LordMike Jun 20, 2018

LordMike Jun 20, 2018

LordMike commented Jun 20, 2018

FrederikBolding commented Jun 20, 2018

LordMike commented Jun 20, 2018 •

edited

Loading

LordMike commented Jun 20, 2018 •

edited

Loading

LordMike Jun 20, 2018

FrederikBolding Jun 20, 2018

LordMike Jun 21, 2018

FrederikBolding Jun 21, 2018

LordMike Jun 20, 2018

FrederikBolding commented Jun 23, 2018

LordMike commented Jun 23, 2018

LordMike commented Jun 23, 2018

LordMike commented Jun 23, 2018 •

edited

Loading

FrederikBolding commented Jun 23, 2018

Error Handling #275

Error Handling #275

Conversation

FrederikBolding commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LordMike Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LordMike commented Jun 20, 2018

Nulls

Exceptions

FrederikBolding commented Jun 20, 2018

LordMike commented Jun 20, 2018 • edited Loading

LordMike commented Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrederikBolding commented Jun 23, 2018

LordMike commented Jun 23, 2018

LordMike commented Jun 23, 2018

LordMike commented Jun 23, 2018 • edited Loading

FrederikBolding commented Jun 23, 2018

LordMike Jun 20, 2018 •

edited

Loading

LordMike commented Jun 20, 2018 •

edited

Loading

LordMike commented Jun 20, 2018 •

edited

Loading

LordMike commented Jun 23, 2018 •

edited

Loading