Improve search performance

[tobyzerner]

• Remove fulltext drivers so that fulltext search can take place directly in the main search query (important for performance)
• Improve the fulltext query to search discussion titles, sort by relevance
• Simplify the DiscussionSearcher, extract unnecessary eager loading responsibility
• Only return a single "most relevant post" in the API

Todo:

• Add migration to add fulltext index to discussions.title (we can leave the table as InnoDB - we should probably convert posts to InnoDB at some point too)
• Don't search posts that the user doesn't have permission to see! Same problem as filter out private posts before limiting the number of results #1334 - requires larger scale fix
• Update appearance of search results on frontend

Overhauled search extensibility API will come later (0.2?) but for now some level of search extension can still be achieved by replacing the default fulltext gambit with a custom one.

[franzliedke]

So this will only be used when searching, right? Because most_relevant_post_id will just be the name of a dynamically calculated column? If so, that should be made clear in the docblock.

[tobyzerner]

Yes, good point.

[franzliedke]

Do you already want a more detailed review here? If not, just let me know when this is no longer WIP.

[tobyzerner]

@franzliedke I will request one when I push some further changes soon, thanks :)

[luceos]

I was thinking related to my pr #1344, again we're adding mysql only raw statements in a migration. We should try to investigate whether we can encapsulate these statements in a conditional whether we're running the correct driver. Otherwise we'll never be able to support more db drivers 😞

[tobyzerner]

For the moment, the intention is for the gambit class itself is acting as the encapsulation, so it should probably be renamed to MySqlFulltextGambit.

Different search drivers (eg. ElasticSearch, Algolia) can be implemented by replacing this gambit via GambitManager::setFulltextGambit, the same would go for different database drivers I guess.

For 0.2 (or later) we have plans to overhaul the search API: https://github.com/flarum/core/issues/1355

[tobyzerner]

This is ready for review!

Here's the new look for search results:

[luceos]

@tobscure what I was referencing was the migration and the actual ALTER TABLE statements. These are going to kill installation on any other driver than mysql/mariadb.

One way would be to check the getDriverName() from the Connection class.

[tobyzerner]

@luceos Ah I see. True.

But let's defer until we actually implement support for other db drivers before changing the migrations. Even if we wrap in a conditional now, other db drivers still aren't going to work.

[luceos]

I gave it a spin and I'm very much impressed by the looks! Well done, picking part of the post really creates a rich search experience.

[franzliedke]

Looking good. 👍

[franzliedke]

Is this still WIP?

[tobyzerner]

Nope, it's done :D

[franzliedke]

@tobscure I am wondering... should we move the mostRelevantPost stuff into its own pseudo-model, or at least API resource. That way, we don't stuff the discussion model with things that are only relevant for search results. Could be something like searchResult or discussionMatchingSearch or something along those lines... duplicating the few discussion attributes that are needed to display the results?

[tobyzerner]

Good thought. Could be discussionSearchResult, and could have a relationship with a discussion and a mostRelevantPost?

[franzliedke]

Yeah, sounds good. Will you try it out?

[tobyzerner]

Actually, after more thinking, I don't think it makes sense to do this from a JSON-API perspective. GET /api/discussions?filter[q]=whatever must return a list of discussions, not discussionSearchResults.

[franzliedke]

True. We could do a separate endpoint just for searching, but at that point, it probably gets too crazy in another way. 😉

Thanks for considering!

[skywalker512]

@tobscure Mysql 5.5 can't use fulltext in InnoDB. It only support above 5.6. Will Flarum require mysql 5.6 at least.

[tobyzerner]

Have made a note in the relevant issue flarum/docs#14