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.

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

fix(OpenAPI): Fix array syntax ambiguities #13711

Closed
wants to merge 1 commit into from
Closed

fix(OpenAPI): Fix array syntax ambiguities #13711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions lib/Capabilities.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ public function __construct(

/**
* @return array{
* spreed: TalkCapabilities,
* }|array<empty>
* spreed?: TalkCapabilities,
* }
*/
public function getCapabilities(): array {
$user = $this->userSession->getUser();
Expand Down Expand Up @@ -257,18 +257,18 @@ public function getCapabilities(): array {
$capabilities['config']['signaling']['hello-v2-token-key'] = $pubKey;
}

/** @var ?string[] $predefinedBackgrounds */
/** @var ?list<string> $predefinedBackgrounds */
$predefinedBackgrounds = null;
$cachedPredefinedBackgrounds = $this->talkCache->get('predefined_backgrounds');
if ($cachedPredefinedBackgrounds !== null) {
// Try using cached value
/** @var string[]|null $predefinedBackgrounds */
/** @var ?list<string> $predefinedBackgrounds */
$predefinedBackgrounds = json_decode($cachedPredefinedBackgrounds, true);
}

if (!is_array($predefinedBackgrounds)) {
// Cache was empty or invalid, regenerate
/** @var string[] $predefinedBackgrounds */
/** @var list<string> $predefinedBackgrounds */
$predefinedBackgrounds = [];
if (file_exists(__DIR__ . '/../img/backgrounds')) {
$directoryIterator = new \DirectoryIterator(__DIR__ . '/../img/backgrounds');
Expand Down
6 changes: 6 additions & 0 deletions lib/Chat/ChatManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use OCA\Talk\Model\Message;
use OCA\Talk\Model\Poll;
use OCA\Talk\Participant;
use OCA\Talk\ResponseDefinitions;
use OCA\Talk\Room;
use OCA\Talk\Service\AttachmentService;
use OCA\Talk\Service\ParticipantService;
Expand Down Expand Up @@ -59,6 +60,7 @@
*
* When a message is saved the mentioned users are notified as needed, and
* pending notifications are removed if the messages are deleted.
* @psalm-import-type TalkChatMentionSuggestion from ResponseDefinitions
*/
class ChatManager {
public const MAX_CHAT_LENGTH = 32000;
Expand Down Expand Up @@ -950,6 +952,10 @@ public function searchForObjectsWithFilters(string $search, array $objectIds, st
return $this->commentsManager->searchForObjectsWithFilters($search, 'chat', $objectIds, $verb, $since, $until, $actorType, $actorId, $offset, $limit);
}

/**
* @param list<TalkChatMentionSuggestion> $results
* @return list<TalkChatMentionSuggestion>
*/
public function addConversationNotify(array $results, string $search, Room $room, Participant $participant): array {
if ($room->getType() === Room::TYPE_ONE_TO_ONE) {
return $results;
Expand Down
2 changes: 1 addition & 1 deletion lib/Chat/ReactionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public function deleteReactionMessage(Room $chat, string $actorType, string $act
}

/**
* @return array<string, TalkReaction[]>
* @return array<string, list<TalkReaction>>
* @throws PreConditionNotMetException
*/
public function retrieveReactionMessages(Room $chat, Participant $participant, int $messageId, ?string $reaction = null): array {
Expand Down
4 changes: 2 additions & 2 deletions lib/Controller/BanController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public function banActor(string $actorType, string $actorId, string $internalNot
*
* Required capability: `ban-v1`
*
* @return DataResponse<Http::STATUS_OK, TalkBan[], array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkBan>, array{}>
*
* 200: List all bans
*/
Expand All @@ -95,7 +95,7 @@ public function listBans(): DataResponse {
* Required capability: `ban-v1`
*
* @param int $banId ID of the ban to be removed
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
Copy link
Member

Choose a reason for hiding this comment

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

Really not a fan of this... it will never be a list. But we might introduce an 'error' key like we have on many other APIs, and then the response type changes from [] to {}
I guess it's hard to know which case is preferred technically.

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 even then this is a breaking change.
If you'd document it as \stdClass for now and later as array{error: string} it would be fine.
I don't think this change makes it worse as it is the same JSON data in the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO the cleanest way is to have it return null as then you can later add any data and you are not limited to either objects or lists.
Unfortunately I don't think we can change this as it technically is a breaking change, though no client should really be looking at this data (and openapi-extractor also skips it automatically).
Same goes for the default value in the constructor of DataResponse which should be null as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but even then this is a breaking change.

well form array<empty> to array{error? string} it should be okay?
But anyway, we will see.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the former is [] in JSON and the later is {} or {"error": ""} in JSON and those are not compatible.

Copy link
Member

Choose a reason for hiding this comment

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

ah lol, nvm. the list<empty> on root is not added to the OpenAPI definition as a list:

                                                "meta": {
                                                    "$ref": "#/components/schemas/OCSMeta"
                                                },
                                                "data": {}
                                            }
                                        }
                                    }

So I guess we are fine to start using it later either way

Copy link
Member

Choose a reason for hiding this comment

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

Any disadvantage of using array{} 🤔

It does:

diff --git a/openapi-full.json b/openapi-full.json
index 0fd153e89..206856fca 100644
--- a/openapi-full.json
+++ b/openapi-full.json
@@ -2433,7 +2433,9 @@
                                                 "meta": {
                                                     "$ref": "#/components/schemas/OCSMeta"
                                                 },
-                                                "data": {}
+                                                "data": {
+                                                    "type": "object"
+                                                }
                                             }
                                         }
                                     }

I guess the real problem is that the API does not return like this, so it actually creates invalid code-docs combination?

Copy link
Member

@nickvergessen nickvergessen Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think my favorite solution for now is:

diff --git a/lib/Controller/AEnvironmentAwareController.php b/lib/Controller/AEnvironmentAwareController.php
index 25cfa8083..238f0af5a 100644
--- a/lib/Controller/AEnvironmentAwareController.php
+++ b/lib/Controller/AEnvironmentAwareController.php
@@ -73,4 +73,16 @@ abstract class AEnvironmentAwareController extends OCSController {
 
                return $format;
        }
+
+       /**
+        * @psalm-suppress InvalidReturnStatement,InvalidReturnType
+        * @psalm-return array{}
+        */
+       public function getEmptyObjectData(): array {
+               if ($this->getResponseFormat() === 'json') {
+                       // Cheating here to make sure the array is always a JSON object on the API.
+                       return new \stdClass();
+               }
+               return [];
+       }
 }
diff --git a/lib/Controller/BanController.php b/lib/Controller/BanController.php
index a38b9fdbe..3b5d5b690 100644
--- a/lib/Controller/BanController.php
+++ b/lib/Controller/BanController.php
@@ -95,7 +95,7 @@ class BanController extends AEnvironmentAwareController {
         * Required capability: `ban-v1`
         *
         * @param int $banId ID of the ban to be removed
-        * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
+        * @return DataResponse<Http::STATUS_OK, array{}, array{}>
         *
         * 200: Unban successfully or not found
         */
@@ -103,6 +103,6 @@ class BanController extends AEnvironmentAwareController {
        #[RequireModeratorParticipant]
        public function unbanActor(int $banId): DataResponse {
                $this->banService->findAndDeleteBanByIdForRoom($banId, $this->room->getId());
-               return new DataResponse([], Http::STATUS_OK);
+               return new DataResponse($this->getEmptyObjectData(), Http::STATUS_OK);
        }
 }
diff --git a/openapi-full.json b/openapi-full.json
index 0fd153e89..206856fca 100644
--- a/openapi-full.json
+++ b/openapi-full.json
@@ -2433,7 +2433,9 @@
                                                 "meta": {
                                                     "$ref": "#/components/schemas/OCSMeta"
                                                 },
-                                                "data": {}
+                                                "data": {
+                                                    "type": "object"
+                                                }
                                             }
                                         }
                                     }

Copy link
Member Author

Choose a reason for hiding this comment

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

Any disadvantage of using array{}

You can not switch to list<> later without a breaking change.

I guess the real problem is that the API does not return like this, so it actually creates invalid code-docs combination?

Yeah this should not be done, I don't think psalm or openapi-extractor can catch it.

I think my favorite solution for now is:

Looks ok to me, even though I think null is still better as it allows you to use any type later without a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

You can not switch to list<> later without a breaking change.

Well those things never should return lists directly anyway.

I made a poll for the team to give feedback if toggling to null and later on to array{error?: string} is problematic. If it turns out to be, we can also annotate array{error?: string} now already to indicate the correct way going forward.

*
* 200: Unban successfully or not found
*/
Expand Down
10 changes: 5 additions & 5 deletions lib/Controller/BotController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ protected function getBotFromHeaders(string $token, string $message): Bot {
* @param string $referenceId For the message to be able to later identify it again
* @param int $replyTo Parent id which this message is a reply to
* @param bool $silent If sent silent the chat message will not create any notifications
* @return DataResponse<Http::STATUS_CREATED|Http::STATUS_BAD_REQUEST|Http::STATUS_UNAUTHORIZED|Http::STATUS_REQUEST_ENTITY_TOO_LARGE, array<empty>, array{}>
* @return DataResponse<Http::STATUS_CREATED|Http::STATUS_BAD_REQUEST|Http::STATUS_UNAUTHORIZED|Http::STATUS_REQUEST_ENTITY_TOO_LARGE, list<empty>, array{}>
Copy link
Member

Choose a reason for hiding this comment

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

same here 🙈

*
* 201: Message sent successfully
* 400: When the replyTo is invalid or message is empty
Expand Down Expand Up @@ -183,7 +183,7 @@ public function sendMessage(string $token, string $message, string $referenceId
* @param string $token Conversation token
* @param int $messageId ID of the message
* @param string $reaction Reaction to add
* @return DataResponse<Http::STATUS_OK|Http::STATUS_CREATED|Http::STATUS_BAD_REQUEST|Http::STATUS_UNAUTHORIZED|Http::STATUS_NOT_FOUND, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_CREATED|Http::STATUS_BAD_REQUEST|Http::STATUS_UNAUTHORIZED|Http::STATUS_NOT_FOUND, list<empty>, array{}>
*
* 200: Reaction already exists
* 201: Reacted successfully
Expand Down Expand Up @@ -237,7 +237,7 @@ public function react(string $token, int $messageId, string $reaction): DataResp
* @param string $token Conversation token
* @param int $messageId ID of the message
* @param string $reaction Reaction to delete
* @return DataResponse<Http::STATUS_OK|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND|Http::STATUS_UNAUTHORIZED, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND|Http::STATUS_UNAUTHORIZED, list<empty>, array{}>
*
* 200: Reaction deleted successfully
* 400: Reacting is not possible
Expand Down Expand Up @@ -285,7 +285,7 @@ public function deleteReaction(string $token, int $messageId, string $reaction):
/**
* List admin bots
*
* @return DataResponse<Http::STATUS_OK, TalkBotWithDetails[], array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkBotWithDetails>, array{}>
*
* 200: Bot list returned
*/
Expand All @@ -305,7 +305,7 @@ public function adminListBots(): DataResponse {
/**
* List bots
*
* @return DataResponse<Http::STATUS_OK, TalkBot[], array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkBot>, array{}>
*
* 200: Bot list returned
*/
Expand Down
12 changes: 6 additions & 6 deletions lib/Controller/BreakoutRoomController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public function __construct(
* @psalm-param BreakoutRoom::MODE_* $mode
* @param int<1, 20> $amount Number of breakout rooms - Constants {@see BreakoutRoom::MINIMUM_ROOM_AMOUNT} and {@see BreakoutRoom::MAXIMUM_ROOM_AMOUNT}
* @param string $attendeeMap Mapping of the attendees to breakout rooms
* @return DataResponse<Http::STATUS_OK, TalkRoom[], array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkRoom>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
*
* 200: Breakout rooms configured successfully
* 400: Configuring breakout rooms errored
Expand Down Expand Up @@ -87,7 +87,7 @@ public function removeBreakoutRooms(): DataResponse {
* Broadcast a chat message to all breakout rooms
*
* @param string $message Message to broadcast
* @return DataResponse<Http::STATUS_CREATED, TalkRoom[], array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_REQUEST_ENTITY_TOO_LARGE, array{error: string}, array{}>
* @return DataResponse<Http::STATUS_CREATED, list<TalkRoom>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_REQUEST_ENTITY_TOO_LARGE, array{error: string}, array{}>
*
* 201: Chat message broadcasted successfully
* 400: Broadcasting chat message is not possible
Expand All @@ -111,7 +111,7 @@ public function broadcastChatMessage(string $message): DataResponse {
* Apply an attendee map to the breakout rooms
*
* @param string $attendeeMap JSON encoded mapping of the attendees to breakout rooms `array<int, int>`
* @return DataResponse<Http::STATUS_OK, TalkRoom[], array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkRoom>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
*
* 200: Attendee map applied successfully
* 400: Applying attendee map is not possible
Expand Down Expand Up @@ -181,7 +181,7 @@ public function resetRequestForAssistance(): DataResponse {
/**
* Start the breakout rooms
*
* @return DataResponse<Http::STATUS_OK, TalkRoom[], array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkRoom>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
*
* 200: Breakout rooms started successfully
* 400: Starting breakout rooms is not possible
Expand All @@ -202,7 +202,7 @@ public function startBreakoutRooms(): DataResponse {
/**
* Stop the breakout rooms
*
* @return DataResponse<Http::STATUS_OK, TalkRoom[], array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkRoom>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
*
* 200: Breakout rooms stopped successfully
* 400: Stopping breakout rooms is not possible
Expand Down Expand Up @@ -247,7 +247,7 @@ public function switchBreakoutRoom(string $target): DataResponse {
}

/**
* @return TalkRoom[]
* @return list<TalkRoom>
*/
protected function formatMultipleRooms(array $rooms): array {
$return = [];
Expand Down
16 changes: 8 additions & 8 deletions lib/Controller/CallController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public function __construct(
/**
* Get the peers for a call
*
* @return DataResponse<Http::STATUS_OK, TalkCallPeer[], array{}>
* @return DataResponse<Http::STATUS_OK, list<TalkCallPeer>, array{}>
*
* 200: List of peers in the call returned
*/
Expand Down Expand Up @@ -193,7 +193,7 @@ public function downloadParticipantsForCall(string $format = 'csv'): DataDownloa
* (Only needed when the `config => call => recording-consent` capability is set to {@see RecordingService::CONSENT_REQUIRED_YES}
* or the capability is {@see RecordingService::CONSENT_REQUIRED_OPTIONAL}
* and the conversation `recordingConsent` value is {@see RecordingService::CONSENT_REQUIRED_YES} )
* @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_FOUND, array<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error?: string}, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_FOUND, list<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error?: string}, array{}>
*
* 200: Call joined successfully
* 400: No recording consent was given
Expand Down Expand Up @@ -274,7 +274,7 @@ protected function validateRecordingConsent(bool $recordingConsent): void {
* @psalm-param int-mask-of<Participant::FLAG_*>|null $flags
* @param bool $silent Join the call silently
* @param bool $recordingConsent Agreement to be recorded
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error?: string}, array{}>|DataResponse<Http::STATUS_NOT_FOUND, null, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error?: string}, array{}>|DataResponse<Http::STATUS_NOT_FOUND, null, array{}>
*
* 200: Call joined successfully
* 400: Conditions to join not met
Expand Down Expand Up @@ -314,7 +314,7 @@ public function joinFederatedCall(string $sessionId, ?int $flags = null, bool $s
* Ring an attendee
*
* @param int $attendeeId ID of the attendee to ring
* @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_FOUND, array<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_FOUND, list<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
*
* 200: Attendee rang successfully
* 400: Ringing attendee is not possible
Expand Down Expand Up @@ -396,7 +396,7 @@ public function sipDialOut(int $attendeeId): DataResponse {
*
* @param int<0, 15> $flags New flags
* @psalm-param int-mask-of<Participant::FLAG_*> $flags New flags
* @return DataResponse<Http::STATUS_OK|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND, list<empty>, array{}>
*
* 200: In-call flags updated successfully
* 400: Updating in-call flags is not possible
Expand Down Expand Up @@ -439,7 +439,7 @@ public function updateCallFlags(int $flags): DataResponse {
* @param string $sessionId Federated session id to update the flags with
* @param int<0, 15> $flags New flags
* @psalm-param int-mask-of<Participant::FLAG_*> $flags New flags
* @return DataResponse<Http::STATUS_OK|Http::STATUS_BAD_REQUEST, array<empty>, array{}>|DataResponse<Http::STATUS_NOT_FOUND, null, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_BAD_REQUEST, list<empty>, array{}>|DataResponse<Http::STATUS_NOT_FOUND, null, array{}>
*
* 200: In-call flags updated successfully
* 400: Updating in-call flags is not possible
Expand Down Expand Up @@ -469,7 +469,7 @@ public function updateFederatedCallFlags(string $sessionId, int $flags): DataRes
* Leave a call
*
* @param bool $all whether to also terminate the call for all participants
* @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_FOUND, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_FOUND, list<empty>, array{}>
*
* 200: Call left successfully
* 404: Call session not found
Expand Down Expand Up @@ -519,7 +519,7 @@ public function leaveCall(bool $all = false): DataResponse {
* user.
*
* @param string $sessionId Federated session id to leave with
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>|DataResponse<Http::STATUS_NOT_FOUND, null, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>|DataResponse<Http::STATUS_NOT_FOUND, null, array{}>
*
* 200: Call left successfully
* 404: Call session not found
Expand Down
Loading
Loading