feat: locally cache frequently requested LDAP mapping data #54429

blizzz · 2025-08-14T16:56:45Z

Summary

We save that looking up DN by user/group ID is taking up to 5% of all database queries. Albeit they are simple and do not transfer much data, it is still worthwhile to reduce them, since this data barely ever changes.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

come-nc

I tend to be a bit scared about this kind of things, we cache stuff from LDAP to DB to avoid too many LDAP queries, and then we cache stuff from DB to Cache to avoid DB queries.

Why local cache and not distributed? Is it because the cache is local that there is the FIXME about broadcasting?

Why the limit to 2000 by default, what is the risk of putting too much lines in Cache? 1MiB does not sound like that much.

come-nc · 2025-08-18T07:58:39Z

apps/user_ldap/lib/Mapping/AbstractMapping.php

-	/** @var array caches Names (value) by DN (key) */
-	protected $cache = [];
+	protected function initLocalCache(): void {
+		if ($this->isCLI || !$this->cacheFactory->isLocalCacheAvailable()) {


Why not in cli?

For APCu does not have a shared cache. There is no benefit over the runtime cache that was already present.

come-nc · 2025-08-18T07:59:51Z

apps/user_ldap/lib/Mapping/AbstractMapping.php

+			return;
+		}
+
+		$section = \str_contains($this->getTableName(), 'user') ? 'u/' : 'g/';


Suggested change

$section = \str_contains($this->getTableName(), 'user') ? 'u/' : 'g/';

$section = $this->getTableName();

Why make it more complicated than this?

Why make it more complicated than this?

It is shorter and thus may save allocation of bytes in APCU.

apps/user_ldap/lib/Mapping/AbstractMapping.php

apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php

blizzz · 2025-10-15T10:44:06Z

I tend to be a bit scared about this kind of things, we cache stuff from LDAP to DB to avoid too many LDAP queries, and then we cache stuff from DB to Cache to avoid DB queries.

It showed that 5% of our queries are just those lookups.

UPDATE looking at current data it is 7% of all queries that are SELECT ldap_dn FROM oc_ldap_user_mapping WHERE owncloud_name = ? – and primarly during business hours.

Why local cache and not distributed? Is it because the cache is local that there is the FIXME about broadcasting?

Local cache is faster, and does not require any networking. There is also no global state here, but data that barely changes (and for that we have logic already).

Why the limit to 2000 by default, what is the risk of putting too much lines in Cache? 1MiB does not sound like that much.

When APCu overflows, then the entire cache is flushed, which then is quite expensive across the whole instance.

blizzz · 2025-10-15T13:02:20Z

/backport to stable32

artonge

Looks good to me besides nitpick

Does it mean that admin would need to flush the cache if they change one user's display name? If so, we'll need to update the documentation.

artonge · 2025-10-16T08:19:24Z

apps/user_ldap/lib/Mapping/AbstractMapping.php

+			return;
+		}
+
+		$useLocalCache = $this->config->getValueString('user_ldap', 'use_local_mapping_cache', 'auto', false);


Register it in the Lexicon?

👍 I'll check. I saw lexicon flying by, but don't really know what it does yet.

From a first glance, I suppose Lexicon support would be a follow-PR as we have other switches as well.

blizzz · 2025-10-16T08:47:51Z

Does it mean that admin would need to flush the cache if they change one user's display name? If so, we'll need to update the documentation.

No, this is unrelated to the display name. It only stores DNs to a userid, or false if there is none (e.g. local users are thrown against this through the user abstraction layer on userExists calls).

Signed-off-by: Arthur Schiwon <[email protected]>

- has the advantage that queries will be reported in the query.log when configured Signed-off-by: Arthur Schiwon <[email protected]>

blizzz · 2025-10-17T09:10:42Z

I think I can simulate a case where
* the local cache is initilized with false

* the dn has changed
that fails. It is a bit constructed. And actually, I can reproduce with plain master.

But I don't like the way it fails and getting out of this scenario might not be inconvenient.

