jellyfin / TMDbLib

C#.Net library for TheMovieDB
MIT License
344 stars 128 forks source link

AggregateCredits implementation is incorrect/incomplete #410

Closed electroflame closed 1 year ago

electroflame commented 1 year ago

It looks like TMDB's API AggregateCredits returns a different data structure than the standard Credits callback.

For example, using Game of Thrones as a quick example:

AggregateCredits returns data like this (for cast):

{
"adult":false,
"gender":2,
"id":239019,
"known_for_department":"Acting",
"name":"Kit Harington",
"original_name":"Kit Harington",
"popularity":21.146,
"profile_path":"/4MqUjb1SYrzHmFSyGiXnlZWLvBs.jpg",
"roles":[{"credit_id":"5256c8af19c2956ff6047af6","character":"Jon Snow","episode_count":78}],
"total_episode_count":78,
"order":609
}

Credits returns data like this, for comparison:

{
"adult":false,
"gender":1,
"id":17286,
"known_for_department":"Acting",
"name":"Lena Headey",
"original_name":"Lena Headey",
"popularity":60.67,
"profile_path":"/scP1KnpPOwkzrDnhi2iMYldHZDk.jpg",
"character":"Cersei Lannister",
"credit_id":"5256c8ad19c2956ff60479ce",
"order":1
}

AggregateCredits is different from Credits for a few reasons, namely:

The current implementation just tries to map all of this to the standard Credits data structures, but fails pretty spectacularly, leaving most of the data null or empty as, for example, Character doesn't exist in aggregate.

The data for Crew seems to have similar issues.

I'm not sure if you think making a new class (i.e. AggregateCast) or something would be appropriate, or something else, but as it stands right now the AggregateCredits implementation is incomplete and potentially dangerous as the structures don't match (and you'll potentially have empty values you aren't expecting).

electroflame commented 1 year ago

@LordMike I wanted to let you know that I'm taking a look at this again -- I'm testing a preliminary fix right now.

Crew definitely has the same general issues as Cast, so I basically had to make new classes (CrewAggregate, CastAggregate) and then wire them up to a parent CreditsAggregate class. Additionally, CrewAggregate needed a CrewJob class and CastAggregate needed a CastRole to store the extra data returned by TMDB's jobs and roles, respectively.

Everything seems to be working correctly (although I really only needed Cast, so Crew is just getting a surface-level test), so if it holds up I'll send in a pull request.

electroflame commented 1 year ago

@LordMike I just submitted that pull request (#412). Hopefully everything looks good!

electroflame commented 1 year ago

412 fixes this, so this issue is now resolved.