Skip to content

feat(openapi-tooling): validate schema in jest tests #25538

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

Merged

Conversation

aramissennyeydd
Copy link
Contributor

@aramissennyeydd aramissennyeydd commented Jul 7, 2024

Hey, I just made a Pull Request!

Optic was recently acquired by Atlassian (congrats to them! 🎉 ). We're on an older version of Optic CLI that is missing deep object query parameter support, and it looks like it won't be added anytime soon in the upstream repo (last commit was ~3 months ago). I took a stab at implementing the proxy validation myself, to pretty good results. I may take a look at rewriting the breaking changes workflow myself as well later this year.

Most of this code is tests 😅 .

Features:

  1. Instant validation during jest tests. This can now be run directly as you are writing jest tests, so you don't need to run a second command. I debated having this be a toggle on option to keep the same repo schema openapi test workflow, but I don't love it as is.
  2. Better query parameter validation support. The search plugin uses qs which has issues with Optic's validation because it creates non-standard array syntax, either ${name}[${index}] or ${name}[]. We now get control over that syntax support. I also improved the deepObject and object form explode validation.

Missing:

  1. --fix, there's no easy way to do that with this new implementation. I think Github Copilot can fill this gap though, and more expressly match the TS definitions.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@aramissennyeydd aramissennyeydd requested review from a team and backstage-service as code owners July 7, 2024 20:54
@aramissennyeydd aramissennyeydd requested a review from camilaibs July 7, 2024 20:54
@github-actions github-actions bot added area:catalog Related to the Catalog Project Area search area:discoverability Related to the Discoverability Project Area labels Jul 7, 2024
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Jul 7, 2024

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @backstage/plugin-catalog-backend

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/backend-openapi-utils packages/backend-openapi-utils minor v0.1.19-next.0
@backstage/plugin-catalog-backend plugins/catalog-backend none v1.26.2-next.1
@backstage/plugin-search-backend plugins/search-backend patch v1.5.18-next.1

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Loving the new simplified setup without running through a separate process 🎉

There's a bit more custom validation code than I would've expected, but trusting that it's needed and we can ofc refactor bits in future iterations.

Approved basically as is, but let me know when you want to merge

@aramissennyeydd aramissennyeydd force-pushed the openapi-tooling/custom-proxy-server branch from 11acf0e to 5289336 Compare September 24, 2024 23:28
@aramissennyeydd
Copy link
Contributor Author

@Rugvip Did something change with the E2E/CI test checks? I've never seen the unauthorized error before 😅

@Rugvip
Copy link
Member

Rugvip commented Sep 25, 2024

Huh, that is new, probably something on the docker side 🤔

Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
…essages

Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
…plugin

Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
Signed-off-by: aramissennyeydd <aramis.sennyey@doordash.com>
@aramissennyeydd aramissennyeydd force-pushed the openapi-tooling/custom-proxy-server branch from a3e3e2e to 7263f92 Compare October 7, 2024 14:07
@aramissennyeydd
Copy link
Contributor Author

@Rugvip Are we ok with merging this in this week?

Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Yep! 👍

Gonna :shipit: after the small cleanup

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
@Rugvip Rugvip merged commit 6732d1b into backstage:master Oct 8, 2024
29 checks passed
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.32.0 release, scheduled for Tue, 15 Oct 2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area area:discoverability Related to the Discoverability Project Area area:search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants