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

simplify and speed up search in groups #32197

Closed
wants to merge 3 commits into from

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Apr 27, 2022

Searching users in groups is actually implemented and should be reused

(didn't test myself yet :) )

To fix #32188

Searching users in groups is actually implemented and should be reused

Signed-off-by: Arthur Schiwon <[email protected]>
$groupUsers = $group->searchUsers('', $limit, $offset);
}

$users = $group->searchUsers(trim($search), $limit === -1 ? null : $limit, $offset);
Copy link
Member

Choose a reason for hiding this comment

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

I had the same idea but this is a search by userid and not disolay name. At least for the database backend, i have some changes locally to add a search by display name too, but I have not finished it.

Also probably a good idea to use the new DisplayNameCache

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same idea but this is a search by userid and not disolay name. At least for the database backend, i have some changes locally to add a search by display name too, but I have not finished it.

This explains it m(

Also probably a good idea to use the new DisplayNameCache

yeah

Copy link
Member Author

Choose a reason for hiding this comment

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

IGroup::searchDisplayName is what we should use. But the implementation is the same as searchUsers.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I push my work tomorrow morning 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@CarlSchwan is this what you are doing^?

LDAP backend does a proper search in any case. Normally a search for user IDs does not happen and does not have value in itself 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It's trickling in slowly 🧠. The group DB backend may not only check DB users, because it is decoupled. You can add LDAP users into local groups.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe cycle through the users and extend IUser with satisfy(string $search): bool. Downside: will still have a request per users against DB, LDAP or whichever source. Better extend user backend with a batch operations filterUsers(array $userIds, string $search): array.

Copy link
Member

Choose a reason for hiding this comment

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

It's trickling in slowly brain. The group DB backend may not only check DB users, because it is decoupled. You can add LDAP users into local groups.

$group->searchDisplayName will do a search for each group backend so it should still work if you have a ldap user inside a db group. I need to do a bit more testing ;)

This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

@blizzz ant update? wanna rebase? :)

@skjnldsv skjnldsv added the stale Ticket or PR with no recent activity label Feb 27, 2024
@skjnldsv skjnldsv marked this pull request as draft February 27, 2024 17:37
@blizzz
Copy link
Member Author

blizzz commented Mar 11, 2024

@blizzz ant update? wanna rebase? :)

🧹 💨 🤭 gosh, the dust! I am not aware what the state was with @CarlSchwan's changes; and also not having time currently. Pending between keeping it open indefinitely or closing.

This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv
Copy link
Member

skjnldsv commented May 2, 2024

Closing this pull request due to lack of recent activity and updates. We appreciate your contribution and encourage you to reopen or provide further updates if necessary. ☺️
Our aim is to keep the project moving forward with active collaboration. Thank you for your understanding and continued support! 🙏

@skjnldsv skjnldsv closed this May 2, 2024
@skjnldsv skjnldsv deleted the fix/32188/simplify-speedify-search-in-groups branch May 2, 2024 14:37
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug feature: users and groups performance 🚀 stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'searchUsers' executes slowly with ldapquery
3 participants