Handling of not existing movies/tvshow/season/episodes

[DanCooper]

I've tried to get results with wrong IDs and noticed that not all functions give back the same results for not existing media:
GetMovie returns Id 0
GetTvShow returns Id 0
GetTvSeason returns Id Nothing
GetTvEpisode returns Id Nothing

Is there a reason for that? I think it would be easier for the user, if always the same result can be expected (IMO it should be Nothing)

[LordMike]

Are you requesting existing movies and shows here, and the objects contain an incorrect Id?

We do some patching so that we can reuse objects. F.ex., in many properties off of the main object, there is an Id field. This is not set in the response from TMDb, but we "patch" it by setting it manually so that you can use the object (the property) as if it were an individual response. Is this a case of missing one of those?

[DanCooper]

No, i've tried to compare the results for movies, tv shows, season and episodes that are not listed on TMDB. For that i've use fictional IDs that do not exist like:
GetMovie("tt66666666")
GetTvShow(66666)
GetTvSeason(66666, 99)
GetTvEpisode(66666, 99, 99)

GetMovie and GetTvShow return 0 as Result.Id
Get TvSeason and GetTvEpisode return Nothing as Result.Id

IMO all functions should return the same value if there was no entry found on TMDB.
If you use existing IDs, the Result.Id contains a correct TMDB ID in each of this cases.

Sorry for my bad english, i hope you understand what i mean.

[LordMike]

I was just looking over this and came to realize: We return default objects for bad responses... Damn.

I had convinced myself we threw exceptions (this is related to #4). Maybe we should - either that or we return Null on errors (and then we lose a way of telling what went wrong). We have three options as I see it:

1. Throw exceptions
2. Return nulls or default(T)-equivalents
3. Wrap everything in XResponse, which has a Succeeded or Errors property

@Naliath thoughts?

[DanCooper]

Exactly, earlier was the result Result = Nothing, right?

[LordMike]

It might be. We changed from Restsharp to custom webcode using Newtonsoft.Json.. Restsharp might have thrown exceptions on error responses.... woops.

[DanCooper]

No problem, thank you for your good work.

[LordMike]

Scratch that. I just went back in code and made a request as I did before. Restsharp also gave us responses (and no exceptions). Many methods just returned Response.Data which can then be blank (e.g. default(T)).

So no big change, currently ... But we "should" issue exceptions, or something.

[LordMike]

@Naliath take a look at my code. It will throw exceptions at the base layer (thank god that was centralized :D). These will bubble up with the associated infoes attached.

I've seen in ElasticSearch's client, NEST, that they have a ThrowExceptions property on the client, which will determine if errors are reported via. exceptions or via. properties in the response objects. We could do the same, if anyone ever wants to use the library without exceptions (though I doubt it has a use case in this project).

[Naliath]

mmm I think you are on the right track other than the not found part. I corrected part of the RestSharp implementation and can confirm it did not throw any errors at any time. your data would be null and an error property would be set. Based on that we then threw some errors on our side if needed.

For this library I don't like the not found exception, it adds overhead and doesn't really help the consumer. Returning null would be more useful. This does present some issue such as with getting an id, returning the default there is not great. Perhaps we should make those operations return nullable int's?

[LordMike]

So if I change the internal stuff (RestResponse etc.) to include stuff like the return code, then the individual methods can determine what to do and how to handle errors / blanks. ?

[Naliath]

Sounds good. Only thing that would be removed from your general code is the not found handler and setting the code (possibly a isSuccessfull helper). in the methods themself we then might need to change some datatypes

[LordMike]

Check out the latest code. It contains a setup where GetMovie() returns null on notfound. It throws exceptions on bad responses.

A number of tests now fail because the tested methods normally returned false or similar on bad responses, where we now throw exceptions.

To finish this, I will have to go through each method and determine the correct response given an error case. ... should be interesting ;).

[Naliath]

Looks good, only thing I could see was this if
(!_response.IsSuccessStatusCode)

•        {
  

•            Task<string> content = GetContent();
  

•            Task.WaitAll(content);
  

•            Error = JsonConvert.DeserializeObject<TmdbStatusMessage>(content.Result);
  

•        }
  

That might throw and exception if there is a connection issue or something (just looking from the web portal atm so perhaps it is handled further up the chain)

[LordMike]

I've tested a little with no network and with odd responses (such as a proxy in between that throws "502" responses). The result is committed to the branch.

I've also greatly simplified the response object and decided that the content should be eagerly fetched.

[Naliath]

@LordMike Did we ever merge this? Or in other words is this still an issue? I'm working on issue #36 and should have something this weekend (bit of a pain in the arse). After that I can pick this up if it is still an open problem.

[LordMike]

I think I did something, but I never closed it because I didn't feel it was tested enough.
The surface area of this API is large, so testing all is cumbersome :P

But checking the code still reveals this:

            if (resp.IsSuccessStatusCode)
                return resp;

            if (!resp.IsSuccessStatusCode)
                return resp;

[LordMike]

Related #235 #194

[FrederikBolding]

@LordMike Are you considering doing something else or should we maybe go ahead and add something along the lines of what you suggested earlier in this thread? - It seems to me that throwing exceptions, even while a cumbersome task in regards to testing, is quite important for the overall quality of the library?

[LordMike]

Yea. The approach on returning Null's for not founds, and exceptions when other errors happen, is the way to go.

Additionally, a "ThrowOnNotFound" property (or better named) could control whether Not Found objects should throw a matching NotFoundException.

[LordMike]

Fixed in #275 - set the ThrowApiExceptions option on the client.