Better result handling of email search by nickvergessen · Pull Request #8206 · nextcloud/server

nickvergessen · 2018-02-06T17:12:57Z

Local users should not be returned when searching for empty string
The limit of the response should be respected

Master version of #8205

rullzer · 2018-02-06T19:14:25Z

 if (!$this->shareeEnumeration) {
 $result['wide'] = [];
+ } else {
+ $result['wide'] = array_slice($result['wide'], $offset, $limit);


@nickvergessen Can we somehow limit that result at the query already? Otherwise it will still return a list of all users with mail addresses.

Can we somehow limit that result at the query already?
No, because they could have multiple emails (too many results) or could be internal and unreachable (too few results).
Otherwise it will still return a list of all users with mail addresses.
The query yes, but the API returns as per limit and offset with this change.

Ok, just found that it is an LDAP only issue, where the size is not limited. I'll create a new issue for that.

rullzer · 2018-02-06T19:14:59Z

 $addressBookContacts = $this->contactsManager->search($search, ['EMAIL', 'FN']);
 $lowerSearch = strtolower($search);
 foreach ($addressBookContacts as $contact) {
+ if ($search === '' && isset($contact['isLocalSystemBook'])) {


Mmmm that feels weird. Shouldn't the endpoint merge identical results. Because with such a plugin system I would not expect such logic....

Basically what I'm afraid of is that at some somebody wants to search only by email. And then this might do 💥
I mean worst case is that you get another 'maxResults' returned right?

This is only with empty search string. You already get max results of users from getUsers().
Also I would not know how it is possible, to specify which result types you want to receive, so you could only query for emails.

But yeah well we can also remove this and only keep the array_slice. That would improve the problem for the talk app already a lot.

Well if in some app you use https://github.com/nextcloud/server/blob/master/lib/public/Collaboration/Collaborators/ISearch.php#L42 and only pass the email share type ;)

Removed it for now. Lets do this :)

codecov · 2018-02-08T20:21:44Z

Codecov Report

Merging #8206 into master will decrease coverage by <.01%.
The diff coverage is 75%.

@@ Coverage Diff @@## master #8206 +/- ##
============================================
- Coverage 51.75% 51.75% -0.01% 
Complexity 25363 25363 ============================================
Files 1601 1601 Lines 95014 95018 +4 Branches 1377 1377 ============================================
+ Hits 49178 49179 +1 - Misses 45836 45839 +3

Impacted Files	Coverage Δ	Complexity Δ
...private/Collaboration/Collaborators/MailPlugin.php	`73.19% <100%> (-0.49%)`	`27 <0> (ø)`
...ivate/Collaboration/Collaborators/RemotePlugin.php	`72.58% <50%> (-0.76%)`	`17 <0> (ø)`
apps/files_trashbin/lib/Trashbin.php	`72.46% <0%> (-0.25%)`	`136% <0%> (ø)`

nickvergessen · 2018-02-14T15:43:09Z

@schiessle can you review this please?

MorrisJobke · 2018-02-20T11:08:57Z

CI fails:

Test\Collaboration\Collaborators\MailPluginTest::testSearch with data set #5 ('test', array(array('User3 @ Localhost'), array('User2 @ Localhost', array()), array('User @ Localhost', array('username@localhost'))), true, array(array(array('User @ Localhost (username@localhost)', array(4, 'username@localhost'))), array(array())), false, true)

nickvergessen · 2018-02-20T13:49:03Z

Fixed the tests

MorrisJobke · 2018-02-20T15:53:28Z

Fixed the tests

Not really :(

1) Test\Collaboration\Collaborators\RemotePluginTest::testSearch with data set #4 ('test', array(array('User3 @ Localhost'), array('User2 @ Localhost', array()), array('User @ Localhost', array('username@localhost'))), true, array(array(array('User @ Localhost (username@localhost)', array(6, 'username@localhost', 'localhost'))), array(array())), false, true)
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
'remotes' => Array (
- 0 => Array (...)
/drone/src/github.com/nextcloud/server/tests/lib/Collaboration/Collaborators/RemotePluginTest.php:101

1. Local users should not be returned when searching for empty string 2. The limit of the response should be respected Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2018-02-20T16:45:09Z

Not really :(

Right, we modified RemotePlugin too....
Fixed now.

nickvergessen added bug 3. to review Waiting for reviews backport-request labels Feb 6, 2018

nickvergessen added this to the Nextcloud 14 milestone Feb 6, 2018

nickvergessen requested review from MorrisJobke, blizzz and schiessle February 6, 2018 17:12

rullzer reviewed Feb 6, 2018
View reviewed changes

rullzer force-pushed the bugfix/noid/fix-collaboration-result-size branch from e1a5bf4 to fbb6988Compare February 8, 2018 20:16

rullzer approved these changes Feb 8, 2018
View reviewed changes

rullzer mentioned this pull request Feb 8, 2018
[stable13] Better result handling of email search #8267
Merged

rullzer removed the backport-request label Feb 8, 2018

juliusknorr mentioned this pull request Feb 20, 2018
Share panel makes navigator "extremely slow" and crashing nextcloud/deck#422
Closed

MorrisJobke approved these changes Feb 20, 2018
View reviewed changes

nickvergessen force-pushed the bugfix/noid/fix-collaboration-result-size branch from fbb6988 to f36663eCompare February 20, 2018 13:48

Better result handling of email search
b30e035
1. Local users should not be returned when searching for empty string 2. The limit of the response should be respected Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the bugfix/noid/fix-collaboration-result-size branch from f36663e to b30e035Compare February 20, 2018 16:39

blizzz approved these changes Feb 20, 2018
View reviewed changes

blizzz merged commit 8e5120c into masterFeb 20, 2018

blizzz deleted the bugfix/noid/fix-collaboration-result-size branch February 20, 2018 22:03

MorrisJobke mentioned this pull request Feb 26, 2018
Sharees endpoint doesn't respect limit for ldap users #8454
Closed

AsierraDEV mentioned this pull request May 22, 2018
Account cleaning - remains of deleted ldap users in tables. #9535
Closed

Uh oh!

Uh oh!

Conversation

nickvergessen commented Feb 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Feb 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nickvergessen commented Feb 14, 2018

Uh oh!

MorrisJobke commented Feb 20, 2018

Uh oh!

nickvergessen commented Feb 20, 2018

Uh oh!

MorrisJobke commented Feb 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickvergessen commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nickvergessen commented Feb 6, 2018•
edited
Loading

codecovBot commented Feb 8, 2018•
edited
Loading

MorrisJobke commented Feb 20, 2018•
edited
Loading