Skip to content

Conversation

provokateurin
Copy link
Member

Fixes #26

While this is the right approach to achieve correctness, it results in many changes (albeit simple, they are very repetitive) and for example core has this diff:

 core/Controller/OCSController.php                 |  6 +++++-
 core/Controller/ReferenceApiController.php        | 28 ++++++++++++++++++++++------
 core/Controller/TaskProcessingApiController.php   | 11 ++++++++---
 core/Controller/TwoFactorApiController.php        | 24 +++++++++++++++++++-----
 core/ResponseDefinitions.php                      | 32 ++++++++++++++++----------------
 lib/private/Search/SearchComposer.php             |  7 ++++++-
 lib/public/Collaboration/Reference/IReference.php |  2 +-
 lib/public/Collaboration/Reference/Reference.php  | 15 +++++++++++----
 lib/public/TaskProcessing/Task.php                | 16 +++++++++++++---
 9 files changed, 101 insertions(+), 40 deletions(-)

With this Talk has 16 new errors and the rest of server (without core) has 35 new errors in total. So I'm not really sure if it's worth it, especially because it is not so easy to understand why it is necessary, if you are not already familiar with the topic, so it might be harder for community people (but ofc documentation will be added).

@provokateurin provokateurin force-pushed the fix/openapitype/prevent-empty-json-object-mistakes branch from 75f8f8f to 5babd74 Compare May 19, 2025 09:27
@nickvergessen
Copy link
Member

With this Talk has 16 new errors and the rest of server (without core) has 35 new errors in total. So I'm not really sure if it's worth it

Well looking at the error:

Error: Response definitions: TalkBaseMessage: messageParameters: You must ensure JSON objects are not empty using the "non-empty-array" type. To allow return empty JSON objects your code must manually check if the array is empty in order to return "new \stdClass()" and use "non-empty-array|\stdClass" as the type.

I know what the problem is… But we can not fix it. Changing this is a breaking API change that might even break out mobile apps. Also returning \stdClass is breaking XML. So you also need to check the requested format and pipe it through all your layers to the part creating the array: https://github.com/nextcloud/spreed/blob/6af6d5c9f7fa3b71caeaf2960765e0eff8609b42/lib/Model/Message.php#L191-L195

When s/list/non-empty-list works like in TalkCapabilities, it's doable, but generally this is a "no" from me.

@provokateurin
Copy link
Member Author

Changing this is a breaking API change

Why would it be one?

that might even break out mobile apps

I'd actually think it's the opposite, since the clients probably expect {} but currently would receive []. So it's rather unbreaking it by enforcing proper checks for the response data.

Also returning \stdClass is breaking XML

No the case anymore since almost two years (for exactly this reason): nextcloud/server#38745

@provokateurin
Copy link
Member Author

Just thought of another edge-case that isn't handled by this change: non-empty-array{key?: string} can also produce [] in JSON and thus needs to be checked properly.

@provokateurin provokateurin marked this pull request as draft May 19, 2025 11:47
@provokateurin
Copy link
Member Author

And something like array{key: string, array<string, string>} should be allowed, since it is known that it will never produce [] (unless the specified key is optional, then it's the same as above again).

@nickvergessen
Copy link
Member

I'd actually think it's the opposite, since the clients probably expect {} but currently would receive []. So it's rather unbreaking it by enforcing proper checks for the response data.

This was always the case since 6 years on the API so clients check for it. I asked Ivan for a pointer to the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential risky return of array<string, …>
2 participants