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(oas): Improve OAS by adding missing type object to schemas #3177

Merged
merged 2 commits into from Feb 8, 2023

Conversation

asprouse
Copy link
Contributor

@asprouse asprouse commented Feb 3, 2023

This PR improves the OAS by adding type: object to schemas that have properties and no type. While this attribute is not required for a valid spec, omitting it opens the possibilities of non-object input. See https://stackoverflow.com/a/47390723 for a detailed description of this behavior. When generating code or schemas from an OAS with tools like takeshape.io having type attributes is important to get the correct behavior.

@asprouse asprouse requested review from a team as code owners February 3, 2023 18:42
@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

⚠️ No Changeset found

Latest commit: bef761f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@srindom
Copy link
Collaborator

srindom commented Feb 5, 2023

@patrick-medusajs did we already do this with your OAS work or would it make sense to include?

@patrick-medusajs
Copy link
Contributor

@srindom We did similar work but we didn't fix objects in arrays. We actually have a Linear for it - CORE-1024.
This would be a great addition to include. I can review the OAS changes in the JSDoc comments.

@shahednasser How would you like to proceed for merging such changes. Should it target develop? Would you prefer to exclude changes in docs and only merge changes in api/routes?

@shahednasser
Copy link
Member

I don't think this change is necessary for master, so it's fine to do it under develop. I also prefer excluding files under docs from the PR as these are automatically generated when changes are merged into master.

@patrick-medusajs
Copy link
Contributor

@asprouse Would would mind rebasing your branch to point to medusajs:develop and remove docs from the PR?

@asprouse asprouse changed the base branch from master to develop February 6, 2023 16:45
@asprouse
Copy link
Contributor Author

asprouse commented Feb 6, 2023

@asprouse Would would mind rebasing your branch to point to medusajs:develop and remove docs from the PR?

@patrick-medusajs I rebased the PR as re-targeted it to develop. The docs changes were dropped in the rebase.

Copy link
Contributor

@patrick-medusajs patrick-medusajs left a comment

Choose a reason for hiding this comment

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

LGTM

@olivermrbl olivermrbl changed the title Improve OAS by adding missing type object to schemas feat(oas): Improve OAS by adding missing type object to schemas Feb 6, 2023
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM! @asprouse before we merge, can I get you to merge in latest from develop and push?

@kodiakhq kodiakhq bot merged commit f5dced6 into medusajs:develop Feb 8, 2023
@patrick-medusajs
Copy link
Contributor

🎉 @asprouse, thank you for your contribution. Cheers!

adrien2p pushed a commit that referenced this pull request Feb 13, 2023
This PR improves the OAS by adding `type: object` to schemas that have `properties` and no `type`. While this attribute is not required for a valid spec, omitting it opens the possibilities of non-object input. See https://stackoverflow.com/a/47390723 for a detailed description of this behavior. When generating code or schemas from an OAS with tools like takeshape.io having `type` attributes is important to get the correct behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants