Configurable minimum characters before autocomplete user searches #30798

VicDeo · 2018-03-15T20:10:35Z

Description

allow limit user/group search by a certain number of characters

Related Issue

#30313

Motivation and Context

For large user lists searching for the first two characters can be very slow to return.

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

VicDeo · 2018-03-15T20:33:46Z

@PVince81 according to your spec from #30313 (comment)

Backend code UserManager and GroupManager refuse to work if the search pattern given to any find* and search* methods is shorter than the minimum. This saves us for modifying backend for any other endpoints.

I don't see a better place to block a search pattern length. As for decorators: it's possible to extend User|Group manager with LimitedUserManager | LimitedGroupManager class, overload the methods in question and make OC::$server to return LimitedUserManager instead of UserManager. But it looks too nerdy to me 👓

PVince81 · 2018-03-19T11:26:48Z

But adding isSearchable on the IUserManager interface also feels wrong, especially considering that it's not really a capability with that name but a utility function.

@DeepDiver1975 @butonic @jvillafanez any other fancy ideas ?

VicDeo · 2018-03-20T15:45:31Z

@PVince81 I have a fancy idea: non-static method inside \OCP\Util that makes DI possible.

PVince81 · 2018-03-20T21:10:10Z

@VicDeo sounds good. You can even invent new utility services in the server container that suit our purposes.

jvillafanez · 2018-03-23T12:37:32Z

Maybe it's a good time to think about error handling in the user / group manager... returning an empty result just because you don't want to search with less than 4 chars feels bad because any user could think that there isn't any user, not that you're refusing to search with such little information.
Throwing an exception would be better, but it will likely need a lot of changes.

The problem is that the user / group manager doesn't specify any exception that could be thrown, so I don't expect any error handling to be in place, not even the default "catch all" exception handling with the "something went wrong" message.

For the specific check, it depends on how we want this to progress. If there are no plans to add similar functionality, adding the functions in the same class is fine if we consider it as implementation detail (and make those functions private). On the other hand, if we want to make restrictions based on the user type (maybe we want to allow the admin to do a less restrictive search), or any other, we'll likely need to move the functions to an specialized class.

codecov · 2018-03-27T18:40:53Z

Codecov Report

Merging #30798 into master will increase coverage by <.01%.
The diff coverage is 88%.

@@             Coverage Diff              @@
##             master   #30798      +/-   ##
============================================
+ Coverage      62.3%    62.3%   +<.01%     
+ Complexity    18396    18241     -155     
============================================
  Files          1141     1142       +1     
  Lines         68155    68177      +22     
  Branches       1232     1232              
============================================
+ Hits          42461    42481      +20     
- Misses        25333    25335       +2     
  Partials        361      361

Flag	Coverage Δ	Complexity Δ
#javascript	`52.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`63.48% <88%> (ø)`	`18241 <9> (-155)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
core/js/sharedialogview.js	`76.41% <ø> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Group/Manager.php	`90% <100%> (+0.11%)`	`77 <1> (+1)`	⬆️
lib/public/Util/UserSearch.php	`100% <100%> (ø)`	`4 <4> (?)`
lib/private/Server.php	`84.98% <100%> (+0.09%)`	`254 <0> (ø)`	⬇️
apps/files_sharing/lib/Capabilities.php	`100% <100%> (ø)`	`6 <1> (ø)`	⬇️
lib/private/User/Manager.php	`80.95% <70%> (-0.61%)`	`56 <3> (+3)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 744f742...7666497. Read the comment docs.

PVince81 · 2018-03-29T06:49:45Z

returning an empty result just because you don't want to search with less than 4 chars feels bad because any user could think that there isn't any user, not that you're refusing to search with such little information

The other alternative is finding all the current HTTP API endpoints that we have and add the limit there. This includes the sharing autocomplete, custom groups and potentially others (gallery app old share dialog)

PVince81 · 2018-03-29T06:50:29Z

lib/public/Util/UserSearch.php

+/**
+ * This class provides functions to search users
+ *
+ * @since 10.1.0


make that 10.0.8

PVince81

Fair enough, it looks like it's the best compromise 👍

see comment

VicDeo · 2018-04-02T12:57:27Z

Comment addressed.
Fixed tests, rebased, squashed.

PVince81 · 2018-04-03T07:22:14Z

@VicDeo please backport

PVince81 · 2018-04-03T07:23:36Z

lib/public/Util/UserSearch.php

+	 * @return mixed
+	 */
+	public function getSearchMinLength() {
+		return $this->config->getSystemValue('user.search_min_length', 4);


@pmaier1 note: now the minimum search length will be 4 by default, we'll add this to the release notes.
If an admin wants less or 0 they must change it manually.

@PVince81 we decided allow zero length for backward compatibility reasons. /But I think this is a subject to remove and shouldn't be advertised/

PVince81 · 2018-04-03T12:57:34Z

follow up for missed test failure: #30992

PVince81 · 2018-04-03T13:08:11Z

@VicDeo can you backport with both PRs ?

VicDeo · 2018-04-03T13:11:30Z

@PVince81 Sure. Do you mean 'both PRs as single PR'?

VicDeo · 2018-04-03T19:43:28Z

Stable10: #30994

lock · 2019-07-31T14:01:40Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

VicDeo added the 2 - Developing label Mar 15, 2018

VicDeo added this to the development milestone Mar 15, 2018

VicDeo self-assigned this Mar 15, 2018

VicDeo force-pushed the fix-30313 branch from 2f08494 to aed37ae Compare March 27, 2018 18:40

VicDeo force-pushed the fix-30313 branch 2 times, most recently from f3530c9 to 4c4332f Compare March 27, 2018 19:13

PVince81 reviewed Mar 29, 2018

View reviewed changes

PVince81 approved these changes Mar 29, 2018

View reviewed changes

VicDeo force-pushed the fix-30313 branch 5 times, most recently from 7990475 to bd606d0 Compare March 29, 2018 17:15

VicDeo changed the title ~~[WIP] Configurable minimum characters before autocomplete user searches~~ Configurable minimum characters before autocomplete user searches Mar 29, 2018

VicDeo force-pushed the fix-30313 branch 2 times, most recently from b28153d to 7bb43f1 Compare March 29, 2018 20:00

VicDeo added 3 - To Review and removed 2 - Developing labels Mar 29, 2018

VicDeo force-pushed the fix-30313 branch from 7bb43f1 to 00789e5 Compare March 31, 2018 15:46

Introduce UserSearch utils

7666497

VicDeo force-pushed the fix-30313 branch from 00789e5 to 7666497 Compare April 2, 2018 12:55

PVince81 merged commit b6be432 into master Apr 3, 2018

PVince81 deleted the fix-30313 branch April 3, 2018 07:22

PVince81 added the backport-request label Apr 3, 2018

PVince81 reviewed Apr 3, 2018

View reviewed changes

VicDeo mentioned this pull request Apr 3, 2018

[Stable10] Introduce UserSearch utils #30994

Merged

VicDeo removed the backport-request label Apr 3, 2018

VicDeo mentioned this pull request Apr 4, 2018

Configurable minimum characters before autocomplete user searches #30313

Closed

phil-davis mentioned this pull request Apr 5, 2018

Add longer group names added in core 30798 owncloud/user_ldap#222

Merged

PVince81 mentioned this pull request Apr 10, 2018

[stable10] Share autocomplete in frontend starts at one char #31067

Merged

9 tasks

individual-it mentioned this pull request Apr 12, 2018

UI tests for sharing autocompletion with min-char setting #31095

Merged

9 tasks

phil-davis mentioned this pull request Apr 13, 2018

occ user and group list cannot filter on short names #31117

Closed

This was referenced Apr 17, 2018

10.0.8RC1/2/3 - Changelog Testing #31098

Closed

config.sample.php user.search_min_length #31156

Merged

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable minimum characters before autocomplete user searches #30798

Configurable minimum characters before autocomplete user searches #30798

VicDeo commented Mar 15, 2018 •

edited

Loading

VicDeo commented Mar 15, 2018 •

edited

Loading

PVince81 commented Mar 19, 2018

VicDeo commented Mar 20, 2018

PVince81 commented Mar 20, 2018

jvillafanez commented Mar 23, 2018

codecov bot commented Mar 27, 2018 •

edited

Loading

PVince81 commented Mar 29, 2018

PVince81 Mar 29, 2018

PVince81 left a comment

VicDeo commented Apr 2, 2018

PVince81 commented Apr 3, 2018

PVince81 Apr 3, 2018

VicDeo Apr 3, 2018 •

edited

Loading

PVince81 commented Apr 3, 2018

PVince81 commented Apr 3, 2018

VicDeo commented Apr 3, 2018

VicDeo commented Apr 3, 2018

lock bot commented Jul 31, 2019

Configurable minimum characters before autocomplete user searches #30798

Configurable minimum characters before autocomplete user searches #30798

Conversation

VicDeo commented Mar 15, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

VicDeo commented Mar 15, 2018 • edited Loading

PVince81 commented Mar 19, 2018

VicDeo commented Mar 20, 2018

PVince81 commented Mar 20, 2018

jvillafanez commented Mar 23, 2018

codecov bot commented Mar 27, 2018 • edited Loading

Codecov Report

PVince81 commented Mar 29, 2018

PVince81 Mar 29, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

VicDeo commented Apr 2, 2018

PVince81 commented Apr 3, 2018

PVince81 Apr 3, 2018

Choose a reason for hiding this comment

VicDeo Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

PVince81 commented Apr 3, 2018

PVince81 commented Apr 3, 2018

VicDeo commented Apr 3, 2018

VicDeo commented Apr 3, 2018

lock bot commented Jul 31, 2019

VicDeo commented Mar 15, 2018 •

edited

Loading

VicDeo commented Mar 15, 2018 •

edited

Loading

codecov bot commented Mar 27, 2018 •

edited

Loading

VicDeo Apr 3, 2018 •

edited

Loading