From f6bd57e3ecd08937d63189c754717323501b994c Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 17 Aug 2026 21:07:19 +0200 Subject: [PATCH 1/6] feat: Add Link pagination when possible This use a private trait to have a consistent behavior Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: Carl Schwan --- .../lib/Controller/GroupsController.php | 43 ++++++++-- .../lib/Controller/UsersController.php | 58 ++++++++++++-- .../openapi-administration.json | 7 ++ apps/provisioning_api/openapi-full.json | 49 ++++++++++++ apps/provisioning_api/openapi.json | 42 ++++++++++ .../tests/Controller/GroupsControllerTest.php | 4 + .../tests/Controller/UsersControllerTest.php | 36 +++++++++ .../lib/Controller/ApiV1Controller.php | 18 ++++- apps/sharing/openapi.json | 7 ++ .../lib/Controller/StatusesController.php | 16 +++- apps/user_status/openapi.json | 7 ++ .../Controller/StatusesControllerTest.php | 9 ++- core/Controller/AutoCompleteController.php | 27 ++++++- core/openapi-full.json | 17 ++++ core/openapi.json | 17 ++++ .../AppFramework/Http/PaginationTrait.php | 47 +++++++++++ openapi.json | 80 +++++++++++++++++++ .../Controller/AutoCompleteControllerTest.php | 42 +++++++++- .../AppFramework/Http/PaginationTraitTest.php | 75 +++++++++++++++++ 19 files changed, 573 insertions(+), 28 deletions(-) create mode 100644 lib/private/AppFramework/Http/PaginationTrait.php create mode 100644 tests/lib/AppFramework/Http/PaginationTraitTest.php diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 1c1be04c88ded..3b436a4a45033 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -9,6 +9,7 @@ namespace OCA\Provisioning_API\Controller; +use OC\AppFramework\Http\PaginationTrait; use OC\Group\DisplayNameCache as GroupDisplayNameCache; use OCA\Provisioning_API\ResponseDefinitions; use OCA\Settings\Settings\Admin\Sharing; @@ -30,6 +31,7 @@ use OCP\IGroup; use OCP\IGroupManager; use OCP\IRequest; +use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; @@ -42,6 +44,7 @@ * @psalm-import-type Provisioning_APIUserDetailsGroupDisplayname from ResponseDefinitions */ class GroupsController extends AUserDataOCSController { + use PaginationTrait; public function __construct( string $appName, @@ -56,6 +59,7 @@ public function __construct( IRootFolder $rootFolder, private LoggerInterface $logger, GroupDisplayNameCache $groupDisplayNameCache, + private IURLGenerator $urlGenerator, ) { parent::__construct($appName, $request, @@ -77,19 +81,28 @@ public function __construct( * @param string $search Text to search for * @param ?int $limit Limit the amount of groups returned * @param int $offset Offset for searching for groups - * @return DataResponse}, array{}> + * @return DataResponse}, array{Link?: string}> * * 200: Groups returned */ #[NoAdminRequired] public function getGroups(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { $groups = $this->groupManager->search($search, $limit, $offset); + $hasMoreResults = $this->hasMoreResults($groups, $limit); $groups = array_map(function ($group) { /** @var IGroup $group */ return $group->getGID(); }, $groups); - return new DataResponse(['groups' => $groups]); + $response = new DataResponse(['groups' => $groups]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } /** @@ -98,7 +111,7 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = * @param string $search Text to search for * @param ?int $limit Limit the amount of groups returned * @param int $offset Offset for searching for groups - * @return DataResponse}, array{}> + * @return DataResponse}, array{Link?: string}> * * 200: Groups details returned */ @@ -107,6 +120,7 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = #[AuthorizedAdminSetting(settings: Users::class)] public function getGroupsDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { $groups = $this->groupManager->search($search, $limit, $offset); + $hasMoreResults = $this->hasMoreResults($groups, $limit); $groups = array_map(function ($group) { /** @var IGroup $group */ return [ @@ -119,7 +133,15 @@ public function getGroupsDetails(string $search = '', ?int $limit = null, int $o ]; }, $groups); - return new DataResponse(['groups' => $groups]); + $response = new DataResponse(['groups' => $groups]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } /** @@ -191,7 +213,7 @@ public function getGroupUsers(string $groupId): DataResponse { * @param int|null $limit Limit the amount of groups returned * @param int $offset Offset for searching for groups * - * @return DataResponse, groups: list}, array{}> + * @return DataResponse, groups: list}, array{Link?: string}> * @throws OCSException * * 200: Group users details returned @@ -214,6 +236,7 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentUser->getUID()); if ($isAdmin || $isDelegatedAdmin || $isSubadminOfGroup) { $users = $group->searchUsers($search, $limit, $offset); + $hasMoreResults = $this->hasMoreResults($users, $limit); // Extract required number $usersDetails = []; @@ -234,10 +257,18 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int // continue if a users ceased to exist. } } - return new DataResponse([ + $response = new DataResponse([ 'users' => $usersDetails, 'groups' => $this->findGroupsWithDisplayname($usersDetails), ]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } throw new OCSException('The requested group could not be found', OCSController::RESPOND_NOT_FOUND); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index f46084a98741e..0d41535714be4 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -11,6 +11,7 @@ namespace OCA\Provisioning_API\Controller; use InvalidArgumentException; +use OC\AppFramework\Http\PaginationTrait; use OC\Authentication\Token\RemoteWipe; use OC\Group\DisplayNameCache as GroupDisplayNameCache; use OC\Group\Group; @@ -62,6 +63,7 @@ * @psalm-import-type Provisioning_APIUserDetailsGroupDisplayname from ResponseDefinitions */ class UsersController extends AUserDataOCSController { + use PaginationTrait; private IL10N $l10n; @@ -111,7 +113,7 @@ public function __construct( * @param string $search Text to search for * @param int|null $limit Limit the amount of groups returned * @param int $offset Offset for searching for groups - * @return DataResponse}, array{}> + * @return DataResponse}, array{Link?: string}> * * 200: Users returned */ @@ -139,12 +141,22 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = } } + $hasMoreResults = $this->hasMoreResults($users, $limit); + /** @var list $users */ $users = array_keys($users); - return new DataResponse([ + $response = new DataResponse([ 'users' => $users ]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } /** @@ -153,7 +165,7 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = * @param string $search Text to search for * @param int|null $limit Limit the amount of groups returned * @param int $offset Offset for searching for groups - * @return DataResponse, groups: list}, array{}> + * @return DataResponse, groups: list}, array{Link?: string}> * * 200: Users details returned */ @@ -183,6 +195,8 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of $users = array_merge(...$users); } + $hasMoreResults = $this->hasMoreResults($users, $limit); + $usersDetails = []; foreach ($users as $userId) { $userId = (string)$userId; @@ -204,10 +218,18 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of } } - return new DataResponse([ + $response = new DataResponse([ 'users' => $usersDetails, 'groups' => $this->findGroupsWithDisplayname($usersDetails), ]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } /** @@ -216,7 +238,7 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of * @param string $search Text to search for * @param ?int $limit Limit the amount of users returned * @param int $offset Offset - * @return DataResponse}, array{}> + * @return DataResponse}, array{Link?: string}> * * 200: Disabled users details returned */ @@ -267,6 +289,8 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, $users = array_slice($users, $offset, $limit); } + $hasMoreResults = $this->hasMoreResults($users, $limit); + $usersDetails = []; foreach ($users as $userId) { try { @@ -287,9 +311,17 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, } } - return new DataResponse([ + $response = new DataResponse([ 'users' => $usersDetails ]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } /** @@ -298,7 +330,7 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, * @param string $search Text to search for * @param ?int $limit Limit the amount of users returned * @param int $offset Offset - * @return DataResponse}, array{}> + * @return DataResponse}, array{Link?: string}> * * 200: Users details returned based on last logged in information */ @@ -324,6 +356,8 @@ public function getLastLoggedInUsers( // For Admin alone user sorting based on lastLogin. For sub admin and groups this is not supported $users = $this->userManager->getLastLoggedInUsers($limit, $offset, $search); + $hasMoreResults = $this->hasMoreResults($users, $limit); + $usersDetails = []; foreach ($users as $userId) { try { @@ -344,9 +378,17 @@ public function getLastLoggedInUsers( } } - return new DataResponse([ + $response = new DataResponse([ 'users' => $usersDetails ]); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + return $response; } /** diff --git a/apps/provisioning_api/openapi-administration.json b/apps/provisioning_api/openapi-administration.json index 52d5325ba6ffc..1a16cc7b53021 100644 --- a/apps/provisioning_api/openapi-administration.json +++ b/apps/provisioning_api/openapi-administration.json @@ -1492,6 +1492,13 @@ "responses": { "200": { "description": "Users details returned based on last logged in information", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/apps/provisioning_api/openapi-full.json b/apps/provisioning_api/openapi-full.json index ebba71a0259dc..4303f86cd63cd 100644 --- a/apps/provisioning_api/openapi-full.json +++ b/apps/provisioning_api/openapi-full.json @@ -1277,6 +1277,13 @@ "responses": { "200": { "description": "Groups returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -1791,6 +1798,13 @@ "responses": { "200": { "description": "Users details returned based on last logged in information", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -3218,6 +3232,13 @@ "responses": { "200": { "description": "Groups details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -3524,6 +3545,13 @@ "responses": { "200": { "description": "Group users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -3676,6 +3704,13 @@ "responses": { "200": { "description": "Users returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -3996,6 +4031,13 @@ "responses": { "200": { "description": "Users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -4148,6 +4190,13 @@ "responses": { "200": { "description": "Disabled users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/apps/provisioning_api/openapi.json b/apps/provisioning_api/openapi.json index 4550e551a02e6..b47957bcfdf10 100644 --- a/apps/provisioning_api/openapi.json +++ b/apps/provisioning_api/openapi.json @@ -488,6 +488,13 @@ "responses": { "200": { "description": "Groups returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -618,6 +625,13 @@ "responses": { "200": { "description": "Groups details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -924,6 +938,13 @@ "responses": { "200": { "description": "Group users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -1187,6 +1208,13 @@ "responses": { "200": { "description": "Users returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -1507,6 +1535,13 @@ "responses": { "200": { "description": "Users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -1659,6 +1694,13 @@ "responses": { "200": { "description": "Disabled users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php index 72fc57421e59c..7fee2d13e3376 100644 --- a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php +++ b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php @@ -18,6 +18,7 @@ use OCP\IConfig; use OCP\IGroup; use OCP\IRequest; +use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; @@ -39,6 +40,7 @@ class GroupsControllerTest extends \Test\TestCase { protected LoggerInterface&MockObject $logger; protected GroupsController&MockObject $api; private GroupDisplayNameCache&MockObject $groupDisplayNameCache; + private IURLGenerator&MockObject $urlGenerator; private IRootFolder $rootFolder; @@ -56,6 +58,7 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->rootFolder = $this->createMock(IRootFolder::class); $this->groupDisplayNameCache = $this->createMock(GroupDisplayNameCache::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->groupManager ->method('getSubAdmin') @@ -75,6 +78,7 @@ protected function setUp(): void { $this->rootFolder, $this->logger, $this->groupDisplayNameCache, + $this->urlGenerator, ]) ->onlyMethods(['fillStorageInfo']) ->getMock(); diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 08355a5cb1804..f4a4e0a340ae2 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -350,6 +350,42 @@ public function testGetDisabledUsersAsAdmin(): void { $this->assertEquals($expected, $this->api->getDisabledUsersDetails('MyCustomSearch', 3)->getData()); } + public function testGetDisabledUsersDetailsSetsLinkHeaderWhenMoreResultsExist(): void { + $loggedInUser = $this->getMockBuilder(IUser::class) + ->disableOriginalConstructor() + ->getMock(); + $loggedInUser + ->method('getUID') + ->willReturn('admin'); + $this->userSession + ->method('getUser') + ->willReturn($loggedInUser); + $this->groupManager + ->method('isAdmin') + ->willReturn(true); + $this->userManager + ->method('getDisabledUsers') + ->with(2, 0, 'MyCustomSearch') + ->willReturn([ + $this->createUserMock('foo', false), + $this->createUserMock('bar', false), + ]); + + $this->request + ->method('getRequestUri') + ->willReturn('/ocs/v2.php/apps/provisioning_api/api/v1/users/disabled?search=MyCustomSearch&limit=2'); + $this->urlGenerator + ->method('getAbsoluteURL') + ->with('/ocs/v2.php/apps/provisioning_api/api/v1/users/disabled') + ->willReturn('https://cloud.example.com/ocs/v2.php/apps/provisioning_api/api/v1/users/disabled'); + + $response = $this->api->getDisabledUsersDetails('MyCustomSearch', 2, 0); + $this->assertSame( + '; rel="next"', + $response->getHeaders()['Link'] + ); + } + public function testGetDisabledUsersAsSubAdmin(): void { $loggedInUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() diff --git a/apps/sharing/lib/Controller/ApiV1Controller.php b/apps/sharing/lib/Controller/ApiV1Controller.php index d56a653c14793..7029330107741 100644 --- a/apps/sharing/lib/Controller/ApiV1Controller.php +++ b/apps/sharing/lib/Controller/ApiV1Controller.php @@ -28,6 +28,7 @@ use NCU\Sharing\ShareState; use NCU\Sharing\Source\IShareSourceType; use NCU\Sharing\Source\ShareSource; +use OC\AppFramework\Http\PaginationTrait; use OCA\Sharing\ResponseDefinitions; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\ApiRoute; @@ -54,6 +55,8 @@ * @psalm-import-type SharingPermissionPreset from ResponseDefinitions */ final class ApiV1Controller extends OCSController { + use PaginationTrait; + public ShareAccessContext $accessContext; public function __construct( @@ -80,7 +83,7 @@ public function __construct( * @param int<1, 100> $limit The maximum number of participants * @param non-negative-int $offset The offset of the participants * @param ?string $id If provided, recipients that are already part of the share will not be returned. - * @return DataResponse, array{}>|DataResponse + * @return DataResponse, array{Link?: string}>|DataResponse * * 200: Recipients returned * 400: Invalid recipient search parameters @@ -110,7 +113,18 @@ public function searchRecipients(?array $filterRecipientTypeClasses, string $que $forShare = ($id === null) ? null : $this->manager->getShare($this->accessContext, $id); $recipients = $this->manager->searchRecipients($this->accessContext, $filterRecipientTypeClasses, $query, $limit, $offset, $forShare); $this->dbConnection->commit(); - return new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients)); + + $response = new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients)); + if ($this->hasMoreResults($recipients, $limit)) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'filterRecipientTypeClasses' => $filterRecipientTypeClasses, + 'query' => $query, + 'limit' => $limit, + 'offset' => $offset + $limit, + 'id' => $id, + ])]); + } + return $response; } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; diff --git a/apps/sharing/openapi.json b/apps/sharing/openapi.json index 98beb6864c1d6..b98f56fb52c27 100644 --- a/apps/sharing/openapi.json +++ b/apps/sharing/openapi.json @@ -684,6 +684,13 @@ "responses": { "200": { "description": "Recipients returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/apps/user_status/lib/Controller/StatusesController.php b/apps/user_status/lib/Controller/StatusesController.php index b1d55774a1177..ab4687e9b334c 100644 --- a/apps/user_status/lib/Controller/StatusesController.php +++ b/apps/user_status/lib/Controller/StatusesController.php @@ -9,6 +9,7 @@ namespace OCA\UserStatus\Controller; +use OC\AppFramework\Http\PaginationTrait; use OCA\UserStatus\Db\UserStatus; use OCA\UserStatus\ResponseDefinitions; use OCA\UserStatus\Service\StatusService; @@ -20,6 +21,7 @@ use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; use OCP\IRequest; +use OCP\IURLGenerator; use OCP\UserStatus\IUserStatus; /** @@ -27,6 +29,7 @@ * @psalm-import-type UserStatusPublic from ResponseDefinitions */ class StatusesController extends OCSController { + use PaginationTrait; /** * StatusesController constructor. @@ -39,6 +42,7 @@ public function __construct( string $appName, IRequest $request, private StatusService $service, + private IURLGenerator $urlGenerator, ) { parent::__construct($appName, $request); } @@ -48,7 +52,7 @@ public function __construct( * * @param int|null $limit Maximum number of statuses to find * @param non-negative-int|null $offset Offset for finding statuses - * @return DataResponse, array{}> + * @return DataResponse, array{Link?: string}> * * 200: Statuses returned */ @@ -56,10 +60,18 @@ public function __construct( #[ApiRoute(verb: 'GET', url: '/api/v1/statuses')] public function findAll(?int $limit = null, ?int $offset = null): DataResponse { $allStatuses = $this->service->findAll($limit, $offset); + $hasMoreResults = $this->hasMoreResults($allStatuses, $limit); - return new DataResponse(array_values(array_map(function ($userStatus) { + $response = new DataResponse(array_values(array_map(function ($userStatus) { return $this->formatStatus($userStatus); }, $allStatuses))); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'limit' => $limit, + 'offset' => ($offset ?? 0) + $limit, + ])]); + } + return $response; } /** diff --git a/apps/user_status/openapi.json b/apps/user_status/openapi.json index 720d695c3ddf7..88250e2fb0848 100644 --- a/apps/user_status/openapi.json +++ b/apps/user_status/openapi.json @@ -521,6 +521,13 @@ "responses": { "200": { "description": "Statuses returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php b/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php index e623486c82d93..e20e498825cfb 100644 --- a/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php +++ b/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php @@ -15,20 +15,24 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\IRequest; +use OCP\IURLGenerator; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class StatusesControllerTest extends TestCase { private StatusService&MockObject $service; + private IRequest&MockObject $request; + private IURLGenerator&MockObject $urlGenerator; private StatusesController $controller; protected function setUp(): void { parent::setUp(); - $request = $this->createMock(IRequest::class); + $this->request = $this->createMock(IRequest::class); $this->service = $this->createMock(StatusService::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->controller = new StatusesController('user_status', $request, $this->service); + $this->controller = new StatusesController('user_status', $this->request, $this->service, $this->urlGenerator); } public function testFindAll(): void { @@ -47,6 +51,7 @@ public function testFindAll(): void { 'message' => 'On vacation', 'clearAt' => 60000, ]], $response->getData()); + $this->assertArrayNotHasKey('Link', $response->getHeaders()); } public function testFind(): void { diff --git a/core/Controller/AutoCompleteController.php b/core/Controller/AutoCompleteController.php index 786996c694fce..41261fc9cc061 100644 --- a/core/Controller/AutoCompleteController.php +++ b/core/Controller/AutoCompleteController.php @@ -9,6 +9,7 @@ namespace OC\Core\Controller; +use OC\AppFramework\Http\PaginationTrait; use OC\Core\ResponseDefinitions; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\ApiRoute; @@ -20,18 +21,22 @@ use OCP\Collaboration\Collaborators\ISearch; use OCP\EventDispatcher\IEventDispatcher; use OCP\IRequest; +use OCP\IURLGenerator; use OCP\Share\IShare; /** * @psalm-import-type CoreAutocompleteResult from ResponseDefinitions */ class AutoCompleteController extends OCSController { + use PaginationTrait; + public function __construct( string $appName, IRequest $request, private ISearch $collaboratorSearch, private IManager $autoCompleteManager, private IEventDispatcher $dispatcher, + private IURLGenerator $urlGenerator, ) { parent::__construct($appName, $request); } @@ -45,17 +50,18 @@ public function __construct( * @param string|null $sorter can be piped, top priority first, e.g.: "commenters|share-recipients" * @param list $shareTypes Types of shares to search for * @param int $limit Maximum number of results to return + * @param int $offset Offset for searching * - * @return DataResponse, array{}> + * @return DataResponse, array{Link?: string}> * * 200: Autocomplete results returned */ #[NoAdminRequired] #[ApiRoute(verb: 'GET', url: '/autocomplete/get', root: '/core')] - public function get(string $search, ?string $itemType, ?string $itemId, ?string $sorter = null, array $shareTypes = [IShare::TYPE_USER], int $limit = 10): DataResponse { + public function get(string $search, ?string $itemType, ?string $itemId, ?string $sorter = null, array $shareTypes = [IShare::TYPE_USER], int $limit = 10, int $offset = 0): DataResponse { // if enumeration/user listings are disabled, we'll receive an empty // result from search() – thus nothing else to do here. - [$results,] = $this->collaboratorSearch->search($search, $shareTypes, false, $limit, 0); + [$results, $hasMoreResults] = $this->collaboratorSearch->search($search, $shareTypes, false, $limit, $offset); $event = new AutoCompleteFilterEvent( $results, @@ -84,7 +90,20 @@ public function get(string $search, ?string $itemType, ?string $itemId, ?string // transform to expected format $results = $this->prepareResultArray($results); - return new DataResponse($results); + $response = new DataResponse($results); + if ($hasMoreResults) { + $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + 'search' => $search, + 'itemType' => $itemType, + 'itemId' => $itemId, + 'sorter' => $sorter, + 'shareTypes' => $shareTypes, + 'limit' => $limit, + 'offset' => $offset + $limit, + ])]); + } + + return $response; } /** diff --git a/core/openapi-full.json b/core/openapi-full.json index 32740488c7743..6b458c3187a26 100644 --- a/core/openapi-full.json +++ b/core/openapi-full.json @@ -1969,6 +1969,16 @@ "default": 10 } }, + { + "name": "offset", + "in": "query", + "description": "Offset for searching", + "schema": { + "type": "integer", + "format": "int64", + "default": 0 + } + }, { "name": "OCS-APIRequest", "in": "header", @@ -1983,6 +1993,13 @@ "responses": { "200": { "description": "Autocomplete results returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/core/openapi.json b/core/openapi.json index 0673f3f96c649..54a000845c207 100644 --- a/core/openapi.json +++ b/core/openapi.json @@ -1969,6 +1969,16 @@ "default": 10 } }, + { + "name": "offset", + "in": "query", + "description": "Offset for searching", + "schema": { + "type": "integer", + "format": "int64", + "default": 0 + } + }, { "name": "OCS-APIRequest", "in": "header", @@ -1983,6 +1993,13 @@ "responses": { "200": { "description": "Autocomplete results returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/lib/private/AppFramework/Http/PaginationTrait.php b/lib/private/AppFramework/Http/PaginationTrait.php new file mode 100644 index 0000000000000..6e4f05de9804d --- /dev/null +++ b/lib/private/AppFramework/Http/PaginationTrait.php @@ -0,0 +1,47 @@ + 0 && count($items) === $limit; + } + + /** + * Builds a `Link: ; rel="next"` response header value pointing at the + * next page of the current request, keeping its path (including any route + * placeholders already resolved into it) and replacing the query string. + * + * @param array $params Query parameters for the next page, e.g. the incremented offset + */ + protected function buildNextPageLinkHeader(IRequest $request, IURLGenerator $urlGenerator, array $params): string { + $path = (string)parse_url($request->getRequestUri(), PHP_URL_PATH); + $url = $urlGenerator->getAbsoluteURL($path) . '?' . http_build_query($params); + return '<' . $url . '>; rel="next"'; + } +} diff --git a/openapi.json b/openapi.json index 51b027183b69f..38c64e9aae37f 100644 --- a/openapi.json +++ b/openapi.json @@ -6254,6 +6254,16 @@ "default": 10 } }, + { + "name": "offset", + "in": "query", + "description": "Offset for searching", + "schema": { + "type": "integer", + "format": "int64", + "default": 0 + } + }, { "name": "OCS-APIRequest", "in": "header", @@ -6268,6 +6278,13 @@ "responses": { "200": { "description": "Autocomplete results returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -31764,6 +31781,13 @@ "responses": { "200": { "description": "Groups returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -32278,6 +32302,13 @@ "responses": { "200": { "description": "Users details returned based on last logged in information", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -33705,6 +33736,13 @@ "responses": { "200": { "description": "Groups details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -34011,6 +34049,13 @@ "responses": { "200": { "description": "Group users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -34163,6 +34208,13 @@ "responses": { "200": { "description": "Users returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -34483,6 +34535,13 @@ "responses": { "200": { "description": "Users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -34635,6 +34694,13 @@ "responses": { "200": { "description": "Disabled users details returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -37980,6 +38046,13 @@ "responses": { "200": { "description": "Recipients returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { @@ -43612,6 +43685,13 @@ "responses": { "200": { "description": "Statuses returned", + "headers": { + "Link": { + "schema": { + "type": "string" + } + } + }, "content": { "application/json": { "schema": { diff --git a/tests/Core/Controller/AutoCompleteControllerTest.php b/tests/Core/Controller/AutoCompleteControllerTest.php index 5196c0e990878..dff48e357c57d 100644 --- a/tests/Core/Controller/AutoCompleteControllerTest.php +++ b/tests/Core/Controller/AutoCompleteControllerTest.php @@ -12,6 +12,7 @@ use OCP\Collaboration\Collaborators\ISearch; use OCP\EventDispatcher\IEventDispatcher; use OCP\IRequest; +use OCP\IURLGenerator; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -22,6 +23,10 @@ class AutoCompleteControllerTest extends TestCase { protected $autoCompleteManager; /** @var IEventDispatcher|MockObject */ protected $dispatcher; + /** @var IRequest|MockObject */ + protected $request; + /** @var IURLGenerator|MockObject */ + protected $urlGenerator; /** @var AutoCompleteController */ protected $controller; @@ -29,18 +34,19 @@ class AutoCompleteControllerTest extends TestCase { protected function setUp(): void { parent::setUp(); - /** @var IRequest $request */ - $request = $this->createMock(IRequest::class); + $this->request = $this->createMock(IRequest::class); $this->collaboratorSearch = $this->createMock(ISearch::class); $this->autoCompleteManager = $this->createMock(IManager::class); $this->dispatcher = $this->createMock(IEventDispatcher::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->controller = new AutoCompleteController( 'core', - $request, + $this->request, $this->collaboratorSearch, $this->autoCompleteManager, - $this->dispatcher + $this->dispatcher, + $this->urlGenerator, ); } @@ -170,5 +176,33 @@ public function testGet(array $searchResults, array $expected, string $searchTer $list = $response->getData(); $this->assertEquals($expected, $list); // has better error output… $this->assertSame($expected, $list); + $this->assertArrayNotHasKey('Link', $response->getHeaders()); + } + + public function testGetSetsLinkHeaderWhenMoreResultsExist(): void { + $this->collaboratorSearch->expects($this->once()) + ->method('search') + ->with('bob', [0], false, 2, 0) + ->willReturn([[ + 'exact' => ['users' => [], 'robots' => []], + 'users' => [ + ['label' => 'Bob Y.', 'value' => ['shareWith' => 'bob']], + ['label' => 'Bobby R.', 'value' => ['shareWith' => 'bobby']], + ], + ], true]); + + $this->request + ->method('getRequestUri') + ->willReturn('/ocs/v2.php/core/autocomplete/get?search=bob&limit=2'); + $this->urlGenerator + ->method('getAbsoluteURL') + ->with('/ocs/v2.php/core/autocomplete/get') + ->willReturn('https://cloud.example.com/ocs/v2.php/core/autocomplete/get'); + + $response = $this->controller->get('bob', null, null, null, [0], 2, 0); + $this->assertSame( + '; rel="next"', + $response->getHeaders()['Link'] + ); } } diff --git a/tests/lib/AppFramework/Http/PaginationTraitTest.php b/tests/lib/AppFramework/Http/PaginationTraitTest.php new file mode 100644 index 0000000000000..c72fcafbffae2 --- /dev/null +++ b/tests/lib/AppFramework/Http/PaginationTraitTest.php @@ -0,0 +1,75 @@ +subject = new class { + use PaginationTrait; + + public function hasMore(array $items, ?int $limit): bool { + return $this->hasMoreResults($items, $limit); + } + + public function nextLink(IRequest $request, IURLGenerator $urlGenerator, array $params): string { + return $this->buildNextPageLinkHeader($request, $urlGenerator, $params); + } + }; + } + + public function testNullLimitNeverReportsMore(): void { + $this->assertFalse($this->subject->hasMore(['a', 'b', 'c'], null)); + } + + public function testZeroLimitNeverReportsMore(): void { + $this->assertFalse($this->subject->hasMore([], 0)); + } + + public function testFewerResultsThanLimitReportsNoMore(): void { + $this->assertFalse($this->subject->hasMore(['a', 'b'], 5)); + } + + public function testExactlyLimitResultsReportsMore(): void { + $this->assertTrue($this->subject->hasMore(['a', 'b', 'c'], 3)); + } + + public function testEmptyResultWithPositiveLimitReportsNoMore(): void { + $this->assertFalse($this->subject->hasMore([], 5)); + } + + public function testAssociativeArrayIsCountedByEntries(): void { + $this->assertTrue($this->subject->hasMore(['uid1' => 'Alice', 'uid2' => 'Bob'], 2)); + } + + public function testNextLinkKeepsRequestPathAndAppliesGivenQuery(): void { + $request = $this->createMock(IRequest::class); + $request->method('getRequestUri')->willReturn('/ocs/v2.php/apps/provisioning_api/api/v1/groups?search=foo&limit=5&offset=0'); + + $urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator->method('getAbsoluteURL') + ->with('/ocs/v2.php/apps/provisioning_api/api/v1/groups') + ->willReturn('https://cloud.example.com/ocs/v2.php/apps/provisioning_api/api/v1/groups'); + + $link = $this->subject->nextLink($request, $urlGenerator, ['search' => 'foo', 'limit' => 5, 'offset' => 5]); + + $this->assertSame( + '; rel="next"', + $link + ); + } +} From 4a0e024bdca3e9a1f0440e8076ead0d4d3334ec2 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 17 Aug 2026 22:23:32 +0200 Subject: [PATCH 2/6] refactor: Add some more precise type annotation to some controller methods Signed-off-by: Carl Schwan --- core/Controller/AppPasswordController.php | 6 ++-- core/Controller/AutoCompleteController.php | 6 ++-- core/Controller/AvatarController.php | 7 ++-- core/Controller/PreviewController.php | 14 ++++---- core/openapi-full.json | 39 ++++++++++++++-------- core/openapi.json | 39 ++++++++++++++-------- lib/public/Security/ISecureRandom.php | 2 +- openapi.json | 39 ++++++++++++++-------- 8 files changed, 97 insertions(+), 55 deletions(-) diff --git a/core/Controller/AppPasswordController.php b/core/Controller/AppPasswordController.php index 5dcf9b8c4e72c..c7d94085294c8 100644 --- a/core/Controller/AppPasswordController.php +++ b/core/Controller/AppPasswordController.php @@ -171,9 +171,9 @@ public function rotateAppPassword(): DataResponse { /** * Confirm the user password * - * @param string $password The password of the user + * @param non-empty-string $password The password of the user * - * @return DataResponse|DataResponse, array{}> + * @return DataResponse|DataResponse, array{}> * * 200: Password confirmation succeeded * 403: Password confirmation failed @@ -200,7 +200,7 @@ public function confirmUserPassword(string $password): DataResponse { /** * Get app password with one-time password * - * @return DataResponse + * @return DataResponse * @throws OCSForbiddenException Creating app password is not allowed * * 200: App password returned diff --git a/core/Controller/AutoCompleteController.php b/core/Controller/AutoCompleteController.php index 41261fc9cc061..e025de141f6cc 100644 --- a/core/Controller/AutoCompleteController.php +++ b/core/Controller/AutoCompleteController.php @@ -48,9 +48,9 @@ public function __construct( * @param string|null $itemType Type of the items to search for * @param string|null $itemId ID of the items to search for * @param string|null $sorter can be piped, top priority first, e.g.: "commenters|share-recipients" - * @param list $shareTypes Types of shares to search for - * @param int $limit Maximum number of results to return - * @param int $offset Offset for searching + * @param list $shareTypes Types of shares to search for + * @param positive-int $limit Maximum number of results to return + * @param non-negative-int $offset Offset for searching * * @return DataResponse, array{Link?: string}> * diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index c232dd140fc37..2a9a68bc3e302 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -54,7 +54,7 @@ public function __construct( /** * Get the dark avatar * - * @param string $userId ID of the user + * @param non-empty-string $userId ID of the user * @param 64|512 $size Size of the avatar * @param bool $guestFallback Fallback to guest avatar if not found * @return FileDisplayResponse|JSONResponse, array{}>|Response @@ -104,7 +104,7 @@ public function getAvatarDark(string $userId, int $size, bool $guestFallback = f /** * Get the avatar * - * @param string $userId ID of the user + * @param non-empty-string $userId ID of the user * @param 64|512 $size Size of the avatar * @param bool $guestFallback Fallback to guest avatar if not found * @return FileDisplayResponse|JSONResponse, array{}>|Response @@ -151,6 +151,9 @@ public function getAvatar(string $userId, int $size, bool $guestFallback = false return $response; } + /** + * @param ?non-empty-string $path + */ #[NoAdminRequired] #[FrontpageRoute(verb: 'POST', url: '/avatar/')] public function postAvatar(?string $path = null): JSONResponse { diff --git a/core/Controller/PreviewController.php b/core/Controller/PreviewController.php index 310d7f85f74a1..8d79082975215 100644 --- a/core/Controller/PreviewController.php +++ b/core/Controller/PreviewController.php @@ -45,8 +45,8 @@ public function __construct( * Get a preview by file path * * @param string $file Path of the file - * @param int $x Width of the preview. A width of -1 will use the original image width. - * @param int $y Height of the preview. A height of -1 will use the original image height. + * @param int<-1, max> $x Width of the preview. A width of -1 will use the original image width. + * @param int<-1, max> $y Height of the preview. A height of -1 will use the original image height. * @param bool $a Preserve the aspect ratio * @param bool $forceIcon Force returning an icon * @param 'fill'|'cover' $mode How to crop the image @@ -88,9 +88,9 @@ public function getPreview( /** * Get a preview by file ID * - * @param int $fileId ID of the file - * @param int $x Width of the preview. A width of -1 will use the original image width. - * @param int $y Height of the preview. A height of -1 will use the original image height. + * @param positive-int $fileId ID of the file + * @param int<-1, max> $x Width of the preview. A width of -1 will use the original image width. + * @param int<-1, max> $y Height of the preview. A height of -1 will use the original image height. * @param bool $a Preserve the aspect ratio * @param bool $forceIcon Force returning an icon * @param 'fill'|'cover' $mode How to crop the image @@ -108,14 +108,14 @@ public function getPreview( #[FrontpageRoute(verb: 'GET', url: '/core/preview')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] public function getPreviewByFileId( - int $fileId = -1, + int $fileId, int $x = 32, int $y = 32, bool $a = false, bool $forceIcon = true, string $mode = 'fill', bool $mimeFallback = false) { - if ($fileId === -1 || $x === 0 || $y === 0) { + if ($x === 0 || $y === 0) { return new DataResponse([], Http::STATUS_BAD_REQUEST); } diff --git a/core/openapi-full.json b/core/openapi-full.json index 6b458c3187a26..40cdcd3739a1b 100644 --- a/core/openapi-full.json +++ b/core/openapi-full.json @@ -1643,7 +1643,8 @@ "properties": { "password": { "type": "string", - "description": "The password of the user" + "description": "The password of the user", + "minLength": 1 } } } @@ -1691,7 +1692,8 @@ "properties": { "lastLogin": { "type": "integer", - "format": "int64" + "format": "int64", + "minimum": 1 } } } @@ -1823,7 +1825,8 @@ ], "properties": { "apppassword": { - "type": "string" + "type": "string", + "minLength": 1 } } } @@ -1955,7 +1958,8 @@ "default": [], "items": { "type": "integer", - "format": "int64" + "format": "int64", + "minimum": 1 } } }, @@ -1966,7 +1970,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 10 + "default": 10, + "minimum": 1 } }, { @@ -1976,7 +1981,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 0 + "default": 0, + "minimum": 0 } }, { @@ -9264,7 +9270,8 @@ "description": "ID of the user", "required": true, "schema": { - "type": "string" + "type": "string", + "minLength": 1 } }, { @@ -9367,7 +9374,8 @@ "description": "ID of the user", "required": true, "schema": { - "type": "string" + "type": "string", + "minLength": 1 } }, { @@ -9994,7 +10002,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -10004,7 +10013,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -10135,10 +10145,11 @@ "name": "fileId", "in": "query", "description": "ID of the file", + "required": true, "schema": { "type": "integer", "format": "int64", - "default": -1 + "minimum": 1 } }, { @@ -10148,7 +10159,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -10158,7 +10170,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { diff --git a/core/openapi.json b/core/openapi.json index 54a000845c207..7b562d53e3459 100644 --- a/core/openapi.json +++ b/core/openapi.json @@ -1643,7 +1643,8 @@ "properties": { "password": { "type": "string", - "description": "The password of the user" + "description": "The password of the user", + "minLength": 1 } } } @@ -1691,7 +1692,8 @@ "properties": { "lastLogin": { "type": "integer", - "format": "int64" + "format": "int64", + "minimum": 1 } } } @@ -1823,7 +1825,8 @@ ], "properties": { "apppassword": { - "type": "string" + "type": "string", + "minLength": 1 } } } @@ -1955,7 +1958,8 @@ "default": [], "items": { "type": "integer", - "format": "int64" + "format": "int64", + "minimum": 1 } } }, @@ -1966,7 +1970,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 10 + "default": 10, + "minimum": 1 } }, { @@ -1976,7 +1981,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 0 + "default": 0, + "minimum": 0 } }, { @@ -9264,7 +9270,8 @@ "description": "ID of the user", "required": true, "schema": { - "type": "string" + "type": "string", + "minLength": 1 } }, { @@ -9367,7 +9374,8 @@ "description": "ID of the user", "required": true, "schema": { - "type": "string" + "type": "string", + "minLength": 1 } }, { @@ -9994,7 +10002,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -10004,7 +10013,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -10135,10 +10145,11 @@ "name": "fileId", "in": "query", "description": "ID of the file", + "required": true, "schema": { "type": "integer", "format": "int64", - "default": -1 + "minimum": 1 } }, { @@ -10148,7 +10159,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -10158,7 +10170,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { diff --git a/lib/public/Security/ISecureRandom.php b/lib/public/Security/ISecureRandom.php index c2b769e2ba834..2a3c510de287f 100644 --- a/lib/public/Security/ISecureRandom.php +++ b/lib/public/Security/ISecureRandom.php @@ -62,7 +62,7 @@ interface ISecureRandom { * @param int $length The length of the generated string * @param string $characters An optional list of characters to use if no character list is * specified all valid base64 characters are used. - * @return string + * @return non-empty-string * @since 8.0.0 * @deprecated 35.0.0 Use {@see Randomizer::getBytesFromString()} available in PHP 8.3+ instead. */ diff --git a/openapi.json b/openapi.json index 38c64e9aae37f..d56c71f1d989f 100644 --- a/openapi.json +++ b/openapi.json @@ -5928,7 +5928,8 @@ "properties": { "password": { "type": "string", - "description": "The password of the user" + "description": "The password of the user", + "minLength": 1 } } } @@ -5976,7 +5977,8 @@ "properties": { "lastLogin": { "type": "integer", - "format": "int64" + "format": "int64", + "minimum": 1 } } } @@ -6108,7 +6110,8 @@ ], "properties": { "apppassword": { - "type": "string" + "type": "string", + "minLength": 1 } } } @@ -6240,7 +6243,8 @@ "default": [], "items": { "type": "integer", - "format": "int64" + "format": "int64", + "minimum": 1 } } }, @@ -6251,7 +6255,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 10 + "default": 10, + "minimum": 1 } }, { @@ -6261,7 +6266,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 0 + "default": 0, + "minimum": 0 } }, { @@ -13596,7 +13602,8 @@ "description": "ID of the user", "required": true, "schema": { - "type": "string" + "type": "string", + "minLength": 1 } }, { @@ -13699,7 +13706,8 @@ "description": "ID of the user", "required": true, "schema": { - "type": "string" + "type": "string", + "minLength": 1 } }, { @@ -14326,7 +14334,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -14336,7 +14345,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -14467,10 +14477,11 @@ "name": "fileId", "in": "query", "description": "ID of the file", + "required": true, "schema": { "type": "integer", "format": "int64", - "default": -1 + "minimum": 1 } }, { @@ -14480,7 +14491,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { @@ -14490,7 +14502,8 @@ "schema": { "type": "integer", "format": "int64", - "default": 32 + "default": 32, + "minimum": -1 } }, { From 00c1b55b1488386f5e2a9029203bd3537b7de18d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 17 Aug 2026 22:32:31 +0200 Subject: [PATCH 3/6] fix(psalm): Make psalm happy Unfortunately static has return type didn't fix it but it is still more correct. Signed-off-by: Carl Schwan --- .../lib/Controller/GroupsController.php | 30 +++++------ .../lib/Controller/UsersController.php | 50 +++++++++---------- .../lib/Controller/ApiV1Controller.php | 9 ++-- .../lib/Controller/StatusesController.php | 12 ++--- core/Controller/AutoCompleteController.php | 10 ++-- core/openapi-full.json | 2 +- core/openapi.json | 2 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Security/SecureRandom.php | 18 ++----- lib/public/AppFramework/Http/DataResponse.php | 2 +- lib/public/AppFramework/Http/Response.php | 2 +- openapi.json | 2 +- 13 files changed, 67 insertions(+), 74 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 3b436a4a45033..3ad84832aa0e6 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -94,15 +94,15 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = return $group->getGID(); }, $groups); - $response = new DataResponse(['groups' => $groups]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse(['groups' => $groups], headers: $headers); } /** @@ -133,15 +133,15 @@ public function getGroupsDetails(string $search = '', ?int $limit = null, int $o ]; }, $groups); - $response = new DataResponse(['groups' => $groups]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse(['groups' => $groups], headers: $headers); } /** @@ -257,18 +257,18 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int // continue if a users ceased to exist. } } - $response = new DataResponse([ - 'users' => $usersDetails, - 'groups' => $this->findGroupsWithDisplayname($usersDetails), - ]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse([ + 'users' => $usersDetails, + 'groups' => $this->findGroupsWithDisplayname($usersDetails), + ], headers: $headers); } throw new OCSException('The requested group could not be found', OCSController::RESPOND_NOT_FOUND); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 0d41535714be4..d55c56e022771 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -146,17 +146,17 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = /** @var list $users */ $users = array_keys($users); - $response = new DataResponse([ - 'users' => $users - ]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse([ + 'users' => $users + ], headers: $headers); } /** @@ -218,18 +218,18 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of } } - $response = new DataResponse([ - 'users' => $usersDetails, - 'groups' => $this->findGroupsWithDisplayname($usersDetails), - ]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse([ + 'users' => $usersDetails, + 'groups' => $this->findGroupsWithDisplayname($usersDetails), + ], headers: $headers); } /** @@ -311,17 +311,17 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, } } - $response = new DataResponse([ - 'users' => $usersDetails - ]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse([ + 'users' => $usersDetails + ], headers: $headers); } /** @@ -378,17 +378,17 @@ public function getLastLoggedInUsers( } } - $response = new DataResponse([ - 'users' => $usersDetails - ]); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse([ + 'users' => $usersDetails + ], headers: $headers); } /** diff --git a/apps/sharing/lib/Controller/ApiV1Controller.php b/apps/sharing/lib/Controller/ApiV1Controller.php index 7029330107741..483a5b979af7a 100644 --- a/apps/sharing/lib/Controller/ApiV1Controller.php +++ b/apps/sharing/lib/Controller/ApiV1Controller.php @@ -114,17 +114,18 @@ public function searchRecipients(?array $filterRecipientTypeClasses, string $que $recipients = $this->manager->searchRecipients($this->accessContext, $filterRecipientTypeClasses, $query, $limit, $offset, $forShare); $this->dbConnection->commit(); - $response = new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients)); + $headers = []; if ($this->hasMoreResults($recipients, $limit)) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'filterRecipientTypeClasses' => $filterRecipientTypeClasses, 'query' => $query, 'limit' => $limit, 'offset' => $offset + $limit, 'id' => $id, - ])]); + ]); } - return $response; + + return new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients), headers: $headers); } catch (Exception $exception) { $this->dbConnection->rollBack(); throw $exception; diff --git a/apps/user_status/lib/Controller/StatusesController.php b/apps/user_status/lib/Controller/StatusesController.php index ab4687e9b334c..ef67dc3c73407 100644 --- a/apps/user_status/lib/Controller/StatusesController.php +++ b/apps/user_status/lib/Controller/StatusesController.php @@ -62,16 +62,16 @@ public function findAll(?int $limit = null, ?int $offset = null): DataResponse { $allStatuses = $this->service->findAll($limit, $offset); $hasMoreResults = $this->hasMoreResults($allStatuses, $limit); - $response = new DataResponse(array_values(array_map(function ($userStatus) { - return $this->formatStatus($userStatus); - }, $allStatuses))); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'limit' => $limit, 'offset' => ($offset ?? 0) + $limit, - ])]); + ]); } - return $response; + return new DataResponse(array_values(array_map(function ($userStatus) { + return $this->formatStatus($userStatus); + }, $allStatuses)), headers: $headers); } /** diff --git a/core/Controller/AutoCompleteController.php b/core/Controller/AutoCompleteController.php index e025de141f6cc..e0be19e5887ac 100644 --- a/core/Controller/AutoCompleteController.php +++ b/core/Controller/AutoCompleteController.php @@ -48,7 +48,7 @@ public function __construct( * @param string|null $itemType Type of the items to search for * @param string|null $itemId ID of the items to search for * @param string|null $sorter can be piped, top priority first, e.g.: "commenters|share-recipients" - * @param list $shareTypes Types of shares to search for + * @param list $shareTypes Types of shares to search for * @param positive-int $limit Maximum number of results to return * @param non-negative-int $offset Offset for searching * @@ -90,9 +90,9 @@ public function get(string $search, ?string $itemType, ?string $itemId, ?string // transform to expected format $results = $this->prepareResultArray($results); - $response = new DataResponse($results); + $headers = []; if ($hasMoreResults) { - $response->setHeaders(['Link' => $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ 'search' => $search, 'itemType' => $itemType, 'itemId' => $itemId, @@ -100,10 +100,10 @@ public function get(string $search, ?string $itemType, ?string $itemId, ?string 'shareTypes' => $shareTypes, 'limit' => $limit, 'offset' => $offset + $limit, - ])]); + ]); } - return $response; + return new DataResponse($results, headers: $headers); } /** diff --git a/core/openapi-full.json b/core/openapi-full.json index 40cdcd3739a1b..ebc5b8d0fcce2 100644 --- a/core/openapi-full.json +++ b/core/openapi-full.json @@ -1959,7 +1959,7 @@ "items": { "type": "integer", "format": "int64", - "minimum": 1 + "minimum": 0 } } }, diff --git a/core/openapi.json b/core/openapi.json index 7b562d53e3459..41906ac31ac93 100644 --- a/core/openapi.json +++ b/core/openapi.json @@ -1959,7 +1959,7 @@ "items": { "type": "integer", "format": "int64", - "minimum": 1 + "minimum": 0 } } }, diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index be06cdd45b860..028e899d51056 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1207,6 +1207,7 @@ 'OC\\AppFramework\\Http\\Attributes\\TwoFactorSetUpDoneRequired' => $baseDir . '/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php', 'OC\\AppFramework\\Http\\Dispatcher' => $baseDir . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => $baseDir . '/lib/private/AppFramework/Http/Output.php', + 'OC\\AppFramework\\Http\\PaginationTrait' => $baseDir . '/lib/private/AppFramework/Http/PaginationTrait.php', 'OC\\AppFramework\\Http\\Request' => $baseDir . '/lib/private/AppFramework/Http/Request.php', 'OC\\AppFramework\\Http\\RequestId' => $baseDir . '/lib/private/AppFramework/Http/RequestId.php', 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 73277b5502fe1..1e803efed5f4b 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1248,6 +1248,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Http\\Attributes\\TwoFactorSetUpDoneRequired' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php', 'OC\\AppFramework\\Http\\Dispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Output.php', + 'OC\\AppFramework\\Http\\PaginationTrait' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/PaginationTrait.php', 'OC\\AppFramework\\Http\\Request' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Request.php', 'OC\\AppFramework\\Http\\RequestId' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/RequestId.php', 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', diff --git a/lib/private/Security/SecureRandom.php b/lib/private/Security/SecureRandom.php index 8d38bd32df799..d823942e95bd3 100644 --- a/lib/private/Security/SecureRandom.php +++ b/lib/private/Security/SecureRandom.php @@ -14,21 +14,9 @@ /** * Class SecureRandom provides a wrapper around the random_int function to generate - * secure random strings. For PHP 7 the native CSPRNG is used, older versions do - * use a fallback. - * - * Usage: - * \OC::$server->get(ISecureRandom::class)->generate(10); - * @package OC\Security + * secure random strings. This use the native CSPRNG. */ class SecureRandom implements ISecureRandom { - /** - * Generate a secure random string of specified length. - * @param int $length The length of the generated string - * @param string $characters An optional list of characters to use if no character list is - * specified all valid base64 characters are used. - * @throws \LengthException if an invalid length is requested - */ #[\Override] public function generate( int $length, @@ -38,6 +26,8 @@ public function generate( throw new \LengthException('Invalid length specified: ' . $length . ' must be bigger than 0'); } - return (new Randomizer())->getBytesFromString($characters, $length); + /** @var non-empty-string $result */ + $result = (new Randomizer())->getBytesFromString($characters, $length); + return $result; } } diff --git a/lib/public/AppFramework/Http/DataResponse.php b/lib/public/AppFramework/Http/DataResponse.php index ea03ffc877a17..3ba05c21c523b 100644 --- a/lib/public/AppFramework/Http/DataResponse.php +++ b/lib/public/AppFramework/Http/DataResponse.php @@ -42,7 +42,7 @@ public function __construct(mixed $data = [], int $statusCode = Http::STATUS_OK, * Sets values in the data json array * @psalm-suppress InvalidTemplateParam * @param T $data an array or object which will be transformed - * @return DataResponse Reference to this object + * @return static Reference to this object * @since 8.0.0 */ public function setData($data) { diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index cccd6a318a4c7..9aa2eca0d4b85 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -170,7 +170,7 @@ public function getCookies() { * function * @param string $name The name of the HTTP header * @param string $value The value, null will delete it - * @return $this + * @return static * @since 6.0.0 - return value was added in 7.0.0 */ public function addHeader($name, $value) { diff --git a/openapi.json b/openapi.json index d56c71f1d989f..7d2059c3483a5 100644 --- a/openapi.json +++ b/openapi.json @@ -6244,7 +6244,7 @@ "items": { "type": "integer", "format": "int64", - "minimum": 1 + "minimum": 0 } } }, From 69dba26180b8a4858b7328579262a3a6c34e775d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 20 Aug 2026 16:21:38 +0200 Subject: [PATCH 4/6] refactor: Reduce code duplication Signed-off-by: Carl Schwan --- .../lib/Controller/GroupsController.php | 20 ++++----- .../lib/Controller/UsersController.php | 24 ++++------- .../lib/Controller/ApiV1Controller.php | 6 +-- core/Controller/AutoCompleteController.php | 6 +-- .../AppFramework/Http/PaginationTrait.php | 41 +++++++++++++------ .../AppFramework/Http/PaginationTraitTest.php | 24 +++++++---- 6 files changed, 64 insertions(+), 57 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 3ad84832aa0e6..6834f3f45a403 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -59,7 +59,7 @@ public function __construct( IRootFolder $rootFolder, private LoggerInterface $logger, GroupDisplayNameCache $groupDisplayNameCache, - private IURLGenerator $urlGenerator, + private readonly IURLGenerator $urlGenerator, ) { parent::__construct($appName, $request, @@ -96,11 +96,9 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse(['groups' => $groups], headers: $headers); } @@ -135,11 +133,9 @@ public function getGroupsDetails(string $search = '', ?int $limit = null, int $o $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse(['groups' => $groups], headers: $headers); } @@ -259,11 +255,9 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int } $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse([ 'users' => $usersDetails, diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index d55c56e022771..6b5302b5761f1 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -148,11 +148,9 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse([ 'users' => $users @@ -220,11 +218,9 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse([ 'users' => $usersDetails, @@ -313,11 +309,9 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse([ 'users' => $usersDetails @@ -380,11 +374,9 @@ public function getLastLoggedInUsers( $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse([ 'users' => $usersDetails diff --git a/apps/sharing/lib/Controller/ApiV1Controller.php b/apps/sharing/lib/Controller/ApiV1Controller.php index 483a5b979af7a..b651fd1456938 100644 --- a/apps/sharing/lib/Controller/ApiV1Controller.php +++ b/apps/sharing/lib/Controller/ApiV1Controller.php @@ -116,13 +116,11 @@ public function searchRecipients(?array $filterRecipientTypeClasses, string $que $headers = []; if ($this->hasMoreResults($recipients, $limit)) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'filterRecipientTypeClasses' => $filterRecipientTypeClasses, 'query' => $query, - 'limit' => $limit, - 'offset' => $offset + $limit, 'id' => $id, - ]); + ], $limit, $offset); } return new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients), headers: $headers); diff --git a/core/Controller/AutoCompleteController.php b/core/Controller/AutoCompleteController.php index e0be19e5887ac..912b3b0875f24 100644 --- a/core/Controller/AutoCompleteController.php +++ b/core/Controller/AutoCompleteController.php @@ -92,15 +92,13 @@ public function get(string $search, ?string $itemType, ?string $itemId, ?string $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ 'search' => $search, 'itemType' => $itemType, 'itemId' => $itemId, 'sorter' => $sorter, 'shareTypes' => $shareTypes, - 'limit' => $limit, - 'offset' => $offset + $limit, - ]); + ], $limit, $offset); } return new DataResponse($results, headers: $headers); diff --git a/lib/private/AppFramework/Http/PaginationTrait.php b/lib/private/AppFramework/Http/PaginationTrait.php index 6e4f05de9804d..73cf8749983e4 100644 --- a/lib/private/AppFramework/Http/PaginationTrait.php +++ b/lib/private/AppFramework/Http/PaginationTrait.php @@ -17,14 +17,10 @@ * current page, without the backing query natively reporting a total count. */ trait PaginationTrait { + private IURLGenerator $urlGenerator; + private IRequest $request; + /** - * Whether a page of results might be followed by more results. - * - * A page that came back with fewer items than the requested limit cannot - * be followed by more. A full page might be the last one too (the total - * count could be an exact multiple of $limit), but there is no way to - * tell without a second query, so a full page is treated as "more". - * * @param array $items The page of results as returned for $limit * @param non-negative-int|null $limit Requested page size, or null for "no limit" (never "more" in that case) */ @@ -34,14 +30,35 @@ protected function hasMoreResults(array $items, ?int $limit): bool { /** * Builds a `Link: ; rel="next"` response header value pointing at the - * next page of the current request, keeping its path (including any route - * placeholders already resolved into it) and replacing the query string. + * next page of the current request. * * @param array $params Query parameters for the next page, e.g. the incremented offset */ - protected function buildNextPageLinkHeader(IRequest $request, IURLGenerator $urlGenerator, array $params): string { - $path = (string)parse_url($request->getRequestUri(), PHP_URL_PATH); - $url = $urlGenerator->getAbsoluteURL($path) . '?' . http_build_query($params); + protected function buildOffsetNextPageLinkHeader(array $params, int $limit, int $offset): string { + $params = array_merge($params, [ + 'limit' => $limit, + 'offset' => $offset + $limit, + ]); + $path = (string)parse_url($this->request->getRequestUri(), PHP_URL_PATH); + $url = $this->urlGenerator->getAbsoluteURL($path) . '?' . http_build_query($params); + return '<' . $url . '>; rel="next"'; + } + + /** + * Builds a `Link: ; rel="next"` response header value pointing at the + * next page of the current request, using keyset (seek) pagination instead + * of an offset. + * + * @param array $params Query parameters for the next page + * @param int|string $lastId Id of the last entity of the current page + */ + protected function buildCursorNextPageLinkHeader(array $params, int $limit, int|string $lastId): string { + $params = array_merge($params, [ + 'limit' => $limit, + 'lastId' => $lastId, + ]); + $path = (string)parse_url($this->request->getRequestUri(), PHP_URL_PATH); + $url = $this->urlGenerator->getAbsoluteURL($path) . '?' . http_build_query($params); return '<' . $url . '>; rel="next"'; } } diff --git a/tests/lib/AppFramework/Http/PaginationTraitTest.php b/tests/lib/AppFramework/Http/PaginationTraitTest.php index c72fcafbffae2..b2695f2409d2b 100644 --- a/tests/lib/AppFramework/Http/PaginationTraitTest.php +++ b/tests/lib/AppFramework/Http/PaginationTraitTest.php @@ -14,20 +14,30 @@ use Test\TestCase; class PaginationTraitTest extends TestCase { + private IRequest&\PHPUnit\Framework\MockObject\MockObject $request; + private IURLGenerator&\PHPUnit\Framework\MockObject\MockObject $urlGenerator; private object $subject; protected function setUp(): void { parent::setUp(); - $this->subject = new class { + $this->request = $this->createMock(IRequest::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + + $this->subject = new class($this->request, $this->urlGenerator) { use PaginationTrait; + public function __construct(IRequest $request, IURLGenerator $urlGenerator) { + $this->request = $request; + $this->urlGenerator = $urlGenerator; + } + public function hasMore(array $items, ?int $limit): bool { return $this->hasMoreResults($items, $limit); } - public function nextLink(IRequest $request, IURLGenerator $urlGenerator, array $params): string { - return $this->buildNextPageLinkHeader($request, $urlGenerator, $params); + public function nextLink(array $params, int $limit, int $offset): string { + return $this->buildOffsetNextPageLinkHeader($params, $limit, $offset); } }; } @@ -57,15 +67,13 @@ public function testAssociativeArrayIsCountedByEntries(): void { } public function testNextLinkKeepsRequestPathAndAppliesGivenQuery(): void { - $request = $this->createMock(IRequest::class); - $request->method('getRequestUri')->willReturn('/ocs/v2.php/apps/provisioning_api/api/v1/groups?search=foo&limit=5&offset=0'); + $this->request->method('getRequestUri')->willReturn('/ocs/v2.php/apps/provisioning_api/api/v1/groups?search=foo&limit=5&offset=0'); - $urlGenerator = $this->createMock(IURLGenerator::class); - $urlGenerator->method('getAbsoluteURL') + $this->urlGenerator->method('getAbsoluteURL') ->with('/ocs/v2.php/apps/provisioning_api/api/v1/groups') ->willReturn('https://cloud.example.com/ocs/v2.php/apps/provisioning_api/api/v1/groups'); - $link = $this->subject->nextLink($request, $urlGenerator, ['search' => 'foo', 'limit' => 5, 'offset' => 5]); + $link = $this->subject->nextLink(['search' => 'foo'], 5, 0); $this->assertSame( '; rel="next"', From c6506af2ee5402d5a0aaede30b354d9c0a719628 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 20 Aug 2026 16:22:06 +0200 Subject: [PATCH 5/6] feat(user_status): Support cursor based pagination Signed-off-by: Carl Schwan --- .../lib/Controller/StatusesController.php | 23 +++++++--- apps/user_status/lib/Db/UserStatusMapper.php | 28 +++++++++++++ .../user_status/lib/Service/StatusService.php | 18 ++++++++ .../Controller/StatusesControllerTest.php | 42 +++++++++++++++++++ .../tests/Unit/Db/UserStatusMapperTest.php | 13 ++++++ .../tests/Unit/Service/StatusServiceTest.php | 15 +++++++ 6 files changed, 133 insertions(+), 6 deletions(-) diff --git a/apps/user_status/lib/Controller/StatusesController.php b/apps/user_status/lib/Controller/StatusesController.php index ef67dc3c73407..b722a9d4253c2 100644 --- a/apps/user_status/lib/Controller/StatusesController.php +++ b/apps/user_status/lib/Controller/StatusesController.php @@ -52,22 +52,33 @@ public function __construct( * * @param int|null $limit Maximum number of statuses to find * @param non-negative-int|null $offset Offset for finding statuses + * @param non-negative-int|null $lastId Id of the last status returned by the previous page; + * when given, keyset pagination is used and $offset is ignored * @return DataResponse, array{Link?: string}> * + * @note Prefer $lastId over $offset: it does not require the database to scan and discard + * every preceding row on every call. + * * 200: Statuses returned */ #[NoAdminRequired] #[ApiRoute(verb: 'GET', url: '/api/v1/statuses')] - public function findAll(?int $limit = null, ?int $offset = null): DataResponse { - $allStatuses = $this->service->findAll($limit, $offset); + public function findAll(?int $limit = null, ?int $offset = null, ?int $lastId = null): DataResponse { + if ($lastId !== null) { + $allStatuses = $this->service->findAllAfterId($limit, $lastId); + } else { + $allStatuses = $this->service->findAll($limit, $offset); + } $hasMoreResults = $this->hasMoreResults($allStatuses, $limit); $headers = []; if ($hasMoreResults) { - $headers['Link'] = $this->buildNextPageLinkHeader($this->request, $this->urlGenerator, [ - 'limit' => $limit, - 'offset' => ($offset ?? 0) + $limit, - ]); + if ($lastId !== null) { + $lastStatus = end($allStatuses); + $headers['Link'] = $this->buildCursorNextPageLinkHeader([], $limit, $lastStatus->getId()); + } else { + $headers['Link'] = $this->buildOffsetNextPageLinkHeader([], $limit, $offset ?? 0); + } } return new DataResponse(array_values(array_map(function ($userStatus) { return $this->formatStatus($userStatus); diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index ea3b76c8e564d..3c6bd683393e2 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -49,6 +49,34 @@ public function findAll(?int $limit = null, ?int $offset = null):array { return $this->findEntities($qb); } + /** + * Finds statuses one page at a time ordered by id, using keyset (seek) pagination + * instead of OFFSET/LIMIT, so each page costs the same regardless of how deep it is. + * + * @param int|null $limit + * @param int|null $lastId Id of the last status from the previous page, or null to fetch the first page + * @return UserStatus[] + */ + public function findAllAfterId(?int $limit = null, ?int $lastId = null): array { + $qb = $this->db->getQueryBuilder(); + $qb + ->select('*') + ->from($this->tableName) + ->where($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL))); + + if ($lastId !== null) { + $qb->andWhere($qb->expr()->gt('id', $qb->createNamedParameter($lastId, IQueryBuilder::PARAM_INT))); + } + + $qb->orderBy('id', 'ASC'); + + if ($limit !== null) { + $qb->setMaxResults($limit); + } + + return $this->findEntities($qb); + } + /** * @param int|null $limit * @param int|null $offset diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 6c1805963c881..5975a68759ff4 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -97,6 +97,24 @@ public function findAll(?int $limit = null, ?int $offset = null): array { }, $this->mapper->findAll($limit, $offset)); } + /** + * @param int|null $limit + * @param int|null $lastId Id of the last status from the previous page, or null to fetch the first page + * @return UserStatus[] + */ + public function findAllAfterId(?int $limit = null, ?int $lastId = null): array { + // Return empty array if user enumeration is disabled or limited to groups + // TODO: find a solution that scales to get only users from common groups if user enumeration is limited to + // groups. See discussion at https://github.com/nextcloud/server/pull/27879#discussion_r729715936 + if (!$this->shareeEnumeration || $this->shareeEnumerationInGroupOnly || $this->shareeEnumerationPhone) { + return []; + } + + return array_map(function ($status) { + return $this->processStatus($status); + }, $this->mapper->findAllAfterId($limit, $lastId)); + } + /** * @param int|null $limit * @param int|null $offset diff --git a/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php b/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php index e20e498825cfb..6beee8569a96f 100644 --- a/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php +++ b/apps/user_status/tests/Unit/Controller/StatusesControllerTest.php @@ -54,6 +54,48 @@ public function testFindAll(): void { $this->assertArrayNotHasKey('Link', $response->getHeaders()); } + public function testFindAllWithLastId(): void { + $userStatus = $this->getUserStatus(); + + $this->service->expects($this->once()) + ->method('findAllAfterId') + ->with(20, 1336) + ->willReturn([$userStatus]); + $this->service->expects($this->never()) + ->method('findAll'); + + $response = $this->controller->findAll(20, null, 1336); + $this->assertEquals([[ + 'userId' => 'john.doe', + 'status' => 'offline', + 'icon' => '🏝', + 'message' => 'On vacation', + 'clearAt' => 60000, + ]], $response->getData()); + $this->assertArrayNotHasKey('Link', $response->getHeaders()); + } + + public function testFindAllWithLastIdHasMoreResults(): void { + $userStatus = $this->getUserStatus(); + + $this->service->expects($this->once()) + ->method('findAllAfterId') + ->with(1, 1336) + ->willReturn([$userStatus]); + + $this->request->method('getRequestUri') + ->willReturn('/ocs/v2.php/apps/user_status/api/v1/statuses?limit=1&lastId=1336'); + $this->urlGenerator->method('getAbsoluteURL') + ->with('/ocs/v2.php/apps/user_status/api/v1/statuses') + ->willReturn('https://cloud.example.com/ocs/v2.php/apps/user_status/api/v1/statuses'); + + $response = $this->controller->findAll(1, null, 1336); + $this->assertSame( + '; rel="next"', + $response->getHeaders()['Link'] + ); + } + public function testFind(): void { $userStatus = $this->getUserStatus(); diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index a697571bd5709..73466adad4f34 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -48,6 +48,19 @@ public function testGetFindAll(): void { $this->assertEquals('user2', $offsetResults[0]->getUserId()); } + public function testGetFindAllAfterId(): void { + $this->insertSampleStatuses(); + + $firstPage = $this->mapper->findAllAfterId(2); + $this->assertCount(2, $firstPage); + $this->assertEquals('admin', $firstPage[0]->getUserId()); + $this->assertEquals('user1', $firstPage[1]->getUserId()); + + $secondPage = $this->mapper->findAllAfterId(2, $firstPage[1]->getId()); + $this->assertCount(1, $secondPage); + $this->assertEquals('user2', $secondPage[0]->getUserId()); + } + public function testFindAllExcludesBackups(): void { $this->insertSampleStatuses(); diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index 69128033631e0..bb802e11622b5 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -82,6 +82,21 @@ public function testFindAll(): void { ], $this->service->findAll(20, 50)); } + public function testFindAllAfterId(): void { + $status1 = $this->createMock(UserStatus::class); + $status2 = $this->createMock(UserStatus::class); + + $this->mapper->expects($this->once()) + ->method('findAllAfterId') + ->with(20, 1336) + ->willReturn([$status1, $status2]); + + $this->assertEquals([ + $status1, + $status2, + ], $this->service->findAllAfterId(20, 1336)); + } + public function testFindAllRecentStatusChanges(): void { $status1 = $this->createMock(UserStatus::class); $status2 = $this->createMock(UserStatus::class); From 6bbbd7efcbbc4c8792369fc0dd5d5e1306c634df Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 20 Aug 2026 16:39:36 +0200 Subject: [PATCH 6/6] refactor(pagination): Remove code duplication Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: Carl Schwan --- .../lib/Controller/GroupsController.php | 30 +++------ .../lib/Controller/UsersController.php | 44 ++++--------- .../lib/Controller/ApiV1Controller.php | 13 ++-- .../lib/Controller/StatusesController.php | 15 ++--- apps/user_status/openapi.json | 13 ++++ core/Controller/AutoCompleteController.php | 19 +++--- .../AppFramework/Http/PaginationTrait.php | 55 +++++++++++------ lib/public/AppFramework/Http/DataResponse.php | 2 +- lib/public/AppFramework/Http/Response.php | 2 +- openapi.json | 13 ++++ .../AppFramework/Http/PaginationTraitTest.php | 61 ++++++++++++++++--- 11 files changed, 157 insertions(+), 110 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 6834f3f45a403..1e0a61467e7cb 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -88,18 +88,14 @@ public function __construct( #[NoAdminRequired] public function getGroups(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { $groups = $this->groupManager->search($search, $limit, $offset); - $hasMoreResults = $this->hasMoreResults($groups, $limit); $groups = array_map(function ($group) { /** @var IGroup $group */ return $group->getGID(); }, $groups); - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($groups, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse(['groups' => $groups], headers: $headers); } @@ -118,7 +114,6 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = #[AuthorizedAdminSetting(settings: Users::class)] public function getGroupsDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { $groups = $this->groupManager->search($search, $limit, $offset); - $hasMoreResults = $this->hasMoreResults($groups, $limit); $groups = array_map(function ($group) { /** @var IGroup $group */ return [ @@ -131,12 +126,9 @@ public function getGroupsDetails(string $search = '', ?int $limit = null, int $o ]; }, $groups); - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($groups, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse(['groups' => $groups], headers: $headers); } @@ -232,7 +224,6 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentUser->getUID()); if ($isAdmin || $isDelegatedAdmin || $isSubadminOfGroup) { $users = $group->searchUsers($search, $limit, $offset); - $hasMoreResults = $this->hasMoreResults($users, $limit); // Extract required number $usersDetails = []; @@ -253,12 +244,9 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int // continue if a users ceased to exist. } } - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($users, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse([ 'users' => $usersDetails, 'groups' => $this->findGroupsWithDisplayname($usersDetails), diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 6b5302b5761f1..65ac66a18549c 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -141,17 +141,12 @@ public function getUsers(string $search = '', ?int $limit = null, int $offset = } } - $hasMoreResults = $this->hasMoreResults($users, $limit); - /** @var list $users */ $users = array_keys($users); - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($users, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse([ 'users' => $users ], headers: $headers); @@ -193,8 +188,6 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of $users = array_merge(...$users); } - $hasMoreResults = $this->hasMoreResults($users, $limit); - $usersDetails = []; foreach ($users as $userId) { $userId = (string)$userId; @@ -216,12 +209,9 @@ public function getUsersDetails(string $search = '', ?int $limit = null, int $of } } - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($users, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse([ 'users' => $usersDetails, 'groups' => $this->findGroupsWithDisplayname($usersDetails), @@ -285,8 +275,6 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, $users = array_slice($users, $offset, $limit); } - $hasMoreResults = $this->hasMoreResults($users, $limit); - $usersDetails = []; foreach ($users as $userId) { try { @@ -307,12 +295,9 @@ public function getDisabledUsersDetails(string $search = '', ?int $limit = null, } } - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($users, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse([ 'users' => $usersDetails ], headers: $headers); @@ -350,8 +335,6 @@ public function getLastLoggedInUsers( // For Admin alone user sorting based on lastLogin. For sub admin and groups this is not supported $users = $this->userManager->getLastLoggedInUsers($limit, $offset, $search); - $hasMoreResults = $this->hasMoreResults($users, $limit); - $usersDetails = []; foreach ($users as $userId) { try { @@ -372,12 +355,9 @@ public function getLastLoggedInUsers( } } - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($users, [ + 'search' => $search, + ], $limit, $offset); return new DataResponse([ 'users' => $usersDetails ], headers: $headers); diff --git a/apps/sharing/lib/Controller/ApiV1Controller.php b/apps/sharing/lib/Controller/ApiV1Controller.php index b651fd1456938..ca137614e20e8 100644 --- a/apps/sharing/lib/Controller/ApiV1Controller.php +++ b/apps/sharing/lib/Controller/ApiV1Controller.php @@ -114,14 +114,11 @@ public function searchRecipients(?array $filterRecipientTypeClasses, string $que $recipients = $this->manager->searchRecipients($this->accessContext, $filterRecipientTypeClasses, $query, $limit, $offset, $forShare); $this->dbConnection->commit(); - $headers = []; - if ($this->hasMoreResults($recipients, $limit)) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'filterRecipientTypeClasses' => $filterRecipientTypeClasses, - 'query' => $query, - 'id' => $id, - ], $limit, $offset); - } + $headers = $this->buildOffsetNextPageLinkHeader($recipients, [ + 'filterRecipientTypeClasses' => $filterRecipientTypeClasses, + 'query' => $query, + 'id' => $id, + ], $limit, $offset); return new DataResponse(ShareRecipient::formatMultiple($this->registry, $this->l10nFactory, $this->urlGenerator, $this->userManager, $recipients), headers: $headers); } catch (Exception $exception) { diff --git a/apps/user_status/lib/Controller/StatusesController.php b/apps/user_status/lib/Controller/StatusesController.php index b722a9d4253c2..d9520afde6a0b 100644 --- a/apps/user_status/lib/Controller/StatusesController.php +++ b/apps/user_status/lib/Controller/StatusesController.php @@ -69,16 +69,11 @@ public function findAll(?int $limit = null, ?int $offset = null, ?int $lastId = } else { $allStatuses = $this->service->findAll($limit, $offset); } - $hasMoreResults = $this->hasMoreResults($allStatuses, $limit); - - $headers = []; - if ($hasMoreResults) { - if ($lastId !== null) { - $lastStatus = end($allStatuses); - $headers['Link'] = $this->buildCursorNextPageLinkHeader([], $limit, $lastStatus->getId()); - } else { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([], $limit, $offset ?? 0); - } + if ($lastId !== null) { + $lastStatus = end($allStatuses); + $headers = $this->buildCursorNextPageLinkHeader($allStatuses, [], $limit, $lastStatus !== false ? $lastStatus->getId() : null); + } else { + $headers = $this->buildOffsetNextPageLinkHeader($allStatuses, [], $limit, $offset ?? 0); } return new DataResponse(array_values(array_map(function ($userStatus) { return $this->formatStatus($userStatus); diff --git a/apps/user_status/openapi.json b/apps/user_status/openapi.json index 88250e2fb0848..8a023e44b0e46 100644 --- a/apps/user_status/openapi.json +++ b/apps/user_status/openapi.json @@ -472,6 +472,7 @@ "get": { "operationId": "statuses-find-all", "summary": "Find statuses of users", + "description": "Prefer $lastId over $offset: it does not require the database to scan and discard every preceding row on every call.", "tags": [ "statuses" ], @@ -507,6 +508,18 @@ "minimum": 0 } }, + { + "name": "lastId", + "in": "query", + "description": "Id of the last status returned by the previous page; when given, keyset pagination is used and $offset is ignored", + "schema": { + "type": "integer", + "format": "int64", + "nullable": true, + "default": null, + "minimum": 0 + } + }, { "name": "OCS-APIRequest", "in": "header", diff --git a/core/Controller/AutoCompleteController.php b/core/Controller/AutoCompleteController.php index 912b3b0875f24..eb140b7b3587c 100644 --- a/core/Controller/AutoCompleteController.php +++ b/core/Controller/AutoCompleteController.php @@ -90,16 +90,15 @@ public function get(string $search, ?string $itemType, ?string $itemId, ?string // transform to expected format $results = $this->prepareResultArray($results); - $headers = []; - if ($hasMoreResults) { - $headers['Link'] = $this->buildOffsetNextPageLinkHeader([ - 'search' => $search, - 'itemType' => $itemType, - 'itemId' => $itemId, - 'sorter' => $sorter, - 'shareTypes' => $shareTypes, - ], $limit, $offset); - } + // $hasMoreResults comes from the search backend, not from the (differently shaped, + // possibly filtered) $results, so it is passed through as-is rather than re-derived. + $headers = $this->buildOffsetNextPageLinkHeader($hasMoreResults, [ + 'search' => $search, + 'itemType' => $itemType, + 'itemId' => $itemId, + 'sorter' => $sorter, + 'shareTypes' => $shareTypes, + ], $limit, $offset); return new DataResponse($results, headers: $headers); } diff --git a/lib/private/AppFramework/Http/PaginationTrait.php b/lib/private/AppFramework/Http/PaginationTrait.php index 73cf8749983e4..e02154ef9a2b8 100644 --- a/lib/private/AppFramework/Http/PaginationTrait.php +++ b/lib/private/AppFramework/Http/PaginationTrait.php @@ -12,53 +12,74 @@ use OCP\IURLGenerator; /** - * Helper for OCS controllers that paginate a list of results via a limit/offset - * pair and need to tell the caller whether more results might exist beyond the - * current page, without the backing query natively reporting a total count. + * Helper for OCS controllers that paginate a limit/offset list and need to report + * whether more results exist, without the query providing a total count. + * + * Relies on the composing class providing `$this->request` (already declared by + * OCP\AppFramework\Controller) and its own `$this->urlGenerator`; not declared here + * to avoid property composition conflicts with classes that mark theirs readonly. + * + * @property IRequest $request + * @property IURLGenerator $urlGenerator */ trait PaginationTrait { - private IURLGenerator $urlGenerator; - private IRequest $request; - /** * @param array $items The page of results as returned for $limit * @param non-negative-int|null $limit Requested page size, or null for "no limit" (never "more" in that case) */ protected function hasMoreResults(array $items, ?int $limit): bool { - return $limit !== null && $limit > 0 && count($items) === $limit; + return $limit !== null && $limit > 0 && count($items) >= $limit; } /** - * Builds a `Link: ; rel="next"` response header value pointing at the - * next page of the current request. + * @param array|bool $items The page of results, or an already-known "has more" bool + * when the caller has a more authoritative source than the + * item count (e.g. a search backend) + */ + private function resolveHasMoreResults(array|bool $items, ?int $limit): bool { + return is_bool($items) ? $items : $this->hasMoreResults($items, $limit); + } + + /** + * Builds the `headers` for a DataResponse: a `Link: ; rel="next"` header if + * there's a next page, or an empty array otherwise. * * @param array $params Query parameters for the next page, e.g. the incremented offset + * @return array{Link?: string} */ - protected function buildOffsetNextPageLinkHeader(array $params, int $limit, int $offset): string { + protected function buildOffsetNextPageLinkHeader(array|bool $items, array $params, ?int $limit, int $offset): array { + if (!$this->resolveHasMoreResults($items, $limit)) { + return []; + } + $params = array_merge($params, [ 'limit' => $limit, 'offset' => $offset + $limit, ]); $path = (string)parse_url($this->request->getRequestUri(), PHP_URL_PATH); $url = $this->urlGenerator->getAbsoluteURL($path) . '?' . http_build_query($params); - return '<' . $url . '>; rel="next"'; + return ['Link' => '<' . $url . '>; rel="next"']; } /** - * Builds a `Link: ; rel="next"` response header value pointing at the - * next page of the current request, using keyset (seek) pagination instead - * of an offset. + * Same as buildOffsetNextPageLinkHeader(), but using keyset (seek) pagination + * instead of an offset. * * @param array $params Query parameters for the next page - * @param int|string $lastId Id of the last entity of the current page + * @param int|string|null $lastId Id of the last entity of the current page + * @return array{Link?: string} */ - protected function buildCursorNextPageLinkHeader(array $params, int $limit, int|string $lastId): string { + protected function buildCursorNextPageLinkHeader(array|bool $items, array $params, ?int $limit, int|string|null $lastId): array { + if ($lastId === null || !$this->resolveHasMoreResults($items, $limit)) { + return []; + } + $params = array_merge($params, [ 'limit' => $limit, 'lastId' => $lastId, ]); $path = (string)parse_url($this->request->getRequestUri(), PHP_URL_PATH); $url = $this->urlGenerator->getAbsoluteURL($path) . '?' . http_build_query($params); - return '<' . $url . '>; rel="next"'; + return ['Link' => '<' . $url . '>; rel="next"']; } } diff --git a/lib/public/AppFramework/Http/DataResponse.php b/lib/public/AppFramework/Http/DataResponse.php index 3ba05c21c523b..86ccaa7c55a99 100644 --- a/lib/public/AppFramework/Http/DataResponse.php +++ b/lib/public/AppFramework/Http/DataResponse.php @@ -42,7 +42,7 @@ public function __construct(mixed $data = [], int $statusCode = Http::STATUS_OK, * Sets values in the data json array * @psalm-suppress InvalidTemplateParam * @param T $data an array or object which will be transformed - * @return static Reference to this object + * @return $this Reference to this object * @since 8.0.0 */ public function setData($data) { diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index 9aa2eca0d4b85..cccd6a318a4c7 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -170,7 +170,7 @@ public function getCookies() { * function * @param string $name The name of the HTTP header * @param string $value The value, null will delete it - * @return static + * @return $this * @since 6.0.0 - return value was added in 7.0.0 */ public function addHeader($name, $value) { diff --git a/openapi.json b/openapi.json index 7d2059c3483a5..bfb5ed2b96cc2 100644 --- a/openapi.json +++ b/openapi.json @@ -43649,6 +43649,7 @@ "get": { "operationId": "user_status-statuses-find-all", "summary": "Find statuses of users", + "description": "Prefer $lastId over $offset: it does not require the database to scan and discard every preceding row on every call.", "tags": [ "user_status/statuses" ], @@ -43684,6 +43685,18 @@ "minimum": 0 } }, + { + "name": "lastId", + "in": "query", + "description": "Id of the last status returned by the previous page; when given, keyset pagination is used and $offset is ignored", + "schema": { + "type": "integer", + "format": "int64", + "nullable": true, + "default": null, + "minimum": 0 + } + }, { "name": "OCS-APIRequest", "in": "header", diff --git a/tests/lib/AppFramework/Http/PaginationTraitTest.php b/tests/lib/AppFramework/Http/PaginationTraitTest.php index b2695f2409d2b..a2dcaf2d21485 100644 --- a/tests/lib/AppFramework/Http/PaginationTraitTest.php +++ b/tests/lib/AppFramework/Http/PaginationTraitTest.php @@ -27,17 +27,22 @@ protected function setUp(): void { $this->subject = new class($this->request, $this->urlGenerator) { use PaginationTrait; - public function __construct(IRequest $request, IURLGenerator $urlGenerator) { - $this->request = $request; - $this->urlGenerator = $urlGenerator; + public function __construct( + protected IRequest $request, + private IURLGenerator $urlGenerator, + ) { } public function hasMore(array $items, ?int $limit): bool { return $this->hasMoreResults($items, $limit); } - public function nextLink(array $params, int $limit, int $offset): string { - return $this->buildOffsetNextPageLinkHeader($params, $limit, $offset); + public function nextLinkHeaders(array|bool $items, array $params, ?int $limit, int $offset): array { + return $this->buildOffsetNextPageLinkHeader($items, $params, $limit, $offset); + } + + public function cursorLinkHeaders(array|bool $items, array $params, ?int $limit, int|string|null $lastId): array { + return $this->buildCursorNextPageLinkHeader($items, $params, $limit, $lastId); } }; } @@ -73,11 +78,47 @@ public function testNextLinkKeepsRequestPathAndAppliesGivenQuery(): void { ->with('/ocs/v2.php/apps/provisioning_api/api/v1/groups') ->willReturn('https://cloud.example.com/ocs/v2.php/apps/provisioning_api/api/v1/groups'); - $link = $this->subject->nextLink(['search' => 'foo'], 5, 0); + $headers = $this->subject->nextLinkHeaders(['a', 'b', 'c', 'd', 'e'], ['search' => 'foo'], 5, 0); + + $this->assertSame([ + 'Link' => '; rel="next"', + ], $headers); + } + + public function testNextLinkReturnsEmptyHeadersWhenFewerItemsThanLimit(): void { + $this->assertSame([], $this->subject->nextLinkHeaders(['a', 'b'], ['search' => 'foo'], 5, 0)); + } + + public function testNextLinkAcceptsAPrecomputedBool(): void { + $this->assertSame([], $this->subject->nextLinkHeaders(false, ['search' => 'foo'], 5, 0)); + + $this->request->method('getRequestUri')->willReturn('/ocs/v2.php/core/autocomplete/get'); + $this->urlGenerator->method('getAbsoluteURL') + ->with('/ocs/v2.php/core/autocomplete/get') + ->willReturn('https://cloud.example.com/ocs/v2.php/core/autocomplete/get'); + + $this->assertArrayHasKey('Link', $this->subject->nextLinkHeaders(true, ['search' => 'foo'], 5, 0)); + } + + public function testCursorLinkKeepsRequestPathAndUsesLastId(): void { + $this->request->method('getRequestUri')->willReturn('/ocs/v2.php/apps/user_status/api/v1/statuses?limit=1&lastId=1336'); + + $this->urlGenerator->method('getAbsoluteURL') + ->with('/ocs/v2.php/apps/user_status/api/v1/statuses') + ->willReturn('https://cloud.example.com/ocs/v2.php/apps/user_status/api/v1/statuses'); + + $headers = $this->subject->cursorLinkHeaders(['a'], [], 1, 1337); + + $this->assertSame([ + 'Link' => '; rel="next"', + ], $headers); + } + + public function testCursorLinkReturnsEmptyHeadersWhenFewerItemsThanLimit(): void { + $this->assertSame([], $this->subject->cursorLinkHeaders(['a', 'b'], [], 5, 1337)); + } - $this->assertSame( - '; rel="next"', - $link - ); + public function testCursorLinkReturnsEmptyHeadersWhenLastIdIsMissing(): void { + $this->assertSame([], $this->subject->cursorLinkHeaders(['a'], [], 1, null)); } }