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

user_status: Add OpenAPI spec #36408

Closed
wants to merge 1 commit into from
Closed
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
27 changes: 16 additions & 11 deletions apps/user_status/lib/Controller/HeartbeatController.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* @copyright Copyright (c) 2020, Georg Ehrke
*
* @author Georg Ehrke <[email protected]>
* @author Kate Döen <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
Expand All @@ -23,12 +24,12 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\UserStatus\Controller;

use OCA\UserStatus\Db\UserStatus;
use OCA\UserStatus\ResponseDefinitions;
use OCA\UserStatus\Service\StatusService;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCSController;
Expand All @@ -53,12 +54,13 @@ class HeartbeatController extends OCSController {
/** @var StatusService */
private $service;

public function __construct(string $appName,
IRequest $request,
IEventDispatcher $eventDispatcher,
IUserSession $userSession,
ITimeFactory $timeFactory,
StatusService $service) {
public function __construct(
string $appName,
IRequest $request,
IEventDispatcher $eventDispatcher,
IUserSession $userSession,
ITimeFactory $timeFactory,
StatusService $service) {
parent::__construct($appName, $request);
$this->eventDispatcher = $eventDispatcher;
$this->userSession = $userSession;
Expand All @@ -69,11 +71,14 @@ public function __construct(string $appName,
/**
* @NoAdminRequired
*
* @param string $status
* @return DataResponse
* @param string $status online, away
* @psalm-import-type PrivateUserStatus from ResponseDefinitions
* @return DataResponse<PrivateUserStatus> 200 Status successfully updated
* @return DataResponse 204 User has no status to update
* @return DataResponse 400 Invalid status to update
Comment on lines +76 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

Several @return is not allowed it seems, see psalm errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is still a problem I have no solution for. I really need to have this working, but I don't know how. Only a single return annotation is kind of impossible or will get really ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would PHP attributes be a possible solution here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wdym? We probably found a solution for this problem, but I first need to test it.

Copy link
Member

Choose a reason for hiding this comment

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

https://www.php.net/manual/de/language.attributes.syntax.php it something we could use in Nextcloud 26+ as it requires PHP8+

Sample in #36363

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm I see, but it wouldn't help with static analysis, right? The idea seems pretty good though.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you define am attribute and scan statically for that attribute, you can find these. You might be able to use reflections to let the PHP parser do the scanning even.
As PHP 7 is EOL, it should be good to go in general.
You could add a parameter to the attribute that contains an array. The keys would be the status codes and the value either a string of even a structure to describe the status.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this won't help psalm validate the code in the method, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe i got the problem wrong. Are you concerned about static code analysis using psalm or about the extraction of the API specification from the code?
For psalm a single return should be sufficient, right?

Copy link
Member Author

@provokateurin provokateurin Feb 9, 2023

Choose a reason for hiding this comment

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

Both. Please let us discuss this in #36513

*/
public function heartbeat(string $status): DataResponse {
if (!\in_array($status, [IUserStatus::ONLINE, IUserStatus::AWAY], true)) {
if (!in_array($status, [IUserStatus::ONLINE, IUserStatus::AWAY], true)) {
return new DataResponse([], Http::STATUS_BAD_REQUEST);
}

Expand Down
15 changes: 10 additions & 5 deletions apps/user_status/lib/Controller/PredefinedStatusController.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* @copyright Copyright (c) 2020, Georg Ehrke
*
* @author Georg Ehrke <[email protected]>
* @author Kate Döen <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
Expand All @@ -23,8 +24,10 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\UserStatus\Controller;

use OCA\UserStatus\ResponseDefinitions;
use OCA\UserStatus\Service\PredefinedStatusService;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCSController;
Expand All @@ -47,19 +50,21 @@ class PredefinedStatusController extends OCSController {
* @param IRequest $request
* @param PredefinedStatusService $predefinedStatusService
*/
public function __construct(string $appName,
IRequest $request,
PredefinedStatusService $predefinedStatusService) {
public function __construct(
string $appName,
IRequest $request,
PredefinedStatusService $predefinedStatusService) {
parent::__construct($appName, $request);
$this->predefinedStatusService = $predefinedStatusService;
}

/**
* @NoAdminRequired
*
* @return DataResponse
* @psalm-import-type PredefinedStatus from ResponseDefinitions
* @return DataResponse<PredefinedStatus[]> 200
Copy link
Contributor

Choose a reason for hiding this comment

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

DataResponse is not a template in the current code base. Remove the parameter or update the docblocks of DataRepsonse to make it a template.

Copy link
Member Author

Choose a reason for hiding this comment

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

#36355 can you review?

*/
public function findAll():DataResponse {
public function findAll(): DataResponse {
// Filtering out the invisible one, that should only be set by API
return new DataResponse(array_filter($this->predefinedStatusService->getDefaultStatuses(), function (array $status) {
return !array_key_exists('visible', $status) || $status['visible'] === true;
Expand Down
18 changes: 12 additions & 6 deletions apps/user_status/lib/Controller/StatusesController.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*
* @author Christoph Wurst <[email protected]>
* @author Georg Ehrke <[email protected]>
* @author Kate Döen <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
Expand All @@ -24,9 +25,11 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\UserStatus\Controller;

use OCA\UserStatus\Db\UserStatus;
use OCA\UserStatus\ResponseDefinitions;
use OCA\UserStatus\Service\StatusService;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Http\DataResponse;
Expand All @@ -47,9 +50,10 @@ class StatusesController extends OCSController {
* @param IRequest $request
* @param StatusService $service
*/
public function __construct(string $appName,
IRequest $request,
StatusService $service) {
public function __construct(
string $appName,
IRequest $request,
StatusService $service) {
parent::__construct($appName, $request);
$this->service = $service;
}
Expand All @@ -59,7 +63,8 @@ public function __construct(string $appName,
*
* @param int|null $limit
* @param int|null $offset
* @return DataResponse
* @psalm-import-type PublicUserStatus from ResponseDefinitions
* @return DataResponse<PublicUserStatus[]> 200
*/
public function findAll(?int $limit = null, ?int $offset = null): DataResponse {
$allStatuses = $this->service->findAll($limit, $offset);
Expand All @@ -73,7 +78,8 @@ public function findAll(?int $limit = null, ?int $offset = null): DataResponse {
* @NoAdminRequired
*
* @param string $userId
* @return DataResponse
* @psalm-import-type PublicUserStatus from ResponseDefinitions
* @return DataResponse<PublicUserStatus> 200
* @throws OCSNotFoundException
*/
public function find(string $userId): DataResponse {
Expand All @@ -88,7 +94,7 @@ public function find(string $userId): DataResponse {

/**
* @param UserStatus $status
* @return array{userId: string, message: string, icon: string, clearAt: int, status: string}
* @return array
*/
private function formatStatus(UserStatus $status): array {
$visibleStatus = $status->getStatus();
Expand Down
39 changes: 23 additions & 16 deletions apps/user_status/lib/Controller/UserStatusController.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* @author Georg Ehrke <[email protected]>
* @author Joas Schilling <[email protected]>
* @author Simon Spannagel <[email protected]>
* @author Kate Döen <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
Expand All @@ -25,6 +26,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\UserStatus\Controller;

use OCA\UserStatus\Db\UserStatus;
Expand All @@ -33,6 +35,7 @@
use OCA\UserStatus\Exception\InvalidStatusIconException;
use OCA\UserStatus\Exception\InvalidStatusTypeException;
use OCA\UserStatus\Exception\StatusMessageTooLongException;
use OCA\UserStatus\ResponseDefinitions;
use OCA\UserStatus\Service\StatusService;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Http\DataResponse;
Expand All @@ -59,14 +62,15 @@ class UserStatusController extends OCSController {
* @param string $appName
* @param IRequest $request
* @param string $userId
* @param ILogger $logger;
* @param ILogger $logger
* @param StatusService $service
*/
public function __construct(string $appName,
IRequest $request,
string $userId,
ILogger $logger,
StatusService $service) {
public function __construct(
string $appName,
IRequest $request,
string $userId,
ILogger $logger,
StatusService $service) {
parent::__construct($appName, $request);
$this->userId = $userId;
$this->logger = $logger;
Expand All @@ -76,7 +80,8 @@ public function __construct(string $appName,
/**
* @NoAdminRequired
*
* @return DataResponse
* @psalm-import-type PrivateUserStatus from ResponseDefinitions
* @return DataResponse<PrivateUserStatus> 200
* @throws OCSNotFoundException
*/
public function getStatus(): DataResponse {
Expand All @@ -93,7 +98,8 @@ public function getStatus(): DataResponse {
* @NoAdminRequired
*
* @param string $statusType
* @return DataResponse
* @psalm-import-type PrivateUserStatus from ResponseDefinitions
* @return DataResponse<PrivateUserStatus> 200
* @throws OCSBadRequestException
*/
public function setStatus(string $statusType): DataResponse {
Expand All @@ -113,11 +119,11 @@ public function setStatus(string $statusType): DataResponse {
*
* @param string $messageId
* @param int|null $clearAt
* @return DataResponse
* @psalm-import-type PrivateUserStatus from ResponseDefinitions
* @return DataResponse<PrivateUserStatus> 200
* @throws OCSBadRequestException
*/
public function setPredefinedMessage(string $messageId,
?int $clearAt): DataResponse {
public function setPredefinedMessage(string $messageId, ?int $clearAt): DataResponse {
try {
$status = $this->service->setPredefinedMessage($this->userId, $messageId, $clearAt);
$this->service->removeBackupUserStatus($this->userId);
Expand All @@ -137,12 +143,13 @@ public function setPredefinedMessage(string $messageId,
* @param string|null $statusIcon
* @param string|null $message
* @param int|null $clearAt
* @return DataResponse
* @psalm-import-type PrivateUserStatus from ResponseDefinitions
* @return DataResponse<PrivateUserStatus> 200
* @throws OCSBadRequestException
*/
public function setCustomMessage(?string $statusIcon,
?string $message,
?int $clearAt): DataResponse {
?string $message,
?int $clearAt): DataResponse {
try {
if (($message !== null && $message !== '') || ($clearAt !== null && $clearAt !== 0)) {
$status = $this->service->setCustomMessage($this->userId, $statusIcon, $message, $clearAt);
Expand All @@ -167,7 +174,7 @@ public function setCustomMessage(?string $statusIcon,
/**
* @NoAdminRequired
*
* @return DataResponse
* @return DataResponse 200
*/
public function clearStatus(): DataResponse {
$this->service->clearStatus($this->userId);
Expand All @@ -177,7 +184,7 @@ public function clearStatus(): DataResponse {
/**
* @NoAdminRequired
*
* @return DataResponse
* @return DataResponse 200
*/
public function clearMessage(): DataResponse {
$this->service->clearMessage($this->userId);
Expand Down
57 changes: 57 additions & 0 deletions apps/user_status/lib/ResponseDefinitions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php
declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Kate Döen <[email protected]>
*
* @author Kate Döen <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\UserStatus;

/**
* @psalm-type ClearAt = array{
* type: string,
* time: string|int,
* }
*
* @psalm-type PredefinedStatus = array{
* id: string,
* icon: string,
* message: string,
* clearAt: ClearAt|null,
* visible: bool|null,
* }
*
* @psalm-type PublicUserStatus = array{
* userId: string,
* message: string|null,
* icon: string|null,
* clearAt: int|ClearAt|null,
* status: string,
* }
*
* @psalm-type PrivateUserStatus = PublicUserStatus&array{
* messageId: string|null,
* messageIsPredefined: bool,
* statusIsUserDefined: bool,
* }
*/
class ResponseDefinitions {
}
Loading