Search Filter Split, Use Same Controller #2454

askvortsov1 · 2020-11-14T07:51:18Z

Start of flarum/issue-archive#286
Supercedes #2453

Changes proposed in this pull request:

Split search and filter into separate logic
Add a "filterer" that applies filters and filter mutations for all resources
Add filter equivalent of default search gambits
Add integration tests for all filters and gambits present in core (this is probably 75% of the PR)

TODO:

Convert bundled extensions to use this system
Clean up code / improve abstractions
Convert ListPostsController to use this system (in a separate PR)

Reviewers should focus on:

If we register things both as filters and as gambits for SimpleFlarumSearch, we're gonna have a lot of duplicated code. That being said, I'm not sure if there's anything we can do here if we want to truly separate the two. Maybe gambits could implement both interfaces?
For simplicity and to reduce diff size, I omitted some of the abstractions present in the old search system for filtering (SearchCriteria, etc). I also reused some SimpleFlarumSearch utils / components, like SearchResults, ApplySearchParametersTrait, AbstractSearch. What do we want to do about this? IMO simpler is better, especially seeing that custom filtration drivers aren't going to be a thing, so unnecessary abstraction is unnecessary. Did I cut out too much? Also, I want to eventually move the shared stuff into a Query namespace, but didn't do so here to minimize diff.
Should we modify the new gambits system to also be applied based on key/value pairs in the filter param? And q could just be the fulltext search?

luceos

The arguments for the controller construct should be similar. In the DiscussionController the repository is the first argument, in the UsersController it's the last.

Great job!

Aside from that I see nothing wrong with this. How this impacts extensions is still a bit unclear and I think seeing it in action will give more insight in whether this is the best approach or not.

askvortsov1 · 2020-12-01T15:59:24Z

The arguments for the controller construct should be similar. In the DiscussionController the repository is the first argument, in the UsersController it's the last.

We generally stick to alphabetical order for constructor arguments, I think that's something worth sticking to.

One thing I'm considering is:

Defining a FilterCriteria class, like SearchCriteria, and passing that into the Filterer
Defining a Filterer class for each resource (which takes ModelRepository as a constructor dependency), and injecting THAT into the controllers

That would make the API for filtering much more similar to the API for searching, but isn't strictly necessary. Any thoughts @flarum/core @SychO9?

SychO9

I don't see any issues with using a single endpoint, I think it'll work just fine.

I'm leaning towards having a single class implementing both the gambit and the filter interfaces, even if we don't reduce code duplication, having them in one place would be better I think.

That would make the API for filtering much more similar to the API for searching, but isn't strictly necessary. Any thoughts

I would be in favour. It's more consistent with the search API, but also cleaner imho. Although having a filterer for each resource would mean more duplicated code similarly to resource searchers 🤔

src/Filter/Filterer.php

src/Extend/Filter.php

src/Filter/FilterServiceProvider.php

src/Discussion/Filter/UnreadFilter.php

src/User/Filter/GroupFilter.php

SychO9 · 2021-01-25T19:45:11Z

src/Filter/Filterer.php

+        $this->applyOffset($wrappedFilter, $offset);
+        $this->applyLimit($wrappedFilter, $limit + 1);
+
+        foreach (Arr::get(static::$filterMutators, $resource, []) as $mutator) {


should we dispatch a filtering event before this ?

Is there such a thing? Do you mean Searching? Not including it would be a breaking change, but we don't have an instance of AbstractSearch or SearchCriteria to include.

ah true, nvm then

@SychO9 hey, guess what, we now have an instance of AbstractSearch and SearchCriteria to include! Yay!!!

That being said, we don't have a list of applied gambits, so it's a partial BC patch if any. Since any extensions that use Searching or add gambits will almost definitely need to be updated anyways, I'm not sure if it's worth it.

Yeah I thought about that, I also don't think that we need to do that, we're good as is imho.

askvortsov1 · 2021-01-28T01:30:09Z

I'm leaning towards having a single class implementing both the gambit and the filter interfaces, even if we don't reduce code duplication, having them in one place would be better I think.

Any thoughts on where to house these? At one point I was thinking of setting up a Query namespace but that would break imports from the current Search namespace. I think under Filter might be more appropriate though, since filters won't really be replacable with an alternate driver

askvortsov1 · 2021-01-28T01:31:32Z

I'm leaning towards having a single class implementing both the gambit and the filter interfaces, even if we don't reduce code duplication, having them in one place would be better I think.

Any thoughts on where to house these? At one point I was thinking of setting up a Query namespace but that would break imports from the current Search namespace. I think under Filter might be more appropriate though, since filters won't really be replacable with an alternate driver

I would be in favour. It's more consistent with the search API, but also cleaner imho.

I'll leave this alone for now and work on the filter/gambit combo. If we were to make a Query namespace, some shared stuff could go there?

SychO9 · 2021-01-29T22:46:24Z

Any thoughts on where to house these? At one point I was thinking of setting up a Query namespace but that would break imports from the current Search namespace. I think under Filter might be more appropriate though, since filters won't really be replacable with an alternate driver

Yea that's the problem, I'd be fine with it under Filter (filters implementing the gambit interface), not sure about the other devs though.

askvortsov1 · 2021-02-10T06:00:01Z

I've taken some time to think about this, and I'm now 99% sure we need to have per-resource filterers that get passed a FilterCriteria argument, and are responsible for constructing their own data sources. It's just honestly more flexible and consistent with our search approach.

askvortsov1 · 2021-02-10T15:56:11Z

I've reimplemented Filterer to be abstract and have per-resource classes like with searchers, and looking over the code again, I think this was the right move. As a followup, there are several "abstraction" classes used by both filterers and searchers (SearchCriteria/FIlterCriteria, SearchResults, AbstractSearch/WrappedFilter). I think we could get away with combining these into a shared class (QueryCriteria, QueryResults, AbstractQuery (this one is challenging since AbstractSearch keeps track of applied gambits)). Only problem is, where should these be located in the code? Maybe a Query namespace would be useful (and then we could move all the FilterGambits there)? But this would also a breaking change for mutators that typehint AbstractSearch in their constructors.

One big benefit of having a common signature to the filter and search methods is that we could then run all queries through a central manager singleton, which would facilitate search drivers quite well.

SychO9 · 2021-02-11T15:41:51Z

src/Discussion/Filter/AuthorFilterGambit.php

-        if (! $search instanceof DiscussionSearch) {
-            throw new LogicException('This gambit can only be applied on a DiscussionSearch');
-        }


why are we dropping these ?

I'm not sure it's that useful. Core's gambits aren't really used outside of core, so little risk of them being applied to the wrong search. And removing it here doesn't really affect external gambits, although with those, now that gambits are registered directly (and not via an event) it's much less likely to attach a gambit to the wrong searcher.

Good point, doesn't hurt to have it though. Looking at the code, it seems to have been there because we can't explicitly typehint the conditions method with the exact search class, because the method signature has to be identical to the parent class's abstract method. And this bit of code also helps IDEs like phpStorm know the specific type of the $search object, which isn't really much useful at the moment considering these classes don't have anything more than their abstract parent (DiscussionSearch does have additional methods but none of them seem to be used anywhere).

That's fair. In that case, should WrappedFilter be redesigned as AbstractFilter, so we can have a similar system for filtering? I'm not sure it'd be that useful, since you wouldn't really have "relevant posts" when filtering.

While I'm all for consistency, I also don't really see it being of use.

For that matter, do we have a use case for separate "search" classes for each model? As you noted, the specialized methods/fields on the DiscussionSearch one aren't used, and extensions can't add new methods onto it, so there's no use there. Furthermore, if we made AbstractSearch non-abstract and just used that, it wouldn't block us from switching back to subclass of it eventually, since polymorphism. But if we were to get rid of it, that'd be yet another reason to switch to WrappedQuery for both filter and search, and WrappedSearch for search, which would keep track of applied gambits. Although preferably with better naming 😆

Indeed! That really seems to be the case, just need to make 100% sure those methods are not used, and then we could in a separate pull request look into the Query namespace (just to avoid more drastic changes in this PR so that other devs can review more easily).

Actually, never mind: DiscussionSearch has a $defaultSort field. And THAT could be applicable to filters too. Urgh....

I'll think some more about this, but I think we're nearing completion for this refactor other than this.

New proposal: https://github.com/flarum/core/issues/1355

src/Search/AbstractRegexGambit.php

[ci skip] [skip ci]

…tate

[ci skip] [skip ci]

This is already done in the searcher

$load is the mechanism, $include is the actual important bit.

On second thought, the searcher / filterer shouldn't be responsible for loading related items. It centralizes code, but isn't proper (and breaks `Discussion::setStateUser()`)

…id BC breaks

[ci skip] [skip ci]

tankerkiller125

Have not tested locally, but it looks good code wise as far as I can tell

askvortsov1 mentioned this pull request Nov 14, 2020

WIP: Split Search Filter and Search #2453

Closed

askvortsov1 changed the title ~~WIP: Search Filter Split Try 2~~ Search Filter Split Try 2 Nov 15, 2020

askvortsov1 changed the title ~~Search Filter Split Try 2~~ Search Filter Split, Use Same Controller Nov 15, 2020

askvortsov1 requested review from franzliedke, luceos, clarkwinkelmann, SychO9 and tankerkiller125 November 15, 2020 22:50

askvortsov1 mentioned this pull request Nov 30, 2020

Use filterer for ListPostsController #2479

Merged

2 tasks

luceos requested changes Dec 1, 2020

View reviewed changes

askvortsov1 force-pushed the as/search-filter-split-try-2 branch from 71c90b3 to 07fbdb1 Compare December 2, 2020 00:48

askvortsov1 requested a review from luceos December 2, 2020 00:48

askvortsov1 mentioned this pull request Dec 2, 2020

Search Extender and tests #2483

Merged

2 tasks

tankerkiller125 approved these changes Dec 6, 2020

View reviewed changes

askvortsov1 force-pushed the as/search-filter-split-try-2 branch from 07fbdb1 to 46674fa Compare January 19, 2021 23:39

SychO9 requested changes Jan 25, 2021

View reviewed changes

askvortsov1 force-pushed the as/search-filter-split-try-2 branch from 173056b to 583ed0d Compare February 1, 2021 18:50

tankerkiller125 self-requested a review February 5, 2021 16:48

askvortsov1 marked this pull request as draft February 10, 2021 05:58

askvortsov1 force-pushed the as/search-filter-split-try-2 branch from 30a06c2 to ca5ffcc Compare February 10, 2021 15:02

askvortsov1 marked this pull request as ready for review February 10, 2021 15:47

SychO9 reviewed Feb 15, 2021

View reviewed changes

SychO9 requested changes Feb 17, 2021

View reviewed changes

src/Search/AbstractRegexGambit.php Outdated Show resolved Hide resolved

askvortsov1 removed the request for review from franzliedke February 17, 2021 19:52

askvortsov1 and others added 25 commits February 21, 2021 18:08

Merge gambits into "FilterGambit"

6ccd7a5

Use Arr::get in CreatedFilterGambit

a8c9ae8

Inject filters and filter mutators instead of static

837253d

Apply fixes from StyleCI

60a1a2b

[ci skip] [skip ci]

Per-resource filter classes

94823ba

Apply fixes from StyleCI

8703956

[ci skip] [skip ci]

List controllers no longer need repositories

66976fc

Add missing imports

6b96457

Move gambit pattern to method instead of attribute for consistency

6f27690

Combine search and filter criteria classes

2838f11

Apply fixes from StyleCI

ad38f79

[ci skip] [skip ci]

Use null coalesce instead of shorthand ternary

9fcc7cd

Replace WrappedFilter and AbstractSearch with FilterState and SearchS…

70ebed2

…tate

Apply fixes from StyleCI

a217968

[ci skip] [skip ci]

Remove unnecessary load

41835a5

This is already done in the searcher

Rename $load to $include

40da823

$load is the mechanism, $include is the actual important bit.

Typehint AbstractSearcher return value

2b5b14d

Move $include out of searching / filtering

ba3994b

On second thought, the searcher / filterer shouldn't be responsible for loading related items. It centralizes code, but isn't proper (and breaks `Discussion::setStateUser()`)

Fix stupid mistakes

3d0d931

More fixes for stupid stuff, also temporarily loosen interface to avo…

947e5da

…id BC breaks

Apply fixes from StyleCI

993fbed

[ci skip] [skip ci]

Don't pass unused $include to filterer/searcher

ff4f227

Remove includes param from AbstractSearcher

5fb3f29

Add filter mutator params to docblock

5a4bc5a

Capitalization fix

97ac81a

askvortsov1 force-pushed the as/search-filter-split-try-2 branch from fe469ff to 97ac81a Compare February 21, 2021 23:08

tankerkiller125 approved these changes Feb 21, 2021

View reviewed changes

askvortsov1 merged commit 023871e into master Feb 24, 2021

askvortsov1 deleted the as/search-filter-split-try-2 branch February 24, 2021 16:17

askvortsov1 added this to the 0.1.0-beta.16 milestone Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search Filter Split, Use Same Controller #2454

Search Filter Split, Use Same Controller #2454

askvortsov1 commented Nov 14, 2020 •

edited

Loading

luceos left a comment

askvortsov1 commented Dec 1, 2020 •

edited

Loading

SychO9 left a comment

SychO9 Jan 25, 2021

askvortsov1 Jan 28, 2021 •

edited

Loading

SychO9 Jan 29, 2021

askvortsov1 Feb 22, 2021

SychO9 Feb 22, 2021

askvortsov1 commented Jan 28, 2021

askvortsov1 commented Jan 28, 2021

SychO9 commented Jan 29, 2021

askvortsov1 commented Feb 10, 2021

askvortsov1 commented Feb 10, 2021 •

edited

Loading

SychO9 Feb 11, 2021

askvortsov1 Feb 16, 2021

SychO9 Feb 16, 2021

askvortsov1 Feb 16, 2021

SychO9 Feb 16, 2021

askvortsov1 Feb 16, 2021 •

edited

Loading

SychO9 Feb 16, 2021

askvortsov1 Feb 17, 2021 •

edited

Loading

askvortsov1 Feb 17, 2021

askvortsov1 Feb 17, 2021

tankerkiller125 left a comment

Search Filter Split, Use Same Controller #2454

Search Filter Split, Use Same Controller #2454

Conversation

askvortsov1 commented Nov 14, 2020 • edited Loading

luceos left a comment

Choose a reason for hiding this comment

askvortsov1 commented Dec 1, 2020 • edited Loading

SychO9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 commented Jan 28, 2021

askvortsov1 commented Jan 28, 2021

SychO9 commented Jan 29, 2021

askvortsov1 commented Feb 10, 2021

askvortsov1 commented Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tankerkiller125 left a comment

Choose a reason for hiding this comment

askvortsov1 commented Nov 14, 2020 •

edited

Loading

askvortsov1 commented Dec 1, 2020 •

edited

Loading

askvortsov1 Jan 28, 2021 •

edited

Loading

askvortsov1 commented Feb 10, 2021 •

edited

Loading

askvortsov1 Feb 16, 2021 •

edited

Loading

askvortsov1 Feb 17, 2021 •

edited

Loading