Skip to content

Commit 455fb74

Browse files
juliusknorrjuliusknorr
authored
Merge pull request nextcloud#52066 from nextcloud/perf/noid/dont-load-addressbook-on-resolving-cloudid
fix(federation): Don't load the addressbook when resolving a cloud ID
2 parents 3808f86 + 6a3c53d commit 455fb74

4 files changed

Lines changed: 69 additions & 42 deletions

File tree

‎lib/private/Federation/CloudId.php‎

Lines changed: 17 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -9,29 +9,15 @@
99
namespace OC\Federation;
1010

1111
use OCP\Federation\ICloudId;
12+
use OCP\Federation\ICloudIdManager;
1213

1314
class CloudId implements ICloudId {
14-
/** @var string */
15-
private $id;
16-
/** @var string */
17-
private $user;
18-
/** @var string */
19-
private $remote;
20-
/** @var string|null */
21-
private $displayName;
22-
23-
/**
24-
* CloudId constructor.
25-
*
26-
* @param string $id
27-
* @param string $user
28-
* @param string $remote
29-
*/
30-
public function __construct(string $id, string $user, string $remote, ?string $displayName = null) {
31-
$this->id = $id;
32-
$this->user = $user;
33-
$this->remote = $remote;
34-
$this->displayName = $displayName;
15+
public function __construct(
16+
protected string $id,
17+
protected string $user,
18+
protected string $remote,
19+
protected ?string $displayName = null,
20+
) {
3521
}
3622

3723
/**
@@ -44,12 +30,18 @@ public function getId(): string {
4430
}
4531

4632
public function getDisplayId(): string {
33+
if ($this->displayName === null) {
34+
/** @var CloudIdManager $cloudIdManager */
35+
$cloudIdManager = \OCP\Server::get(ICloudIdManager::class);
36+
$this->displayName = $cloudIdManager->getDisplayNameFromContact($this->getId());
37+
}
38+
39+
$atHost = str_replace(['http://', 'https://'], '', $this->getRemote());
40+
4741
if ($this->displayName) {
48-
$atPos = strrpos($this->getId(), '@');
49-
$atHost = substr($this->getId(), $atPos);
50-
return $this->displayName . $atHost;
42+
return $this->displayName . '@' . $atHost;
5143
}
52-
return str_replace('https://', '', str_replace('http://', '', $this->getId()));
44+
return $this->getUser() . '@' . $atHost;
5345
}
5446

5547
/**

‎lib/private/Federation/CloudIdManager.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ class CloudIdManager implements ICloudIdManager {
2828
/** @var IUserManager */
2929
private $userManager;
3030
private ICache $memCache;
31+
private ICache $displayNameCache;
3132
/** @var array[] */
3233
private array $cache = [];
3334

@@ -42,6 +43,7 @@ public function __construct(
4243
$this->urlGenerator = $urlGenerator;
4344
$this->userManager = $userManager;
4445
$this->memCache = $cacheFactory->createDistributed('cloud_id_');
46+
$this->displayNameCache = $cacheFactory->createDistributed('cloudid_name_');
4547
$eventDispatcher->addListener(UserChangedEvent::class, [$this, 'handleUserEvent']);
4648
$eventDispatcher->addListener(CardUpdatedEvent::class, [$this, 'handleCardEvent']);
4749
}
@@ -108,13 +110,18 @@ public function resolveCloudId(string $cloudId): ICloudId {
108110

109111
if (!empty($user) && !empty($remote)) {
110112
$remote = $this->ensureDefaultProtocol($remote);
111-
return new CloudId($id, $user, $remote, $this->getDisplayNameFromContact($id));
113+
return new CloudId($id, $user, $remote, null);
112114
}
113115
}
114116
throw new \InvalidArgumentException('Invalid cloud id');
115117
}
116118

117-
protected function getDisplayNameFromContact(string $cloudId): ?string {
119+
public function getDisplayNameFromContact(string $cloudId): ?string {
120+
$cachedName = $this->displayNameCache->get($cloudId);
121+
if ($cachedName !== null) {
122+
return $cachedName;
123+
}
124+
118125
$addressBookEntries = $this->contactsManager->search($cloudId, ['CLOUD'], [
119126
'limit' => 1,
120127
'enumeration' => false,
@@ -128,14 +135,17 @@ protected function getDisplayNameFromContact(string $cloudId): ?string {
128135
// Warning, if user decides to make their full name local only,
129136
// no FN is found on federated servers
130137
if (isset($entry['FN'])) {
138+
$this->displayNameCache->set($cloudId, $entry['FN'], 15 * 60);
131139
return $entry['FN'];
132140
} else {
141+
$this->displayNameCache->set($cloudId, $cloudID, 15 * 60);
133142
return $cloudID;
134143
}
135144
}
136145
}
137146
}
138147
}
148+
$this->displayNameCache->set($cloudId, $cloudId, 15 * 60);
139149
return null;
140150
}
141151

@@ -168,7 +178,7 @@ public function getCloudId(string $user, ?string $remote): ICloudId {
168178
$localUser = $this->userManager->get($user);
169179
$displayName = $localUser ? $localUser->getDisplayName() : '';
170180
} else {
171-
$displayName = $this->getDisplayNameFromContact($user . '@' . $host);
181+
$displayName = null;
172182
}
173183

174184
// For the visible cloudID we only strip away https

‎tests/lib/Federation/CloudIdManagerTest.php‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
<?php
2+
3+
declare(strict_types=1);
4+
25
/**
36
* SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
47
* SPDX-License-Identifier: AGPL-3.0-or-later
@@ -10,11 +13,15 @@
1013
use OC\Memcache\ArrayCache;
1114
use OCP\Contacts\IManager;
1215
use OCP\EventDispatcher\IEventDispatcher;
16+
use OCP\Federation\ICloudIdManager;
1317
use OCP\ICacheFactory;
1418
use OCP\IURLGenerator;
1519
use OCP\IUserManager;
1620
use Test\TestCase;
1721

22+
/**
23+
* @group DB
24+
*/
1825
class CloudIdManagerTest extends TestCase {
1926
/** @var IManager|\PHPUnit\Framework\MockObject\MockObject */
2027
protected $contactsManager;
@@ -36,7 +43,7 @@ protected function setUp(): void {
3643
$this->userManager = $this->createMock(IUserManager::class);
3744

3845
$this->cacheFactory = $this->createMock(ICacheFactory::class);
39-
$this->cacheFactory->method('createLocal')
46+
$this->cacheFactory->method('createDistributed')
4047
->willReturn(new ArrayCache(''));
4148

4249
$this->cloudIdManager = new CloudIdManager(
@@ -46,6 +53,7 @@ protected function setUp(): void {
4653
$this->cacheFactory,
4754
$this->createMock(IEventDispatcher::class)
4855
);
56+
$this->overwriteService(ICloudIdManager::class, $this->cloudIdManager);
4957
}
5058

5159
public function cloudIdProvider(): array {
@@ -70,7 +78,7 @@ public function testResolveCloudId(string $cloudId, string $user, string $noProt
7078
->willReturn([
7179
[
7280
'CLOUD' => [$cleanId],
73-
'FN' => 'Ample Ex',
81+
'FN' => $displayName,
7482
]
7583
]);
7684

@@ -92,9 +100,6 @@ public function invalidCloudIdProvider(): array {
92100

93101
/**
94102
* @dataProvider invalidCloudIdProvider
95-
*
96-
* @param string $cloudId
97-
*
98103
*/
99104
public function testInvalidCloudId(string $cloudId): void {
100105
$this->expectException(\InvalidArgumentException::class);

‎tests/lib/Federation/CloudIdTest.php‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
<?php
2+
3+
declare(strict_types=1);
4+
25
/**
36
* SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
47
* SPDX-License-Identifier: AGPL-3.0-or-later
@@ -7,25 +10,42 @@
710
namespace Test\Federation;
811

912
use OC\Federation\CloudId;
13+
use OC\Federation\CloudIdManager;
14+
use OCP\Federation\ICloudIdManager;
15+
use PHPUnit\Framework\MockObject\MockObject;
1016
use Test\TestCase;
1117

18+
/**
19+
* @group DB
20+
*/
1221
class CloudIdTest extends TestCase {
13-
public function dataGetDisplayCloudId() {
22+
protected CloudIdManager&MockObject $cloudIdManager;
23+
24+
protected function setUp(): void {
25+
parent::setUp();
26+
27+
$this->cloudIdManager = $this->createMock(CloudIdManager::class);
28+
$this->overwriteService(ICloudIdManager::class, $this->cloudIdManager);
29+
}
30+
31+
public function dataGetDisplayCloudId(): array {
1432
return [
15-
['test@example.com', 'test@example.com'],
16-
['test@http://example.com', 'test@example.com'],
17-
['test@https://example.com', 'test@example.com'],
33+
['test@example.com', 'test', 'example.com', 'test@example.com'],
34+
['test@http://example.com', 'test', 'http://example.com', 'test@example.com'],
35+
['test@https://example.com', 'test', 'https://example.com', 'test@example.com'],
36+
['test@https://example.com', 'test', 'https://example.com', 'Beloved Amy@example.com', 'Beloved Amy'],
1837
];
1938
}
2039

2140
/**
2241
* @dataProvider dataGetDisplayCloudId
23-
*
24-
* @param string $id
25-
* @param string $display
2642
*/
27-
public function testGetDisplayCloudId($id, $display): void {
28-
$cloudId = new CloudId($id, '', '');
43+
public function testGetDisplayCloudId(string $id, string $user, string $remote, string $display, ?string $addressbookName = null): void {
44+
$this->cloudIdManager->expects($this->once())
45+
->method('getDisplayNameFromContact')
46+
->willReturn($addressbookName);
47+
48+
$cloudId = new CloudId($id, $user, $remote);
2949
$this->assertEquals($display, $cloudId->getDisplayId());
3050
}
3151
}

0 commit comments

Comments
 (0)