Skip to content

Improved remote user search#27996

Merged
butonic merged 8 commits intomasterfrom
improved-remote-user-search
Jun 22, 2017
Merged

Improved remote user search#27996
butonic merged 8 commits intomasterfrom
improved-remote-user-search

Conversation

@tomneedham
Copy link
Contributor

Description

Implemented paginated searching of (local|remote)addressbooks for improved search performance when searching through (multiple) very large addressbooks.

Also added configurable search properites for remote addressbook contact search - used for shareelookup on remote servers.

Motivation and Context

  • When connected to federated servers with large addressbooks, this search method is extremely slow and can use large amount of memory due to the lack of search limits and offsets.

How Has This Been Tested?

So far: unit tests

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.

@tomneedham tomneedham added this to the 10.0.2 milestone May 24, 2017
@tomneedham tomneedham self-assigned this May 24, 2017
// Search in contacts
//@todo Pagination missing
$addressBookContacts = $this->contactsManager->search($search, ['CLOUD', 'FN']);
$addressBookContacts = $this->contactsManager->search($search, $searchProperties, [], $this->limit, $this->offset);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@butonic thoughts on this? I think we should always include CLOUD and FN since they are needed to represent the user in the search dialog, but allow additional to be provided?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably give an occ command that just adds the email by setting it to CLOUD,FN,EMAIL. Discuss if we should make it the default?

@mrow4a
Copy link
Contributor

mrow4a commented May 24, 2017

Any update here, can I review?

@mrow4a
Copy link
Contributor

mrow4a commented May 25, 2017

Apart from doubt to @butonic about CLOUD and FN, looks good to me, no doubts here 👍

@PVince81 PVince81 modified the milestones: 10.0.2, 10.0.3 May 26, 2017
@tomneedham tomneedham force-pushed the improved-remote-user-search branch from 9d08ce6 to 82877b0 Compare June 7, 2017 12:15
@tomneedham
Copy link
Contributor Author

tomneedham commented Jun 12, 2017

Failing with php5.6 on pgsql... just jenkins weirdness. Tested with pg locally and fine, rebased and I think its fine now.

@tomneedham tomneedham force-pushed the improved-remote-user-search branch from b17b6a7 to 862c0e2 Compare June 14, 2017 11:48
@tomneedham
Copy link
Contributor Author

Rebased to kick jenkins into action

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these interfaces exist since 5.0.0 ... so make the new params optional

*/
public function search($pattern, $searchProperties, $options) {
$results = $this->backend->search($this->getKey(), $pattern, $searchProperties);
public function search($pattern, $searchProperties, $options, $limit, $offset) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add = null to make the params optional or give sane defaults.

* @since 5.0.0
*/
public function search($pattern, $searchProperties, $options);
public function search($pattern, $searchProperties, $options, $limit, $offset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add = null to make the params optional or give sane defaults.

@butonic butonic merged commit cb88d72 into master Jun 22, 2017
@butonic butonic deleted the improved-remote-user-search branch June 22, 2017 16:49
butonic pushed a commit that referenced this pull request Jun 23, 2017
* Use dav appconfig for customisable remote contact search properties

* Add limit and offset to remote user search

* Implement & fix carddavbackend search tests for limit and offset

* Fix sharee test to get search properties from config

* Improve sharee controller remote user search tests

* Include custom search properties in sharee exact matches

* Add explicit sorting of remote user searches

* Make addressbook search limit and offset optional
butonic pushed a commit that referenced this pull request Jun 26, 2017
* Use dav appconfig for customisable remote contact search properties

* Add limit and offset to remote user search

* Implement & fix carddavbackend search tests for limit and offset

* Fix sharee test to get search properties from config

* Improve sharee controller remote user search tests

* Include custom search properties in sharee exact matches

* Add explicit sorting of remote user searches

* Make addressbook search limit and offset optional
butonic pushed a commit that referenced this pull request Jun 28, 2017
* Use dav appconfig for customisable remote contact search properties

* Add limit and offset to remote user search

* Implement & fix carddavbackend search tests for limit and offset

* Fix sharee test to get search properties from config

* Improve sharee controller remote user search tests

* Include custom search properties in sharee exact matches

* Add explicit sorting of remote user searches

* Make addressbook search limit and offset optional
DeepDiver1975 pushed a commit that referenced this pull request Jul 6, 2017
* Use dav appconfig for customisable remote contact search properties

* Add limit and offset to remote user search

* Implement & fix carddavbackend search tests for limit and offset

* Fix sharee test to get search properties from config

* Improve sharee controller remote user search tests

* Include custom search properties in sharee exact matches

* Add explicit sorting of remote user searches

* Make addressbook search limit and offset optional
@lock
Copy link

lock bot commented Aug 3, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants