Skip to content

perf(files_sharing): simplify tag population in ShareAPIController #61853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joshtrichards wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

perf(files_sharing): simplify tag population in ShareAPIController #61853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b403b05
perf(files_sharing): simplify tag population in ShareAPIController
joshtrichards Jul 6, 2026
0fd8479
test(files_sharing): cover duplicate file_source entries in populateTags
joshtrichards Jul 6, 2026
8dec059
fix(files_sharing): adjust populateTags Psalm return type
joshtrichards Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@
*
* @param string $id ID of the share
* @param bool $include_tags Include tags in the share
* @return DataResponse<Http::STATUS_OK, list<Files_SharingShare>, array{}>

Check failure on line 491 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

MoreSpecificReturnType 

apps/files_sharing/lib/Controller/ShareAPIController.php:491:13: MoreSpecificReturnType: The declared return type 'OCP\AppFramework\Http\DataResponse<200, list<array{'is-mount-root': bool, 'mount-type': string, attributes: null|string, can_delete: bool, can_edit: bool, displayname_file_owner: string, displayname_owner: string, expiration: null|string, file_parent: int, file_source: int, file_target: string, has_preview: bool, hide_download: 0|1, id: string, is_trusted_server?: bool, item_mtime: int, item_permissions?: int, item_size: float|int, item_source: int, item_type: 'file'|'folder', label: string, mail_send: 0|1, mimetype: string, note: string, parent: null, password?: null|string, password_expiration_time?: null|string, path: null|string, permissions: int, send_password_by_talk?: bool, share_type: int, share_with?: null|string, share_with_avatar?: string, share_with_displayname?: string, share_with_displayname_unique?: null|string, share_with_link?: string, status?: array{clearAt: int|null, icon: null|string, message: null|string, status: string}, stime: int, storage: int, storage_id: string, token: null|string, uid_file_owner: string, uid_owner: string, url?: null|string}>, array<never, never>>' for OCA\Files_Sharing\Controller\ShareAPIController::getShare is more specific than the inferred return type 'OCP\AppFramework\Http\DataResponse<200, array<array-key, mixed>, array<never, never>>' (see https://psalm.dev/070)
* @throws OCSNotFoundException Share not found
*
* 200: Share returned
Expand All @@ -511,7 +511,7 @@
$share = [$share];
}

return new DataResponse($share);

Check failure on line 514 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

LessSpecificReturnStatement 

