fix(OpenAPI): Fix array syntax ambiguities

[provokateurin]

Fixes for nextcloud/openapi-extractor#168.
No real issues found, Talk code quality is great as always 🚀

🏁 Checklist

• ⛑️ Tests (unit and/or integration) are included or not possible
• 📘 API documentation in docs/ has been updated or is not required
• 🔖 Capability is added or not needed

[nickvergessen]

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.

[provokateurin]

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.

[provokateurin]

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.

[nickvergessen]

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.

[provokateurin]

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

[nickvergessen]

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

[nickvergessen]

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?

[nickvergessen]

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"
+                                                }
                                             }
                                         }
                                     }

[provokateurin]

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.

[nickvergessen]

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.

[nickvergessen]

same here 🙈

[nickvergessen]

• Replacing bit by bit in fix(openapi): Improve empty array cases #13830