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 themoviedb rating matching #1927

Closed
wants to merge 3 commits into from

Conversation

JCalvi
Copy link

@JCalvi JCalvi commented Feb 20, 2023

Rating Type was looking for tmdb in plex audienceRatingImage when it should have been matching themoviedb.

Rating Type was looking for tmdb in plex audienceRatingImage when it should have been matching themoviedb.
@JCalvi
Copy link
Author

JCalvi commented Feb 21, 2023

This is potentially not right either. Kodi developers are still debating whether the tag should be tmdb or themoviedb in Kodi. There have been plenty of instances of both in the past so probably won't matter which it is. Would be nice to have a defined solution though.

I'll fix later when I get some time along with the ratings duplication bug, for now ignore this pull request.

Fix rating type to match themoviedb correctly as well as critics.
Also remove trakt and metacritic as these are not checked against plex and plex has no mechanism to make these tags.
@JCalvi
Copy link
Author

JCalvi commented Feb 21, 2023

Ok this one's done and verified working correctly for me now.
I get imdb, themoviedb, tvdb and tomatoes audience and critic ratings populating correctly.
default is now only applying to unrated items from plex.

Ratings were appending rows to the kodi rating table instead of replacing them.
Removed the add rating function and consolidated code to update_ratings function.
Function now deletes rows first with matching media_id, media_type and rating type before inserting new data.
Also streamlined calls to the function.
@JCalvi
Copy link
Author

JCalvi commented Feb 21, 2023

Croneter, sorry tried to raise a separate pull request but the rating append changes want to merge into this pull request.

@croneter
Copy link
Owner

croneter commented Feb 25, 2023

Thanks so much again, @JCalvi! I split your 3 commits. The first two concerning the rating matching (#1930) will be merged shorty.

I'm not so sure about the last one, JCalvi@77d0eb4. There's an additional SQL delete statement that will be executed even on the very first library sync. That seems overkill and will potentially increase sync time. I will look into it.

@JCalvi
Copy link
Author

JCalvi commented Feb 25, 2023

Thanks so much again, @JCalvi! I split your 3 commits. The first two concerning the rating matching (#1930) will be merged shorty.

I'm not so sure about the last one, JCalvi@77d0eb4. There's an additional SQL delete statement that will be executed even on the very first library sync. That seems overkill and will potentially increase sync time. I will look into it.

@croneter, Yes I considered this but often when working with databases, doing a check if you need to delete first in python is no faster than the sql database will take to work out it does not need to delete if there is nothing there.

I profiled my database doing a full repair with the delete, 742 Movies, 147 shows, 6862 Episodes and 4992 Songs
info : PLEX.utils: It took 0:00:56.494763 to run the function process_new_and_changed_items
info : PLEX.utils: It took 0:00:54.811351 to run the function process_new_and_changed_items

Without the delete:
info : PLEX.utils: It took 0:00:54.780497 to run the function process_new_and_changed_items
info : PLEX.utils: It took 0:00:55.071732 to run the function process_new_and_changed_items

Two runs for each which shows any differences are just random variation, so no penalty for the delete call.

The other advantage of the delete call is that it will clear the rating table that has been previously affected by this bug.

@JCalvi
Copy link
Author

JCalvi commented Feb 26, 2023

@croneter
I have an alternative for you to consider...

    def update_ratings(self, media_id, media_type, rating_type, rating, votes):
        """
        Adds or Updates a rating in the Kodi rating table
        Feed with media_id, media_type, rating_type, rating, rating
        """

        # Insert the new data
        self.cursor.execute('''
            INSERT OR REPLACE INTO
            rating(rating_id, media_id, media_type, rating_type, rating, votes)
            VALUES ((SELECT rating_id FROM rating WHERE media_id = ? AND media_type = ? AND rating_type = ? ),
            ?, ?, ?, ?, ?)
        ''', (media_id, media_type, rating_type, media_id, media_type, rating_type, rating, votes))
        return self.cursor.lastrowid

    @db.catch_operationalerrors

The only disadvantage of the delete version is that the rating_id keeps incrementing up every time the repair is run or a sync is done.
ON investigating sqlite (and the kodi rating table) this is not considered an issue as because the rating table is "integer primary key" and not autoincrement so it will keep going until it runs out of values and then just grab random unused keys again (if it were autoincrement it would then throw a table full error instead).
I tested this myself by setting the last rating_id to 9223372036854775807 ( 2^63 - 1 == 9223372036854775807 max integer value) and it did then use random other values for rating_id when I ran a repair again.
So unless you like to look at the database and see nice numbered primary keys from 1-max this is not an actual issue, it is just a number after all.

But if you still want an option that does not keep incrementing the primary key the above code only replaces items assuming they are unique based on the media_id, media_type and rating_type otherwise inserts a new one.
Profiling it seems to take slightly longer ~ 60s vs the 55-56s of the delete version, most likely the time to select the primary key for every update is slower than a delete command.
The other disadvantage of the above method I can think of are that it would not remove 'default' values and trim extra values in the table like the delete version did.

So in summary I think the delete version is better even though the rating table ends up looking a bit messier internally afterwards as it is faster and more robust.

@croneter
Copy link
Owner

I already implemented the delete version for the update rating function only, see #1933. Has been rolled out with yesterdays update

@croneter
Copy link
Owner

croneter commented Feb 26, 2023

Thanks so much for your thoughts though! 😄

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