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

Enable AppFramework dispatcher to enforce integer ranges #41578

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Nov 17, 2023

Summary

Psalm allows to specify an int range for integer parameters. We can consider them in the Http Dispatcher so that incoming int values are checked for their range directly in the app framework. Example use case for dashboard added.

Set 28 as milestone, but could go into 29 only.

Checklist

@blizzz blizzz added this to the Nextcloud 28 milestone Nov 17, 2023
@blizzz blizzz requested review from nickvergessen, st3iny, provokateurin, a team, ArtificialOwl, nfebe and sorbaugh and removed request for a team November 17, 2023 13:11
@blizzz blizzz added the pending documentation This pull request needs an associated documentation update label Nov 17, 2023
@blizzz blizzz mentioned this pull request Nov 20, 2023
5 tasks
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Fancy stuff ✨

@nickvergessen
Copy link
Member

-                        "description": "Limit number of result items per widget",
+                        "description": "Limit number of result items per widget, not more than 30 are allowed",

Need to rebuild openapi with composer run openapi

@blizzz

This comment was marked as resolved.

@blizzz
Copy link
Member Author

blizzz commented Nov 20, 2023

build/openapi-checker.sh it shall be

@blizzz
Copy link
Member Author

blizzz commented Nov 21, 2023

@provokateurin yeah or no?

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Sorry, I thought this was solved already. I opened an issue in openapi-extractor to handle the @psalm-* annotations and there is another one already for supporting these integer ranges. So at some point this will also properly show up in the openapi docs :)

@provokateurin
Copy link
Member

(Although I think it is debatable if 30 is a good limit since it is still quite high 🤷‍♀️)

@provokateurin
Copy link
Member

And 0 as lower limit is also not useful, maybe set it to at least 1

@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@blizzz
Copy link
Member Author

blizzz commented Nov 24, 2023

Checked by with @st3iny, he also thinks 30 is a good value.

Either way, the numbers are mostly gut estimations, if necessary we can always tune them.

The 0 I increase to 1.

- allows devs to provide int ranges for API arguments

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the enh/noid/dispatcher-test-argument-range branch from c0f519e to 4827fc3 Compare November 24, 2023 11:46
@blizzz blizzz enabled auto-merge November 24, 2023 11:47
@blizzz
Copy link
Member Author

blizzz commented Nov 24, 2023

/backport to stable28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews backport-request enhancement feature: dashboard pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants