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

feat(scopes): Allow apps to define different API scopes for different… #24

Merged
merged 29 commits into from
Jan 17, 2024

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Oct 28, 2023

… target clients

Sibling PRs

Possible optimisations

Fix #20

@nickvergessen nickvergessen self-assigned this Oct 28, 2023
@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from 26debec to c11ce8f Compare October 28, 2023 15:53
@nickvergessen nickvergessen added the enhancement New feature or request label Oct 28, 2023
@provokateurin
Copy link
Member

@nickvergessen would it be possible to get this done before the server release so we can update it in the server? This is quite a significant change so it would be great to have it from the start.

@provokateurin
Copy link
Member

The title of the specs should reflect the scope so a short note should be prepended to the current titles.

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.

LGTM from the code.
I'll give it some testing with the apps that are supported to check for any problems.

src/Helpers.php Outdated Show resolved Hide resolved
@provokateurin
Copy link
Member

Please rebase again 😬

@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from 93a6490 to d9e3844 Compare December 7, 2023 10:40
@nickvergessen
Copy link
Member Author

I had rebased before, so was only the rename from $parameter->docParameter to $parameter->docType

All done now

@provokateurin
Copy link
Member

Running in server (core):

Warning: app: Could not read used schemas for response to 'get /index.php/avatar/{userId}/{size}/dark' with status code 200
PHP Fatal error:  Uncaught TypeError: OpenAPIExtractor\Helpers::collectUsedRefs(): Argument #1 ($data) must be of type array, stdClass given, called in /home/jld3103/src/github.com/nextcloud/openapi-extractor/generate-spec on line 739 and defined in /home/jld3103/src/github.com/nextcloud/openapi-extractor/src/Helpers.php:251
Stack trace:
#0 /home/jld3103/src/github.com/nextcloud/openapi-extractor/generate-spec(739): OpenAPIExtractor\Helpers::collectUsedRefs()
#1 {main}
  thrown in /home/jld3103/src/github.com/nextcloud/openapi-extractor/src/Helpers.php on line 251

@provokateurin
Copy link
Member

And a few more of these errors in other apps:

Warning: app: Could not read used schemas for response to 'post /ocs/v2.php/cloud/shares/{id}/reshare' with status code 400
Warning: app: Could not read used schemas for response to 'post /ocs/v2.php/cloud/shares/{id}/permissions' with status code 400
Warning: app: Could not read used schemas for response to 'post /ocs/v2.php/cloud/shares/{id}/accept' with status code 500
Warning: app: Could not read used schemas for response to 'post /ocs/v2.php/cloud/shares/{id}/revoke' with status code 400
Warning: app: Could not read used schemas for response to 'post /ocs/v2.php/cloud/shares/{id}/move' with status code 400

@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from bf187cd to fd01672 Compare December 7, 2023 12:12
generate-spec Outdated Show resolved Hide resolved
generate-spec Outdated Show resolved Hide resolved
src/ControllerMethod.php Outdated Show resolved Hide resolved
@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from 8d60dd8 to d3bd5c3 Compare December 7, 2023 13:52
generate-spec Outdated Show resolved Hide resolved
@nickvergessen
Copy link
Member Author

Rebased after #44

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from 810ef64 to 4d20003 Compare January 17, 2024 12:56
@nickvergessen
Copy link
Member Author

Rebased again to solve the conflicts from #72

@nickvergessen
Copy link
Member Author

Arg

Info: app: Extracting OpenAPI spec for theming 2.4.0
Info: app: Generated 1 routes!

Need to fix the logic more

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.

LGTM! Can you update the demo PRs you did so I can review the changes in those and check that everything is good?

generate-spec Outdated Show resolved Hide resolved
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from 1f914f7 to ed40c15 Compare January 17, 2024 14:49
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Can you update the demo PRs you did so I can review the changes in those and check that everything is good?

@provokateurin
Copy link
Member

Running this in Talk results in

Info: app: Generated scope default with 66 routes!
Info: app: Generated scope bots with 2 routes!
Info: app: Generated scope administration with 9 routes!
Info: app: Generated scope backend-recording with 2 routes!
Info: app: Generated scope backend-sipbridge with 6 routes!
Info: app: Generated scope backend-signaling with 1 routes!
Info: app: Generated scope full with 85 routes!

But the sum of all routes is 86 and the full spec has only 85 🤔

@provokateurin
Copy link
Member

From the server branch:
https://github.com/nextcloud/server/blob/e1af7061e7c1598e5ccdfcb2fc6144070d0bcef9/apps/settings/openapi.json#L22
Empty schemas, probably missing a stdClass fix (might have been a problem on main already).

@nickvergessen
Copy link
Member Author

But the sum of all routes is 86 and the full spec has only 85

Because getSingleRoom is in default scope and backend-sipbridge scope:
https://github.com/nextcloud/spreed/pull/10856/files#diff-d583983624ef8769460d4aa59e64b75ee54b6955e01e620e3eacedb7ba208786R314-R315

Empty schemas, probably missing a stdClass fix (might have been a problem on main already).

Fixed locally.

Checking atm why comments e.g. generates a -full variant while it only has the default.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Both fixed and pushed now

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/20/openapi-scopes branch from 8746cb0 to a479a34 Compare January 17, 2024 15:34
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.

Noice 🎉

@nickvergessen nickvergessen merged commit bfe4acc into main Jan 17, 2024
10 checks passed
@delete-merged-branch delete-merged-branch bot deleted the techdebt/20/openapi-scopes branch January 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Allow different OpenAPI groups
2 participants