Skip to content
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.

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

fix(psalm): Fix all @throws annotations and add missing ones #3270

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions lib/ACL/ACLManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OCA\GroupFolders\Trash\TrashManager;
use OCP\Cache\CappedMemoryCache;
use OCP\Constants;
use OCP\DB\Exception;
use OCP\Files\IRootFolder;
use OCP\IUser;
use Psr\Log\LoggerInterface;
Expand Down Expand Up @@ -206,6 +207,7 @@ private function calculatePermissionsForPath(array $rules): int {

/**
* Get the combined "lowest" permissions for an entire directory tree
* @throws Exception
*/
public function getPermissionsForTree(string $path): int {
$path = ltrim($path, '/');
Expand All @@ -223,6 +225,9 @@ public function getPermissionsForTree(string $path): int {
}, Constants::PERMISSION_ALL);
}

/**
* @throws Exception
*/
public function preloadRulesForFolder(string $path): void {
$this->ruleManager->getRulesForFilesByParent($this->user, $this->getRootStorageId(), $path);
}
Expand Down
2 changes: 2 additions & 0 deletions lib/ACL/ACLStorageWrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use OC\Files\Cache\Scanner;
use OC\Files\Storage\Wrapper\Wrapper;
use OCP\Constants;
use OCP\DB\Exception;
use OCP\Files\Cache\ICache;
use OCP\Files\Storage\IStorage;

