Merge pull request #52066 from nextcloud/perf/noid/dont-load-addressbook-on-resolving-cloudid

fix(federation): Don't load the addressbook when resolving a cloud ID

4 files changed, 69 insertions, 42 deletions

diff --git a/lib/private/Federation/CloudId.php b/lib/private/Federation/CloudId.php
index c20dbfc6418..b807c29d812 100644
--- a/lib/private/Federation/CloudId.php
+++ b/lib/private/Federation/CloudId.php
@@ -9,29 +9,15 @@ declare(strict_types=1);
 namespace OC\Federation;
 
 use OCP\Federation\ICloudId;
+use OCP\Federation\ICloudIdManager;
 
 class CloudId implements ICloudId {
-	/** @var string */
-	private $id;
-	/** @var string */
-	private $user;
-	/** @var string */
-	private $remote;
-	/** @var string|null */
-	private $displayName;
-
-	/**
-	 * CloudId constructor.
-	 *
-	 * @param string $id
-	 * @param string $user
-	 * @param string $remote
-	 */
-	public function __construct(string $id, string $user, string $remote, ?string $displayName = null) {
-		$this->id = $id;
-		$this->user = $user;
-		$this->remote = $remote;
-		$this->displayName = $displayName;
+	public function __construct(
+		protected string $id,
+		protected string $user,
+		protected string $remote,
+		protected ?string $displayName = null,
+	) {
 	}
 
 	/**
@@ -44,12 +30,18 @@ class CloudId implements ICloudId {
 	}
 
 	public function getDisplayId(): string {
+		if ($this->displayName === null) {
+			/** @var CloudIdManager $cloudIdManager */
+			$cloudIdManager = \OCP\Server::get(ICloudIdManager::class);
+			$this->displayName = $cloudIdManager->getDisplayNameFromContact($this->getId());
+		}
+
+		$atHost = str_replace(['http://', 'https://'], '', $this->getRemote());
+
 		if ($this->displayName) {
-			$atPos = strrpos($this->getId(), '@');
-			$atHost = substr($this->getId(), $atPos);
-			return $this->displayName . $atHost;
+			return $this->displayName . '@' . $atHost;
 		}
-		return str_replace('https://', '', str_replace('http://', '', $this->getId()));
+		return $this->getUser() . '@' . $atHost;
 	}
 
 	/**
diff --git a/lib/private/Federation/CloudIdManager.php b/lib/private/Federation/CloudIdManager.php
index 1c808c03eda..fa8ac0fa0c2 100644
--- a/lib/private/Federation/CloudIdManager.php
+++ b/lib/private/Federation/CloudIdManager.php
@@ -28,6 +28,7 @@ class CloudIdManager implements ICloudIdManager {
 	/** @var IUserManager */
 	private $userManager;
 	private ICache $memCache;
+	private ICache $displayNameCache;
 	/** @var array[] */
 	private array $cache = [];
 
@@ -42,6 +43,7 @@ class CloudIdManager implements ICloudIdManager {
 		$this->urlGenerator = $urlGenerator;
 		$this->userManager = $userManager;
 		$this->memCache = $cacheFactory->createDistributed('cloud_id_');
+		$this->displayNameCache = $cacheFactory->createDistributed('cloudid_name_');
 		$eventDispatcher->addListener(UserChangedEvent::class, [$this, 'handleUserEvent']);
 		$eventDispatcher->addListener(CardUpdatedEvent::class, [$this, 'handleCardEvent']);
 	}
@@ -108,13 +110,18 @@ class CloudIdManager implements ICloudIdManager {
 
 			if (!empty($user) && !empty($remote)) {
 				$remote = $this->ensureDefaultProtocol($remote);
-				return new CloudId($id, $user, $remote, $this->getDisplayNameFromContact($id));
+				return new CloudId($id, $user, $remote, null);
 			}
 		}
 		throw new \InvalidArgumentException('Invalid cloud id');
 	}
 
-	protected function getDisplayNameFromContact(string $cloudId): ?string {
+	public function getDisplayNameFromContact(string $cloudId): ?string {
+		$cachedName = $this->displayNameCache->get($cloudId);
+		if ($cachedName !== null) {
+			return $cachedName;
+		}
+
 		$addressBookEntries = $this->contactsManager->search($cloudId, ['CLOUD'], [
 			'limit' => 1,
 			'enumeration' => false,
@@ -128,14 +135,17 @@ class CloudIdManager implements ICloudIdManager {
 						// Warning, if user decides to make their full name local only,
 						// no FN is found on federated servers
 						if (isset($entry['FN'])) {
+							$this->displayNameCache->set($cloudId, $entry['FN'], 15 * 60);
 							return $entry['FN'];
 						} else {
+							$this->displayNameCache->set($cloudId, $cloudID, 15 * 60);
 							return $cloudID;
 						}
 					}
 				}
 			}
 		}
+		$this->displayNameCache->set($cloudId, $cloudId, 15 * 60);
 		return null;
 	}
 
@@ -168,7 +178,7 @@ class CloudIdManager implements ICloudIdManager {
 			$localUser = $this->userManager->get($user);
 			$displayName = $localUser ? $localUser->getDisplayName() : '';
 		} else {
-			$displayName = $this->getDisplayNameFromContact($user . '@' . $host);
+			$displayName = null;
 		}
 
 		// For the visible cloudID we only strip away https
diff --git a/tests/lib/Federation/CloudIdManagerTest.php b/tests/lib/Federation/CloudIdManagerTest.php
index 13e6b111864..14daef27142 100644
--- a/tests/lib/Federation/CloudIdManagerTest.php
+++ b/tests/lib/Federation/CloudIdManagerTest.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -10,11 +13,15 @@ use OC\Federation\CloudIdManager;
 use OC\Memcache\ArrayCache;
 use OCP\Contacts\IManager;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Federation\ICloudIdManager;
 use OCP\ICacheFactory;
 use OCP\IURLGenerator;
 use OCP\IUserManager;
 use Test\TestCase;
 
+/**
+ * @group DB
+ */
 class CloudIdManagerTest extends TestCase {
 	/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
 	protected $contactsManager;
@@ -36,7 +43,7 @@ class CloudIdManagerTest extends TestCase {
 		$this->userManager = $this->createMock(IUserManager::class);
 
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
-		$this->cacheFactory->method('createLocal')
+		$this->cacheFactory->method('createDistributed')
 			->willReturn(new ArrayCache(''));
 
 		$this->cloudIdManager = new CloudIdManager(
@@ -46,6 +53,7 @@ class CloudIdManagerTest extends TestCase {
 			$this->cacheFactory,
 			$this->createMock(IEventDispatcher::class)
 		);
+		$this->overwriteService(ICloudIdManager::class, $this->cloudIdManager);
 	}
 
 	public function cloudIdProvider(): array {
@@ -70,7 +78,7 @@ class CloudIdManagerTest extends TestCase {
 			->willReturn([
 				[
 					'CLOUD' => [$cleanId],
-					'FN' => 'Ample Ex',
+					'FN' => $displayName,
 				]
 			]);
 
@@ -92,9 +100,6 @@ class CloudIdManagerTest extends TestCase {
 
 	/**
 	 * @dataProvider invalidCloudIdProvider
-	 *
-	 * @param string $cloudId
-	 *
 	 */
 	public function testInvalidCloudId(string $cloudId): void {
 		$this->expectException(\InvalidArgumentException::class);
diff --git a/tests/lib/Federation/CloudIdTest.php b/tests/lib/Federation/CloudIdTest.php
index 4087f45622d..ca949d163c7 100644
--- a/tests/lib/Federation/CloudIdTest.php
+++ b/tests/lib/Federation/CloudIdTest.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -7,25 +10,42 @@
 namespace Test\Federation;
 
 use OC\Federation\CloudId;
+use OC\Federation\CloudIdManager;
+use OCP\Federation\ICloudIdManager;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
+/**
+ * @group DB
+ */
 class CloudIdTest extends TestCase {
-	public function dataGetDisplayCloudId() {
+	protected CloudIdManager&MockObject $cloudIdManager;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->cloudIdManager = $this->createMock(CloudIdManager::class);
+		$this->overwriteService(ICloudIdManager::class, $this->cloudIdManager);
+	}
+
+	public function dataGetDisplayCloudId(): array {
 		return [
-			['test@example.com', 'test@example.com'],
-			['test@http://example.com', 'test@example.com'],
-			['test@https://example.com', 'test@example.com'],
+			['test@example.com', 'test', 'example.com', 'test@example.com'],
+			['test@http://example.com', 'test', 'http://example.com', 'test@example.com'],
+			['test@https://example.com', 'test', 'https://example.com', 'test@example.com'],
+			['test@https://example.com', 'test', 'https://example.com', 'Beloved Amy@example.com', 'Beloved Amy'],
 		];
 	}
 
 	/**
 	 * @dataProvider dataGetDisplayCloudId
-	 *
-	 * @param string $id
-	 * @param string $display
 	 */
-	public function testGetDisplayCloudId($id, $display): void {
-		$cloudId = new CloudId($id, '', '');
+	public function testGetDisplayCloudId(string $id, string $user, string $remote, string $display, ?string $addressbookName = null): void {
+		$this->cloudIdManager->expects($this->once())
+			->method('getDisplayNameFromContact')
+			->willReturn($addressbookName);
+
+		$cloudId = new CloudId($id, $user, $remote);
 		$this->assertEquals($display, $cloudId->getDisplayId());
 	}
 }