-
Notifications
You must be signed in to change notification settings - Fork 225
feat: configurable operation-4xx-response (excludeMethods); add tests… #3028
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
Open
jeremyfiel
wants to merge
1
commit into
Redocly:main
Choose a base branch
from
jeremyfiel:fix/operation-4xx-skip-safe-methods
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| '@redocly/openapi-core': patch | ||
| '@redocly/cli': patch | ||
| --- | ||
|
|
||
| Make the `operation-4xx-response` rule configurable to exclude safe HTTP | ||
| methods by default (get, head, options). This allows projects to avoid | ||
| requiring 4XX responses for read-only operations while keeping the default | ||
| behavior conservative. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
packages/core/src/rules/common/__tests__/operation-4xx-response.exclude.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import { outdent } from 'outdent'; | ||
|
|
||
| import { parseYamlToDocument, replaceSourceWithRef } from '../../../../__tests__/utils.js'; | ||
| import { createConfig } from '../../../config/index.js'; | ||
| import { lintDocument } from '../../../lint.js'; | ||
| import { BaseResolver } from '../../../resolve.js'; | ||
|
|
||
| describe('Oas3 operation-4xx-response (exclude methods)', () => { | ||
| it('should not report for excluded methods by default (GET)', async () => { | ||
| const document = parseYamlToDocument( | ||
| outdent` | ||
| openapi: 3.0.0 | ||
| paths: | ||
| '/test': | ||
| get: | ||
| responses: | ||
| 200: | ||
| description: ok response | ||
| `, | ||
| 'foobar.yaml' | ||
| ); | ||
|
|
||
| const results = await lintDocument({ | ||
| externalRefResolver: new BaseResolver(), | ||
| document, | ||
| config: await createConfig({ rules: { 'operation-4xx-response': 'error' } }), | ||
| }); | ||
|
|
||
| expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(`[]`); | ||
| }); | ||
|
|
||
| it('should report for non-excluded methods (POST) when missing 4xx', async () => { | ||
| const document = parseYamlToDocument( | ||
| outdent` | ||
| openapi: 3.0.0 | ||
| paths: | ||
| '/test': | ||
| post: | ||
| responses: | ||
| 200: | ||
| description: ok response | ||
| `, | ||
| 'foobar.yaml' | ||
| ); | ||
|
|
||
| const results = await lintDocument({ | ||
| externalRefResolver: new BaseResolver(), | ||
| document, | ||
| config: await createConfig({ rules: { 'operation-4xx-response': 'error' } }), | ||
| }); | ||
|
|
||
| expect(replaceSourceWithRef(results)).toMatchInlineSnapshot(` | ||
| [ | ||
| { | ||
| "location": [ | ||
| { | ||
| "pointer": "#/paths/~1test/post/responses", | ||
| "reportOnKey": true, | ||
| "source": "foobar.yaml", | ||
| }, | ||
| ], | ||
| "message": "Operation must have at least one \`4XX\` response.", | ||
| "reference": "https://redocly.com/docs/cli/rules/oas/operation-4xx-response", | ||
| "ruleId": "operation-4xx-response", | ||
| "severity": "error", | ||
| "suggest": [], | ||
| }, | ||
| ] | ||
| `); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,32 +2,55 @@ import type { Oas3Rule, Oas2Rule } from '../../visitors.js'; | |||||
| import type { UserContext } from '../../walk.js'; | ||||||
| import { validateResponseCodes } from '../utils.js'; | ||||||
|
|
||||||
| export const Operation4xxResponse: Oas3Rule | Oas2Rule = ({ validateWebhooks }) => { | ||||||
| export const Operation4xxResponse: Oas3Rule | Oas2Rule = (opts: any = {}) => { | ||||||
| const { validateWebhooks, excludeMethods: rawExcludeMethods } = opts || {}; | ||||||
| const defaultExcluded = ['get', 'head', 'options']; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think I would start with this possibly. Also, even this is a breaking change. |
||||||
| const excludeMethods = Array.isArray(rawExcludeMethods) | ||||||
| ? rawExcludeMethods.map((m: string) => String(m).toLowerCase()) | ||||||
| : defaultExcluded; | ||||||
|
|
||||||
| return { | ||||||
| Paths: { | ||||||
| Responses(responses: Record<string, object>, { report }: UserContext) { | ||||||
| const codes = Object.keys(responses || {}); | ||||||
|
|
||||||
| validateResponseCodes({ | ||||||
| responseCodes: codes, | ||||||
| codeRange: '4XX', | ||||||
| report: report as UserContext['report'], | ||||||
| reference: 'https://redocly.com/docs/cli/rules/oas/operation-4xx-response', | ||||||
| }); | ||||||
| Operation: { | ||||||
| leave(operation: Record<string, any>, { report, key, location }: UserContext) { | ||||||
| const method = String(key).toLowerCase(); | ||||||
| if (excludeMethods.includes(method)) return; | ||||||
|
|
||||||
| const codes = Object.keys((operation.responses as Record<string, object>) || {}); | ||||||
|
|
||||||
| // keep the reported location consistent with previous implementation | ||||||
| const childReport: UserContext['report'] = (problem) => | ||||||
| report({ ...problem, location: location.child(['responses']).key() }); | ||||||
|
|
||||||
| validateResponseCodes({ | ||||||
| responseCodes: codes, | ||||||
| codeRange: '4XX', | ||||||
| report: childReport, | ||||||
| reference: 'https://redocly.com/docs/cli/rules/oas/operation-4xx-response', | ||||||
| }); | ||||||
|
cursor[bot] marked this conversation as resolved.
|
||||||
| }, | ||||||
| }, | ||||||
| }, | ||||||
| WebhooksMap: { | ||||||
| Responses(responses: Record<string, object>, { report }: UserContext) { | ||||||
| if (!validateWebhooks) return; | ||||||
| Operation: { | ||||||
| leave(operation: Record<string, any>, { report, key, location }: UserContext) { | ||||||
| if (!validateWebhooks) return; | ||||||
|
|
||||||
| const method = String(key).toLowerCase(); | ||||||
| if (excludeMethods.includes(method)) return; | ||||||
|
|
||||||
| const codes = Object.keys((operation.responses as Record<string, object>) || {}); | ||||||
|
|
||||||
| const codes = Object.keys(responses || {}); | ||||||
| const childReport: UserContext['report'] = (problem) => | ||||||
| report({ ...problem, location: location.child(['responses']).key() }); | ||||||
|
|
||||||
| validateResponseCodes({ | ||||||
| responseCodes: codes, | ||||||
| codeRange: '4XX', | ||||||
| report: report as UserContext['report'], | ||||||
| reference: 'https://redocly.com/docs/cli/rules/oas/operation-4xx-response', | ||||||
| }); | ||||||
| validateResponseCodes({ | ||||||
| responseCodes: codes, | ||||||
| codeRange: '4XX', | ||||||
| report: childReport, | ||||||
| reference: 'https://redocly.com/docs/cli/rules/oas/operation-4xx-response', | ||||||
| }); | ||||||
| }, | ||||||
| }, | ||||||
| }, | ||||||
| }; | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not sure if this should be the default. GET requests can still have 4xx responses like 401, 403, 429 which could be documented and the documentation could help consumers and code gen.
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.
good call.
i wonder if it makes more sense to target the operations and codes directly. the way it stands, it's fairly noisy but to your point, there are definitely use cases where it's helpful for those other non validation responses
how do you feel about this?