Expand Down Expand Up @@ -148,6 +149,7 @@ public function unlink($path): bool {
/**
* When deleting we need to ensure that there is no file inside the folder being deleted that misses delete permissions
* This check is fairly expensive so we only do it for the actual delete and not metadata operations
* @throws Exception
*/
private function canDeleteTree(string $path): int {
return $this->aclManager->getPermissionsForTree($path) & Constants::PERMISSION_DELETE;
Expand Down
18 changes: 18 additions & 0 deletions lib/ACL/RuleManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use OCA\GroupFolders\ACL\UserMapping\IUserMapping;
use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager;
use OCP\DB\Exception;
use OCP\DB\QueryBuilder\ICompositeExpression;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\EventDispatcher\IEventDispatcher;
Expand Down Expand Up @@ -42,6 +43,7 @@ private function createRule(array $data): ?Rule {
/**
* @param int[] $fileIds
* @return array<int, Rule[]>
* @throws Exception
*/
public function getRulesForFilesById(IUser $user, array $fileIds): array {
$userMappings = $this->userMappingManager->getMappingsForUser($user);
Expand Down Expand Up @@ -107,6 +109,7 @@ public function getRulesForFilesByPath(IUser $user, int $storageId, array $fileP

/**
* @return array<string, Rule[]>
* @throws Exception
*/
public function getRulesForFilesByParent(IUser $user, int $storageId, string $parent): array {
$userMappings = $this->userMappingManager->getMappingsForUser($user);
Expand Down Expand Up @@ -153,6 +156,9 @@ public function getRulesForFilesByParent(IUser $user, int $storageId, string $pa
return $result;
}

/**
* @throws Exception
*/
private function getId(int $storageId, string $path): int {
$query = $this->connection->getQueryBuilder();
$query->select(['fileid'])
Expand All @@ -166,6 +172,7 @@ private function getId(int $storageId, string $path): int {
/**
* @param string[] $filePaths
* @return array<string, Rule[]>
* @throws Exception
*/
public function getAllRulesForPaths(int $storageId, array $filePaths): array {
$hashes = array_map(fn (string $path): string => md5(trim($path, '/')), $filePaths);
Expand Down Expand Up @@ -198,6 +205,7 @@ private function rulesByPath(array $rows, array $result = []): array {

/**
* @return array<string, Rule[]>
* @throws Exception
*/
public function getAllRulesForPrefix(int $storageId, string $prefix): array {
$query = $this->connection->getQueryBuilder();
Expand All @@ -217,6 +225,7 @@ public function getAllRulesForPrefix(int $storageId, string $prefix): array {

/**
* @return array<string, Rule[]>
* @throws Exception
*/
public function getRulesForPrefix(IUser $user, int $storageId, string $prefix): array {
$userMappings = $this->userMappingManager->getMappingsForUser($user);
Expand All @@ -240,6 +249,9 @@ public function getRulesForPrefix(IUser $user, int $storageId, string $prefix):
return $this->rulesByPath($rows);
}

/**
* @throws Exception
*/
private function hasRule(IUserMapping $mapping, int $fileId): bool {
$query = $this->connection->getQueryBuilder();
$query->select('fileid')
Expand All @@ -251,6 +263,9 @@ private function hasRule(IUserMapping $mapping, int $fileId): bool {
return (bool)$query->executeQuery()->fetch();
}

/**
* @throws Exception
*/
public function saveRule(Rule $rule): void {
if ($this->hasRule($rule->getUserMapping(), $rule->getFileId())) {
$query = $this->connection->getQueryBuilder();
Expand Down Expand Up @@ -315,6 +330,9 @@ public function saveRule(Rule $rule): void {
}
}

/**
* @throws Exception
*/
public function deleteRule(Rule $rule): void {
$query = $this->connection->getQueryBuilder();
$query->delete('group_folders_acl')
Expand Down
6 changes: 6 additions & 0 deletions lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\Server;
use Psr\Container\ContainerExceptionInterface;
use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use Throwable;

class Application extends App implements IBootstrap {
public const APP_ID = 'groupfolders';
Expand Down Expand Up @@ -227,6 +229,10 @@ public function register(IRegistrationContext $context): void {
$context->registerMiddleware(AuthorizedAdminSettingMiddleware::class);
}

/**
* @throws ContainerExceptionInterface
* @throws Throwable
Comment on lines +233 to +234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does that throw Throwable? For me that should never be documented, Throwable which are not Exception are bugs to be fixed, not documented. Anything can throw a Throwable if you use it wrong.
Would also be good if those @throws tag would include a description of when to expect a throw.

*/
public function boot(IBootContext $context): void {
$context->injectFn(function (IMountProviderCollection $mountProviderCollection, CacheListener $cacheListener, IEventDispatcher $eventDispatcher): void {
$mountProviderCollection->registerProvider(Server::get(MountProvider::class));
Expand Down
10 changes: 10 additions & 0 deletions lib/AppInfo/Capabilities.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
*/
namespace OCA\GroupFolders\AppInfo;

use OCA\Circles\Exceptions\RequestBuilderException;
use OCA\GroupFolders\Folder\FolderManager;
use OCP\App\IAppManager;
use OCP\Capabilities\ICapability;
use OCP\DB\Exception;
use OCP\IUser;
use OCP\IUserSession;

Expand All @@ -21,6 +23,10 @@ public function __construct(
) {
}

/**
* @throws Exception
* @throws RequestBuilderException
*/
Comment on lines +26 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be caught inside, this method is not documented as throwing in ICapability and should not throw.

public function getCapabilities(): array {
$user = $this->userSession->getUser();
if (!$user) {
Expand All @@ -35,6 +41,10 @@ public function getCapabilities(): array {
];
}

/**
* @throws RequestBuilderException
* @throws Exception
*/
private function hasFolders(IUser $user): bool {
$folders = $this->folderManager->getFoldersForUser($user);
return count($folders) > 0;
Expand Down
1 change: 1 addition & 0 deletions lib/AuthorizedAdminSettingMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function __construct(

/**
* Throws an error when the user is not allowed to use the app's APIs
* @throws Exception
*/
public function beforeController(Controller $controller, string $methodName): void {
$method = new ReflectionMethod($controller, $methodName);
Expand Down
2 changes: 2 additions & 0 deletions lib/BackgroundJob/ExpireGroupVersions.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use OCA\GroupFolders\Versions\GroupVersionsExpireManager;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\TimedJob;
use OCP\DB\Exception;
use OCP\IAppConfig;
use Psr\Log\LoggerInterface;

Expand All @@ -36,6 +37,7 @@ public function __construct(
* @inheritDoc
* Expiring groupfolder versions can be quite expensive.
* We need to limit the amount of folders we expire per run.
* @throws Exception
*/
protected function run(mixed $argument): void {
$lastFolder = $this->appConfig->getValueInt(Application::APP_ID, 'cron_last_folder_index', 0);
Expand Down
13 changes: 13 additions & 0 deletions lib/Command/Create.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,26 @@

use OC\Core\Command\Base;
use OCA\GroupFolders\Folder\FolderManager;
use OCP\DB\Exception;
use Symfony\Component\Console\Exception\InvalidArgumentException;
use Symfony\Component\Console\Exception\LogicException;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class Create extends Base {
/**
* @throws LogicException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when? Add a description please

*/
public function __construct(
private FolderManager $folderManager,
) {
parent::__construct();
}

/**
* @throws InvalidArgumentException
*/
protected function configure(): void {
$this
->setName('groupfolders:create')
Expand All @@ -29,6 +38,10 @@ protected function configure(): void {
parent::configure();
}

/**
* @throws Exception
* @throws InvalidArgumentException
*/
protected function execute(InputInterface $input, OutputInterface $output): int {
$id = $this->folderManager->createFolder($input->getArgument('name'));
$output->writeln((string)$id);
Expand Down
8 changes: 8 additions & 0 deletions lib/Command/ExpireGroup/ExpireGroupBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,25 @@
namespace OCA\GroupFolders\Command\ExpireGroup;

use OC\Core\Command\Base;
use Symfony\Component\Console\Exception\InvalidArgumentException;
use Symfony\Component\Console\Exception\LogicException;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

/**
* Base class for the group folder expiration commands.
*/
class ExpireGroupBase extends Base {
/**
* @throws LogicException
*/
public function __construct() {
parent::__construct();
}
Comment on lines +21 to 26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* @throws LogicException
*/
public function __construct() {
parent::__construct();
}

dead code


/**
* @throws InvalidArgumentException
*/
protected function configure(): void {
$this
->setName('groupfolders:expire')
Expand Down
4 changes: 4 additions & 0 deletions lib/Command/ExpireGroupVersionsPlaceholder.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@
namespace OCA\GroupFolders\Command;

use OC\Core\Command\Base;
use Symfony\Component\Console\Exception\InvalidArgumentException;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class ExpireGroupVersionsPlaceholder extends Base {
/**
* @throws InvalidArgumentException
*/
protected function configure(): void {
$this
->setName('groupfolders:expire')
Expand Down
9 changes: 8 additions & 1 deletion lib/Command/FolderCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,20 @@
use OC\Core\Command\Base;
use OCA\GroupFolders\Folder\FolderManager;
use OCA\GroupFolders\Mount\MountProvider;
use OCP\DB\Exception;
use OCP\Files\IRootFolder;
use Symfony\Component\Console\Exception\InvalidArgumentException;
use Symfony\Component\Console\Exception\LogicException;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

/**
* Base command for commands asking the user for a folder id.
*/
abstract class FolderCommand extends Base {

/**
* @throws LogicException
*/
public function __construct(
protected FolderManager $folderManager,
protected IRootFolder $rootFolder,
Expand All @@ -30,6 +35,8 @@ public function __construct(

/**
* @psalm-return ?array{id: mixed, mount_point: string, groups: array<empty, empty>|mixed, quota: int, size: int|mixed, acl: bool}
* @throws Exception
* @throws InvalidArgumentException
*/
protected function getFolder(InputInterface $input, OutputInterface $output): ?array {
$folderId = (int)$input->getArgument('folder_id');
Expand Down
14 changes: 13 additions & 1 deletion lib/Command/ListCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@
use OC\Core\Command\Base;
use OCA\GroupFolders\Folder\FolderManager;
use OCP\Constants;
use OCP\DB\Exception;
use OCP\Files\IRootFolder;
use OCP\IGroupManager;
use OCP\IUserManager;
use Symfony\Component\Console\Exception\InvalidArgumentException;
use Symfony\Component\Console\Exception\LogicException;
use Symfony\Component\Console\Helper\Table;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
Expand All @@ -28,7 +31,9 @@ class ListCommand extends Base {
Constants::PERMISSION_DELETE => 'delete'
];


/**
* @throws LogicException
*/
public function __construct(
private FolderManager $folderManager,
private IRootFolder $rootFolder,
Expand All @@ -38,6 +43,9 @@ public function __construct(
parent::__construct();
}

/**
* @throws InvalidArgumentException
*/
protected function configure(): void {
$this
->setName('groupfolders:list')
Expand All @@ -46,6 +54,10 @@ protected function configure(): void {
parent::configure();
}

/**
* @throws Exception
* @throws InvalidArgumentException
*/
protected function execute(InputInterface $input, OutputInterface $output): int {
$userId = $input->getOption('user');
$groups = $this->groupManager->search('');
Expand Down
Loading
Loading