From ac92581b5d7f71fdedb7e828446f0edc33a91667 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 24 Jun 2026 17:14:31 -0400 Subject: [PATCH 1/2] refactor(files): simplify Folder::search result handling and validation - clearer and slightly more robust owner-limited home-folder validation - replace array_map/array_merge with nested foreach loop for better readability, performance, and maintainability - eliminate mixed usage of of $this->getPath() and $this->path for local for path-related checks; introduce local $currentPath for readability/micro-optimization in loop - improve comments around cache merging, filtering, and ordering - skip sorting when the result set has fewer than two items - drop redundant docblock; covered in interface Signed-off-by: Josh --- lib/private/Files/Node/Folder.php | 48 +++++++++++++++++-------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index b6774987193c8..12f5f18ef79af 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -206,10 +206,8 @@ private function queryFromOperator(ISearchOperator $operator, ?string $uid = nul } /** - * search for files with the name matching $query - * * @param string|ISearchQuery $query - * @return \OC\Files\Node\Node[] + * @return Node[] */ #[\Override] public function search($query) { @@ -217,35 +215,43 @@ public function search($query) { $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%' . $query . '%')); } - // search is handled by a single query covering all caches that this folder contains - // this is done by collect - + $currentPath = $this->path; $limitToHome = $query->limitToHome(); - if ($limitToHome && count(explode('/', $this->path)) !== 3) { - throw new \InvalidArgumentException('searching by owner is only allowed in the users home folder'); + $pathParts = explode('/', trim($currentPath, '/')); + $isUserHomeFolder = count($pathParts) === 2 && $pathParts[1] === 'files'; + if ($limitToHome && !$isUserHomeFolder) { + throw new \InvalidArgumentException('Searching by owner is only allowed in a user home folder'); } /** @var QuerySearchHelper $searchHelper */ $searchHelper = Server::get(QuerySearchHelper::class); - [$caches, $mountByMountPoint] = $searchHelper->getCachesAndMountPointsForSearch($this->root, $this->path, $limitToHome); + + // Execute one logical search across all caches reachable from this folder. + // The helper returns results grouped by mount point; we merge them and apply ordering below. + [$caches, $mountByMountPoint] = $searchHelper->getCachesAndMountPointsForSearch( + $this->root, + $currentPath, + $limitToHome, + ); $resultsPerCache = $searchHelper->searchInCaches($query, $caches); - // loop through all results per-cache, constructing the FileInfo object from the CacheEntry and merge them all - $files = array_merge(...array_map(function (array $results, string $relativeMountPoint) use ($mountByMountPoint) { + // Flatten per-cache results into FileInfo objects using their mount context. + $files = []; + foreach ($resultsPerCache as $relativeMountPoint => $results) { $mount = $mountByMountPoint[$relativeMountPoint]; - return array_map(function (ICacheEntry $result) use ($relativeMountPoint, $mount) { - return $this->cacheEntryToFileInfo($mount, $relativeMountPoint, $result); - }, $results); - }, array_values($resultsPerCache), array_keys($resultsPerCache))); - - // don't include this folder in the results - $files = array_values(array_filter($files, function (FileInfo $file) { - return $file->getPath() !== $this->getPath(); + foreach ($results as $result) { + $files[] = $this->cacheEntryToFileInfo($mount, $relativeMountPoint, $result); + } + } + + // Exclude the folder being searched itself from the result set. + $files = array_values(array_filter($files, function (FileInfo $file) use ($currentPath) { + return $file->getPath() !== $currentPath; })); - // since results were returned per-cache, they are no longer fully sorted + // Apply the requested ordering to the merged result set. $order = $query->getOrder(); - if ($order) { + if ($order && count($files) > 1) { usort($files, function (FileInfo $a, FileInfo $b) use ($order) { foreach ($order as $orderField) { $cmp = $orderField->sortFileInfo($a, $b); From 02a6b9f5bd8a7072aadd029bc87825f3c3e97bf2 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 24 Jun 2026 18:13:42 -0400 Subject: [PATCH 2/2] test(files): fix flaky sub-storage search pagination assertions - make multi-storage search pagination test deterministic - split sub-storage pagination coverage by explicit vs default ordering Signed-off-by: Josh --- tests/lib/Files/Node/FolderTest.php | 142 ++++++++++++++++++++-------- 1 file changed, 103 insertions(+), 39 deletions(-) diff --git a/tests/lib/Files/Node/FolderTest.php b/tests/lib/Files/Node/FolderTest.php index d67f25f622b7e..0697907fc9b00 100644 --- a/tests/lib/Files/Node/FolderTest.php +++ b/tests/lib/Files/Node/FolderTest.php @@ -934,40 +934,102 @@ public function testRecentJail(): void { $this->assertEquals([$id1], $ids); } - public static function offsetLimitProvider(): array { + public static function defaultOffsetLimitProvider(): array { return [ - [0, 10, ['/bar/foo/foo1', '/bar/foo/foo2', '/bar/foo/foo3', '/bar/foo/foo4', '/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []], - [0, 5, ['/bar/foo/foo1', '/bar/foo/foo2', '/bar/foo/foo3', '/bar/foo/foo4', '/bar/foo/sub1/foo5'], []], - [0, 2, ['/bar/foo/foo1', '/bar/foo/foo2'], []], - [3, 2, ['/bar/foo/foo4', '/bar/foo/sub1/foo5'], []], - [3, 5, ['/bar/foo/foo4', '/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []], - [5, 2, ['/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7'], []], - [6, 2, ['/bar/foo/sub2/foo7', '/bar/foo/sub2/foo8'], []], - [7, 2, ['/bar/foo/sub2/foo8'], []], - [10, 2, [], []], - [0, 5, ['/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/sub1/foo5', '/bar/foo/foo2', '/bar/foo/foo3'], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]], - [3, 2, ['/bar/foo/foo2', '/bar/foo/foo3'], [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime')]], - [0, 5, ['/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/foo2'], [ - new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'size'), - new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime') - ]], + [0, 10, 8], + [0, 5, 5], + [0, 2, 2], + [3, 2, 2], + [3, 5, 5], + [5, 2, 2], + [6, 2, 2], + [7, 2, 1], + [10, 2, 0], ]; } /** - * @param int $offset - * @param int $limit - * @param string[] $expectedPaths - * @param ISearchOrder[] $ordering - * @throws NotFoundException - * @throws InvalidPathException + * @return array */ - #[\PHPUnit\Framework\Attributes\DataProvider('offsetLimitProvider')] - public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $expectedPaths, array $ordering): void { - if (!$ordering) { - $ordering = [new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'fileid')]; + public static function orderedOffsetLimitProvider(): array { + return [ + [ + 0, 5, ['/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/sub1/foo5', '/bar/foo/foo2', '/bar/foo/foo3'], + [ + new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime'), + ], + ], + [ + 3, 2, ['/bar/foo/foo2', '/bar/foo/foo3'], + [ + new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime'), + ], + ], + [ + 0, 5, ['/bar/foo/sub1/foo5', '/bar/foo/sub1/foo6', '/bar/foo/sub2/foo7', '/bar/foo/foo1', '/bar/foo/foo2'], + [ + new SearchOrder(ISearchOrder::DIRECTION_DESCENDING, 'size'), + new SearchOrder(ISearchOrder::DIRECTION_ASCENDING, 'mtime'), + ], + ], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('defaultOffsetLimitProvider')] + public function testSearchSubStoragesLimitOffsetDefaultOrdering(int $offset, int $limit, int $expectedCount): void { + [$node, $cache, $subCache1, $subCache2] = $this->createFolderForSubStorageSearch(); + + try { + $query = $this->createSubStorageSearchQuery($limit, $offset, []); + $result = $node->search($query); + + $paths = array_map(fn (Node $info) => $info->getPath(), $result); + + $this->assertCount($expectedCount, $paths); + $this->assertSame($expectedCount, count(array_unique($paths))); + + $allExpectedPaths = [ + '/bar/foo/foo1', + '/bar/foo/foo2', + '/bar/foo/foo3', + '/bar/foo/foo4', + '/bar/foo/sub1/foo5', + '/bar/foo/sub1/foo6', + '/bar/foo/sub2/foo7', + '/bar/foo/sub2/foo8', + ]; + + foreach ($paths as $path) { + $this->assertContains($path, $allExpectedPaths); + } + } finally { + $cache->clear(); + $subCache1->clear(); + $subCache2->clear(); + } + } + + #[\PHPUnit\Framework\Attributes\DataProvider('orderedOffsetLimitProvider')] + public function testSearchSubStoragesLimitOffsetWithExplicitOrdering(int $offset, int $limit, array $expectedPaths, array $ordering): void { + [$node, $cache, $subCache1, $subCache2] = $this->createFolderForSubStorageSearch(); + + try { + $query = $this->createSubStorageSearchQuery($limit, $offset, $ordering); + $result = $node->search($query); + + $paths = array_map(fn (Node $info) => $info->getPath(), $result); + $this->assertSame($expectedPaths, $paths); + } finally { + $cache->clear(); + $subCache1->clear(); + $subCache2->clear(); } + } + /** + * @return array{0: Folder, 1: Cache, 2: Cache, 3: Cache} + */ + private function createFolderForSubStorageSearch(): array { $manager = $this->createMock(Manager::class); $view = $this->getRootViewMock(); $root = $this->getMockBuilder(Root::class) @@ -976,13 +1038,16 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $root->expects($this->any()) ->method('getUser') ->willReturn($this->user); + $storage = $this->createMock(IStorage::class); $storage->method('getId')->willReturn('test::1'); $cache = new Cache($storage); + $subStorage1 = $this->createMock(IStorage::class); $subStorage1->method('getId')->willReturn('test::2'); $subCache1 = new Cache($subStorage1); $subMount1 = $this->getMockBuilder(MountPoint::class)->setConstructorArgs([Temporary::class, ''])->getMock(); + $subStorage2 = $this->createMock(IStorage::class); $subStorage2->method('getId')->willReturn('test::3'); $subCache2 = new Cache($subStorage2); @@ -996,7 +1061,6 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $subMount1->method('getStorage') ->willReturn($subStorage1); - $subMount1->method('getMountPoint') ->willReturn('/bar/foo/sub1/'); @@ -1012,7 +1076,6 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array $subMount2->method('getStorage') ->willReturn($subStorage2); - $subMount2->method('getMountPoint') ->willReturn('/bar/foo/sub2/'); @@ -1044,21 +1107,22 @@ public function testSearchSubStoragesLimitOffset(int $offset, int $limit, array ->with('/bar/foo') ->willReturn($mount); - $node = new Folder($root, $view, '/bar/foo'); + return [new Folder($root, $view, '/bar/foo'), $cache, $subCache1, $subCache2]; + } + + /** + * @param SearchOrder[] $ordering + */ + private function createSubStorageSearchQuery(int $limit, int $offset, array $ordering): SearchQuery { $comparison = new SearchComparison(ISearchComparison::COMPARE_LIKE, 'name', '%foo%'); $operator = new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_AND, [ $comparison, - new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_NOT, [new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'mimetype', ICacheEntry::DIRECTORY_MIMETYPE)]), + new SearchBinaryOperator(ISearchBinaryOperator::OPERATOR_NOT, [ + new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'mimetype', ICacheEntry::DIRECTORY_MIMETYPE), + ]), ]); - $query = new SearchQuery($operator, $limit, $offset, $ordering); - $result = $node->search($query); - $cache->clear(); - $subCache1->clear(); - $subCache2->clear(); - $ids = array_map(function (Node $info) { - return $info->getPath(); - }, $result); - $this->assertEquals($expectedPaths, $ids); + + return new SearchQuery($operator, $limit, $offset, $ordering); } public static function dataGetOrCreateFolder(): \Generator {