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

Stricter typing to match psalm level 2 #4584

Merged
merged 4 commits into from Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/Controller/AttachmentController.php
Expand Up @@ -161,13 +161,17 @@ private function getUploadedFile(string $key): array {

/**
* Serve the image files in the editor
*
* @return DataDownloadResponse|DataResponse
*
* @psalm-return DataDownloadResponse<200, string, array<never, never>>|DataResponse<404, '', array<never, never>>
*/
#[NoAdminRequired]
#[PublicPage]
#[NoCSRFRequired]
#[RequireDocumentSession]
public function getImageFile(string $imageFileName, ?string $shareToken = null,
int $preferRawImage = 0) {
int $preferRawImage = 0): DataResponse|DataDownloadResponse {
$documentId = $this->getSession()->getDocumentId();

try {
Expand All @@ -192,12 +196,16 @@ public function getImageFile(string $imageFileName, ?string $shareToken = null,

/**
* Serve the media files in the editor
*
* @return DataDownloadResponse|DataResponse
*
* @psalm-return DataDownloadResponse<200, string, array<never, never>>|DataResponse<404, '', array<never, never>>
*/
#[NoAdminRequired]
#[PublicPage]
#[NoCSRFRequired]
#[RequireDocumentSession]
public function getMediaFile(string $mediaFileName, ?string $shareToken = null) {
public function getMediaFile(string $mediaFileName, ?string $shareToken = null): DataResponse|DataDownloadResponse {
$documentId = $this->getSession()->getDocumentId();

try {
Expand Down
22 changes: 17 additions & 5 deletions lib/Controller/PublicSessionController.php
Expand Up @@ -40,7 +40,7 @@
class PublicSessionController extends PublicShareController implements ISessionAwareController {
use TSessionAwareController;

private IShare $share;
private ?IShare $share = null;

public function __construct(
string $appName,
Expand All @@ -52,8 +52,16 @@ public function __construct(
parent::__construct($appName, $request, $session);
}

private function getShare(): IShare {
if ($this->share === null) {
throw new \Exception('Share has not been set yet');
}

return $this->share;
}

protected function getPasswordHash(): string {
return $this->share->getPassword();
return $this->getShare()->getPassword();
}

public function isValidToken(): bool {
Expand All @@ -66,12 +74,13 @@ public function isValidToken(): bool {
}

protected function isPasswordProtected(): bool {
return $this->share->getPassword() !== null;
/** @psalm-suppress RedundantConditionGivenDocblockType */
return $this->getShare()->getPassword() !== null;
}

#[NoAdminRequired]
#[PublicPage]
public function create(string $token, string $file = null, $guestName = null): DataResponse {
public function create(string $token, string $file = null, ?string $guestName = null): DataResponse {
return $this->apiService->create(null, $file, $token, $guestName);
}

Expand All @@ -95,10 +104,13 @@ public function sync(string $token, int $documentId, int $sessionId, string $ses
return $this->apiService->sync($this->getSession(), $this->getDocument(), $version, $autosaveContent, $documentState, $force, $manualSave, $token);
}

/**
* @psalm-return DataResponse<int, array|null|object|scalar, array<string, mixed>>
*/
#[NoAdminRequired]
#[PublicPage]
#[RequireDocumentSession]
public function updateSession(string $guestName) {
public function updateSession(string $guestName): DataResponse {
return $this->apiService->updateSession($this->getSession(), $guestName);
}
}
4 changes: 2 additions & 2 deletions lib/Controller/SessionController.php
Expand Up @@ -96,14 +96,14 @@ public function sync(int $version = 0, string $autosaveContent = null, string $d
#[RequireDocumentSession]
#[UserRateLimit(limit: 5, period: 120)]
public function mention(string $mention): DataResponse {
if ($this->getSession()->getUserId() === null && !$this->sessionService->isUserInDocument($this->getDocument()->getId(), $mention)) {
if ($this->getSession()->isGuest() && !$this->sessionService->isUserInDocument($this->getDocument()->getId(), $mention)) {
return new DataResponse([], 403);
}

return new DataResponse($this->notificationService->mention($this->getDocument()->getId(), $mention));
}

private function loginSessionUser() {
private function loginSessionUser(): void {
$currentSession = $this->getSession();
if (!$this->userSession->isLoggedIn()) {
$user = $this->userManager->get($currentSession->getUserId());
Expand Down
14 changes: 5 additions & 9 deletions lib/Controller/SettingsController.php
Expand Up @@ -32,30 +32,26 @@
use OCP\IRequest;

class SettingsController extends Controller {
private IConfig $config;
private ?string $userId;

public const ACCEPTED_KEYS = [
'workspace_enabled'
];

public function __construct($appName, IRequest $request, IConfig $config, $userId) {
public function __construct(string $appName, IRequest $request, private IConfig $config, private ?string $userId) {
parent::__construct($appName, $request);

$this->config = $config;
$this->userId = $userId;
}

/**
* @throws \OCP\PreConditionNotMetException
*
* @psalm-return DataResponse<200|400, array{workspace_enabled?: mixed, message?: 'Invalid config key'}, array<never, never>>
*/
#[NoAdminRequired]
public function updateConfig(string $key, $value) {
public function updateConfig(string $key, int|string $value): DataResponse {
if (!in_array($key, self::ACCEPTED_KEYS, true)) {
return new DataResponse(['message' => 'Invalid config key'], Http::STATUS_BAD_REQUEST);
}
/** @psalm-suppress PossiblyNullArgument */
$this->config->setUserValue($this->userId, Application::APP_NAME, $key, $value);
$this->config->setUserValue($this->userId, Application::APP_NAME, $key, (string)$value);
return new DataResponse([
$key => $value
]);
Expand Down
2 changes: 1 addition & 1 deletion lib/Controller/UserApiController.php
Expand Up @@ -47,7 +47,7 @@ public function index(string $filter = '', int $limit = 5): DataResponse {
}
}

if ($this->getSession()->getUserId() !== null) {
if (!$this->getSession()->isGuest()) {
// Add other users to the autocomplete list
[$result] = $this->collaboratorSearch->search($filter, [IShare::TYPE_USER], false, $limit, 0);
$userSearch = array_merge($result['users'], $result['exact']['users']);
Expand Down
1 change: 1 addition & 0 deletions lib/Controller/WorkspaceController.php
Expand Up @@ -145,6 +145,7 @@ public function publicFolder(string $shareToken, string $path = '/'): DataRespon
if (!($share->getPermissions() & Constants::PERMISSION_READ)) {
throw new ShareNotFound();
}
/** @psalm-suppress RedundantConditionGivenDocblockType */
if ($share->getPassword() !== null) {
$shareId = $this->session->get('public_link_authenticated');
if ($share->getId() !== $shareId) {
Expand Down
4 changes: 4 additions & 0 deletions lib/Cron/Cleanup.php
Expand Up @@ -54,6 +54,10 @@ public function __construct(ITimeFactory $time,
$this->setInterval(SessionService::SESSION_VALID_TIME);
}

/**
* @param array $argument
* @return void
*/
protected function run($argument) {
$this->logger->debug('Run cleanup job for text documents');
$documents = $this->documentService->getAll();
Expand Down
2 changes: 1 addition & 1 deletion lib/Db/Document.php
Expand Up @@ -41,7 +41,7 @@
* @method setBaseVersionEtag(string $etag): void
*/
class Document extends Entity implements \JsonSerializable {
public $id;
public $id = null;
// TODO: Remove obsolete field `currentVersion`
protected int $currentVersion = 0;
protected int $lastSavedVersion = 0;
Expand Down
2 changes: 1 addition & 1 deletion lib/Db/DocumentMapper.php
Expand Up @@ -39,7 +39,7 @@ public function __construct(IDBConnection $db) {
* @return Document
* @throws DoesNotExistException
*/
public function find($documentId): Document {
public function find(int $documentId): Document {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$result = $qb->select('*')
Expand Down
13 changes: 12 additions & 1 deletion lib/Db/Session.php
Expand Up @@ -24,10 +24,10 @@
namespace OCA\Text\Db;

use JsonSerializable;
use OCA\Text\Exception\InvalidSessionException;
use OCP\AppFramework\Db\Entity;

/**
* @method ?string getUserId()
* @method void setUserId(?string $userId)
* @method string getToken()
* @method void setToken(string $token)
Expand Down Expand Up @@ -58,6 +58,17 @@ public function __construct() {
$this->addType('lastContact', 'integer');
}

public function isGuest(): bool {
return $this->userId === null;
}

public function getUserId(): string {
if ($this->userId === null) {
throw new InvalidSessionException('No user id found');
}
return $this->userId;
}

public function jsonSerialize(): array {
return [
'id' => $this->id,
Expand Down
27 changes: 21 additions & 6 deletions lib/Db/SessionMapper.php
Expand Up @@ -53,7 +53,7 @@ public function findAllDocuments(): array {
* @return Session
* @throws DoesNotExistException
*/
public function find($documentId, $sessionId, $token): Session {
public function find(int $documentId, int $sessionId, string $token): Session {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$result = $qb->select('*')
Expand All @@ -71,7 +71,12 @@ public function find($documentId, $sessionId, $token): Session {
return Session::fromRow($data);
}

public function findAll($documentId) {
/**
* @return Session[]
*
* @psalm-return array<Session>
*/
public function findAll(int $documentId): array {
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'color', 'document_id', 'last_awareness_message', 'last_contact', 'user_id', 'guest_name')
->from($this->getTableName())
Expand All @@ -80,7 +85,12 @@ public function findAll($documentId) {
return $this->findEntities($qb);
}

public function findAllActive($documentId) {
/**
* @return Session[]
*
* @psalm-return array<Session>
*/
public function findAllActive(int $documentId): array {
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'color', 'document_id', 'last_awareness_message', 'last_contact', 'user_id', 'guest_name')
->from($this->getTableName())
Expand All @@ -90,7 +100,12 @@ public function findAllActive($documentId) {
return $this->findEntities($qb);
}

public function findAllInactive() {
/**
* @return Session[]
*
* @psalm-return array<Session>
*/
public function findAllInactive(): array {
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'color', 'document_id', 'last_awareness_message', 'last_contact', 'user_id', 'guest_name')
->from($this->getTableName())
Expand Down Expand Up @@ -132,14 +147,14 @@ public function deleteInactiveWithoutSteps(?int $documentId = null): int {
return $deletedCount;
}

public function deleteByDocumentId($documentId): int {
public function deleteByDocumentId(int $documentId): int {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())
->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)));
return $qb->executeStatement();
}

public function isUserInDocument($documentId, $userId): bool {
public function isUserInDocument(int $documentId, string $userId): bool {
$qb = $this->db->getQueryBuilder();
$result = $qb->select('*')
->from($this->getTableName())
Expand Down
7 changes: 4 additions & 3 deletions lib/Db/Step.php
Expand Up @@ -37,6 +37,7 @@
* @method setDocumentId(int $documentId): void
*/
class Step extends Entity implements JsonSerializable {
public $id = null;
protected string $data = '';
protected int $version = 0;
protected int $sessionId = 0;
Expand All @@ -55,10 +56,10 @@ public function jsonSerialize(): array {
throw new \InvalidArgumentException('Failed to parse step data');
}
return [
'id' => $this->id,
'id' => $this->getId(),
'data' => $jsonData,
'version' => $this->version,
'sessionId' => $this->sessionId
'version' => $this->getVersion(),
'sessionId' => $this->getSessionId()
];
}
}
15 changes: 9 additions & 6 deletions lib/Db/StepMapper.php
Expand Up @@ -33,7 +33,10 @@ public function __construct(IDBConnection $db) {
parent::__construct($db, 'text_steps', Step::class);
}

public function find($documentId, $fromVersion, $lastAckedVersion = null) {
/**
* @return Step[]
*/
public function find(int $documentId, int $fromVersion, int $lastAckedVersion = null): array {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$qb->select('*')
Expand All @@ -51,7 +54,7 @@ public function find($documentId, $fromVersion, $lastAckedVersion = null) {
return $this->findEntities($qb);
}

public function getLatestVersion($documentId): ?int {
public function getLatestVersion(int $documentId): ?int {
/* @var $qb IQueryBuilder */
$qb = $this->db->getQueryBuilder();
$result = $qb->select('version')
Expand All @@ -69,22 +72,22 @@ public function getLatestVersion($documentId): ?int {
return $data['version'];
}

public function deleteAll($documentId): void {
public function deleteAll(int $documentId): void {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())
->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
->executeStatement();
}

public function deleteBeforeVersion($documentId, $version): void {
public function deleteBeforeVersion(int $documentId, int $version): int {
$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())
return $qb->delete($this->getTableName())
->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
->andWhere($qb->expr()->lte('version', $qb->createNamedParameter($version)))
->executeStatement();
}

public function deleteAfterVersion($documentId, $version): int {
public function deleteAfterVersion(int $documentId, int $version): int {
$qb = $this->db->getQueryBuilder();
return $qb->delete($this->getTableName())
->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
Expand Down
4 changes: 4 additions & 0 deletions lib/Migration/Version010000Date20190617184535.php
Expand Up @@ -19,6 +19,8 @@ class Version010000Date20190617184535 extends SimpleMigrationStep {
* @param IOutput $output
* @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
* @param array $options
*
* @return void
*/
public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
}
Expand Down Expand Up @@ -139,6 +141,8 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt
* @param IOutput $output
* @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
* @param array $options
*
* @return void
*/
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
}
Expand Down