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

fix: do not fetch LDAP display name all the time #38624

Closed
wants to merge 1 commit into from

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jun 2, 2023

Summary

  • use cache and rely on background update mechanism
  • with ajax cron it will still run
  • core User must not cache uid as displayname to address edge case (early announcement with displayname not ready). Also, the 98% case is that a displayname is set, I think.

One one hand this targets a theoretical/rare edge case, where the Accounts Manager would attempt to insert a row, and in the process fetches the display name, upon with a change would also insert the same row, ending in a database exception.

On the other hand, this removes a lot of LDAP requests only looking for the displayname. Like with the email address, this value would be only updated twice a day in the background and upon login (unless ajax is used as background job, then updates will happen always, which is slowing doing responsiveness).

Checklist

@blizzz blizzz added bug 3. to review Waiting for reviews labels Jun 2, 2023
@blizzz blizzz added this to the Nextcloud 28 milestone Jun 2, 2023
@@ -462,7 +462,7 @@
/**
* get display name of the user
* @param string $uid user ID of the user
* @return string|false display name
* @return ?string|false display name

Check notice

Code scanning / Psalm

ImplementedReturnTypeMismatch Note

The inherited return type 'string' for OCP\UserInterface::getDisplayName is different to the implemented return type for OCA\User_LDAP\User_LDAP::getdisplayname 'false|null|string'
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the interface this should always return a string. It seems other backends return the uid when a display name cannot be found.

@icewind1991
Copy link
Member

How does this work with new instances? If I setup a fresh instance with ldap, will it have no display names till cron has done it's thing?

@blizzz
Copy link
Member Author

blizzz commented Jun 5, 2023

How does this work with new instances? If I setup a fresh instance with ldap, will it have no display names till cron has done it's thing?

It's only about updates. Newly discovered users will have all their details set when fetched for the first time on demand.

@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 6, 2023
@blizzz

This comment was marked as off-topic.

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 6, 2023
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

A small refactor is needed to default to uid in getDisplayName directly in User_LDAP insead of returning null or false, otherwise good for me.

@@ -462,7 +462,7 @@
/**
* get display name of the user
* @param string $uid user ID of the user
* @return string|false display name
* @return ?string|false display name
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the interface this should always return a string. It seems other backends return the uid when a display name cannot be found.

@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
- use cache and rely on background update mechanism
- with ajax cron it will still run
- core User must not cache uid as displayname to address edge case (early
  announcement with displayname not ready)

Signed-off-by: Arthur Schiwon <[email protected]>
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@Altahrim Altahrim mentioned this pull request Mar 25, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@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 stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants