Skip to content

Commit 4236d0c

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #54423 from nextcloud/backport/54120/stable31
[stable31] fix(search): Fix SearchComposer.php filtering logic
2 parents 74f838d + 8150036 commit 4236d0c

File tree

2 files changed

+300
-8
lines changed

2 files changed

+300
-8
lines changed

‎lib/private/Search/SearchComposer.php‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ private function loadLazyProviders(?string $targetProviderId = null): void {
118118
}
119119
}
120120

121-
$this->providers = $this->filterProviders($this->providers);
121+
$this->filterProviders();
122122

123123
$this->loadFilters();
124124
}
@@ -211,19 +211,21 @@ function (array $providerData) use ($route, $routeParameters) {
211211

212212
/**
213213
* Filter providers based on 'unified_search.providers_allowed' core app config array
214-
* @param array $providers
215-
* @return array
214+
* Will remove providers that are not in the allowed list
216215
*/
217-
private function filterProviders(array $providers): array {
216+
private function filterProviders(): void {
218217
$allowedProviders = $this->appConfig->getValueArray('core', 'unified_search.providers_allowed');
219218

220219
if (empty($allowedProviders)) {
221-
return $providers;
220+
return;
222221
}
223222

224-
return array_values(array_filter($providers, function ($p) use ($allowedProviders) {
225-
return in_array($p['id'], $allowedProviders);
226-
}));
223+
foreach (array_keys($this->providers) as $providerId) {
224+
if (!in_array($providerId, $allowedProviders, true)) {
225+
unset($this->providers[$providerId]);
226+
unset($this->handlers[$providerId]);
227+
}
228+
}
227229
}
228230

229231
private function fetchIcon(string $appId, string $providerId): string {

‎tests/lib/Search/SearchComposerTest.php‎

Lines changed: 290 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,290 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace Test\Search;
11+
12+
use InvalidArgumentException;
13+
use OC\AppFramework\Bootstrap\Coordinator;
14+
use OC\AppFramework\Bootstrap\RegistrationContext;
15+
use OC\AppFramework\Bootstrap\ServiceRegistration;
16+
use OC\Search\SearchComposer;
17+
use OCP\IAppConfig;
18+
use OCP\IURLGenerator;
19+
use OCP\IUser;
20+
use OCP\Search\IInAppSearch;
21+
use OCP\Search\IProvider;
22+
use OCP\Search\ISearchQuery;
23+
use PHPUnit\Framework\MockObject\MockObject;
24+
use Psr\Container\ContainerInterface;
25+
use Psr\Log\LoggerInterface;
26+
use Test\TestCase;
27+
28+
class SearchComposerTest extends TestCase {
29+
private Coordinator&MockObject $bootstrapCoordinator;
30+
private ContainerInterface&MockObject $container;
31+
private IURLGenerator&MockObject $urlGenerator;
32+
private LoggerInterface&MockObject $logger;
33+
private IAppConfig&MockObject $appConfig;
34+
private SearchComposer $searchComposer;
35+
36+
protected function setUp(): void {
37+
parent::setUp();
38+
39+
$this->bootstrapCoordinator = $this->createMock(Coordinator::class);
40+
$this->container = $this->createMock(ContainerInterface::class);
41+
$this->urlGenerator = $this->createMock(IURLGenerator::class);
42+
$this->logger = $this->createMock(LoggerInterface::class);
43+
$this->appConfig = $this->createMock(IAppConfig::class);
44+
45+
$this->searchComposer = new SearchComposer(
46+
$this->bootstrapCoordinator,
47+
$this->container,
48+
$this->urlGenerator,
49+
$this->logger,
50+
$this->appConfig
51+
);
52+
53+
$this->setupUrlGenerator();
54+
}
55+
56+
private function setupUrlGenerator(): void {
57+
$this->urlGenerator->method('imagePath')
58+
->willReturnCallback(function ($appId, $imageName) {
59+
return "/apps/$appId/img/$imageName";
60+
});
61+
}
62+
63+
private function setupEmptyRegistrationContext(): void {
64+
$this->bootstrapCoordinator->expects($this->once())
65+
->method('getRegistrationContext')
66+
->willReturn(null);
67+
}
68+
69+
private function setupAppConfigForAllowedProviders(array $allowedProviders = []): void {
70+
$this->appConfig->method('getValueArray')
71+
->with('core', 'unified_search.providers_allowed')
72+
->willReturn($allowedProviders);
73+
}
74+
75+
/**
76+
* @param array<string, array{service: string, appId: string, order: int, isInApp?: bool}> $providerConfigs
77+
* @return array{registrations: ServiceRegistration[], providers: IProvider[]}
78+
*/
79+
private function createMockProvidersAndRegistrations(array $providerConfigs): array {
80+
$registrations = [];
81+
$providers = [];
82+
$containerMap = [];
83+
84+
foreach ($providerConfigs as $providerId => $config) {
85+
// Create registration mock
86+
$registration = $this->createMock(ServiceRegistration::class);
87+
$registration->method('getService')->willReturn($config['service']);
88+
$registration->method('getAppId')->willReturn($config['appId']);
89+
$registrations[] = $registration;
90+
91+
// Create provider mock
92+
$providerClass = $config['isInApp'] ?? false ? IInAppSearch::class : IProvider::class;
93+
$provider = $this->createMock($providerClass);
94+
$provider->method('getId')->willReturn($providerId);
95+
$provider->method('getName')->willReturn("Provider $providerId");
96+
$provider->method('getOrder')->willReturn($config['order']);
97+
98+
$providers[$providerId] = $provider;
99+
$containerMap[] = [$config['service'], $provider];
100+
}
101+
102+
$this->container->expects($this->exactly(count($providerConfigs)))
103+
->method('get')
104+
->willReturnMap($containerMap);
105+
106+
return ['registrations' => $registrations, 'providers' => $providers];
107+
}
108+
109+
private function setupRegistrationContextWithProviders(array $registrations): void {
110+
$registrationContext = $this->createMock(RegistrationContext::class);
111+
$registrationContext->method('getSearchProviders')->willReturn($registrations);
112+
113+
$this->bootstrapCoordinator->expects($this->once())
114+
->method('getRegistrationContext')
115+
->willReturn($registrationContext);
116+
}
117+
118+
public function testGetProvidersWithNoRegisteredProviders(): void {
119+
$this->setupEmptyRegistrationContext();
120+
121+
$providers = $this->searchComposer->getProviders('/test/route', []);
122+
123+
$this->assertIsArray($providers);
124+
$this->assertEmpty($providers);
125+
}
126+
127+
public function testSearchWithUnknownProvider(): void {
128+
$this->setupEmptyRegistrationContext();
129+
130+
$user = $this->createMock(IUser::class);
131+
$query = $this->createMock(ISearchQuery::class);
132+
133+
$this->expectException(InvalidArgumentException::class);
134+
$this->expectExceptionMessage('Provider unknown_provider is unknown');
135+
136+
$this->searchComposer->search($user, 'unknown_provider', $query);
137+
}
138+
139+
public function testGetProvidersWithMultipleProviders(): void {
140+
$providerConfigs = [
141+
'provider1' => ['service' => 'provider1_service', 'appId' => 'app1', 'order' => 10],
142+
'provider2' => ['service' => 'provider2_service', 'appId' => 'app2', 'order' => 5],
143+
'provider3' => ['service' => 'provider3_service', 'appId' => 'app3', 'order' => 15, 'isInApp' => true],
144+
];
145+
146+
$mockData = $this->createMockProvidersAndRegistrations($providerConfigs);
147+
$this->setupRegistrationContextWithProviders($mockData['registrations']);
148+
$this->setupAppConfigForAllowedProviders();
149+
150+
$providers = $this->searchComposer->getProviders('/test/route', []);
151+
152+
$this->assertProvidersStructureAndSorting($providers, [
153+
['id' => 'provider2', 'name' => 'Provider provider2', 'appId' => 'app2', 'order' => 5, 'inAppSearch' => false],
154+
['id' => 'provider1', 'name' => 'Provider provider1', 'appId' => 'app1', 'order' => 10, 'inAppSearch' => false],
155+
['id' => 'provider3', 'name' => 'Provider provider3', 'appId' => 'app3', 'order' => 15, 'inAppSearch' => true],
156+
]);
157+
}
158+
159+
public function testGetProvidersWithEmptyAllowedProvidersConfiguration(): void {
160+
$providerConfigs = [
161+
'provider1' => ['service' => 'provider1_service', 'appId' => 'app1', 'order' => 10],
162+
'provider2' => ['service' => 'provider2_service', 'appId' => 'app2', 'order' => 5],
163+
];
164+
165+
$mockData = $this->createMockProvidersAndRegistrations($providerConfigs);
166+
$this->setupRegistrationContextWithProviders($mockData['registrations']);
167+
$this->setupAppConfigForAllowedProviders();
168+
169+
$providers = $this->searchComposer->getProviders('/test/route', []);
170+
171+
$this->assertCount(2, $providers);
172+
$this->assertProvidersAreSortedByOrder($providers);
173+
$this->assertEquals('provider2', $providers[0]['id']);
174+
$this->assertEquals('provider1', $providers[1]['id']);
175+
}
176+
177+
public function testGetProvidersWithAllowedProvidersRestriction(): void {
178+
$providerConfigs = [
179+
'provider1' => ['service' => 'provider1_service', 'appId' => 'app1', 'order' => 10],
180+
'provider2' => ['service' => 'provider2_service', 'appId' => 'app2', 'order' => 5],
181+
'provider3' => ['service' => 'provider3_service', 'appId' => 'app3', 'order' => 15],
182+
'provider4' => ['service' => 'provider4_service', 'appId' => 'app4', 'order' => 8],
183+
];
184+
185+
$mockData = $this->createMockProvidersAndRegistrations($providerConfigs);
186+
$this->setupRegistrationContextWithProviders($mockData['registrations']);
187+
$this->setupAppConfigForAllowedProviders(['provider1', 'provider3']);
188+
189+
$providers = $this->searchComposer->getProviders('/test/route', []);
190+
191+
$this->assertProvidersStructureAndSorting($providers, [
192+
['id' => 'provider1', 'name' => 'Provider provider1', 'appId' => 'app1', 'order' => 10, 'inAppSearch' => false],
193+
['id' => 'provider3', 'name' => 'Provider provider3', 'appId' => 'app3', 'order' => 15, 'inAppSearch' => false],
194+
]);
195+
196+
// Verify excluded providers are not present
197+
$providerIds = array_column($providers, 'id');
198+
$this->assertNotContains('provider2', $providerIds);
199+
$this->assertNotContains('provider4', $providerIds);
200+
}
201+
202+
public function testGetProvidersFiltersByAllowedProvidersCompletely(): void {
203+
$providerConfigs = [
204+
'provider1' => ['service' => 'provider1_service', 'appId' => 'app1', 'order' => 10],
205+
'provider2' => ['service' => 'provider2_service', 'appId' => 'app2', 'order' => 5],
206+
];
207+
208+
$mockData = $this->createMockProvidersAndRegistrations($providerConfigs);
209+
$this->setupRegistrationContextWithProviders($mockData['registrations']);
210+
$this->setupAppConfigForAllowedProviders(['provider_not_exists']);
211+
212+
$providers = $this->searchComposer->getProviders('/test/route', []);
213+
214+
$this->assertIsArray($providers);
215+
$this->assertEmpty($providers);
216+
}
217+
218+
public function testGetProvidersWithMixedOrderValues(): void {
219+
$providerConfigs = [
220+
'provider1' => ['service' => 'provider1_service', 'appId' => 'app1', 'order' => 100],
221+
'provider2' => ['service' => 'provider2_service', 'appId' => 'app2', 'order' => 1],
222+
'provider3' => ['service' => 'provider3_service', 'appId' => 'app3', 'order' => 50],
223+
];
224+
225+
$mockData = $this->createMockProvidersAndRegistrations($providerConfigs);
226+
$this->setupRegistrationContextWithProviders($mockData['registrations']);
227+
$this->setupAppConfigForAllowedProviders();
228+
229+
$providers = $this->searchComposer->getProviders('/test/route', []);
230+
231+
$this->assertCount(3, $providers);
232+
$this->assertProvidersAreSortedByOrder($providers);
233+
234+
// Verify correct ordering: provider2 (1), provider3 (50), provider1 (100)
235+
$this->assertEquals('provider2', $providers[0]['id']);
236+
$this->assertEquals('provider3', $providers[1]['id']);
237+
$this->assertEquals('provider1', $providers[2]['id']);
238+
}
239+
240+
public function testProviderIconGeneration(): void {
241+
$providerConfigs = [
242+
'provider1' => ['service' => 'provider1_service', 'appId' => 'app1', 'order' => 10],
243+
];
244+
245+
$mockData = $this->createMockProvidersAndRegistrations($providerConfigs);
246+
$this->setupRegistrationContextWithProviders($mockData['registrations']);
247+
$this->setupAppConfigForAllowedProviders();
248+
249+
$providers = $this->searchComposer->getProviders('/test/route', []);
250+
251+
$this->assertCount(1, $providers);
252+
$this->assertArrayHasKey('icon', $providers[0]);
253+
$this->assertStringContainsString('/apps/provider1/img/provider1.svg', $providers[0]['icon']);
254+
}
255+
256+
/**
257+
* Assert providers array structure and expected sorting
258+
*/
259+
private function assertProvidersStructureAndSorting(array $actualProviders, array $expectedProviders): void {
260+
$this->assertIsArray($actualProviders);
261+
$this->assertCount(count($expectedProviders), $actualProviders);
262+
263+
foreach ($actualProviders as $index => $provider) {
264+
$this->assertProviderHasRequiredFields($provider);
265+
266+
$expected = $expectedProviders[$index];
267+
$this->assertEquals($expected['id'], $provider['id']);
268+
$this->assertEquals($expected['name'], $provider['name']);
269+
$this->assertEquals($expected['appId'], $provider['appId']);
270+
$this->assertEquals($expected['order'], $provider['order']);
271+
$this->assertEquals($expected['inAppSearch'], $provider['inAppSearch']);
272+
}
273+
274+
$this->assertProvidersAreSortedByOrder($actualProviders);
275+
}
276+
277+
private function assertProviderHasRequiredFields(array $provider): void {
278+
$requiredFields = ['id', 'appId', 'name', 'icon', 'order', 'triggers', 'filters', 'inAppSearch'];
279+
foreach ($requiredFields as $field) {
280+
$this->assertArrayHasKey($field, $provider, "Provider must have '$field' field");
281+
}
282+
}
283+
284+
private function assertProvidersAreSortedByOrder(array $providers): void {
285+
$orders = array_column($providers, 'order');
286+
$sortedOrders = $orders;
287+
sort($sortedOrders);
288+
$this->assertEquals($sortedOrders, $orders, 'Providers should be sorted by order');
289+
}
290+
}

0 commit comments

Comments
 (0)