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

Add filter to meilisearch in backend #530

Merged
merged 13 commits into from
Jun 16, 2024

Conversation

scme0
Copy link
Contributor

@scme0 scme0 commented Jun 9, 2024

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).

@scme0 scme0 mentioned this pull request Jun 9, 2024
@zoriya zoriya added this to the v4.7.0 milestone Jun 9, 2024
@zoriya zoriya added enhancement New feature or request api labels Jun 9, 2024
@zoriya
Copy link
Owner

zoriya commented Jun 9, 2024

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 scme0 force-pushed the scme/add-filter-to-search branch from 35226d5 to 084ef0d Compare June 10, 2024 07:01
@scme0 scme0 marked this pull request as ready for review June 10, 2024 07:01
@scme0
Copy link
Contributor Author

scme0 commented Jun 10, 2024

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?

@scme0 scme0 changed the title (WIP) Add filter to meilisearch in backend Add filter to meilisearch in backend Jun 10, 2024
@scme0 scme0 requested a review from zoriya June 10, 2024 08:23
@zoriya
Copy link
Owner

zoriya commented Jun 10, 2024

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 Support library translations (name, descriptions, images of shows/movies...) #87, Support multi-episodes files #282 and Support multiples versions of episodes/movies #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
Copy link
Contributor Author

scme0 commented Jun 10, 2024

  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
Copy link
Owner

zoriya commented Jun 10, 2024

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
Copy link
Contributor Author

scme0 commented Jun 10, 2024

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?

@scme0 scme0 requested a review from zoriya June 10, 2024 09:13
@zoriya
Copy link
Owner

zoriya commented Jun 10, 2024

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
Copy link
Contributor Author

scme0 commented Jun 11, 2024

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

@zoriya
Copy link
Owner

zoriya commented Jun 12, 2024

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
Copy link
Contributor Author

scme0 commented Jun 12, 2024

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
Copy link
Owner

zoriya commented Jun 12, 2024

Yes

@zoriya zoriya enabled auto-merge June 16, 2024 15:20
@zoriya zoriya disabled auto-merge June 16, 2024 15:22
@zoriya zoriya merged commit 12f3808 into zoriya:master Jun 16, 2024
11 checks passed
@scme0 scme0 deleted the scme/add-filter-to-search branch June 16, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants