diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8bd68409b10..d4c8e1a062f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -66,8 +66,8 @@ When working in the `tools/cli` directory: - `make e2e-test` - Run end-to-end tests - `make build` - Build the CLI binary - `make gen-docs` - Generate CLI documentation -- `go generate ./...` - Regenerate generated Go sources. Run this after editing - `tools/spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json`; a unit test - fails if the generated copy is out of date, so a forgotten run cannot pass CI +- `go generate ./...` - Regenerate the Go copy of the legacy long-running operation list. Run + this after editing `tools/spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js`; a + unit test fails if the generated copy is out of date, so a forgotten run cannot pass CI **Note: Please ensure the usage of [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) in PR titles** \ No newline at end of file diff --git a/tools/cli/internal/cli/merge/gen/main.go b/tools/cli/internal/cli/merge/gen/main.go index bcb2773b07a..836d45153d9 100644 --- a/tools/cli/internal/cli/merge/gen/main.go +++ b/tools/cli/internal/cli/merge/gen/main.go @@ -17,23 +17,24 @@ package main import ( - "encoding/json" "fmt" "go/format" "log" "os" + "regexp" "sort" "strings" ) const ( - sourcePath = "../../../../spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json" + sourcePath = "../../../../spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js" outputPath = "legacy_long_running_operations.go" + + arrayStart = "export const legacyLroOperationIds = [" + arrayEnd = "];" ) -type legacyLongRunningOperations struct { - OperationIDs []string `json:"operationIds"` -} +var operationIDLine = regexp.MustCompile(`^ {2}'([a-zA-Z0-9]+)',$`) func main() { if err := run(); err != nil { @@ -47,28 +48,16 @@ func run() error { return fmt.Errorf("reading %s: %w", sourcePath, err) } - var list legacyLongRunningOperations - if err = json.Unmarshal(data, &list); err != nil { - return fmt.Errorf("parsing %s: %w", sourcePath, err) - } - - if len(list.OperationIDs) == 0 { - return fmt.Errorf("%s defines no operationIds", sourcePath) - } - - seen := map[string]struct{}{} - for _, operationID := range list.OperationIDs { - if _, duplicate := seen[operationID]; duplicate { - return fmt.Errorf("%s defines %q more than once", sourcePath, operationID) - } - seen[operationID] = struct{}{} + operationIDs, err := extractOperationIDs(string(data)) + if err != nil { + return err } - sort.Strings(list.OperationIDs) + sort.Strings(operationIDs) var builder strings.Builder builder.WriteString("// Code generated by go generate; DO NOT EDIT.\n") - builder.WriteString("// Source: tools/spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json\n") + builder.WriteString("// Source: tools/spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js\n") builder.WriteString("//\n") builder.WriteString("// Generated by this command:\n") builder.WriteString("//\n") @@ -77,7 +66,7 @@ func run() error { builder.WriteString("// legacyLongRunningOperationIDs is the set of operations returning HTTP 202 that\n") builder.WriteString("// predate IPA-132 and do not follow the long-running operation contract.\n") builder.WriteString("var legacyLongRunningOperationIDs = map[string]struct{}{\n") - for _, operationID := range list.OperationIDs { + for _, operationID := range operationIDs { fmt.Fprintf(&builder, "%q: {},\n", operationID) } builder.WriteString("}\n") @@ -94,3 +83,44 @@ func run() error { return nil } + +// extractOperationIDs reads the operationIds from the JavaScript module, accepting only the +// prettier-enforced format of one single-quoted operationId per line, so that any unexpected +// content fails the generation instead of being silently dropped. +func extractOperationIDs(source string) ([]string, error) { + lines := strings.Split(source, "\n") + + start := -1 + for i, line := range lines { + if line == arrayStart { + start = i + 1 + break + } + } + if start == -1 { + return nil, fmt.Errorf("%s does not declare %q", sourcePath, arrayStart) + } + + seen := map[string]struct{}{} + operationIDs := make([]string, 0, len(lines)) + for _, line := range lines[start:] { + if line == arrayEnd { + if len(operationIDs) == 0 { + return nil, fmt.Errorf("%s declares no operationIds", sourcePath) + } + return operationIDs, nil + } + + match := operationIDLine.FindStringSubmatch(line) + if match == nil { + return nil, fmt.Errorf("unexpected line %q in %s", line, sourcePath) + } + if _, duplicate := seen[match[1]]; duplicate { + return nil, fmt.Errorf("%s declares %q more than once", sourcePath, match[1]) + } + seen[match[1]] = struct{}{} + operationIDs = append(operationIDs, match[1]) + } + + return nil, fmt.Errorf("%s does not terminate the operationIds array with %q", sourcePath, arrayEnd) +} diff --git a/tools/cli/internal/cli/merge/legacy_long_running_operations.go b/tools/cli/internal/cli/merge/legacy_long_running_operations.go index b6e106091c8..a7c6e77167c 100644 --- a/tools/cli/internal/cli/merge/legacy_long_running_operations.go +++ b/tools/cli/internal/cli/merge/legacy_long_running_operations.go @@ -1,5 +1,5 @@ // Code generated by go generate; DO NOT EDIT. -// Source: tools/spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json +// Source: tools/spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js // // Generated by this command: // diff --git a/tools/cli/internal/cli/merge/legacy_long_running_operations_test.go b/tools/cli/internal/cli/merge/legacy_long_running_operations_test.go index ec6b92fb5eb..bb4afb241a6 100644 --- a/tools/cli/internal/cli/merge/legacy_long_running_operations_test.go +++ b/tools/cli/internal/cli/merge/legacy_long_running_operations_test.go @@ -15,8 +15,8 @@ package merge import ( - "encoding/json" "os" + "regexp" "testing" "github.com/stretchr/testify/assert" @@ -24,20 +24,19 @@ import ( ) // TestLegacyLongRunningOperationIDs_MatchSharedList pins the generated Go map to the shared -// list the IPA-132 validation rules consume. On failure, run go generate ./... in tools/cli. +// JavaScript module the IPA-132 validation rules import. On failure, run go generate ./... in +// tools/cli. func TestLegacyLongRunningOperationIDs_MatchSharedList(t *testing.T) { - data, err := os.ReadFile("../../../../spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json") + data, err := os.ReadFile("../../../../spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js") require.NoError(t, err) - var list struct { - OperationIDs []string `json:"operationIds"` - } - require.NoError(t, json.Unmarshal(data, &list)) + matches := regexp.MustCompile(`'([a-zA-Z0-9]+)',`).FindAllStringSubmatch(string(data), -1) + require.NotEmpty(t, matches, "no operationIds found in the shared list") - assert.Len(t, legacyLongRunningOperationIDs, len(list.OperationIDs), + assert.Len(t, legacyLongRunningOperationIDs, len(matches), "the generated map and the shared list differ in size; run go generate ./... in tools/cli") - for _, operationID := range list.OperationIDs { - assert.Contains(t, legacyLongRunningOperationIDs, operationID, + for _, match := range matches { + assert.Contains(t, legacyLongRunningOperationIDs, match[1], "missing from the generated map; run go generate ./... in tools/cli") } } diff --git a/tools/spectral/ipa/__tests__/IPA132Lro202ResponseMustNotIncludeContent.test.js b/tools/spectral/ipa/__tests__/IPA132Lro202ResponseMustNotIncludeContent.test.js new file mode 100644 index 00000000000..066eb8248ab --- /dev/null +++ b/tools/spectral/ipa/__tests__/IPA132Lro202ResponseMustNotIncludeContent.test.js @@ -0,0 +1,271 @@ +import testRule from './__helpers__/testRule'; +import { DiagnosticSeverity } from '@stoplight/types'; + +const ERROR_MESSAGE = + 'The 202 Accepted response must not include a response body schema. The operation handle is conveyed through the Location header.'; + +const lroExtension = { 'x-xgen-long-running-operation': { legacy: false } }; +const legacyLroExtension = { 'x-xgen-long-running-operation': { legacy: true } }; + +const acceptedWithoutContent = { + 202: { + description: 'Accepted', + headers: { + Location: { + schema: { type: 'string', format: 'uri' }, + }, + }, + }, +}; + +// Media types carrying only versioning metadata, without a schema, are allowed +const acceptedWithVersionedContent = { + 202: { + description: 'Accepted', + content: { + 'application/vnd.atlas.2024-08-05+json': { + 'x-xgen-version': '2024-08-05', + }, + }, + }, +}; + +const acceptedWithSchema = { + 202: { + description: 'Accepted', + content: { + 'application/vnd.atlas.2024-08-05+json': { + schema: { + type: 'object', + }, + }, + }, + }, +}; + +testRule('xgen-IPA-132-lro-202-response-must-not-include-content', [ + { + name: 'valid 202 responses without a body schema', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: acceptedWithoutContent, + }, + }, + '/api/atlas/v2/resourceName/{pathParam}': { + delete: { + ...lroExtension, + responses: acceptedWithVersionedContent, + }, + }, + }, + }, + errors: [], + }, + { + name: 'synchronous operations without a 202 response are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + responses: { + 200: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + // Per-service source specs carry no extension: the declared 202 selects the operation + name: 'invalid untagged operation declaring a 202 with a body schema', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + responses: acceptedWithSchema, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-202-response-must-not-include-content', + message: ERROR_MESSAGE, + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'responses', + '202', + 'content', + 'application/vnd.atlas.2024-08-05+json', + ], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'legacy long-running operations are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...legacyLroExtension, + responses: acceptedWithSchema, + }, + }, + }, + }, + errors: [], + }, + { + // On per-service source specs, where the merge step has not marked legacy operations, + // the shared legacy operationId list excludes them + name: 'legacy operations matched by operationId are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName/{pathParam}': { + delete: { + operationId: 'deleteGroupCluster', + responses: acceptedWithSchema, + }, + }, + }, + }, + errors: [], + }, + { + name: 'invalid 202 response with a body schema', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: acceptedWithSchema, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-202-response-must-not-include-content', + message: ERROR_MESSAGE, + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'responses', + '202', + 'content', + 'application/vnd.atlas.2024-08-05+json', + ], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid 202 response with schemas in multiple media types', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + 202: { + content: { + 'application/vnd.atlas.2024-08-05+json': { + schema: { type: 'object' }, + }, + 'application/vnd.atlas.2025-01-01+json': { + schema: { type: 'object' }, + }, + }, + }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-202-response-must-not-include-content', + message: ERROR_MESSAGE, + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'responses', + '202', + 'content', + 'application/vnd.atlas.2024-08-05+json', + ], + severity: DiagnosticSeverity.Warning, + }, + { + code: 'xgen-IPA-132-lro-202-response-must-not-include-content', + message: ERROR_MESSAGE, + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'responses', + '202', + 'content', + 'application/vnd.atlas.2025-01-01+json', + ], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid long-running operations with exceptions', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: acceptedWithSchema, + 'x-xgen-IPA-exception': { + 'xgen-IPA-132-lro-202-response-must-not-include-content': 'reason', + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'compliant long-running operations do not need an exception', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: acceptedWithoutContent, + 'x-xgen-IPA-exception': { + 'xgen-IPA-132-lro-202-response-must-not-include-content': 'reason', + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-202-response-must-not-include-content', + message: 'This component adopts the rule and does not need an exception. Please remove the exception.', + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'x-xgen-IPA-exception', + 'xgen-IPA-132-lro-202-response-must-not-include-content', + ], + severity: DiagnosticSeverity.Warning, + }, + ], + }, +]); diff --git a/tools/spectral/ipa/__tests__/IPA132LroInitiationMustReturn202AcceptedWithLocation.test.js b/tools/spectral/ipa/__tests__/IPA132LroInitiationMustReturn202AcceptedWithLocation.test.js new file mode 100644 index 00000000000..f678ee9565c --- /dev/null +++ b/tools/spectral/ipa/__tests__/IPA132LroInitiationMustReturn202AcceptedWithLocation.test.js @@ -0,0 +1,258 @@ +import testRule from './__helpers__/testRule'; +import { DiagnosticSeverity } from '@stoplight/types'; + +const MISSING_LOCATION_ERROR_MESSAGE = + 'The 202 Accepted response must include a Location header pointing at the Operation resource URI.'; +const WRONG_CASING_ERROR_MESSAGE = "The 202 Accepted response header must be named 'Location', found 'location'."; + +const lroExtension = { 'x-xgen-long-running-operation': { legacy: false } }; +const legacyLroExtension = { 'x-xgen-long-running-operation': { legacy: true } }; + +const acceptedWithLocation = { + responses: { + 202: { + description: 'Accepted', + headers: { + Location: { + schema: { type: 'string', format: 'uri' }, + }, + }, + }, + }, +}; + +const acceptedWithoutLocation = { + responses: { + 202: { + description: 'Accepted', + }, + }, +}; + +testRule('xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', [ + { + name: 'valid long-running operation returning 202 with a Location header', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + ...acceptedWithLocation, + }, + }, + '/api/atlas/v2/resourceName/{pathParam}': { + delete: { + ...lroExtension, + ...acceptedWithLocation, + }, + }, + // Per-service source specs carry no extension: the declared 202 selects the operation + '/api/atlas/v2/otherResourceName': { + post: acceptedWithLocation, + }, + }, + }, + errors: [], + }, + { + name: 'invalid Location header declared with non-canonical casing', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + 202: { + headers: { + location: { + schema: { type: 'string', format: 'uri' }, + }, + }, + }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', + message: WRONG_CASING_ERROR_MESSAGE, + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '202'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'synchronous operations without a 202 response are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + responses: { + 201: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + // The merge step derives the extension from the 202 and removes it otherwise, so a + // hand-authored extension without a 202 does not make an operation long-running + name: 'operations tagged without a 202 response are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + 201: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'legacy long-running operations are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName/{pathParam}': { + delete: { + ...legacyLroExtension, + ...acceptedWithoutLocation, + }, + }, + }, + }, + errors: [], + }, + { + // On per-service source specs, where the merge step has not marked legacy operations, + // the shared legacy operationId list excludes them + name: 'legacy operations matched by operationId are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName/{pathParam}': { + delete: { + operationId: 'deleteGroupCluster', + ...acceptedWithoutLocation, + }, + }, + }, + }, + errors: [], + }, + { + name: 'invalid untagged operation declaring a 202 without a Location header', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...acceptedWithoutLocation, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', + message: MISSING_LOCATION_ERROR_MESSAGE, + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '202'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid 202 response without a Location header', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + ...acceptedWithoutLocation, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', + message: MISSING_LOCATION_ERROR_MESSAGE, + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '202'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'extensions without a legacy marker are validated', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + 'x-xgen-long-running-operation': {}, + ...acceptedWithoutLocation, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', + message: MISSING_LOCATION_ERROR_MESSAGE, + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '202'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid long-running operations with exceptions', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + ...acceptedWithoutLocation, + 'x-xgen-IPA-exception': { + 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location': 'reason', + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'compliant long-running operations do not need an exception', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + ...acceptedWithLocation, + 'x-xgen-IPA-exception': { + 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location': 'reason', + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', + message: 'This component adopts the rule and does not need an exception. Please remove the exception.', + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'x-xgen-IPA-exception', + 'xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location', + ], + severity: DiagnosticSeverity.Warning, + }, + ], + }, +]); diff --git a/tools/spectral/ipa/__tests__/IPA132LroMustNotReturnSuccessStatusCodeOtherThan202.test.js b/tools/spectral/ipa/__tests__/IPA132LroMustNotReturnSuccessStatusCodeOtherThan202.test.js new file mode 100644 index 00000000000..e942ed28f89 --- /dev/null +++ b/tools/spectral/ipa/__tests__/IPA132LroMustNotReturnSuccessStatusCodeOtherThan202.test.js @@ -0,0 +1,248 @@ +import testRule from './__helpers__/testRule'; +import { DiagnosticSeverity } from '@stoplight/types'; + +const lroExtension = { 'x-xgen-long-running-operation': { legacy: false } }; +const legacyLroExtension = { 'x-xgen-long-running-operation': { legacy: true } }; + +const accepted = { + 202: { + description: 'Accepted', + headers: { + Location: { + schema: { type: 'string', format: 'uri' }, + }, + }, + }, +}; + +testRule('xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', [ + { + name: 'valid long-running operation returning only 202', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + ...accepted, + 400: {}, + 404: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'synchronous operations without a 202 response are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + responses: { + 200: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + // Per-service source specs carry no extension: the declared 202 selects the operation + name: 'invalid untagged operation declaring a 202 alongside another success status code', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + responses: { + 200: {}, + 202: {}, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + message: + 'The long-running operation must not return the 200 success status code. 202 Accepted is the only success response of a long-running operation.', + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '200'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'legacy long-running operations are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...legacyLroExtension, + responses: { + 200: {}, + 202: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + // On per-service source specs, where the merge step has not marked legacy operations, + // the shared legacy operationId list excludes them + name: 'legacy operations matched by operationId are ignored', + document: { + paths: { + '/api/atlas/v2/resourceName/{pathParam}': { + patch: { + operationId: 'updateGroupUserSecurity', + responses: { + 200: {}, + 202: {}, + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'invalid long-running operation with an additional success status code', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + 200: {}, + ...accepted, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + message: + 'The long-running operation must not return the 200 success status code. 202 Accepted is the only success response of a long-running operation.', + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '200'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid long-running operation with a wildcard success status code', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + ...accepted, + '2XX': {}, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + message: + 'The long-running operation must not return the 2XX success status code. 202 Accepted is the only success response of a long-running operation.', + path: ['paths', '/api/atlas/v2/resourceName', 'post', 'responses', '2XX'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid long-running operation with multiple additional success status codes', + document: { + paths: { + '/api/atlas/v2/resourceName': { + put: { + ...lroExtension, + responses: { + 200: {}, + ...accepted, + 204: {}, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + message: + 'The long-running operation must not return the 200 success status code. 202 Accepted is the only success response of a long-running operation.', + path: ['paths', '/api/atlas/v2/resourceName', 'put', 'responses', '200'], + severity: DiagnosticSeverity.Warning, + }, + { + code: 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + message: + 'The long-running operation must not return the 204 success status code. 202 Accepted is the only success response of a long-running operation.', + path: ['paths', '/api/atlas/v2/resourceName', 'put', 'responses', '204'], + severity: DiagnosticSeverity.Warning, + }, + ], + }, + { + name: 'invalid long-running operations with exceptions', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + 200: {}, + ...accepted, + }, + 'x-xgen-IPA-exception': { + 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202': 'reason', + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'compliant long-running operations do not need an exception', + document: { + paths: { + '/api/atlas/v2/resourceName': { + post: { + ...lroExtension, + responses: { + ...accepted, + }, + 'x-xgen-IPA-exception': { + 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202': 'reason', + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + message: 'This component adopts the rule and does not need an exception. Please remove the exception.', + path: [ + 'paths', + '/api/atlas/v2/resourceName', + 'post', + 'x-xgen-IPA-exception', + 'xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202', + ], + severity: DiagnosticSeverity.Warning, + }, + ], + }, +]); diff --git a/tools/spectral/ipa/__tests__/utils/longRunningOperations.test.js b/tools/spectral/ipa/__tests__/utils/longRunningOperations.test.js index d64ab03e1ab..dd7af4d6fc0 100644 --- a/tools/spectral/ipa/__tests__/utils/longRunningOperations.test.js +++ b/tools/spectral/ipa/__tests__/utils/longRunningOperations.test.js @@ -1,10 +1,12 @@ import { describe, expect, it } from '@jest/globals'; import { containsOperationsSegment, + isNonLegacyLongRunningOperation, isExactEnumMatch, isOperationsCollectionPath, isOperationsPath, isSingleOperationPath, + LEGACY_LRO_OPERATION_IDS, operationsSegmentIsLeaf, usesComposition, } from '../../rulesets/functions/utils/longRunningOperations'; @@ -117,4 +119,52 @@ describe('tools/spectral/ipa/utils/longRunningOperations.js', () => { expect(usesComposition({ type: 'object', properties: { id: { type: 'string' } } })).toBe(false); }); }); + + describe('isNonLegacyLongRunningOperation', () => { + it('selects operations declaring a 202 response, with or without the extension', () => { + expect(isNonLegacyLongRunningOperation({ operationId: 'createGroupThing', responses: { 202: {} } })).toBe(true); + expect( + isNonLegacyLongRunningOperation({ + operationId: 'createGroupThing', + responses: { 202: {} }, + 'x-xgen-long-running-operation': { legacy: false }, + }) + ).toBe(true); + }); + + it('ignores operations without a 202 response, even when the extension is present', () => { + expect(isNonLegacyLongRunningOperation({ operationId: 'createGroupThing', responses: { 201: {} } })).toBe(false); + expect( + isNonLegacyLongRunningOperation({ + operationId: 'createGroupThing', + responses: { 201: {} }, + 'x-xgen-long-running-operation': { legacy: false }, + }) + ).toBe(false); + expect(isNonLegacyLongRunningOperation({ operationId: 'createGroupThing' })).toBe(false); + }); + + it('excludes operations marked legacy by the merge step', () => { + expect( + isNonLegacyLongRunningOperation({ + operationId: 'createGroupThing', + responses: { 202: {} }, + 'x-xgen-long-running-operation': { legacy: true }, + }) + ).toBe(false); + }); + + it('excludes operations matched by the shared legacy operationId list', () => { + expect(isNonLegacyLongRunningOperation({ operationId: 'deleteGroupCluster', responses: { 202: {} } })).toBe( + false + ); + }); + }); + + describe('LEGACY_LRO_OPERATION_IDS', () => { + it('is loaded from the shared legacy operation list', () => { + expect(LEGACY_LRO_OPERATION_IDS.size).toBeGreaterThan(0); + expect(LEGACY_LRO_OPERATION_IDS.has('deleteGroupCluster')).toBe(true); + }); + }); }); diff --git a/tools/spectral/ipa/rulesets/IPA-132.yaml b/tools/spectral/ipa/rulesets/IPA-132.yaml index a328d2fd9df..584a5f736ff 100644 --- a/tools/spectral/ipa/rulesets/IPA-132.yaml +++ b/tools/spectral/ipa/rulesets/IPA-132.yaml @@ -15,6 +15,9 @@ functions: - IPA132GetMethodMustNotBeLro - IPA132ListMethodMustNotBeLro - IPA132OperationsEndpointMustNotDefineCustomMethods + - IPA132LroInitiationMustReturn202AcceptedWithLocation + - IPA132LroMustNotReturnSuccessStatusCodeOtherThan202 + - IPA132Lro202ResponseMustNotIncludeContent aliases: OperationResponseSchema: @@ -326,7 +329,6 @@ rules: given: '#GetOperationObject' then: function: IPA132ListMethodMustNotBeLro - xgen-IPA-132-operations-endpoint-must-not-define-custom-methods: description: | Operations endpoints must not define custom methods. Control actions require mutating @@ -348,3 +350,73 @@ rules: then: field: '@key' function: IPA132OperationsEndpointMustNotDefineCustomMethods + + xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location: + description: | + A method that starts a long-running operation must return 202 Accepted with a Location + header pointing at the Operation resource URI that clients poll for status. + + ##### Implementation details + Rule checks for the following conditions: + + - Applies to mutating methods that declare a 202 response; the + `x-xgen-long-running-operation` extension is derived from the 202 during the merge + step, so the rule behaves identically on merged and per-service specs + - Legacy operations that predate IPA-132 are excluded from validation, whether marked + `legacy: true` during the merge step or matched by the shared legacy operationId list + - The 202 response must declare a `Location` header, named with that exact casing for + consistency across the spec + - Operations with `x-xgen-IPA-exception` for this rule are excluded from validation + + message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location' + severity: warn + given: $.paths[*][post,put,patch,delete] + then: + function: IPA132LroInitiationMustReturn202AcceptedWithLocation + + xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202: + description: | + A method that starts a long-running operation must not return anything other than 202 + Accepted as a success status code. + + ##### Implementation details + Rule checks for the following conditions: + + - Applies to mutating methods that declare a 202 response; the + `x-xgen-long-running-operation` extension is derived from the 202 during the merge + step, so the rule behaves identically on merged and per-service specs + - Legacy operations that predate IPA-132 are excluded from validation, whether marked + `legacy: true` during the merge step or matched by the shared legacy operationId list + - The operation must not declare any 2xx response other than 202, including wildcard + response codes such as `2XX` + - Operations with `x-xgen-IPA-exception` for this rule are excluded from validation + + message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202' + severity: warn + given: $.paths[*][post,put,patch,delete] + then: + function: IPA132LroMustNotReturnSuccessStatusCodeOtherThan202 + + xgen-IPA-132-lro-202-response-must-not-include-content: + description: | + A long-running operation must not return a body with 202 Accepted. The operation handle is + conveyed through the Location header. + + ##### Implementation details + Rule checks for the following conditions: + + - Applies to mutating methods that declare a 202 response; the + `x-xgen-long-running-operation` extension is derived from the 202 during the merge + step, so the rule behaves identically on merged and per-service specs + - Legacy operations that predate IPA-132 are excluded from validation, whether marked + `legacy: true` during the merge step or matched by the shared legacy operationId list + - No media type of the 202 response may declare a schema + - Media types carrying only versioning metadata, such as `x-xgen-version`, without a + schema, are allowed + - Operations with `x-xgen-IPA-exception` for this rule are excluded from validation + + message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-132-lro-202-response-must-not-include-content' + severity: warn + given: $.paths[*][post,put,patch,delete] + then: + function: IPA132Lro202ResponseMustNotIncludeContent diff --git a/tools/spectral/ipa/rulesets/README.md b/tools/spectral/ipa/rulesets/README.md index 3053dea3fb5..8a94a67038a 100644 --- a/tools/spectral/ipa/rulesets/README.md +++ b/tools/spectral/ipa/rulesets/README.md @@ -1630,6 +1630,61 @@ Rule checks for the following conditions: violation - Paths with `x-xgen-IPA-exception` for this rule are excluded from validation +#### xgen-IPA-132-lro-initiation-must-return-202-accepted-with-location + + ![warn](https://img.shields.io/badge/warning-yellow) +A method that starts a long-running operation must return 202 Accepted with a Location +header pointing at the Operation resource URI that clients poll for status. + +##### Implementation details +Rule checks for the following conditions: + + - Applies to mutating methods that declare a 202 response; the + `x-xgen-long-running-operation` extension is derived from the 202 during the merge + step, so the rule behaves identically on merged and per-service specs + - Legacy operations that predate IPA-132 are excluded from validation, whether marked + `legacy: true` during the merge step or matched by the shared legacy operationId list + - The 202 response must declare a `Location` header, named with that exact casing for + consistency across the spec + - Operations with `x-xgen-IPA-exception` for this rule are excluded from validation + +#### xgen-IPA-132-lro-must-not-return-success-status-code-other-than-202 + + ![warn](https://img.shields.io/badge/warning-yellow) +A method that starts a long-running operation must not return anything other than 202 +Accepted as a success status code. + +##### Implementation details +Rule checks for the following conditions: + + - Applies to mutating methods that declare a 202 response; the + `x-xgen-long-running-operation` extension is derived from the 202 during the merge + step, so the rule behaves identically on merged and per-service specs + - Legacy operations that predate IPA-132 are excluded from validation, whether marked + `legacy: true` during the merge step or matched by the shared legacy operationId list + - The operation must not declare any 2xx response other than 202, including wildcard + response codes such as `2XX` + - Operations with `x-xgen-IPA-exception` for this rule are excluded from validation + +#### xgen-IPA-132-lro-202-response-must-not-include-content + + ![warn](https://img.shields.io/badge/warning-yellow) +A long-running operation must not return a body with 202 Accepted. The operation handle is +conveyed through the Location header. + +##### Implementation details +Rule checks for the following conditions: + + - Applies to mutating methods that declare a 202 response; the + `x-xgen-long-running-operation` extension is derived from the 202 during the merge + step, so the rule behaves identically on merged and per-service specs + - Legacy operations that predate IPA-132 are excluded from validation, whether marked + `legacy: true` during the merge step or matched by the shared legacy operationId list + - No media type of the 202 response may declare a schema + - Media types carrying only versioning metadata, such as `x-xgen-version`, without a + schema, are allowed + - Operations with `x-xgen-IPA-exception` for this rule are excluded from validation + diff --git a/tools/spectral/ipa/rulesets/functions/IPA132Lro202ResponseMustNotIncludeContent.js b/tools/spectral/ipa/rulesets/functions/IPA132Lro202ResponseMustNotIncludeContent.js new file mode 100644 index 00000000000..06389720ac4 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/IPA132Lro202ResponseMustNotIncludeContent.js @@ -0,0 +1,44 @@ +import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +import { isNonLegacyLongRunningOperation } from './utils/longRunningOperations.js'; + +const ERROR_MESSAGE = + 'The 202 Accepted response must not include a response body schema. The operation handle is conveyed through the Location header.'; + +/** + * Checks that the 202 Accepted response of a long-running operation defined by IPA-132 does not + * include a response body. Media types carrying only versioning metadata, without a schema, are + * allowed. + * + * @param {object} input - The operation object of a mutating method + * @param {object} _ - Unused + * @param {object} context - The context object containing the path, documentInventory and rule + */ +export default (input, _, { path, rule }) => { + const ruleName = rule.name; + + if (!isNonLegacyLongRunningOperation(input)) { + return; + } + + const errors = checkViolationsAndReturnErrors(input, path, ruleName); + return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); +}; + +function checkViolationsAndReturnErrors(operation, path, ruleName) { + try { + const acceptedResponse = operation.responses?.['202']; + if (!acceptedResponse || !acceptedResponse.content) { + return []; + } + + const errors = []; + for (const [mediaType, mediaTypeObject] of Object.entries(acceptedResponse.content)) { + if (mediaTypeObject && mediaTypeObject.schema) { + errors.push({ path: [...path, 'responses', '202', 'content', mediaType], message: ERROR_MESSAGE }); + } + } + return errors; + } catch (e) { + return handleInternalError(ruleName, path, e); + } +} diff --git a/tools/spectral/ipa/rulesets/functions/IPA132LroInitiationMustReturn202AcceptedWithLocation.js b/tools/spectral/ipa/rulesets/functions/IPA132LroInitiationMustReturn202AcceptedWithLocation.js new file mode 100644 index 00000000000..c990326bc2d --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/IPA132LroInitiationMustReturn202AcceptedWithLocation.js @@ -0,0 +1,43 @@ +import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +import { isNonLegacyLongRunningOperation } from './utils/longRunningOperations.js'; + +const MISSING_LOCATION_ERROR_MESSAGE = + 'The 202 Accepted response must include a Location header pointing at the Operation resource URI.'; +const LOCATION_HEADER = 'Location'; +const wrongCasingErrorMessage = (headerName) => + `The 202 Accepted response header must be named '${LOCATION_HEADER}', found '${headerName}'.`; + +/** + * Checks that a method starting a long-running operation defined by IPA-132 returns 202 Accepted + * with a Location header pointing at the Operation resource URI. + * + * @param {object} input - The operation object of a mutating method + * @param {object} _ - Unused + * @param {object} context - The context object containing the path, documentInventory and rule + */ +export default (input, _, { path, rule }) => { + const ruleName = rule.name; + + if (!isNonLegacyLongRunningOperation(input)) { + return; + } + + const errors = checkViolationsAndReturnErrors(input, path, ruleName); + return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); +}; + +function checkViolationsAndReturnErrors(operation, path, ruleName) { + try { + const headers = operation.responses['202'].headers; + if (headers?.[LOCATION_HEADER] !== undefined) { + return []; + } + // HTTP treats header names as case-insensitive, but the spec standardizes on the canonical + // casing, so a case variant is reported as a casing violation rather than a missing header + const caseVariant = headers && Object.keys(headers).find((header) => header.toLowerCase() === 'location'); + const message = caseVariant ? wrongCasingErrorMessage(caseVariant) : MISSING_LOCATION_ERROR_MESSAGE; + return [{ path: [...path, 'responses', '202'], message }]; + } catch (e) { + return handleInternalError(ruleName, path, e); + } +} diff --git a/tools/spectral/ipa/rulesets/functions/IPA132LroMustNotReturnSuccessStatusCodeOtherThan202.js b/tools/spectral/ipa/rulesets/functions/IPA132LroMustNotReturnSuccessStatusCodeOtherThan202.js new file mode 100644 index 00000000000..743a8606df3 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/IPA132LroMustNotReturnSuccessStatusCodeOtherThan202.js @@ -0,0 +1,43 @@ +import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +import { isNonLegacyLongRunningOperation } from './utils/longRunningOperations.js'; + +/** + * Checks that a method starting a long-running operation defined by IPA-132 does not advertise + * any success status code other than 202 Accepted. + * + * @param {object} input - The operation object of a mutating method + * @param {object} _ - Unused + * @param {object} context - The context object containing the path, documentInventory and rule + */ +export default (input, _, { path, rule }) => { + const ruleName = rule.name; + + if (!isNonLegacyLongRunningOperation(input)) { + return; + } + + const errors = checkViolationsAndReturnErrors(input, path, ruleName); + return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); +}; + +function checkViolationsAndReturnErrors(operation, path, ruleName) { + try { + const responses = operation.responses; + if (!responses) { + return []; + } + + const errors = []; + for (const responseCode of Object.keys(responses)) { + if (responseCode.startsWith('2') && responseCode !== '202') { + errors.push({ + path: [...path, 'responses', responseCode], + message: `The long-running operation must not return the ${responseCode} success status code. 202 Accepted is the only success response of a long-running operation.`, + }); + } + } + return errors; + } catch (e) { + return handleInternalError(ruleName, path, e); + } +} diff --git a/tools/spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json b/tools/spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json deleted file mode 100644 index 5eef4b8df75..00000000000 --- a/tools/spectral/ipa/rulesets/functions/utils/legacyLongRunningOperations.json +++ /dev/null @@ -1,22 +0,0 @@ -{ - "description": "Operations returning HTTP 202 that predate IPA-132 and do not follow the long-running operation contract. Single source of truth shared by the foascli merge tagger (tools/cli/internal/cli/merge, via go generate) and the IPA-132 validation rules.", - "operationIds": [ - "acceptGroupStreamVpcPeeringConnection", - "createGroupClusterIndexRollingIndex", - "createGroupCustomDbRoleRole", - "createGroupEncryptionAtRestPrivateEndpoint", - "createOrgBillingCostExplorerUsageProcess", - "cutoverGroupLiveMigration", - "deleteGroupCluster", - "deleteGroupClusterOverloadSimulation", - "deleteGroupPeer", - "deleteGroupStreamConnection", - "deleteGroupStreamPrivateLinkConnection", - "deleteGroupStreamVpcPeeringConnection", - "deleteGroupStreamWorkspace", - "deleteGroupUserSecurityLdapUserToDnMapping", - "disableGroupUserSecurityCustomerX509", - "rejectGroupStreamVpcPeeringConnection", - "updateGroupUserSecurity" - ] -} diff --git a/tools/spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js b/tools/spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js new file mode 100644 index 00000000000..8cbaf95da6a --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/legacyLroOperationIds.js @@ -0,0 +1,25 @@ +/** + * The operations returning HTTP 202 that predate IPA-132 and do not follow the long-running + * operation contract. This module is the single source of truth: the IPA-132 rules import it + * directly, and the foascli merge tagger compiles a Go copy generated from it. After editing, + * run go generate ./... in tools/cli to update the Go copy. + */ +export const legacyLroOperationIds = [ + 'acceptGroupStreamVpcPeeringConnection', + 'createGroupClusterIndexRollingIndex', + 'createGroupCustomDbRoleRole', + 'createGroupEncryptionAtRestPrivateEndpoint', + 'createOrgBillingCostExplorerUsageProcess', + 'cutoverGroupLiveMigration', + 'deleteGroupCluster', + 'deleteGroupClusterOverloadSimulation', + 'deleteGroupPeer', + 'deleteGroupStreamConnection', + 'deleteGroupStreamPrivateLinkConnection', + 'deleteGroupStreamVpcPeeringConnection', + 'deleteGroupStreamWorkspace', + 'deleteGroupUserSecurityLdapUserToDnMapping', + 'disableGroupUserSecurityCustomerX509', + 'rejectGroupStreamVpcPeeringConnection', + 'updateGroupUserSecurity', +]; diff --git a/tools/spectral/ipa/rulesets/functions/utils/longRunningOperations.js b/tools/spectral/ipa/rulesets/functions/utils/longRunningOperations.js index 9dfd2e4e3e7..25c957c74f8 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/longRunningOperations.js +++ b/tools/spectral/ipa/rulesets/functions/utils/longRunningOperations.js @@ -1,7 +1,15 @@ import { isCustomMethodIdentifier, isPathParam, removePrefix } from './resourceEvaluation.js'; +import { legacyLroOperationIds } from './legacyLroOperationIds.js'; export const OPERATIONS_SEGMENT = 'operations'; +/** + * The operations returning HTTP 202 that predate IPA-132 and do not follow the long-running + * operation contract. The merge step marks them `legacy: true` in the federated spec; this list + * excludes them on specs the merge step has not processed, such as the per-service source specs. + */ +export const LEGACY_LRO_OPERATION_IDS = new Set(legacyLroOperationIds); + /** * Splits a path into its resource identifier segments, ignoring the standard path prefix, so that * the segments describe the resource hierarchy only. @@ -103,6 +111,31 @@ export function operationsSegmentIsLeaf(path) { export const OPERATION_RESPONSE_SCHEMA_NAME = 'OperationResponse'; +export const LRO_EXTENSION = 'x-xgen-long-running-operation'; + +/** + * Checks if an operation is a long-running operation that is in scope for the IPA-132 contract. + * This says nothing about whether the operation already follows the contract, only that the rules + * are expected to hold for it. A long-running operation declares a 202 response: the + * x-xgen-long-running-operation extension is derived from the 202 during the merge step, so + * selecting on the declared 202 gives the same result on the federated spec while also working on + * specs the merge step has not processed, such as the per-service source specs. Legacy operations + * predate IPA-132 and are excluded, whether marked `legacy: true` by the merge step or matched by + * the legacy operationId list. + * + * @param {object} operation the operation object to evaluate + * @returns {boolean} true if the operation is a long-running operation that is not legacy + */ +export function isNonLegacyLongRunningOperation(operation) { + if (!operation.responses?.['202']) { + return false; + } + if (operation[LRO_EXTENSION]?.legacy === true) { + return false; + } + return !LEGACY_LRO_OPERATION_IDS.has(operation.operationId); +} + /** * Checks if the enum values defined in a schema match an expected set of values exactly, in any * order.