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

Use $ref to definitions in oneOf for Parameter locations #3258

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

handrews
Copy link
Member

I'd like to use a JSON Merge Patch (RFC 7386) document to add keywords for the OAS compliance parser project.

JSON Merge Patch does not handle arrays well, so moving these to their own definitions and having the "oneOf" directly in the "Parameter" definition (avoiding duplicate "description" fields on the same instance location) allows merge-patch to work where I need it.

Otherwise, JSON Patch (RFC 6902) will be needed, which is less intuitive.

The 3.1 schema already uses "$defs" here.

@darrelmiller darrelmiller self-requested a review May 11, 2023 15:35
@MikeRalphson
Copy link
Member

Just to be clear, you're wanting to patch the current OAS schema document in order not to have redundancy in the compliance parser chain?

@Saud96525 Saud96525 linked an issue May 30, 2023 that may be closed by this pull request
@handrews
Copy link
Member Author

@MikeRalphson sorry I did not notice your question when you asked it! It's not so much avoiding redundancy as it is making the tools more user-friendly. I'm perfectly comfortable with JSON Patch (RFC 6902), but it is not incredibly intuitive to read compared to JSON Merge Patch (RFC 7396). For folks wanting to modify oascomply in the future, it would be slightly nicer to have everything be JSON Merge Patch.

@handrews
Copy link
Member Author

@OAI/openapi-maintainers one more approval on this and I can merge it 🙏

anyOf:
- required: [allOf]
- required: [anyOf]
- required: [oneOf]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you meant for this to go in this PR, it seems to not fit with the title/description.

If you did: I don't know discriminator well, but the example Models with Polymorphism Support has discriminator in /components/schemas/Pet without a *Of keyword. But then /components/schemas/Pet is never used in the example in a way that would invoke discriminator, so that's confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@notEthan oops- looks like I need to rebase this (there was a whole thing where this got added and then removed), doing so momentarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's rebased but seems to be the same as what I commented on

Copy link
Member Author

Choose a reason for hiding this comment

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

wat... oh %$@#! I fixed it locally and forgot to add that commit. I'm still not sure how it got there in the first place. Fixed for real now, sorry about that 🤦

@handrews handrews requested a review from a team as a code owner January 31, 2024 01:01
I'd like to use a JSON Merge Patch (RFC 7386) document to add
keywords for the OAS compliance parser project.

JSON Merge Patch does not handle arrays well, so moving these to
their own definitions and having the "oneOf" directly in the
"Parameter" definition (avoiding duplicate "description" fields
on the same instance location) allows merge-patch to work where
I need it.

Otherwise, JSON Patch (RFC 6902) will be needed, which is less
intuitive.

The 3.1 schema already uses "$defs" here.
@earth2marsh earth2marsh merged commit b4189b6 into OAI:main Feb 1, 2024
2 checks passed
@handrews handrews deleted the schema-30-use-defs branch March 5, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tbarn:coc-enforcement-doc
6 participants