diff --git a/lib/Db/SignRequestMapper.php b/lib/Db/SignRequestMapper.php index f5d73d0294..8cb48adb95 100644 --- a/lib/Db/SignRequestMapper.php +++ b/lib/Db/SignRequestMapper.php @@ -483,8 +483,6 @@ public function getFilesAssociatedFilesWithMe( public function getFilesToSearchProvider(IUser $user, string $fileName, int $limit, int $offset): array { $filter = [ - 'page' => ($offset / $limit) + 1, - 'length' => $limit, 'fileName' => $fileName, ]; @@ -494,6 +492,9 @@ public function getFilesToSearchProvider(IUser $user, string $fileName, int $lim ]; $qb = $this->getFilesAssociatedFilesWithMeQueryBuilder($user->getUID(), $filter, false, $sort); + $qb->setFirstResult($offset); + $qb->setMaxResults($limit); + $qb->addOrderBy('f.id', 'DESC'); $result = $qb->executeQuery(); $files = []; diff --git a/lib/Search/FileSearchProvider.php b/lib/Search/FileSearchProvider.php index 29117e267e..d2564636da 100644 --- a/lib/Search/FileSearchProvider.php +++ b/lib/Search/FileSearchProvider.php @@ -64,21 +64,40 @@ public function search(IUser $user, ISearchQuery $query): SearchResult { $limit = $query->getLimit(); $offset = $query->getCursor(); + $isOffsetAnInt = is_null($offset) || is_int($offset) || ctype_digit($offset); + if ($isOffsetAnInt) { + $offset = (int)$offset; + } else { + return SearchResult::complete($this->l10n->t('LibreSign documents'), []); + } + + $isOffsetPositive = $offset >= 0; + $isOffsetNotOverflowing = $offset + $limit < PHP_INT_MAX; + + $isLimitStrictlyPositive = $limit > 0; + + if (!$isOffsetPositive || !$isOffsetNotOverflowing || !$isLimitStrictlyPositive) { + return SearchResult::complete($this->l10n->t('LibreSign documents'), []); + } + try { - $files = $this->fileMapper->getFilesToSearchProvider($user, $term, $limit, (int)$offset); + $files = $this->fileMapper->getFilesToSearchProvider($user, $term, $limit + 1, $offset); } catch (\Exception) { // TRANSLATORS Nextcloud unified search provider name for finding LibreSign documents. return SearchResult::complete($this->l10n->t('LibreSign documents'), []); } + $hasNextPage = count($files) > $limit; + $files = array_slice($files, 0, $limit); $results = array_map(fn (File $file) => $this->formatResult($file, $user), $files); - return SearchResult::paginated( - // TRANSLATORS Nextcloud unified search provider name for finding LibreSign documents. - $this->l10n->t('LibreSign documents'), - $results, - $offset + $limit - ); + // TRANSLATORS Nextcloud unified search provider name for finding LibreSign documents. + $name = $this->l10n->t('LibreSign documents'); + + if ($hasNextPage) { + return SearchResult::paginated($name, $results, $offset + $limit); + } + return SearchResult::complete($name, $results); } private function formatResult(File $file, IUser $user): SearchResultEntry { @@ -147,5 +166,4 @@ private function formatSubline(string $path): string { return ''; } } - } diff --git a/tests/php/Integration/Db/SignRequestMapperSearchPaginationTest.php b/tests/php/Integration/Db/SignRequestMapperSearchPaginationTest.php new file mode 100644 index 0000000000..74eb536d4f --- /dev/null +++ b/tests/php/Integration/Db/SignRequestMapperSearchPaginationTest.php @@ -0,0 +1,152 @@ + */ + private array $insertedFiles = []; + + public function setUp(): void { + parent::setUp(); + $this->fileMapper = Server::get(FileMapper::class); + $this->signRequestMapper = Server::get(SignRequestMapper::class); + + $this->userId = 'search-pagination-' . uniqid(); + $this->user = $this->createMock(IUser::class); + $this->user->method('getUID')->willReturn($this->userId); + } + + public function tearDown(): void { + foreach ($this->insertedFiles as $file) { + $this->fileMapper->delete($file); + } + $this->insertedFiles = []; + parent::tearDown(); + } + + /** + * Newest first, and files created at the same instant are ordered by + * descending id, so the order never changes between two queries. + */ + public function testOrderingIsDeterministicWhenCreationTimesAreEqual(): void { + $expectedIds = $this->insertFilesWithTwoCreationTimes(); + + $actualIds = $this->ids($this->signRequestMapper->getFilesToSearchProvider($this->user, '', 100, 0)); + + $this->assertSame($expectedIds, $actualIds); + } + + /** + * Reading consecutive pages must return every file exactly once: + * no duplicate and no skipped file. + */ + public function testConsecutivePagesDoNotDuplicateNorSkipFiles(): void { + $expectedIds = $this->insertFilesWithTwoCreationTimes(); + $limit = 3; + + $collected = []; + for ($offset = 0; $offset < count($expectedIds); $offset += $limit) { + $page = $this->signRequestMapper->getFilesToSearchProvider($this->user, '', $limit, $offset); + $this->assertLessThanOrEqual($limit, count($page)); + $collected = array_merge($collected, $this->ids($page)); + } + + $this->assertSame($expectedIds, $collected); + $this->assertSame($collected, array_values(array_unique($collected))); + } + + /** + * The offset is applied as is, even when it is not a multiple of the limit + * (the old page-based computation shifted such offsets). + */ + public function testOffsetNotMultipleOfLimitIsAppliedDirectly(): void { + $expectedIds = $this->insertFilesWithTwoCreationTimes(); + + $page = $this->signRequestMapper->getFilesToSearchProvider($this->user, '', 3, 2); + + $this->assertSame(array_slice($expectedIds, 2, 3), $this->ids($page)); + } + + public function testLimitIsRespected(): void { + $this->insertFilesWithTwoCreationTimes(); + + $page = $this->signRequestMapper->getFilesToSearchProvider($this->user, '', 4, 0); + + $this->assertCount(4, $page); + } + + public function testOffsetBeyondTheLastFileReturnsNothing(): void { + $expectedIds = $this->insertFilesWithTwoCreationTimes(); + + $page = $this->signRequestMapper->getFilesToSearchProvider($this->user, '', 3, count($expectedIds)); + + $this->assertSame([], $page); + } + + /** + * Inserts 3 recent files and 4 older ones. Inside each group all files + * share the exact same creation time. + * + * @return list ids in the order the search must return them + */ + private function insertFilesWithTwoCreationTimes(): array { + $older = new \DateTime('2026-01-01 10:00:00', new \DateTimeZone('UTC')); + $newer = new \DateTime('2026-01-02 10:00:00', new \DateTimeZone('UTC')); + + $olderIds = []; + for ($i = 1; $i <= 4; $i++) { + $olderIds[] = $this->insertFile('older-' . $i, $older)->getId(); + } + $newerIds = []; + for ($i = 1; $i <= 3; $i++) { + $newerIds[] = $this->insertFile('newer-' . $i, $newer)->getId(); + } + + rsort($newerIds); + rsort($olderIds); + return array_merge($newerIds, $olderIds); + } + + private function insertFile(string $label, \DateTime $createdAt): File { + $file = new File(); + $file->setNodeId(random_int(100000, 999999999)); + $file->setUserId($this->userId); + $file->setUuid(uniqid('', true) . '-' . $label); + $file->setCreatedAt(clone $createdAt); + $file->setName('search-pagination-' . $label . '.pdf'); + $file->setStatus(FileStatus::DRAFT->value); + + $inserted = $this->fileMapper->insert($file); + $this->insertedFiles[] = $inserted; + return $inserted; + } + + /** + * @param list $files + * @return list + */ + private function ids(array $files): array { + return array_map(static fn (File $file): int => $file->getId(), $files); + } +} diff --git a/tests/php/Unit/Search/FileSearchProviderTest.php b/tests/php/Unit/Search/FileSearchProviderTest.php new file mode 100644 index 0000000000..8360453d1a --- /dev/null +++ b/tests/php/Unit/Search/FileSearchProviderTest.php @@ -0,0 +1,183 @@ +l10n = $this->createMock(IL10N::class); + $this->l10n->method('t')->willReturnArgument(0); + + $this->appManager = $this->createMock(IAppManager::class); + $this->appManager->method('isEnabledForUser')->willReturn(true); + + $this->fileMapper = $this->createMock(SignRequestMapper::class); + + $this->provider = new FileSearchProvider( + $this->l10n, + $this->createMock(IURLGenerator::class), + $this->createMock(IRootFolder::class), + $this->appManager, + $this->createMock(IMimeTypeDetector::class), + $this->fileMapper, + ); + } + + /** + * Invalid cursors and limits must be rejected before touching the database. + */ + #[DataProvider('providerInvalidCursorOrLimit')] + public function testSearchReturnsEmptyWithoutQueryingTheDatabase(mixed $cursor, int $limit): void { + $this->fileMapper->expects($this->never())->method('getFilesToSearchProvider'); + + $data = $this->provider->search( + $this->createMock(IUser::class), + $this->buildQuery($cursor, $limit), + )->jsonSerialize(); + + $this->assertFalse($data['isPaginated']); + $this->assertEmpty($data['entries']); + } + + public static function providerInvalidCursorOrLimit(): array { + return [ + 'letters' => ['abc', self::LIMIT], + 'empty string' => ['', self::LIMIT], + 'decimal string' => ['1.5', self::LIMIT], + 'negative string' => ['-1', self::LIMIT], + 'negative int' => [-1, self::LIMIT], + 'zero limit' => ['10', 0], + 'negative limit' => ['10', -3], + 'offset + limit overflows' => [PHP_INT_MAX - 5, self::LIMIT], + 'digits beyond int range' => ['99999999999999999999', self::LIMIT], + ]; + } + + /** + * Valid cursors are normalized to an int and the mapper is asked for + * one extra row, used only to detect that a next page exists. + */ + #[DataProvider('providerValidCursor')] + public function testSearchAsksTheMapperForOneExtraRow(mixed $cursor, int $expectedOffset): void { + $user = $this->createMock(IUser::class); + + $this->fileMapper->expects($this->once()) + ->method('getFilesToSearchProvider') + ->with($user, 'contrat', self::LIMIT + 1, $expectedOffset) + ->willReturn([]); + + $this->provider->search($user, $this->buildQuery($cursor, self::LIMIT)); + } + + public static function providerValidCursor(): array { + return [ + 'null means first page' => [null, 0], + 'zero' => [0, 0], + 'int' => [10, 10], + 'numeric string' => ['10', 10], + 'largest offset that still fits' => [PHP_INT_MAX - 10, PHP_INT_MAX - 10], + ]; + } + + /** + * Pagination metadata: a next page exists only if the mapper returned more + * rows than the requested limit. + */ + #[DataProvider('providerPagination')] + public function testSearchPaginationMetadata(int $rowsReturned, int $offset, bool $isPaginated, int $expectedEntries, ?int $expectedCursor): void { + $this->fileMapper->method('getFilesToSearchProvider') + ->willReturn($this->buildFiles($rowsReturned)); + + $data = $this->provider->search( + $this->createMock(IUser::class), + $this->buildQuery($offset, self::LIMIT), + )->jsonSerialize(); + + $this->assertSame($isPaginated, $data['isPaginated']); + $this->assertCount($expectedEntries, $data['entries']); + $this->assertSame($expectedCursor, $data['cursor']); + } + + public static function providerPagination(): array { + return [ + 'empty dataset' => [0, 0, false, 0, null], + 'partial page' => [3, 0, false, 3, null], + 'exactly the limit' => [5, 0, false, 5, null], + 'one more than the limit' => [6, 0, true, 5, 5], + 'next page of a multi-page dataset' => [6, 10, true, 5, 15], + 'last page of a multi-page dataset' => [2, 10, false, 2, null], + ]; + } + + /** + * The extra row fetched only to detect the next page must never be shown. + */ + public function testSearchDoesNotReturnTheExtraRow(): void { + $this->fileMapper->method('getFilesToSearchProvider') + ->willReturn($this->buildFiles(self::LIMIT + 1)); + + $data = $this->provider->search( + $this->createMock(IUser::class), + $this->buildQuery(0, self::LIMIT), + )->jsonSerialize(); + + $titles = array_map( + static fn (SearchResultEntry $entry): string => $entry->jsonSerialize()['title'], + $data['entries'], + ); + $this->assertSame(['doc-1.pdf', 'doc-2.pdf', 'doc-3.pdf', 'doc-4.pdf', 'doc-5.pdf'], $titles); + } + + private function buildQuery(mixed $cursor, int $limit): ISearchQuery&MockObject { + $query = $this->createMock(ISearchQuery::class); + $query->method('getTerm')->willReturn('contrat'); + $query->method('getLimit')->willReturn($limit); + $query->method('getCursor')->willReturn($cursor); + return $query; + } + + /** + * @return list + */ + private function buildFiles(int $count): array { + $files = []; + for ($i = 1; $i <= $count; $i++) { + $file = new File(); + $file->setUserId('user-' . $i); + $file->setUuid('uuid-' . $i); + $file->setName('doc-' . $i . '.pdf'); + $file->setNodeId($i); + $files[] = $file; + } + return $files; + } +}