Solved with https://github.com/nextcloud/server/pull/54429/files#diff-607694866b0eede88107508c374cd8cb64bd1518dda2bd98a6dae3dae25c4e27R73-R75

blizzz added this to the Nextcloud 32 milestone Aug 14, 2025

blizzz requested a review from a team as a code owner August 14, 2025 16:56

blizzz added the 2. developing Work in progress label Aug 14, 2025

blizzz requested review from Altahrim and removed request for a team August 14, 2025 16:56

blizzz added the performance 🚀 label Aug 14, 2025

blizzz requested review from come-nc and yemkareems August 14, 2025 16:56

blizzz marked this pull request as draft August 14, 2025 16:57

blizzz force-pushed the feat/noid/cache-ldap-mapping branch 3 times, most recently from 29d5976 to 4e03364 Compare August 15, 2025 16:44

come-nc reviewed Aug 18, 2025

View reviewed changes

skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025

blizzz force-pushed the feat/noid/cache-ldap-mapping branch from 4e03364 to 7d99e6e Compare October 15, 2025 13:01

backportbot bot added the backport-request label Oct 15, 2025

blizzz force-pushed the feat/noid/cache-ldap-mapping branch from 7d99e6e to 8fc543b Compare October 15, 2025 15:57

blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 15, 2025

blizzz marked this pull request as ready for review October 15, 2025 15:57

This comment was marked as resolved.

Sign in to view

blizzz force-pushed the feat/noid/cache-ldap-mapping branch from 8fc543b to d91ce32 Compare October 15, 2025 16:45

blizzz requested a review from CarlSchwan October 15, 2025 16:51

blizzz mentioned this pull request Oct 16, 2025

[stable32] 32.0.1 RC1 #55744

Merged

artonge approved these changes Oct 16, 2025

View reviewed changes

blizzz removed the 3. to review Waiting for reviews label Oct 16, 2025

blizzz added the 2. developing Work in progress label Oct 16, 2025

This comment was marked as resolved.

Sign in to view

blizzz force-pushed the feat/noid/cache-ldap-mapping branch 2 times, most recently from b3483f0 to 81e7d06 Compare October 16, 2025 11:45

come-nc approved these changes Oct 16, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

blizzz added 2 commits October 17, 2025 11:09

feat: locally cache frequently requested LDAP mapping data

155b750

Signed-off-by: Arthur Schiwon <[email protected]>

refactor(LDAP): switch from prepares statement to query builder

49f1c3f

- has the advantage that queries will be reported in the query.log when configured Signed-off-by: Arthur Schiwon <[email protected]>

blizzz force-pushed the feat/noid/cache-ldap-mapping branch from 81e7d06 to 49f1c3f Compare October 17, 2025 09:09

blizzz added 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress 3. to review Waiting for reviews labels Oct 17, 2025

blizzz mentioned this pull request Oct 17, 2025

[stable32] 32.0.1 RC2 #55822

Merged

11 tasks

blizzz merged commit 9d63530 into master Oct 17, 2025
209 of 219 checks passed

blizzz deleted the feat/noid/cache-ldap-mapping branch October 17, 2025 10:08

backportbot bot mentioned this pull request Oct 17, 2025

[stable32] feat: locally cache frequently requested LDAP mapping data #55823

Merged

backportbot bot removed the backport-request label Oct 17, 2025

	$section = \str_contains($this->getTableName(), 'user') ? 'u/' : 'g/';
	$section = $this->getTableName();

Uh oh!

feat: locally cache frequently requested LDAP mapping data #54429

feat: locally cache frequently requested LDAP mapping data #54429

Uh oh!

Conversation

blizzz commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

come-nc Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

blizzz Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

come-nc Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

blizzz Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blizzz commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented Oct 15, 2025

Uh oh!

This comment was marked as resolved.

artonge left a comment

Choose a reason for hiding this comment

Uh oh!

artonge Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

blizzz Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

blizzz Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

blizzz commented Oct 16, 2025

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

blizzz commented Oct 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

blizzz commented Aug 14, 2025 •

edited

Loading

blizzz commented Oct 15, 2025 •

edited

Loading