diff --git a/docs/rules.md b/docs/rules.md index 2702a001..47590089 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -1250,7 +1250,7 @@ Per [ARM guidelines](https://github.com/Azure/azure-resource-manager-rpc/blob/ma Please refer to [top-level-resources-list-by-subscription.md](./top-level-resources-list-by-subscription.md) for details. -### trackedExtensionResourcesAreNotAllowed +### TrackedExtensionResourcesAreNotAllowed Extension resources are always considered to be proxy and must not be of the type tracked. @@ -1350,6 +1350,12 @@ Only valid types are allowed for properties. Please refer to [valid-formats.md](./valid-formats.md) for details. +### ValidQueryParametersForPointOperations + +Point operations (GET, PUT, PATCH, DELETE) must not include any query parameters other than api-version. + +Please refer to [valid-query-parameters-for-point-operations.md](./valid-query-parameters-for-point-operations.md) for details. + ### ValidResponseCodeRequired Every operation response must contain a valid code like "200","201","202" or "204" which indicates the operation is succeed and it's not allowed that a response schema just contains a "default" code. diff --git a/docs/tracked-extension-resources-are-not-allowed.md b/docs/tracked-extension-resources-are-not-allowed.md index 5d422206..a8240d25 100644 --- a/docs/tracked-extension-resources-are-not-allowed.md +++ b/docs/tracked-extension-resources-are-not-allowed.md @@ -1,4 +1,4 @@ -# trackedExtensionResourcesAreNotAllowed +# TrackedExtensionResourcesAreNotAllowed ## Category diff --git a/docs/valid-query-parameters-for-point-operations.md b/docs/valid-query-parameters-for-point-operations.md new file mode 100644 index 00000000..54f60c51 --- /dev/null +++ b/docs/valid-query-parameters-for-point-operations.md @@ -0,0 +1,107 @@ +# ValidQueryParametersForPointOperations + +## Category + +ARM Error + +## Applies to + +ARM OpenAPI(swagger) specs + +## Related ARM Guideline Code + +- RPC-Uri-V1-13 + +## Description + +Point operations (GET, PUT, PATCH, DELETE) must not include any query parameters other than api-version. + +## How to fix the violation + +Remove all query params other than api-version for point operations (GET, PUT, PATCH, DELETE). + +## Good Examples + +```json + "Microsoft.Music/Songs": { + "get": { + "operationId": "Foo_Get", + "description": "Test Description", + "parameters": [ + { + "name": "api-version", + "in": "query" + }, + ], + }, + "put": { + "operationId": "Foo_Update", + "description": "Test Description", + "parameters": [ + { + "name": "api-version", + "in": "query" + }, + ], + }, + "patch": { + "operationId": "Foo_Update", + "description": "Test Description", + "parameters": [ + { + "name": "api-version", + "in": "query" + }, + ], + }, + "delete": { + "operationId": "Foo_Update", + "description": "Test Description", + "parameters": [ + { + "name": "api-version", + "in": "query" + }, + ], + }, + }, +``` + +## Bad Examples + +```json + "Microsoft.Music/Songs": { + "get": { + "operationId": "Foo_get", + "description": "Test Description", + "parameters": [ + { + "name": "name", + "in": "query", + "required": false, + "type": "string", + }, + ], + }, + "put": { + "operationId": "Foo_Create", + "description": "Test Description", + "parameters": [ + { + "name": "$filter", + "in": "query" + }, + ], + }, + "patch": { + "operationId": "Foo_Update", + "description": "Test Description", + "parameters": [ + { + "name": "name", + "in": "query" + }, + ], + }, + }, +``` \ No newline at end of file diff --git a/packages/rulesets/generated/spectral/az-arm.js b/packages/rulesets/generated/spectral/az-arm.js index b384155f..e37a18ca 100644 --- a/packages/rulesets/generated/spectral/az-arm.js +++ b/packages/rulesets/generated/spectral/az-arm.js @@ -385,6 +385,18 @@ const providerAndNamespace = "/providers/[^/]+"; const resourceTypeAndResourceName = "(?:/\\w+/default|/\\w+/{[^/]+})"; const queryParam = "(?:\\?\\w+)"; const resourcePathRegEx = new RegExp(`${providerAndNamespace}${resourceTypeAndResourceName}+${queryParam}?$`, "gi"); +function isPointOperation(path) { + const index = path.lastIndexOf("/providers/"); + if (index === -1) { + return false; + } + const lastProvider = path.substr(index); + const matches = lastProvider.match(resourcePathRegEx); + if (matches) { + return true; + } + return false; +} function getResourcesPathHierarchyBasedOnResourceType(path) { const index = path.lastIndexOf("/providers/"); if (index === -1) { @@ -3013,6 +3025,37 @@ const trackedResourceTagsPropertyInRequest = (pathItem, _opts, paths) => { return errors; }; +const validQueryParametersForPointOperations = (pathItem, _opts, ctx) => { + if (pathItem === null || typeof pathItem !== "object") { + return []; + } + const path = ctx.path || []; + const uris = Object.keys(pathItem); + if (uris.length < 1) { + return []; + } + const pointOperations = new Set(["get", "put", "patch", "delete"]); + const errors = []; + for (const uri of uris) { + if (isPointOperation(uri)) { + const verbs = Object.keys(pathItem[uri]); + for (const verb of verbs) { + if (pointOperations.has(verb)) { + const params = pathItem[uri][verb]["parameters"]; + const queryParams = params === null || params === void 0 ? void 0 : params.filter((param) => param.in === "query" && param.name !== "api-version"); + queryParams === null || queryParams === void 0 ? void 0 : queryParams.map((param) => { + errors.push({ + message: `Query parameter ${param.name} should be removed. Point operation '${verb}' MUST not have query parameters other than api-version.`, + path: [path, uri, verb, "parameters"], + }); + }); + } + } + } + } + return errors; +}; + const validatePatchBodyParamProperties = createRulesetFunction({ input: null, options: { @@ -3917,6 +3960,19 @@ const ruleset = { function: trackedExtensionResourcesAreNotAllowed, }, }, + ValidQueryParametersForPointOperations: { + rpcGuidelineCode: "RPC-Uri-V1-13", + description: "Point operations (GET, PUT, PATCH, DELETE) must not include any query parameters other than api-version.", + message: "{{error}}", + stagingOnly: true, + severity: "error", + resolved: true, + formats: [oas2], + given: "$[paths,'x-ms-paths']", + then: { + function: validQueryParametersForPointOperations, + }, + }, SystemDataDefinitionsCommonTypes: { rpcGuidelineCode: "RPC-SystemData-V1-01, RPC-SystemData-V1-02", description: "System data references must utilize common types.", diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index 80291ac6..0f11042e 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -52,6 +52,7 @@ import { tagsAreNotAllowedForProxyResources } from "./functions/tags-are-not-all import { tenantLevelAPIsNotAllowed } from "./functions/tenant-level-apis-not-allowed" import { trackedExtensionResourcesAreNotAllowed } from "./functions/tracked-extension-resources-are-not-allowed" import trackedResourceTagsPropertyInRequest from "./functions/trackedresource-tags-property-in-request" +import { validQueryParametersForPointOperations } from "./functions/valid-query-parameters-for-point-operations" import { validatePatchBodyParamProperties } from "./functions/validate-patch-body-param-properties" import withXmsResource from "./functions/with-xms-resource" import verifyXMSLongRunningOperationProperty from "./functions/xms-long-running-operation-property" @@ -958,6 +959,20 @@ const ruleset: any = { function: trackedExtensionResourcesAreNotAllowed, }, }, + // RPC Code: RPC-Uri-V1-13 + ValidQueryParametersForPointOperations: { + rpcGuidelineCode: "RPC-Uri-V1-13", + description: "Point operations (GET, PUT, PATCH, DELETE) must not include any query parameters other than api-version.", + message: "{{error}}", + stagingOnly: true, + severity: "error", + resolved: true, + formats: [oas2], + given: "$[paths,'x-ms-paths']", + then: { + function: validQueryParametersForPointOperations, + }, + }, /// /// ARM RPC rules for SystemData diff --git a/packages/rulesets/src/spectral/functions/utils.ts b/packages/rulesets/src/spectral/functions/utils.ts index feddc5e8..c3434f7a 100644 --- a/packages/rulesets/src/spectral/functions/utils.ts +++ b/packages/rulesets/src/spectral/functions/utils.ts @@ -242,6 +242,25 @@ const providerAndNamespace = "/providers/[^/]+" const resourceTypeAndResourceName = "(?:/\\w+/default|/\\w+/{[^/]+})" const queryParam = "(?:\\?\\w+)" const resourcePathRegEx = new RegExp(`${providerAndNamespace}${resourceTypeAndResourceName}+${queryParam}?$`, "gi") +/** + * Checks if the provided path is a point operation + * i.e, if its a path that can have point GET, PUT, PATCH, DELETE + * @param path path/uri + * @returns true or false + */ +export function isPointOperation(path: string) { + const index = path.lastIndexOf("/providers/") + if (index === -1) { + return false + } + const lastProvider = path.substr(index) + const matches = lastProvider.match(resourcePathRegEx) + if(matches){ + return true + } + return false +} + export function getResourcesPathHierarchyBasedOnResourceType(path: string) { const index = path.lastIndexOf("/providers/") if (index === -1) { diff --git a/packages/rulesets/src/spectral/functions/valid-query-parameters-for-point-operations.ts b/packages/rulesets/src/spectral/functions/valid-query-parameters-for-point-operations.ts new file mode 100644 index 00000000..6f4ecbc3 --- /dev/null +++ b/packages/rulesets/src/spectral/functions/valid-query-parameters-for-point-operations.ts @@ -0,0 +1,37 @@ +import { isPointOperation } from "./utils" + +export const validQueryParametersForPointOperations = (pathItem: any, _opts: any, ctx: any) => { + if (pathItem === null || typeof pathItem !== "object") { + return [] + } + + const path = ctx.path || [] + const uris = Object.keys(pathItem) + if (uris.length < 1) { + return [] + } + const pointOperations = new Set(["get", "put", "patch", "delete"]) + const errors: any[] = [] + + for (const uri of uris) { + //check if the path is a point operation + if (isPointOperation(uri)) { + const verbs = Object.keys(pathItem[uri]) + for (const verb of verbs) { + //check query params only for point operations/verbs + if (pointOperations.has(verb)) { + const params = pathItem[uri][verb]["parameters"] + const queryParams = params?.filter((param: { in: string; name: string }) => param.in === "query" && param.name !== "api-version") + queryParams?.map((param: { name: any }) => { + errors.push({ + message: `Query parameter ${param.name} should be removed. Point operation '${verb}' MUST not have query parameters other than api-version.`, + path: [path, uri, verb, "parameters"], + }) + }) + } + } + } + } + + return errors +} diff --git a/packages/rulesets/src/spectral/test/latest-version-of-common-types-must-be-used.test.ts b/packages/rulesets/src/spectral/test/latest-version-of-common-types-must-be-used.test.ts index 86bf16fb..14bd34bf 100644 --- a/packages/rulesets/src/spectral/test/latest-version-of-common-types-must-be-used.test.ts +++ b/packages/rulesets/src/spectral/test/latest-version-of-common-types-must-be-used.test.ts @@ -116,7 +116,7 @@ test("LatestVersionOfCommonTypesMustBeUsed should find no errors", async () => { expect(results.length).toBe(0) }) }) -test("ParametersInPointGet should find no errors when common-types ref is not present", async () => { +test("LatestVersionOfCommonTypesMustBeUsed should find no errors when common-types ref is not present", async () => { const myOpenApiDocument = { swagger: "2.0", paths: { diff --git a/packages/rulesets/src/spectral/test/valid-query-parameters-for-point-operations.test.ts b/packages/rulesets/src/spectral/test/valid-query-parameters-for-point-operations.test.ts new file mode 100644 index 00000000..cce4f521 --- /dev/null +++ b/packages/rulesets/src/spectral/test/valid-query-parameters-for-point-operations.test.ts @@ -0,0 +1,372 @@ +import { Spectral } from "@stoplight/spectral-core" +import linterForRule from "./utils" + +let linter: Spectral + +beforeAll(async () => { + linter = await linterForRule("ValidQueryParametersForPointOperations") + return linter +}) + +test("ValidQueryParametersForPointOperations should find errors for top level path", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs/{unstoppable}": { + get: { + operationId: "foo_get", + parameters: [ + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + delete: { + operationId: "foo_delete", + parameters: [ + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + }, + "/providers/Microsoft.Music/Albums/{Great}": { + get: { + operationId: "foo_get", + parameters: [ + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + put: { + operationId: "foo_put", + parameters: [ + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + }, + }, + parameters: { + QuotaBucketNameParameter: { + in: "query", + name: "quotaBucketName", + description: "Quota Bucket name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(4) + expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}.get.parameters") + expect(results[0].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + expect(results[1].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}.delete.parameters") + expect(results[1].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'delete' MUST not have query parameters other than api-version.", + ) + expect(results[2].path.join(".")).toBe("paths./providers/Microsoft.Music/Albums/{Great}.get.parameters") + expect(results[2].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + expect(results[3].path.join(".")).toBe("paths./providers/Microsoft.Music/Albums/{Great}.put.parameters") + expect(results[3].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'put' MUST not have query parameters other than api-version.", + ) + }) +}) + +test("ValidQueryParametersForPointOperations should find errors for nested path", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}": { + get: { + operationId: "foo_get", + parameters: [ + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + put: { + operationId: "foo_put", + parameters: [ + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + }, + }, + parameters: { + QuotaBucketNameParameter: { + in: "query", + name: "quotaBucketName", + description: "Quota Bucket name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(2) + expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}.get.parameters") + expect(results[0].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + expect(results[1].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}.put.parameters") + expect(results[1].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'put' MUST not have query parameters other than api-version.", + ) + }) +}) + +test("ValidQueryParametersForPointOperations should find errors for more than one query param", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}": { + get: { + operationId: "foo_get", + parameters: [ + { + $ref: "#/parameters/LoadTestNameParameter", + }, + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + }, + }, + parameters: { + LoadTestNameParameter: { + in: "query", + name: "loadTestName", + description: "Load Test name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + QuotaBucketNameParameter: { + in: "query", + name: "quotaBucketName", + description: "Quota Bucket name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(2) + expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}.get.parameters") + expect(results[0].message).toContain( + "Query parameter loadTestName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + expect(results[1].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}.get.parameters") + expect(results[1].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + }) +}) + +test("ValidQueryParametersForPointOperations should flag error for other query params but should not flag error for api-version param", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}": { + get: { + operationId: "foo_post", + parameters: [ + { + $ref: "src/spectral/test/resources/types.json#/parameters/ApiVersionParameter", + }, + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + }, + }, + parameters: { + QuotaBucketNameParameter: { + in: "query", + name: "quotaBucketName", + description: "Quota Bucket name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}.get.parameters") + expect(results[0].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + }) +}) + +test("ValidQueryParametersForPointOperations should find no errors when query parameters are not present", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}": { + put: { + operationId: "foo_post", + parameters: [ + { + $ref: "src/spectral/test/resources/types.json#/parameters/ResourceGroupNameParameter", + }, + { + $ref: "#/parameters/LoadTestNameParameter", + }, + ], + responses: { + 200: { + description: "Success", + }, + }, + }, + }, + }, + parameters: { + LoadTestNameParameter: { + in: "path", + name: "loadTestName", + description: "Load Test name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(0) + }) +}) + +test("ValidQueryParametersForPointOperations should find no errors for a list operation", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs": { + get: { + operationId: "foo_post", + parameters: [ + { + $ref: "src/spectral/test/resources/types.json#/parameters/ResourceGroupNameParameter", + }, + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + responses: { + 200: { + description: "Success", + }, + }, + }, + }, + "/providers/Microsoft.Music/songs/{unstoppable}/artist": { + get: { + operationId: "foo_post", + parameters: [ + { + $ref: "src/spectral/test/resources/types.json#/parameters/ResourceGroupNameParameter", + }, + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + responses: { + 200: { + description: "Success", + }, + }, + }, + }, + }, + parameters: { + QuotaBucketNameParameter: { + in: "query", + name: "quotaBucketName", + description: "Quota Bucket name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(0) + }) +}) + +test("ValidQueryParametersForPointOperations should find errors for x-ms-paths", () => { + const myOpenApiDocument = { + swagger: "2.0", + "x-ms-paths": { + "/providers/Microsoft.Music/songs/{unstoppable}?disambiguation_dummy": { + get: { + operationId: "foo_post", + parameters: [ + { + $ref: "src/spectral/test/resources/types.json#/parameters/SubscriptionIdParameter", + }, + { + $ref: "#/parameters/QuotaBucketNameParameter", + }, + ], + }, + }, + }, + parameters: { + QuotaBucketNameParameter: { + in: "query", + name: "quotaBucketName", + description: "Quota Bucket name.", + required: true, + "x-ms-parameter-location": "method", + type: "string", + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("x-ms-paths./providers/Microsoft.Music/songs/{unstoppable}?disambiguation_dummy.get.parameters") + expect(results[0].message).toContain( + "Query parameter quotaBucketName should be removed. Point operation 'get' MUST not have query parameters other than api-version.", + ) + }) +}) + +test("ValidQueryParametersForPointOperations should find no errors if parameters is not defined", () => { + const myOpenApiDocument = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Music/songs/{unstoppable}/artist/{sia}": { + get: { + operationId: "foo_post", + responses: { + 200: { + description: "Success", + }, + }, + }, + }, + }, + } + return linter.run(myOpenApiDocument).then((results) => { + expect(results.length).toBe(0) + }) +})