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(openapi): improvements to circular ref detection, validation errors #1189

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

erunion
Copy link
Member

@erunion erunion commented Mar 5, 2025

🧰 Changes

Over the past couple weeks I've been rewriting our OpenAPI parser, @readme/openapi-parser, to support a new error leveling system -- you can read up on details on this in readmeio/oas#943. This PR pulls in the fruits of my labor there and across oas and oas-normalize.

  • Upgrades oas-normalize to pull in our new parser engine and error handling. The error leveling work is not being used within rdme but for things like specification validation that our parser does rdme will now be able to surface more than one of these errors at a time. 🎉
    • Support for Node 18 has also been removed.
  • Upgrades oas and @readme/oas-examples to pull in the removal of Node 18 support.

All told this overhaul is mostly a drop-in replacement with the exception that instead of SyntaxError and MissingPointerError exceptions being thrown from the parser for some invalid API definition cases it will now always be a ValidationError.

@erunion erunion added enhancement New feature or request command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands labels Mar 5, 2025
@@ -93,8 +93,13 @@ additionalProperties:
· #/components/schemas/ParameterizedHeader/properties/parameters/additionalProperties

circularRefs:
· #/components/schemas/BodyPart/properties/parent
Copy link
Member Author

Choose a reason for hiding this comment

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

These circular refs weren't being picked up before but @apidevtools/json-schema-ref-parser is better at detecting them now with an onCircular hook I submitted to them. APIDevTools/json-schema-ref-parser#366

@@ -172,10 +172,14 @@ ADDITIONAL PROPERTY must NOT have additional properties
29 | "summary": "Finds Pets by status",]
`;

exports[`rdme openapi validate > error handling > should throw an error if an invalid OpenAPI 3.0 definition is supplied 1`] = `[MissingPointerError: Token "Error" does not exist.]`;
exports[`rdme openapi validate > error handling > should throw an error if an invalid OpenAPI 3.0 definition is supplied 1`] = `
[ValidationError: API definition schema validation failed.
Copy link
Member Author

Choose a reason for hiding this comment

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

Slightly nicer error message here with this header being shown instead of dumping the user out with "Token "Error" does not exist."

@@ -47,7 +47,7 @@
"@oclif/plugin-help": "^6.2.15",
"@oclif/plugin-not-found": "^3.2.28",
"@oclif/plugin-warn-if-update-available": "^3.1.19",
"@readme/better-ajv-errors": "^2.0.0",
"@readme/better-ajv-errors": "^2.3.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

v2.3.2 was already installed but I just bumped it in the package file.

@erunion erunion marked this pull request as ready for review March 5, 2025 19:47
@erunion erunion requested review from kanadgupta, emilyskuo and a team March 5, 2025 19:47
@kanadgupta
Copy link
Member

sorry i think #1188 might have caused merge conflicts 🫠

@kanadgupta kanadgupta added the dependencies Pull requests that update a dependency file label Mar 5, 2025
@kanadgupta kanadgupta changed the title feat: upgrading our OpenAPI parser and associated tooling feat(openapi): improvements to circular ref detection, validation errors Mar 5, 2025
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

just updated the PR title so it's a bit more customer-facing. LGTM!

one quick non-blocking q — should we add a test that illustrates this? 👇🏽

The error leveling work is not being used within rdme but for things like specification validation that our parser does rdme will now be able to surface more than one of these errors at a time. 🎉

@erunion
Copy link
Member Author

erunion commented Mar 5, 2025

one quick non-blocking q — should we add a test that illustrates this? 👇🏽

The error leveling work is not being used within rdme but for things like specification validation that our parser does rdme will now be able to surface more than one of these errors at a time. 🎉

Feel like that's already well tested across oas-normalize and the parser

@kanadgupta kanadgupta merged commit 57b732f into next Mar 5, 2025
10 checks passed
@kanadgupta kanadgupta deleted the feat/upgrade-oas-parser branch March 5, 2025 21:15
kanadgupta pushed a commit that referenced this pull request Mar 5, 2025
# [10.2.0-next.5](v10.2.0-next.4...v10.2.0-next.5) (2025-03-05)

### Features

* **openapi:** improvements to circular ref detection, validation errors ([#1189](#1189)) ([57b732f](57b732f))

[skip ci]
@kanadgupta
Copy link
Member

🎉 This PR is included in version 10.2.0-next.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands dependencies Pull requests that update a dependency file enhancement New feature or request needs-backport-to-v9 released on @next
Projects
None yet
2 participants