nextcloud / openapi-extractor

A tool for extracting OpenAPI specifications from Nextcloud source code
https://docs.nextcloud.com/server/latest/developer_manual/client_apis/OCS/ocs-openapi.html
GNU Affero General Public License v3.0
7 stars 2 forks source link

fix: use schemas only from request body #146

Closed vitormattos closed 3 months ago

vitormattos commented 3 months ago

Scope

Have schemas that only exists at requestBody and at this case, if we don't fetch from body will throw an error "Can't resolve $ref" when run openapi-typescript build.

How to reproduce

Expected behavior

> dummyapp@0.0.1 typescript:generate
> npx openapi-typescript -t

🚀 openapi@v1 → ./src/types/openapi/openapi.ts [141.7ms]

Actual behavior

> dummyapp@0.0.1 typescript:generate
> npx openapi-typescript -t

 ✘  Can't resolve $ref
file:///var/www/html/apps-extra/dummyapp/node_modules/openapi-typescript/dist/lib/redoc.js:121
                throw new Error(problem.message);
                      ^

Error: Can't resolve $ref
    at validateAndBundle (file:///var/www/html/apps-extra/dummyapp/node_modules/openapi-typescript/dist/lib/redoc.js:121:23)
    at async openapiTS (file:///var/www/html/apps-extra/dummyapp/node_modules/openapi-typescript/dist/index.js:40:20)
    at async generateSchema (file:///var/www/html/apps-extra/dummyapp/node_modules/openapi-typescript/bin/cli.js:119:5)
    at async file:///var/www/html/apps-extra/dummyapp/node_modules/openapi-typescript/bin/cli.js:195:24
    at async Promise.all (index 0)
    at async main (file:///var/www/html/apps-extra/dummyapp/node_modules/openapi-typescript/bin/cli.js:181:5)

Node.js v20.15.1
Please manually regenerate the typescript OpenAPI models

Artifacts

Without fix

Screenshot_20240802_161158

Screenshot_20240802_161041


With fix

Screenshot_20240802_161424

Screenshot_20240802_161346

Tests

Considering that the test only build the json files and not the ts files, I didn't implemented tests.

vitormattos commented 3 months ago

Can I go ahead and implement a test to check if openapi-typescript will build without problems?

Preferably the logic is not duplicated with the code above, since it is exactly the same and only operates on a different part of the spec.

Could you comment more about this?

provokateurin commented 3 months ago

Can I go ahead and implement a test to check if openapi-typescript will build without problems?

Yes that would be nice, I thought about extending our current checks against existing repos by running openapi-typescript inside them if they use it. That should at least prevent regressions.

Could you comment more about this?

The logic is the same as a few lines above but complex enough that it should not be implemented twice.

vitormattos commented 3 months ago

Yes that would be nice, I thought about extending our current checks against existing repos by running openapi-typescript inside them if they use it. That should at least prevent regressions.

What you think about put this in a separated PR to don't mix with the context of this PR? I understand that tests are very important but would be best separate the scope to merge this fix more quickly.

vitormattos commented 3 months ago

The logic is the same as a few lines above but complex enough that it should not be implemented twice.

Did you have any suggestion to this? This file isn't object oriented, is a point that make the reuse of code a bit complex. To prevent a duplication of code, maybe the both loop could be moved to Helper class.

provokateurin commented 3 months ago

What you think about put this in a separated PR to don't mix with the context of this PR? I understand that tests are very important but would be best separate the scope to merge this fix more quickly.

Yes, but a simple test for this fix still needs to be added to this fix.

Did you have any suggestion to this? This file isn't object oriented, is a point that make the reuse of code a bit complex. To prevent a duplication of code, maybe the both loop could be moved to Helper class.

Can be done easily by putting all elements into a single array and then looping over them instead of looping over them separately.

github-actions[bot] commented 2 months ago

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)