From 0bf80135d6f96b07ee339035f0c6501fe8718700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6?= Date: Fri, 28 Apr 2023 10:59:34 +0200 Subject: [PATCH] fix(psalm): systemtags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ --- apps/dav/lib/SystemTag/SystemTagList.php | 12 +++---- .../lib/SystemTag/SystemTagMappingNode.php | 5 +++ apps/dav/lib/SystemTag/SystemTagNode.php | 9 ++++- apps/dav/lib/SystemTag/SystemTagPlugin.php | 34 +++++++++++++------ .../SystemTag/SystemTagsByIdCollection.php | 25 +++++++++++++- .../SystemTagsObjectMappingCollection.php | 22 +++++++++++- .../SystemTagsObjectTypeCollection.php | 18 ++++++++-- .../unit/SystemTag/SystemTagPluginTest.php | 2 +- lib/public/SystemTag/ISystemTagManager.php | 4 +-- 9 files changed, 107 insertions(+), 24 deletions(-) diff --git a/apps/dav/lib/SystemTag/SystemTagList.php b/apps/dav/lib/SystemTag/SystemTagList.php index 67d33b9701ead..678c8042a394b 100644 --- a/apps/dav/lib/SystemTag/SystemTagList.php +++ b/apps/dav/lib/SystemTag/SystemTagList.php @@ -1,10 +1,8 @@ * - * @author Christoph Wurst - * @author Thomas Müller - * @author Vincent Petry + * @author Robin Appelman * * @license AGPL-3.0 * @@ -19,7 +17,6 @@ * * You should have received a copy of the GNU Affero General Public License, version 3, * along with this program. If not, see - * */ namespace OCA\DAV\SystemTag; @@ -49,7 +46,10 @@ public function __construct(array $tags, ISystemTagManager $tagManager, IUser $u $this->user = $user; } - public function getTags() { + /** + * @return ISystemTag[] + */ + public function getTags(): array { return $this->tags; } diff --git a/apps/dav/lib/SystemTag/SystemTagMappingNode.php b/apps/dav/lib/SystemTag/SystemTagMappingNode.php index 344ff1dbc70a9..9762b6e1db931 100644 --- a/apps/dav/lib/SystemTag/SystemTagMappingNode.php +++ b/apps/dav/lib/SystemTag/SystemTagMappingNode.php @@ -137,6 +137,8 @@ public function getName() { * @param string $name The new name * * @throws MethodNotAllowed not allowed to rename node + * + * @return never */ public function setName($name) { throw new MethodNotAllowed(); @@ -145,6 +147,7 @@ public function setName($name) { /** * Returns null, not supported * + * @return null */ public function getLastModified() { return null; @@ -152,6 +155,8 @@ public function getLastModified() { /** * Delete tag to object association + * + * @return void */ public function delete() { try { diff --git a/apps/dav/lib/SystemTag/SystemTagNode.php b/apps/dav/lib/SystemTag/SystemTagNode.php index a31deb59a93a8..7310cdb19a2e5 100644 --- a/apps/dav/lib/SystemTag/SystemTagNode.php +++ b/apps/dav/lib/SystemTag/SystemTagNode.php @@ -103,6 +103,8 @@ public function getSystemTag() { * @param string $name The new name * * @throws MethodNotAllowed not allowed to rename node + * + * @return never */ public function setName($name) { throw new MethodNotAllowed(); @@ -114,11 +116,12 @@ public function setName($name) { * @param string $name new tag name * @param bool $userVisible user visible * @param bool $userAssignable user assignable + * * @throws NotFound whenever the given tag id does not exist * @throws Forbidden whenever there is no permission to update said tag * @throws Conflict whenever a tag already exists with the given attributes */ - public function update($name, $userVisible, $userAssignable) { + public function update($name, $userVisible, $userAssignable): void { try { if (!$this->tagManager->canUserSeeTag($this->tag, $this->user)) { throw new NotFound('Tag with id ' . $this->tag->getId() . ' does not exist'); @@ -151,11 +154,15 @@ public function update($name, $userVisible, $userAssignable) { /** * Returns null, not supported * + * @return null */ public function getLastModified() { return null; } + /** + * @return void + */ public function delete() { try { if (!$this->isAdmin) { diff --git a/apps/dav/lib/SystemTag/SystemTagPlugin.php b/apps/dav/lib/SystemTag/SystemTagPlugin.php index 5fe1c01357103..c5c828cfbff30 100644 --- a/apps/dav/lib/SystemTag/SystemTagPlugin.php +++ b/apps/dav/lib/SystemTag/SystemTagPlugin.php @@ -28,6 +28,7 @@ use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\Node; use OCP\IGroupManager; +use OCP\IUser; use OCP\IUserSession; use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; @@ -81,9 +82,9 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { */ protected $groupManager; - /** @var array */ + /** @var array */ private array $cachedTagMappings = []; - /** @var array */ + /** @var array */ private array $cachedTags = []; private ISystemTagObjectMapper $tagMapper; @@ -225,6 +226,8 @@ private function createTag($data, $contentType = 'application/json') { * * @param PropFind $propFind * @param \Sabre\DAV\INode $node + * + * @return void */ public function handleGetProperties( PropFind $propFind, @@ -279,16 +282,19 @@ private function propfindForFile(PropFind $propFind, Node $node): void { if ($node instanceof Directory && $propFind->getDepth() !== 0 && !is_null($propFind->getStatus(self::SYSTEM_TAGS_PROPERTYNAME))) { + $fileIds = [$node->getId()]; + // note: pre-fetching only supported for depth <= 1 $folderContent = $node->getNode()->getDirectoryListing(); - $fileIds[] = (int)$node->getId(); foreach ($folderContent as $info) { - $fileIds[] = (int)$info->getId(); + $fileIds[] = $info->getId(); } + $tags = $this->tagMapper->getTagIdsForObjects($fileIds, 'files'); $this->cachedTagMappings = $this->cachedTagMappings + $tags; $emptyFileIds = array_diff($fileIds, array_keys($tags)); + // also cache the ones that were not found foreach ($emptyFileIds as $fileId) { $this->cachedTagMappings[$fileId] = []; @@ -296,8 +302,13 @@ private function propfindForFile(PropFind $propFind, Node $node): void { } $propFind->handle(self::SYSTEM_TAGS_PROPERTYNAME, function () use ($node) { - $tags = $this->getTagsForFile($node->getId()); - return new SystemTagList($tags, $this->tagManager, $this->userSession->getUser()); + $user = $this->userSession->getUser(); + if ($user === null) { + return; + } + + $tags = $this->getTagsForFile($node->getId(), $user); + return new SystemTagList($tags, $this->tagManager, $user); }); } @@ -305,8 +316,8 @@ private function propfindForFile(PropFind $propFind, Node $node): void { * @param int $fileId * @return ISystemTag[] */ - private function getTagsForFile(int $fileId): array { - $user = $this->userSession->getUser(); + private function getTagsForFile(int $fileId, IUser $user): array { + if (isset($this->cachedTagMappings[$fileId])) { $tagIds = $this->cachedTagMappings[$fileId]; } else { @@ -318,13 +329,15 @@ private function getTagsForFile(int $fileId): array { $tagIds = []; } } - $tags = array_filter(array_map(function($tagId) { + + $tags = array_filter(array_map(function(string $tagId) { return $this->cachedTags[$tagId] ?? null; }, $tagIds)); - $uncachedTagIds = array_filter($tagIds, function($tagId): bool { + $uncachedTagIds = array_filter($tagIds, function(string $tagId): bool { return !isset($this->cachedTags[$tagId]); }); + if (count($uncachedTagIds)) { $retrievedTags = $this->tagManager->getTagsByIds($uncachedTagIds); foreach ($retrievedTags as $tag) { @@ -332,6 +345,7 @@ private function getTagsForFile(int $fileId): array { } $tags += $retrievedTags; } + return array_filter($tags, function(ISystemTag $tag) use ($user) { return $this->tagManager->canUserSeeTag($tag, $user); }); diff --git a/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php b/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php index 1256c58921e8f..86ccadf5f56d9 100644 --- a/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php +++ b/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php @@ -84,7 +84,10 @@ private function isAdmin() { /** * @param string $name * @param resource|string $data Initial payload + * * @throws Forbidden + * + * @return never */ public function createFile($name, $data = null) { throw new Forbidden('Cannot create tags by id'); @@ -92,6 +95,8 @@ public function createFile($name, $data = null) { /** * @param string $name + * + * @return never */ public function createDirectory($name) { throw new Forbidden('Permission denied to create collections'); @@ -99,6 +104,8 @@ public function createDirectory($name) { /** * @param string $name + * + * @return SystemTagNode */ public function getChild($name) { try { @@ -115,6 +122,11 @@ public function getChild($name) { } } + /** + * @return SystemTagNode[] + * + * @psalm-return array + */ public function getChildren() { $visibilityFilter = true; if ($this->isAdmin()) { @@ -145,14 +157,25 @@ public function childExists($name) { } } + /** + * @return never + */ public function delete() { throw new Forbidden('Permission denied to delete this collection'); } + /** + * @return string + * + * @psalm-return 'systemtags' + */ public function getName() { return 'systemtags'; } + /** + * @return never + */ public function setName($name) { throw new Forbidden('Permission denied to rename this collection'); } @@ -160,7 +183,7 @@ public function setName($name) { /** * Returns the last modification time, as a unix timestamp * - * @return int + * @return null */ public function getLastModified() { return null; diff --git a/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php b/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php index 8bb34182b0c2b..4d73c17d7dd19 100644 --- a/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php +++ b/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php @@ -92,6 +92,9 @@ public function __construct( $this->user = $user; } + /** + * @return void + */ public function createFile($name, $data = null) { $tagId = $name; try { @@ -110,10 +113,16 @@ public function createFile($name, $data = null) { } } + /** + * @return never + */ public function createDirectory($name) { throw new Forbidden('Permission denied to create collections'); } + /** + * @return SystemTagMappingNode + */ public function getChild($tagName) { try { if ($this->tagMapper->haveTag([$this->objectId], $this->objectType, $tagName, true)) { @@ -131,6 +140,11 @@ public function getChild($tagName) { } } + /** + * @return SystemTagMappingNode[] + * + * @psalm-return list + */ public function getChildren() { $tagIds = current($this->tagMapper->getTagIdsForObjects([$this->objectId], $this->objectType)); if (empty($tagIds)) { @@ -168,6 +182,9 @@ public function childExists($tagId) { } } + /** + * @return never + */ public function delete() { throw new Forbidden('Permission denied to delete this collection'); } @@ -176,6 +193,9 @@ public function getName() { return $this->objectId; } + /** + * @return never + */ public function setName($name) { throw new Forbidden('Permission denied to rename this collection'); } @@ -183,7 +203,7 @@ public function setName($name) { /** * Returns the last modification time, as a unix timestamp * - * @return int + * @return null */ public function getLastModified() { return null; diff --git a/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php b/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php index 1ca45c32ce46b..3fa40278cdbfb 100644 --- a/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php +++ b/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php @@ -98,7 +98,9 @@ public function __construct( /** * @param string $name * @param resource|string $data Initial payload - * @return null|string + * + * @return never + * * @throws Forbidden */ public function createFile($name, $data = null) { @@ -107,7 +109,10 @@ public function createFile($name, $data = null) { /** * @param string $name + * * @throws Forbidden + * + * @return never */ public function createDirectory($name) { throw new Forbidden('Permission denied to create collections'); @@ -133,6 +138,9 @@ public function getChild($objectName) { ); } + /** + * @return never + */ public function getChildren() { // do not list object ids throw new MethodNotAllowed(); @@ -148,6 +156,9 @@ public function childExists($name) { return call_user_func($this->childExistsFunction, $name); } + /** + * @return never + */ public function delete() { throw new Forbidden('Permission denied to delete this collection'); } @@ -158,7 +169,10 @@ public function getName() { /** * @param string $name + * * @throws Forbidden + * + * @return never */ public function setName($name) { throw new Forbidden('Permission denied to rename this collection'); @@ -167,7 +181,7 @@ public function setName($name) { /** * Returns the last modification time, as a unix timestamp * - * @return int + * @return null */ public function getLastModified() { return null; diff --git a/apps/dav/tests/unit/SystemTag/SystemTagPluginTest.php b/apps/dav/tests/unit/SystemTag/SystemTagPluginTest.php index 199bf28fb7da2..965787d078f01 100644 --- a/apps/dav/tests/unit/SystemTag/SystemTagPluginTest.php +++ b/apps/dav/tests/unit/SystemTag/SystemTagPluginTest.php @@ -85,7 +85,7 @@ class SystemTagPluginTest extends \Test\TestCase { */ private $plugin; - private ISystemTagObjectMapper $tagMapper; + private ISystemTagObjectMapper|\PHPUnit\Framework\MockObject\MockObject $tagMapper; protected function setUp(): void { parent::setUp(); diff --git a/lib/public/SystemTag/ISystemTagManager.php b/lib/public/SystemTag/ISystemTagManager.php index 9918e3a4f9964..1cf7263b45619 100644 --- a/lib/public/SystemTag/ISystemTagManager.php +++ b/lib/public/SystemTag/ISystemTagManager.php @@ -125,7 +125,7 @@ public function deleteTags($tagIds); * @param ISystemTag $tag tag to check permission for * @param IUser $user user to check permission for * - * @return true if the user is allowed to assign/unassign the tag, false otherwise + * @return bool true if the user is allowed to assign/unassign the tag, false otherwise * * @since 9.1.0 */ @@ -137,7 +137,7 @@ public function canUserAssignTag(ISystemTag $tag, IUser $user): bool; * @param ISystemTag $tag tag to check permission for * @param IUser $user user to check permission for * - * @return true if the user can see the tag, false otherwise + * @return bool true if the user can see the tag, false otherwise * * @since 9.1.0 */