Skip to content
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

fix: add DB migration & endpoint to download missing TMDB People #1203

Merged

Conversation

fearnlj01
Copy link
Contributor

Optionally allows for the complete removal of the person/cast/crew entry if unable to update it.

This allows for a user-driven resolution to the problems that issue #1202 demonstrate.

Optionally allows for the complete removal of the person/cast/crew entry if unable to update it.
@fearnlj01
Copy link
Contributor Author

This is only a workaround to the original issue... but I needed this to fix my own database so figured that a PR wouldn't hurt, even if it gets closed.

In my own testing, I encountered an NRE for a person ID which appears to no longer exist on TMDB, hence this being the only thing that's caught.

Copy link
Member

@revam revam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would do better as a db migration once the core issue is fixed.

@fearnlj01 fearnlj01 requested a review from revam November 19, 2024 23:49
@fearnlj01
Copy link
Contributor Author

Confirming that this works fine™ in my SQLite setup.

I removed ~125 TMDB_Person records prior to launching the server, and after the migration occured, hitting the new endpoint resulted in no missing records being found.

Copy link
Member

@revam revam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things before we merge things. If you're unsure about something then you can add a comment here on GH or ask on discord.

Shoko.Server/Providers/TMDB/TmdbMetadataService.cs Outdated Show resolved Hide resolved
Shoko.Server/API/v3/Controllers/TmdbController.cs Outdated Show resolved Hide resolved
Shoko.Server/Providers/TMDB/TmdbMetadataService.cs Outdated Show resolved Hide resolved
Shoko.Server/Providers/TMDB/TmdbMetadataService.cs Outdated Show resolved Hide resolved
Shoko.Server/Providers/TMDB/TmdbMetadataService.cs Outdated Show resolved Hide resolved
Shoko.Server/Providers/TMDB/TmdbMetadataService.cs Outdated Show resolved Hide resolved
@fearnlj01 fearnlj01 marked this pull request as draft November 20, 2024 16:46
…ues.

Misc: Remove parallelization in `/TMDB/Person/DownloadMissing` endpoint
@fearnlj01
Copy link
Contributor Author

Updated in line with feedback now.

One issue I can forsee... If a TMDB show has been updated in the last calendar hour when the migration runs, it may not update as expected in the event TMDB returns null for a person.

N.B. I've used PersonRecords in favour of People; This is with the intention of keeping it as close to the name for the underlying table as possible. (I will change accordingly if so desired)

@revam
Copy link
Member

revam commented Nov 20, 2024

Updated in line with feedback now.

One issue I can forsee... If a TMDB show has been updated in the last calendar hour when the migration runs, it may not update as expected in the event TMDB returns null for a person.

Just run it with force refresh set to true.

N.B. I've used PersonRecords in favour of People; This is with the intention of keeping it as close to the name for the underlying table as possible. (I will change accordingly if so desired)

Personally, i would had just used peole.

@fearnlj01 fearnlj01 changed the title Misc: Add endpoint to download missing TMDB People Misc: Add DB migration & endpoint to download missing TMDB People Nov 20, 2024
@revam revam changed the title Misc: Add DB migration & endpoint to download missing TMDB People fix: add DB migration & endpoint to download missing TMDB People Nov 21, 2024
@fearnlj01 fearnlj01 marked this pull request as ready for review November 21, 2024 10:21
Copy link
Member

@revam revam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one nit-pick left, but I'll just fix that through GH.

Shoko.Server/Databases/MySQL.cs Outdated Show resolved Hide resolved
Shoko.Server/Databases/SQLite.cs Outdated Show resolved Hide resolved
@revam revam merged commit 85cac43 into ShokoAnime:master Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants