Skip to content
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

Optimize retrieving display name when searching for users in a group #32866

Merged
merged 8 commits into from
May 2, 2023

Conversation

CarlSchwan
Copy link
Member

This is a recurrent scenario where we are searching for users and then for
each user, we fetch the displayName. This is inefficient, so instead try
to do one query to fetch everything (e.g. Database backend) or use the
already existing DisplayNameCache helper.

Splitted from #32201

@CarlSchwan CarlSchwan requested review from blizzz, come-nc and a team June 13, 2022 16:59
@CarlSchwan CarlSchwan self-assigned this Jun 13, 2022
@CarlSchwan CarlSchwan requested review from PVince81 and removed request for a team June 13, 2022 16:59
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from 473c0b2 to e05dfe3 Compare June 13, 2022 17:01
lib/private/User/Manager.php Fixed Show resolved Hide resolved
lib/private/User/Manager.php Fixed Show fixed Hide fixed
@PVince81 PVince81 requested a review from icewind1991 June 14, 2022 07:11
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from e05dfe3 to e4e1cea Compare June 14, 2022 09:31
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from e4e1cea to 1e78349 Compare June 21, 2022 09:44
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from 1e78349 to c1986b5 Compare June 22, 2022 14:08
lib/public/Group/Backend/ABackend.php Fixed Show fixed Hide fixed
lib/public/Group/Backend/ABackend.php Fixed Show fixed Hide fixed
lib/public/Group/Backend/ABackend.php Fixed Show fixed Hide fixed
lib/public/Group/Backend/ABackend.php Fixed Show fixed Hide fixed
lib/public/Group/Backend/ABackend.php Fixed Show fixed Hide fixed
lib/private/Group/Database.php Outdated Show resolved Hide resolved
lib/private/Group/Database.php Outdated Show resolved Hide resolved
lib/private/Group/Database.php Outdated Show resolved Hide resolved
lib/public/Group/Backend/ABackend.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from c1986b5 to b4f3295 Compare June 23, 2022 17:52
lib/private/Group/Database.php Show resolved Hide resolved
lib/private/Group/Database.php Outdated Show resolved Hide resolved
lib/public/GroupInterface.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch 2 times, most recently from 2714e80 to 9e67ff9 Compare October 16, 2022 18:00
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from 9e67ff9 to 0544e8c Compare October 16, 2022 18:08
lib/private/User/Manager.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the performance/searchInGroup-displayname-cache branch from 0544e8c to c586cdb Compare October 20, 2022 10:12
@PVince81 PVince81 assigned come-nc and unassigned CarlSchwan Dec 19, 2022
@come-nc come-nc force-pushed the performance/searchInGroup-displayname-cache branch 3 times, most recently from 2cc2e13 to 82759db Compare December 22, 2022 11:22
@come-nc
Copy link
Contributor

come-nc commented Dec 22, 2022

We should decide if the array returned by usersInGroup and searchInGroup is using the uid as key or integers.
This is currently not consistent and why the tests fails.

I would say usersInGroup need to use int for BC, but searchInGroup could use uid as it may be handy in some cases.

@come-nc come-nc force-pushed the performance/searchInGroup-displayname-cache branch 2 times, most recently from ed19592 to 8e2c487 Compare January 2, 2023 16:57
@come-nc
Copy link
Contributor

come-nc commented Jan 2, 2023

This is ready for review I think.
I would like to test and confirm that it reduces requests though.

CarlSchwan and others added 6 commits April 27, 2023 11:57
This is recurrent scenario that we are searching for users and then for
each users we fetch the displayName. This is inefficient, so instead try
to do one query to fetch everything (e.g. Database backend) or use the
already existing DisplayNameCache helper.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Better for backward compatibility, also move new interfaces to nc 26

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…ion from ABackend

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
usersInGroup index by int for BC, searchInGroup index by uid (string).

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the performance/searchInGroup-displayname-cache branch from 8e2c487 to 346344c Compare April 27, 2023 10:05
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Also went back to searchUsers indexing the array by uid as it was the
 previous behavior and the IGroup phpdoc does not say anything about the
 keys.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added this to the Nextcloud 27 milestone May 2, 2023
@come-nc come-nc merged commit f7632f2 into master May 2, 2023
@come-nc come-nc deleted the performance/searchInGroup-displayname-cache branch May 2, 2023 14:42
@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label May 2, 2023
@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants