zoriya / Kyoo

A portable and vast media library solution.
https://kyoo.zoriya.dev
GNU General Public License v3.0
1.32k stars 29 forks source link

Add filter to meilisearch in backend #530

Closed scme0 closed 2 weeks ago

scme0 commented 3 weeks ago

Required if we want to seamlessly add filtering to the browse page. Otherwise filtering won't work when users use the Search bar.

I've now tested this and it seems to work with enums (genres) and dates (airDate, startAir, endAir).

zoriya commented 3 weeks ago

Tested and it work for simple cases, what does not work is mostly due to our current implementation of meilisearch syncing.

Enums are stored as integer and not string values so comparisons fail DateTime/DateOnly are stored as string (and not unix timestamps) so meilisearch treat them as basic strings (see doc)

scme0 commented 3 weeks ago

Tested and it work for simple cases, what does not work is mostly due to our current implementation of meilisearch syncing.

Enums are stored as integer and not string values so comparisons fail DateTime/DateOnly are stored as string (and not unix timestamps) so meilisearch treat them as basic strings (see doc)

@zoriya I've updated the sync code to send enums as strings and DateTime/DateOnlys as unix timestamps. I've also made it resolve all elements in any enumerable types (for instance genres is an array of enums) so they are also filterable mith meili.

Now genres, dateAir, startAir, and endAir all seem to work correctly.

I have a few questions:

  1. Testing - I was considering writing some unit tests to test the conversion from a kyoo filter string to a meilisearch filter string but I don't see any Unit Test infrastructure in the backend solution.
  2. Should we only send the properties that are indexed to meilisearch? is there a reason to send the whole object? I suppose that means we could add indexes later easily.
  3. Is there a way to trigger a reload of all database entries into meili? When I've been testing this, I've been pulling down the whole system, deleting the databases and then reinstalling it all. Is there an easier way?
zoriya commented 3 weeks ago

Great job!

  1. I wrote most of the back a few years ago and wrote terrible tests at the time. I decided to simply remove them since they were a burden more than a help. I am actually considering making huge structural changes to the API to support things like #87, #282 and #283, so I don't want to invest too much on tests that would become irrelevant soon.
  2. The whole object is sent to meili since most properties can be used for filtering or sorting. Some are not used, but they should not affect meilisearch negatively so no need to manually remove them.
  3. No there is nothing setup to handle meilisearch migrations right now (or to resolve diverging information between postgres & meili, like if meili was down during an update). Not sure of how to handle those cases, do you have an idea?
scme0 commented 3 weeks ago
  1. No problems, that makes sense.
  2. Ah yeah sorry. I was only thinking about filtering (which is restricted to certain properties) but of course you can do a search over all the properties with the query parameter. 😅
  3. Yeah this a tricky one. You could potentially trigger it as part of a database migration? It would probably take a while though as you'd need to drop the meilisearch database and then iterate every document type and load them back in. But if it's not done, searches wont work correctly for some users. Maybe as an escape hatch we could add a button a bit like "rescan library" which reloads data into meilisearch.
zoriya commented 3 weeks ago

Yeah, I think doing that as part of the current migration process would be the easiest. Meilisearch works with upserts so we could just do something like:

for item in db.{Movie,Show,...}:
    meili.CreateOrUpdate(item)

After the discussion in #480, I want to rework migrations, but I don't think this will be done soon. Let's just use it inside the migration container for now (this would require adding meilisearch as a dependency of the migration container in docker-compose files).

Metadata refreshes also updates meilisearch, so the database can't drift too much. I'm fine not solving the drift issue if meili was unavailable during a db update for now.

scme0 commented 3 weeks ago

Ah yep, that would probably work. Meilisearch should be available when the migration is running as it doesn't depend on anything else?

I could look into it later.

How often does the metadata refresh?

zoriya commented 3 weeks ago

Depend on the air date of the item. Here is the snipet responsible for calculating the next refresh date:

    public static DateTime ComputeNextRefreshDate(DateOnly airDate)
    {
        int days = DateOnly.FromDateTime(DateTime.UtcNow).DayNumber - airDate.DayNumber;
        return days switch
        {
            <= 4 => DateTime.UtcNow.AddDays(1),
            <= 21 => DateTime.UtcNow.AddDays(14),
            _ => DateTime.UtcNow.AddMonths(2)
        };
    }
scme0 commented 3 weeks ago

@zoriya This is ready for another review when you have some time 🙏

zoriya commented 3 weeks ago

Apart from the escaping issue, it looks good. Do you want me to add the meilisearch's migration, or do I leave that to you?

scme0 commented 3 weeks ago

Apart from the escaping issue, it looks good. Do you want me to add the meilisearch's migration, or do I leave that to you?

What migration are you talking about? Adding a database migration which updates all documents in meilisearch?

zoriya commented 3 weeks ago

Yes