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

Search implementation overhaul #2857

Merged
merged 193 commits into from Apr 20, 2018

Conversation

4 participants
@notbakaneko
Contributor

notbakaneko commented Mar 30, 2018

reorganized with a brick.

fixes #2707
fixes #2590

  • adds counts to each tab in /home/search

  • reduce forum search page size to 20

  • switched the beatmap listing caching from caching the search response to caching the transformed json output.

  • added the 100 user search result limit back in.

  • user quick search only returns recently active users.

  • Moved parts of the search queries that probably should not affect scoring to run in a filter query instead.

  • Separate search classes for different searches; parameters should be parsed before going into search.

  • Makes the parameters used for building the queries less dependent on the request parameters.

  • Explicit parameter handlers for search classes.

  • make more use of the query builder and typings when handling searches.

  • SearchParams comes with an interface for determining cacheability of queries.

  • Search::response() is automatically memoized.

  • querying elasticsearch for no term matching is now always considered different from ''

Was originally going to implement ->paginate() but that turned out to be a terrible interface to actually use for more complicated scenarios.

*/
public function overLimit()
{
return $this->response()->total() > config('osu.search.max.user');

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

create a maxResults() and then override it?

public function __construct(string $index, $type, SearchParams $params)
{
parent::__construct($index, $params);
$this->recordType = $type;

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

will there be case where index and type are from different thing? Or are they all just class name and its esIndexName?

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

index and model type can be different

This comment has been minimized.

@nanaya

nanaya Apr 18, 2018

Collaborator

which is...?

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

models won't have direct mappings to a single index, that's going to be removed.

This comment has been minimized.

@nanaya

nanaya Apr 18, 2018

Collaborator

not sure I understand what you mean ?_? are you saying models may have two indices or something?

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

models are not going to be coupled directly to indices; we can have a different index where it's valid to call ->records() to get Beatmapsets from indices that are not beatmap. RecordSearch is supposed to support any arbitrary combination.

class UserSearch extends RecordSearch
{
public function __construct(UserSearchParams $params)

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

Looks dumb defining the coupling in multiple places (AllSearch, QuickSearch) though.

'all' => null,
'user' => [
'type' => UserSearch::class,
'paramsType' => UserSearchRequestParams::class,

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

is it just me feeling weird seeing this manually specified? ಠ_ಠ (also in soon-to-be-gone(?) QuickSearch)

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

depends on how the new quick search is going to behave, so I'm leaving this here for now instead of string building the class names.

$this->params = $params;
if ($this->params->page !== null) {
$this->page($this->params->page);

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

isn't it a bit redundant having params in two places...?

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

could implement magic methods™ to reroute them but not sure how I feel about that..

This comment has been minimized.

@nanaya

nanaya Apr 18, 2018

Collaborator

why not just access the params directly...?

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

HasSearch shouldn't know about the params object and having all those values empty is a valid way for a search class to use its own defaults.

Also, I don't want to create a separate holder for the non-query context (page, size, source, etc) values just yet

This comment has been minimized.

@nanaya

nanaya Apr 18, 2018

Collaborator

or merge HasSearch to params?

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

HasSearch is usable as a base for ES joining queries, params contains stuff that shouldn't be included. Params is for inputs from some source to be parsed and allowed to be handled separately before a Search subclass is called; they aren't going to be combined

*/
public function total()
{
return min($this->response()->total(), config('osu.search.max.user'));

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

same thing with overLimit

return html_excerpt($post->source('search_content'));
}, iterator_to_array($firstPost)));
$user = $users->where('user_id', $entry->source('poster_id'))->first() ?? new App\Models\DeletedUser();

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

thankfully there shouldn't be too many users otherwise looping the whole thing repeatedly would be a bit slow.

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

When this was initially written, I was hoping the implementation would be smart enough since it didn't do any extra queries. Then, I actually looked at the implementation 🥉

The issue will be less the number of users and more that it's running array_filteron every iteration

<div class="page-mode__item">
<a
href="{{ route('search', ['mode' => $mode, 'query' => request('query')]) }}"
class="page-mode-link {{ isset($active) && $active ? 'page-mode-link--is-active' : '' }}"

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

The isset check is useless. Futhermore, the assignment itself is rather useless.

/**
* {@inheritdoc}
*/
public function getCacheKey() : string

This comment has been minimized.

@nanaya

nanaya Apr 17, 2018

Collaborator

having getCacheKey function when isCacheable always returning false doesn't seem useful.

This comment has been minimized.

@notbakaneko

notbakaneko Apr 18, 2018

Contributor

The conditions for caching will be worked out later so it's all disabled except for the beatmap listing.

This comment has been minimized.

@nanaya

nanaya Apr 18, 2018

Collaborator

and then the key will need updating thus rendering this one obsolete already 💃

@nanaya nanaya dismissed their stale review Apr 20, 2018

no sure

nekodex and others added some commits Apr 20, 2018

@nekodex nekodex merged commit 1d94797 into ppy:master Apr 20, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@notbakaneko notbakaneko deleted the notbakaneko:feature/search-update-calls branch Apr 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment