-
Notifications
You must be signed in to change notification settings - Fork 227
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,10 +23,11 @@ While this thinking has mostly changed (for the better in our opinion), it does | |
|
|
||
| ## Configuration | ||
|
|
||
| | Option | Type | Description | | ||
| | ---------------- | ------- | ----------------------------------------------------------------------------------------- | | ||
| | severity | string | Possible values: `off`, `warn`, `error`. Default `warn` (in `recommended` configuration). | | ||
| | validateWebhooks | boolean | Determines if responses inside webhooks are validated. Default `false`. | | ||
| | Option | Type | Description | | ||
| | ---------------- | ------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | severity | string | Possible values: `off`, `warn`, `error`. Default `warn` (in `recommended` configuration). | | ||
| | validateWebhooks | boolean | Determines if responses inside webhooks are validated. Default `false`. | | ||
| | excludeMethods | array | List of HTTP methods (case-insensitive) to exclude from 4XX validation. Default: `['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. 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.
Contributor
Author
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. good call. how do you feel about this? rules:
operation-4xx-response:
severity: warn
exclusions:
get:
- 400
head:
- 400
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. I'm not sure I follow what does the exclusion I would read it to mean that if the I think |
||
|
|
||
| An example configuration: | ||
|
|
||
|
|
@@ -44,6 +45,15 @@ rules: | |
| validateWebhooks: true | ||
| ``` | ||
|
|
||
| To exclude additional methods from 4XX validation: | ||
|
|
||
| ```yaml | ||
| rules: | ||
| operation-4xx-response: | ||
| severity: error | ||
| excludeMethods: [get, head, options, trace] | ||
| ``` | ||
|
|
||
| ## Examples | ||
|
|
||
| Given this configuration: | ||
|
|
||
| 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": [], | ||
| }, | ||
| ] | ||
| `); | ||
| }); | ||
| }); |
| 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; | ||||||||||||||||||||||||
|
Comment on lines
+5
to
+10
Contributor
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
What if we simplify this and take excludeMethods straight from the rule options, like other rules do? Also sets the default to [] per the discussion above. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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) { | ||||||||||||||||||||||||
|
Contributor
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. This body is the same as |
||||||||||||||||||||||||
| 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', | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
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.
Since this is the addition of a new feature, it makes more sense to classify it as a minor change.