apps/files_sharing/lib/Controller/ShareAPIController.php:514:12: LessSpecificReturnStatement: The type 'OCP\AppFramework\Http\DataResponse<200, array<array-key, mixed>, array<never, never>>' is more general than the declared return type 'OCP\AppFramework\Http\DataResponse<200, list<array{'is-mount-root': bool, 'mount-type': string, attributes: null|string, can_delete: bool, can_edit: bool, displayname_file_owner: string, displayname_owner: string, expiration: null|string, file_parent: int, file_source: int, file_target: string, has_preview: bool, hide_download: 0|1, id: string, is_trusted_server?: bool, item_mtime: int, item_permissions?: int, item_size: float|int, item_source: int, item_type: 'file'|'folder', label: string, mail_send: 0|1, mimetype: string, note: string, parent: null, password?: null|string, password_expiration_time?: null|string, path: null|string, permissions: int, send_password_by_talk?: bool, share_type: int, share_with?: null|string, share_with_avatar?: string, share_with_displayname?: string, share_with_displayname_unique?: null|string, share_with_link?: string, status?: array{clearAt: int|null, icon: null|string, message: null|string, status: string}, stime: int, storage: int, storage_id: string, token: null|string, uid_file_owner: string, uid_owner: string, url?: null|string}>, array<never, never>>' for OCA\Files_Sharing\Controller\ShareAPIController::getShare (see https://psalm.dev/129)
}
} catch (NotFoundException $e) {
// Fall through
Expand Down Expand Up @@ -832,7 +832,7 @@
} catch (HintException $e) {
$code = $e->getCode() === 0 ? 403 : $e->getCode();
throw new OCSException($e->getHint(), $code);
} catch (GenericShareException|\InvalidArgumentException $e) {

Check failure on line 835 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

MoreSpecificReturnType 

apps/files_sharing/lib/Controller/ShareAPIController.php:835:13: MoreSpecificReturnType: The declared return type 'list<array{'is-mount-root': bool, 'mount-type': string, attributes: null|string, can_delete: bool, can_edit: bool, displayname_file_owner: string, displayname_owner: string, expiration: null|string, file_parent: int, file_source: int, file_target: string, has_preview: bool, hide_download: 0|1, id: string, is_trusted_server?: bool, item_mtime: int, item_permissions?: int, item_size: float|int, item_source: int, item_type: 'file'|'folder', label: string, mail_send: 0|1, mimetype: string, note: string, parent: null, password?: null|string, password_expiration_time?: null|string, path: null|string, permissions: int, send_password_by_talk?: bool, share_type: int, share_with?: null|string, share_with_avatar?: string, share_with_displayname?: string, share_with_displayname_unique?: null|string, share_with_link?: string, status?: array{clearAt: int|null, icon: null|string, message: null|string, status: string}, stime: int, storage: int, storage_id: string, token: null|string, uid_file_owner: string, uid_owner: string, url?: null|string}>' for OCA\Files_Sharing\Controller\ShareAPIController::getSharedWithMe is more specific than the inferred return type 'array<array-key, mixed>' (see https://psalm.dev/070)
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new OCSForbiddenException($e->getMessage(), $e);
} catch (\Exception $e) {
Expand Down Expand Up @@ -862,7 +862,7 @@

$filteredShares = array_filter($shares, function (IShare $share) {
return $share->getShareOwner() !== $this->userId && $share->getSharedBy() !== $this->userId;
});

Check failure on line 865 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

LessSpecificReturnStatement 

apps/files_sharing/lib/Controller/ShareAPIController.php:865:10: LessSpecificReturnStatement: The type 'array<array-key, mixed>' is more general than the declared return type 'list<array{'is-mount-root': bool, 'mount-type': string, attributes: null|string, can_delete: bool, can_edit: bool, displayname_file_owner: string, displayname_owner: string, expiration: null|string, file_parent: int, file_source: int, file_target: string, has_preview: bool, hide_download: 0|1, id: string, is_trusted_server?: bool, item_mtime: int, item_permissions?: int, item_size: float|int, item_source: int, item_type: 'file'|'folder', label: string, mail_send: 0|1, mimetype: string, note: string, parent: null, password?: null|string, password_expiration_time?: null|string, path: null|string, permissions: int, send_password_by_talk?: bool, share_type: int, share_with?: null|string, share_with_avatar?: string, share_with_displayname?: string, share_with_displayname_unique?: null|string, share_with_link?: string, status?: array{clearAt: int|null, icon: null|string, message: null|string, status: string}, stime: int, storage: int, storage_id: string, token: null|string, uid_file_owner: string, uid_owner: string, url?: null|string}>' for OCA\Files_Sharing\Controller\ShareAPIController::getSharedWithMe (see https://psalm.dev/129)

$formatted = [];
foreach ($filteredShares as $share) {
Expand Down Expand Up @@ -1036,7 +1036,7 @@
if (!$this->hasPermission($permissions, Constants::PERMISSION_READ)
&& !$this->hasPermission($permissions, Constants::PERMISSION_CREATE)) {
throw new OCSBadRequestException($this->l->t('Share must at least have READ or CREATE permissions'));
}

Check failure on line 1039 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

MoreSpecificReturnType 

apps/files_sharing/lib/Controller/ShareAPIController.php:1039:13: MoreSpecificReturnType: The declared return type 'list<array{'is-mount-root': bool, 'mount-type': string, attributes: null|string, can_delete: bool, can_edit: bool, displayname_file_owner: string, displayname_owner: string, expiration: null|string, file_parent: int, file_source: int, file_target: string, has_preview: bool, hide_download: 0|1, id: string, is_trusted_server?: bool, item_mtime: int, item_permissions?: int, item_size: float|int, item_source: int, item_type: 'file'|'folder', label: string, mail_send: 0|1, mimetype: string, note: string, parent: null, password?: null|string, password_expiration_time?: null|string, path: null|string, permissions: int, send_password_by_talk?: bool, share_type: int, share_with?: null|string, share_with_avatar?: string, share_with_displayname?: string, share_with_displayname_unique?: null|string, share_with_link?: string, status?: array{clearAt: int|null, icon: null|string, message: null|string, status: string}, stime: int, storage: int, storage_id: string, token: null|string, uid_file_owner: string, uid_owner: string, url?: null|string}>' for OCA\Files_Sharing\Controller\ShareAPIController::getFormattedShares is more specific than the inferred return type 'array<array-key, mixed>' (see https://psalm.dev/070)

// UPDATE and DELETE require a READ permission
if (!$this->hasPermission($permissions, Constants::PERMISSION_READ)
Expand Down Expand Up @@ -1109,7 +1109,7 @@
// by the current user only, in case
// there is no resharing rights
if ($share->getSharedBy() === $this->userId) {
$miniFormatted[] = $format;

Check failure on line 1112 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

LessSpecificReturnStatement 

apps/files_sharing/lib/Controller/ShareAPIController.php:1112:10: LessSpecificReturnStatement: The type 'array<array-key, mixed>' is more general than the declared return type 'list<array{'is-mount-root': bool, 'mount-type': string, attributes: null|string, can_delete: bool, can_edit: bool, displayname_file_owner: string, displayname_owner: string, expiration: null|string, file_parent: int, file_source: int, file_target: string, has_preview: bool, hide_download: 0|1, id: string, is_trusted_server?: bool, item_mtime: int, item_permissions?: int, item_size: float|int, item_source: int, item_type: 'file'|'folder', label: string, mail_send: 0|1, mimetype: string, note: string, parent: null, password?: null|string, password_expiration_time?: null|string, path: null|string, permissions: int, send_password_by_talk?: bool, share_type: int, share_with?: null|string, share_with_avatar?: string, share_with_displayname?: string, share_with_displayname_unique?: null|string, share_with_link?: string, status?: array{clearAt: int|null, icon: null|string, message: null|string, status: string}, stime: int, storage: int, storage_id: string, token: null|string, uid_file_owner: string, uid_owner: string, url?: null|string}>' for OCA\Files_Sharing\Controller\ShareAPIController::getFormattedShares (see https://psalm.dev/129)
}

// check if one of those share is shared with me
Expand Down Expand Up @@ -2203,7 +2203,7 @@
* @throws OCSException Failed to generate a unique token
*
* @return DataResponse<Http::STATUS_OK, array{token: string}, array{}>
*

Check failure on line 2206 in apps/files_sharing/lib/Controller/ShareAPIController.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

InvalidDocblock 

apps/files_sharing/lib/Controller/ShareAPIController.php:2206:2: InvalidDocblock: The intersection type must not mix array and object types! in docblock for OCA\Files_Sharing\Controller\ShareAPIController::populateTags (see https://psalm.dev/008)
* 200: Token generated successfully
*/
#[ApiRoute(verb: 'GET', url: '/api/v1/token')]
Expand All @@ -2224,32 +2224,33 @@
*
* @psalm-template T of array{tags?: list<string>, file_source: int, ...array<string, mixed>}
* @param list<T> $fileList
* @return list<T> file list populated with tags
* @return list<T&array{tags: list<string>}> file list populated with tags
*/
private function populateTags(array $fileList): array {
if ($fileList === []) {
return [];
}

$tagger = $this->tagManager->load('files');
$tags = $tagger->getTagsForObjects(array_map(static fn (array $fileData) => $fileData['file_source'], $fileList));
$tags = $tagger->getTagsForObjects(array_map(
static fn (array $fileData) => $fileData['file_source'],
$fileList
));

if (!is_array($tags)) {
throw new \UnexpectedValueException('$tags must be an array');
}

// Set empty tag array
foreach ($fileList as &$fileData) {
$fileData['tags'] = [];
$index = [];
foreach ($fileList as $i => $fileData) {
$fileList[$i]['tags'] = [];
// Multiple share rows can point to the same file_source, so store all positions.
$index[$fileData['file_source']][] = $i;
}
unset($fileData);

if (!empty($tags)) {
foreach ($tags as $fileId => $fileTags) {
foreach ($fileList as &$fileData) {
if ($fileId !== $fileData['file_source']) {
continue;
}

$fileData['tags'] = $fileTags;
}
unset($fileData);
foreach ($tags as $fileSource => $fileTags) {
foreach ($index[$fileSource] ?? [] as $i) {
$fileList[$i]['tags'] = $fileTags;
}
}

Expand Down
25 changes: 25 additions & 0 deletions apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5257,6 +5257,31 @@ public function testPopulateTags(): void {
], $result);
}

public function testPopulateTagsWithDuplicateFileSource(): void {
$tagger = $this->createMock(ITags::class);
$this->tagManager->method('load')
->with('files')
->willReturn($tagger);
$data = [
['file_source' => 42, 'foo' => 'bar'],
['file_source' => 10],
['file_source' => 42, 'x' => 'y'],
];
$tags = [
42 => ['tag1', 'tag2'],
];
$tagger->method('getTagsForObjects')
->with([42, 10, 42])
->willReturn($tags);

$result = self::invokePrivate($this->ocs, 'populateTags', [$data]);
$this->assertSame([
['file_source' => 42, 'foo' => 'bar', 'tags' => ['tag1', 'tag2']],
['file_source' => 10, 'tags' => []],
['file_source' => 42, 'x' => 'y', 'tags' => ['tag1', 'tag2']],
], $result);
}

public static function trustedServerProvider(): array {
return [
'Trusted server' => [true, true],
Expand Down
Loading