-
Notifications
You must be signed in to change notification settings - Fork 193
fix: no-invalid-media-type-examples now respects readOnly and writeOnly properties #2412
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
base: main
Are you sure you want to change the base?
fix: no-invalid-media-type-examples now respects readOnly and writeOnly properties #2412
Conversation
…edProperties function
🦋 Changeset detectedLatest commit: f85ee35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tatomyr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for spending your time on this @sakurai-ryo! Left a couple of suggestion/concerns.
Also, I'm not sure how this solution will work with circular references and nested objects (for instance, objects in arrays). Could you check that?
| mediaTypeLocation.pointer.includes('/components/responses') || | ||
| /paths\/[^/]+\/[^/]+\/responses/.test(mediaTypeLocation.pointer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit fragile and hard to read. I suggest using Responses and RequestBody visitors to narrow down the context:
Responses: {
MediaType: {
leave(mediaType, ctx: UserContext) {
// Filter out `writeOnly` props...| return; | ||
| } | ||
|
|
||
| const resolvedSchema = ensureSchemaIsResolved(mediaType.schema!, ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it will still be easier to fix that on the AJV side. We use our own AJV fork, and we can try passing down a new parameter, something like ignoreWriteOnly. Let me think this through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tatomyr you are absolutely right. In AJV it looks better. I try to create a quick fix directly in AJV and it works. I created draft PR: Redocly/ajv#35.
What/Why/How?
Fixed an issue where the
no-invalid-media-type-examplesrule incorrectly reported validation errors/warnings when example/examples properly omitted readOnly properties in request bodies or writeOnly properties in response bodies.ajv does not support
readOnlyandwriteOnlyproperties, and the request and response context is specific to OpenAPI.So this must be handled in the lint rule rather than in ajv.
ajv-validator/ajv#2097
How
This PR introduces context-aware validation.
It modifies the object schema based on whether mediaType examples are request or response context.
Check the
propertiesfield ofmediaType.schemaand remove properties that should not be required in the request or response context from thepropertiesfield and therequiredfield.This PR skips validating cases where mediaType.schema contains complex elements like allOf.
If there are any utility functions that support this case, please let us know.
Reference
Fixes #1416
Testing
Added unit tests.
Screenshots (optional)
Check yourself
Security