From eba3caebced177f331644e5c05758b2a2cf9fdea Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Fri, 24 Jan 2025 22:13:17 -0700 Subject: [PATCH 01/11] Add linter rule to suggest the scope parameter --- docs/suggest-scope-parameter.md | 35 +++++++++++++++++++ .../GetCollectionResponseSchema.ts | 2 +- packages/rulesets/src/spectral/az-arm.ts | 16 +++++++++ .../functions/suggest-scope-parameter.ts | 26 ++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 docs/suggest-scope-parameter.md create mode 100644 packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts diff --git a/docs/suggest-scope-parameter.md b/docs/suggest-scope-parameter.md new file mode 100644 index 000000000..197a56fae --- /dev/null +++ b/docs/suggest-scope-parameter.md @@ -0,0 +1,35 @@ +# SuggestScopeParameter + +## Category + +ARM Warning + +## Applies to + +ARM OpenAPI(swagger) specs + +## Related ARM Guideline Code + +- N/a + +## Description + +OpenAPI authors can use the `scope` path parameter to indicate that an API is applicable to various scopes (Tenant, +Management Group, Subscription, Resource Group, etc.) rather than explicitly defining each scope in the spec. + +## How to fix + +Remove all explicitly-scoped paths that only vary in scope and create a path with the `scope` parameter. + +Example of explicitly-scoped paths that only vary in scope: + +2. Explicitly-scoped path (by subscription) + **`/subscriptions/{subscriptionId}`**`/providers/Microsoft.Bakery/breads` + +3. Explicitly-scoped path (by resource group): + **`/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}`**`/providers/Microsoft.Bakery/breads` + +These two paths can be replaced with a single path that uses the `scope` parameter: + +1. Path with scope parameter: + **`/{scope}`**`/providers/Microsoft.Bakery/breads` diff --git a/packages/rulesets/src/native/legacyRules/GetCollectionResponseSchema.ts b/packages/rulesets/src/native/legacyRules/GetCollectionResponseSchema.ts index 1ecdf7485..3e4a5825c 100644 --- a/packages/rulesets/src/native/legacyRules/GetCollectionResponseSchema.ts +++ b/packages/rulesets/src/native/legacyRules/GetCollectionResponseSchema.ts @@ -14,7 +14,7 @@ rules.push({ appliesTo_JsonQuery: "$", *run(doc, node, path, ctx) { const msg = - 'The response in the GET collection operation "{0}" does not match the response definition in the individual GET operation "{1}" .' + 'The response in the GET collection operation "{0}" does not match the response definition in the individual GET operation "{1}".' /** * 1 travel all resources and find all the resources that have a collection get * - by searching all the models return by a get operation and verify the schema diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index af8bd0b0e..e674a3a8e 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -56,6 +56,8 @@ import { validatePatchBodyParamProperties } from "./functions/validate-patch-bod import withXmsResource from "./functions/with-xms-resource" import verifyXMSLongRunningOperationProperty from "./functions/xms-long-running-operation-property" import xmsPageableForListCalls from "./functions/xms-pageable-for-list-calls" +import { resolve } from "path" +import suggestScopeParameter from "./functions/suggest-scope-parameter" const ruleset: any = { extends: [common], @@ -319,6 +321,20 @@ const ruleset: any = { }, }, + SuggestScopeParameter: { + description: + "Duplicate operations that vary only by scope can be defined with a single operation that has a scope parameter. This reduces the number of operations in the spec.", + severity: "warn", + stagingOnly: true, + message: "{{error}}", + resolved: true, + formats: [oas2], + given: ["$.paths", "$.x-ms-paths"], + then: { + function: suggestScopeParameter, + }, + }, + /// /// ARM RPC rules for Get patterns /// diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts new file mode 100644 index 000000000..8963dc5ac --- /dev/null +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -0,0 +1,26 @@ +/** + * Suggests combining paths that differ only in scope by defining a scope parameter. + * + * This function checks if the given path can be combined with other paths in the Swagger document + * by introducing a scope parameter. It returns suggestions for paths that differ only in scope. + */ +const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { + const swagger = ctx?.documentInventory?.resolved + + if (path === null || typeof path !== "string" || path.length === 0 || swagger === null || path.startsWith("{scope}")) { + return [] + } + + const resourceTypeName = path.substring(path.lastIndexOf("/providers")) + + const otherPaths = Object.keys(swagger.paths).filter((p: string) => p !== path && p.endsWith(resourceTypeName)) + + return otherPaths.map((p) => { + return { + message: `Path "${p}" differs from path "${path}" only in scope. These paths can be combined by defining a scope parameter.`, + path: ctx.path, + } + }) +} + +export default suggestScopeParameter From 9b4104eceeeb8ee1cb05c192bb0091ca29babb8c Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Fri, 24 Jan 2025 22:17:48 -0700 Subject: [PATCH 02/11] fix build --- packages/rulesets/src/spectral/az-arm.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index e674a3a8e..bef58fe6c 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -56,7 +56,6 @@ import { validatePatchBodyParamProperties } from "./functions/validate-patch-bod import withXmsResource from "./functions/with-xms-resource" import verifyXMSLongRunningOperationProperty from "./functions/xms-long-running-operation-property" import xmsPageableForListCalls from "./functions/xms-pageable-for-list-calls" -import { resolve } from "path" import suggestScopeParameter from "./functions/suggest-scope-parameter" const ruleset: any = { From e801dd45d3322746541a97a3437d9d5432455aaf Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Mon, 27 Jan 2025 11:48:06 -0700 Subject: [PATCH 03/11] fix lint --- packages/rulesets/src/spectral/az-arm.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index bef58fe6c..1864b4342 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -46,6 +46,7 @@ import resourceNameRestriction from "./functions/resource-name-restriction" import responseSchemaSpecifiedForSuccessStatusCode from "./functions/response-schema-specified-for-success-status-code" import { securityDefinitionsStructure } from "./functions/security-definitions-structure" import skuValidation from "./functions/sku-validation" +import suggestScopeParameter from "./functions/suggest-scope-parameter" import { systemDataInPropertiesBag } from "./functions/system-data-in-properties-bag" import { tagsAreNotAllowedForProxyResources } from "./functions/tags-are-not-allowed-for-proxy-resources" import { tenantLevelAPIsNotAllowed } from "./functions/tenant-level-apis-not-allowed" @@ -56,7 +57,6 @@ import { validatePatchBodyParamProperties } from "./functions/validate-patch-bod import withXmsResource from "./functions/with-xms-resource" import verifyXMSLongRunningOperationProperty from "./functions/xms-long-running-operation-property" import xmsPageableForListCalls from "./functions/xms-pageable-for-list-calls" -import suggestScopeParameter from "./functions/suggest-scope-parameter" const ruleset: any = { extends: [common], From 9a7c8bca90207e1467774172213d74b7c1f061b9 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Mon, 27 Jan 2025 14:58:23 -0700 Subject: [PATCH 04/11] broken --- .../functions/suggest-scope-parameter.ts | 49 +++++++++++++++---- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts index 8963dc5ac..c7699c579 100644 --- a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -4,23 +4,52 @@ * This function checks if the given path can be combined with other paths in the Swagger document * by introducing a scope parameter. It returns suggestions for paths that differ only in scope. */ -const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { - const swagger = ctx?.documentInventory?.resolved +import _ from "lodash" - if (path === null || typeof path !== "string" || path.length === 0 || swagger === null || path.startsWith("{scope}")) { +const suggestScopeParameter = (paths: any, _opts: any, ctx: any) => { + if (paths === null || typeof paths !== "object") { return [] } - const resourceTypeName = path.substring(path.lastIndexOf("/providers")) + const entries = Object.entries(paths) + if ( + !Array.isArray(entries) || + entries.length === 0 || + !entries.every((p) => Array.isArray(p)) || + !entries.every((p) => p.length === 2) || + !entries.every((p) => typeof p[0] === "string") || + !entries.every((p) => typeof p[1] === "object") + ) { + return [] + } + + // Find paths that do not start with the scope parameter + const scopedPaths = entries.filter((p: Array) => !p[0].startsWith("{scope}")) - const otherPaths = Object.keys(swagger.paths).filter((p: string) => p !== path && p.endsWith(resourceTypeName)) + const errors = [] - return otherPaths.map((p) => { - return { - message: `Path "${p}" differs from path "${path}" only in scope. These paths can be combined by defining a scope parameter.`, - path: ctx.path, + for (const scopedPath of scopedPaths) { + // Find other paths that differ only in scope + const otherPaths = scopedPaths.filter( + (p: Array) => + p[0] !== scopedPath[0] && + // Paths have the same "/providers/..." suffix + p[0].endsWith(scopedPath[0].substring(scopedPath[0].indexOf("/providers"))), + ) + + if (otherPaths.length > 0) { + errors.push( + otherPaths.map((p) => { + return { + message: `Path "${p}" differs from path "${scopedPath}" only in scope. These paths can be combined by defining a scope parameter.`, + path: ctx.path, + } + }), + ) } - }) + } + + return errors } export default suggestScopeParameter From ec018525d46118ca271b590976768aa1900254b5 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 09:39:11 -0700 Subject: [PATCH 05/11] fixed rule/test. need to clean up --- packages/rulesets/src/spectral/az-arm.ts | 2 +- .../functions/suggest-scope-parameter.ts | 23 +- .../test/suggest-scope-parameter.test.ts | 386 ++++++++++++++++++ 3 files changed, 395 insertions(+), 16 deletions(-) create mode 100644 packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index 1864b4342..a42ee66e0 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -328,7 +328,7 @@ const ruleset: any = { message: "{{error}}", resolved: true, formats: [oas2], - given: ["$.paths", "$.x-ms-paths"], + given: "$[paths,'x-ms-paths'].*", then: { function: suggestScopeParameter, }, diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts index c7699c579..159b91e69 100644 --- a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -11,38 +11,31 @@ const suggestScopeParameter = (paths: any, _opts: any, ctx: any) => { return [] } - const entries = Object.entries(paths) - if ( - !Array.isArray(entries) || - entries.length === 0 || - !entries.every((p) => Array.isArray(p)) || - !entries.every((p) => p.length === 2) || - !entries.every((p) => typeof p[0] === "string") || - !entries.every((p) => typeof p[1] === "object") - ) { + const keys = Object.keys(paths) + if (!Array.isArray(keys) || keys.length === 0 || !keys.every((p) => typeof p === "string")) { return [] } // Find paths that do not start with the scope parameter - const scopedPaths = entries.filter((p: Array) => !p[0].startsWith("{scope}")) + const scopedPaths = keys.filter((p: string) => !p.startsWith("{scope}")) const errors = [] for (const scopedPath of scopedPaths) { // Find other paths that differ only in scope const otherPaths = scopedPaths.filter( - (p: Array) => - p[0] !== scopedPath[0] && + (p: string) => + p !== scopedPath && // Paths have the same "/providers/..." suffix - p[0].endsWith(scopedPath[0].substring(scopedPath[0].indexOf("/providers"))), + p.endsWith(scopedPath[0].substring(scopedPath.indexOf("/providers"))), ) if (otherPaths.length > 0) { errors.push( otherPaths.map((p) => { return { - message: `Path "${p}" differs from path "${scopedPath}" only in scope. These paths can be combined by defining a scope parameter.`, - path: ctx.path, + message: "placeholder error message", //`Path "${p}" differs from path "${scopedPath}" only in scope. These paths can be combined by defining a scope parameter.`, + path: "placeholder path", //.concat(p), } }), ) diff --git a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts new file mode 100644 index 000000000..7f10275d7 --- /dev/null +++ b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts @@ -0,0 +1,386 @@ +import { Spectral } from "@stoplight/spectral-core" +import linterForRule from "./utils" + +let linter: Spectral + +//beforeAll(() => linterForRule("SuggestScopeParameter")) + +beforeAll(async () => { + linter = await linterForRule("SuggestScopeParameter") + return linter +}) + +test("SuggestScopeParameter should find errors", async () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }, { $ref: "#/parameters/ResourceGroupParameter" }], + }, + }, + "/subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }], + }, + }, + }, + parameters: { + SubscriptionIdParameter: { + name: "subscriptionId", + in: "path", + required: true, + }, + ResourceGroupParameter: { + name: "resourceGroupName", + in: "path", + required: true, + }, + }, + } + + let result + + try { + result = await linter.run(oasDoc).then((results) => { + const paths = Object.keys(oasDoc.paths) + expect(results.length).toBe(2) + + // path 0 + expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Bakery/breads") + expect(results[0].message).toContain(`Path "${paths[0]}" differs from path "${paths[1]}" only in scope`) + //expect(results[0].message).toContain(`"${paths[2]}" that has the scope parameter`) + + // path 1 + //expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") + //expect(results[1].message).toContain(`"${paths[1]}" with explicitly defined scope`) + //expect(results[1].message).toContain(`"${paths[2]}" that has the scope parameter`) + }) + } catch (error) { + if (error && error.errors && Array.isArray(error.errors)) { + throw new Error(`Errors found. ${error.errors}`) + } + } + return result +}) + +// test("NoDuplicatePathsForScopeParameter should find no errors with scope parameter", () => { +// const oasDoc = { +// swagger: "2.0", +// paths: { +// "/{scope}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ScopeParameter" }], +// }, +// }, +// }, +// parameters: { +// ScopeParameter: { +// name: "scope", +// in: "path", +// required: true, +// }, +// }, +// } +// return linter.run(oasDoc).then((results) => { +// expect(results.length).toBe(0) +// }) +// }) + +// test("NoDuplicatePathsForScopeParameter should find no errors with explicitly defined scopes", () => { +// const oasDoc = { +// swagger: "2.0", +// paths: { +// "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }, { $ref: "#/parameters/ResourceGroupParameter" }], +// }, +// }, +// "/subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }], +// }, +// }, +// }, +// parameters: { +// SubscriptionIdParameter: { +// name: "subscriptionId", +// in: "path", +// required: true, +// }, +// ResourceGroupParameter: { +// name: "resourceGroupName", +// in: "path", +// required: true, +// }, +// ScopeParameter: { +// name: "scope", +// in: "path", +// required: true, +// }, +// }, +// } +// return linter.run(oasDoc).then((results) => { +// expect(results.length).toBe(0) +// }) +// }) + +// test("NoDuplicatePathsForScopeParameter should find errors for billing scope", () => { +// const oasDoc = { +// swagger: "2.0", +// paths: { +// "/{scope}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ScopeParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/departments/{departmentId}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/DepartmentIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/enrollmentAccounts/{enrollmentAccountId}/providers/Microsoft.Bakery/breads": +// { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/EnrollmentAccountIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/billingProfiles/{billingProfileId}/providers/Microsoft.Bakery/breads": +// { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/BillingProfileIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/invoiceSections/{invoiceSectionId}/providers/Microsoft.Bakery/breads": +// { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/InvoiceSectionIdParameter" }], +// }, +// }, +// }, +// parameters: { +// ScopeParameter: { +// name: "scope", +// in: "path", +// required: true, +// }, +// BillingAccountIdParameter: { +// name: "billAcc", +// in: "path", +// required: true, +// }, +// DepartmentIdParameter: { +// name: "test", +// in: "path", +// required: true, +// }, +// EnrollmentAccountIdParameter: { +// name: "enrollmentAcc", +// in: "path", +// required: true, +// }, +// BillingProfileIdParameter: { +// name: "billProf", +// in: "path", +// required: true, +// }, +// InvoiceSectionIdParameter: { +// name: "invoiceSection", +// in: "path", +// required: true, +// }, +// }, +// } +// return linter.run(oasDoc).then((results) => { +// expect(results.length).toBe(5) +// // explicitly not using a loop here to keep the test logic basic +// const paths = Object.keys(oasDoc.paths) +// // path 1 +// expect(results[0].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[0].message).toContain(`"${paths[1]}" with explicitly defined scope`) +// expect(results[0].message).toContain(`"${paths[0]}" that has the scope parameter`) +// // path 2 +// expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[1].message).toContain(`"${paths[2]}" with explicitly defined scope`) +// expect(results[1].message).toContain(`"${paths[0]}" that has the scope parameter`) +// // path 3 +// expect(results[2].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[2].message).toContain(`"${paths[3]}" with explicitly defined scope`) +// expect(results[2].message).toContain(`"${paths[0]}" that has the scope parameter`) +// // path 4 +// expect(results[3].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[3].message).toContain(`"${paths[4]}" with explicitly defined scope`) +// expect(results[3].message).toContain(`"${paths[0]}" that has the scope parameter`) +// // path 5 +// expect(results[4].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[4].message).toContain(`"${paths[5]}" with explicitly defined scope`) +// expect(results[4].message).toContain(`"${paths[0]}" that has the scope parameter`) +// }) +// }) + +// test("NoDuplicatePathsForScopeParameter should not find errors for billing scope", () => { +// const oasDoc = { +// swagger: "2.0", +// paths: { +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/departments/{departmentId}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/DepartmentIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/enrollmentAccounts/{enrollmentAccountId}/providers/Microsoft.Bakery/breads": +// { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/EnrollmentAccountIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/billingProfiles/{billingProfileId}/providers/Microsoft.Bakery/breads": +// { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/BillingProfileIdParameter" }], +// }, +// }, +// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/invoiceSections/{invoiceSectionId}/providers/Microsoft.Bakery/breads": +// { +// get: { +// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/InvoiceSectionIdParameter" }], +// }, +// }, +// }, +// parameters: { +// BillingAccountIdParameter: { +// name: "billAcc", +// in: "path", +// required: true, +// }, +// DepartmentIdParameter: { +// name: "test", +// in: "path", +// required: true, +// }, +// EnrollmentAccountIdParameter: { +// name: "enrollmentAcc", +// in: "path", +// required: true, +// }, +// BillingProfileIdParameter: { +// name: "billProf", +// in: "path", +// required: true, +// }, +// InvoiceSectionIdParameter: { +// name: "invoiceSection", +// in: "path", +// required: true, +// }, +// }, +// } +// return linter.run(oasDoc).then((results) => { +// expect(results.length).toBe(0) +// }) +// }) + +// test("NoDuplicatePathsForScopeParameter should find errors for other scopes", () => { +// const oasDoc = { +// swagger: "2.0", +// paths: { +// "/{scope}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ScopeParameter" }], +// }, +// }, +// "providers/Microsoft.Management/managementGroups/{managementGroupId}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ManagementGroupIdParameter" }], +// }, +// }, +// "providers/Microsoft.CostManagement/externalBillingAccounts/{externalBillingAccountName}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ExternalBillingAccountNameParameter" }], +// }, +// }, +// "providers/Microsoft.CostManagement/externalSubscriptions/{externalSubscriptionName}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ExternalSubscriptionNameParameter" }], +// }, +// }, +// }, +// parameters: { +// ScopeParameter: { +// name: "scope", +// in: "path", +// required: true, +// }, +// ManagementGroupIdParameter: { +// name: "billAcc", +// in: "path", +// required: true, +// }, +// ExternalBillingAccountNameParameter: { +// name: "test", +// in: "path", +// required: true, +// }, +// ExternalSubscriptionNameParameter: { +// name: "enrollmentAcc", +// in: "path", +// required: true, +// }, +// }, +// } +// return linter.run(oasDoc).then((results) => { +// expect(results.length).toBe(3) +// // explicitly not using a loop here to keep the test logic basic +// const paths = Object.keys(oasDoc.paths) +// // path 1 +// expect(results[0].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[0].message).toContain(`"${paths[1]}" with explicitly defined scope`) +// expect(results[0].message).toContain(`"${paths[0]}" that has the scope parameter`) +// // path 2 +// expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[1].message).toContain(`"${paths[2]}" with explicitly defined scope`) +// expect(results[1].message).toContain(`"${paths[0]}" that has the scope parameter`) +// // path 3 +// expect(results[2].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") +// expect(results[2].message).toContain(`"${paths[3]}" with explicitly defined scope`) +// expect(results[2].message).toContain(`"${paths[0]}" that has the scope parameter`) +// }) +// }) + +// test("NoDuplicatePathsForScopeParameter should not find errors for list and point get paths", () => { +// const oasDoc = { +// swagger: "2.0", +// paths: { +// "/{scope}/providers/Microsoft.Bakery/breads": { +// get: { +// parameters: [{ $ref: "#/parameters/ScopeParameter" }], +// }, +// }, +// "/{scope}/providers/Microsoft.Bakery/breads/{breadName}": { +// get: { +// parameters: [{ $ref: "#/parameters/ScopeParameter" }], +// }, +// }, +// }, +// parameters: { +// ScopeParameter: { +// name: "scope", +// in: "path", +// required: true, +// }, +// }, +// } + +// return linter.run(oasDoc).then((results) => { +// expect(results.length).toBe(0) +// }) +// }) From d6b44839ba02939c2733972c98ae7cbff9bb84f6 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 10:30:51 -0700 Subject: [PATCH 06/11] fix --- packages/rulesets/src/spectral/az-arm.ts | 3 +- .../functions/suggest-scope-parameter.ts | 44 ++++++------------- 2 files changed, 15 insertions(+), 32 deletions(-) diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index a42ee66e0..104282fde 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -328,8 +328,9 @@ const ruleset: any = { message: "{{error}}", resolved: true, formats: [oas2], - given: "$[paths,'x-ms-paths'].*", + given: "$.paths", then: { + field: "@key", function: suggestScopeParameter, }, }, diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts index 159b91e69..034c6a833 100644 --- a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -6,43 +6,25 @@ */ import _ from "lodash" -const suggestScopeParameter = (paths: any, _opts: any, ctx: any) => { - if (paths === null || typeof paths !== "object") { - return [] - } +const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { + const swagger = ctx?.documentInventory?.resolved - const keys = Object.keys(paths) - if (!Array.isArray(keys) || keys.length === 0 || !keys.every((p) => typeof p === "string")) { + if (path === null || typeof path !== "string" || path.length === 0 || swagger === null) { return [] } - // Find paths that do not start with the scope parameter - const scopedPaths = keys.filter((p: string) => !p.startsWith("{scope}")) - - const errors = [] + // Find all paths that differ only in scope + // TODO: this can also probably check paths that start with scope + const matchingPaths = Object.keys(swagger.paths).filter( + (p: string) => !p.startsWith("{scope}") && p !== path && p.endsWith(path.substring(path.indexOf("/providers"))), + ) - for (const scopedPath of scopedPaths) { - // Find other paths that differ only in scope - const otherPaths = scopedPaths.filter( - (p: string) => - p !== scopedPath && - // Paths have the same "/providers/..." suffix - p.endsWith(scopedPath[0].substring(scopedPath.indexOf("/providers"))), - ) - - if (otherPaths.length > 0) { - errors.push( - otherPaths.map((p) => { - return { - message: "placeholder error message", //`Path "${p}" differs from path "${scopedPath}" only in scope. These paths can be combined by defining a scope parameter.`, - path: "placeholder path", //.concat(p), - } - }), - ) + return matchingPaths.map((match: string) => { + return { + message: `Path "${match}" differs from path "${path}" only in scope. These paths can be combined by defining a scope parameter.`, + path: ctx.path.concat(match), } - } - - return errors + }) } export default suggestScopeParameter From 60bf4d82d36d693c9f0b2c545f243c6ad48334e5 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 11:30:43 -0700 Subject: [PATCH 07/11] WIP --- .../functions/suggest-scope-parameter.ts | 5 +- .../test/suggest-scope-parameter.test.ts | 343 +++++++++--------- 2 files changed, 170 insertions(+), 178 deletions(-) diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts index 034c6a833..731f05d20 100644 --- a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -6,6 +6,8 @@ */ import _ from "lodash" +// TODO: this can likely be combined with no-duplicate-paths-for-scope-parameter +// TODO: only have one error per potential scope parameter, not one per path const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { const swagger = ctx?.documentInventory?.resolved @@ -14,9 +16,8 @@ const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { } // Find all paths that differ only in scope - // TODO: this can also probably check paths that start with scope const matchingPaths = Object.keys(swagger.paths).filter( - (p: string) => !p.startsWith("{scope}") && p !== path && p.endsWith(path.substring(path.indexOf("/providers"))), + (p: string) => !p.startsWith("{scope}") && p !== path && p.endsWith(path.substring(path.lastIndexOf("/providers"))), ) return matchingPaths.map((match: string) => { diff --git a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts index 7f10275d7..ae92a94bd 100644 --- a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts +++ b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts @@ -39,189 +39,180 @@ test("SuggestScopeParameter should find errors", async () => { }, } - let result + return await linter.run(oasDoc).then((results) => { + const paths = Object.keys(oasDoc.paths) + expect(results.length).toBe(2) - try { - result = await linter.run(oasDoc).then((results) => { - const paths = Object.keys(oasDoc.paths) - expect(results.length).toBe(2) + // path 0 + expect(results[0].path.join(".")).toBe( + "paths./subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads", + ) + expect(results[0].message).toContain(`Path "${paths[1]}" differs from path "${paths[0]}" only in scope`) - // path 0 - expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Bakery/breads") - expect(results[0].message).toContain(`Path "${paths[0]}" differs from path "${paths[1]}" only in scope`) - //expect(results[0].message).toContain(`"${paths[2]}" that has the scope parameter`) + // path 1 + expect(results[1].path.join(".")).toBe("paths./subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads") + expect(results[1].message).toContain(`Path "${paths[0]}" differs from path "${paths[1]}" only in scope`) + }) +}) - // path 1 - //expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") - //expect(results[1].message).toContain(`"${paths[1]}" with explicitly defined scope`) - //expect(results[1].message).toContain(`"${paths[2]}" that has the scope parameter`) - }) - } catch (error) { - if (error && error.errors && Array.isArray(error.errors)) { - throw new Error(`Errors found. ${error.errors}`) - } +test("SuggestScopeParameter should find no errors with scope parameter", () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/{scope}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/ScopeParameter" }], + }, + }, + }, + parameters: { + ScopeParameter: { + name: "scope", + in: "path", + required: true, + }, + }, } - return result + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) }) -// test("NoDuplicatePathsForScopeParameter should find no errors with scope parameter", () => { -// const oasDoc = { -// swagger: "2.0", -// paths: { -// "/{scope}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ScopeParameter" }], -// }, -// }, -// }, -// parameters: { -// ScopeParameter: { -// name: "scope", -// in: "path", -// required: true, -// }, -// }, -// } -// return linter.run(oasDoc).then((results) => { -// expect(results.length).toBe(0) -// }) -// }) - -// test("NoDuplicatePathsForScopeParameter should find no errors with explicitly defined scopes", () => { -// const oasDoc = { -// swagger: "2.0", -// paths: { -// "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }, { $ref: "#/parameters/ResourceGroupParameter" }], -// }, -// }, -// "/subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }], -// }, -// }, -// }, -// parameters: { -// SubscriptionIdParameter: { -// name: "subscriptionId", -// in: "path", -// required: true, -// }, -// ResourceGroupParameter: { -// name: "resourceGroupName", -// in: "path", -// required: true, -// }, -// ScopeParameter: { -// name: "scope", -// in: "path", -// required: true, -// }, -// }, -// } -// return linter.run(oasDoc).then((results) => { -// expect(results.length).toBe(0) -// }) -// }) +test("SuggestScopeParameter should find no errors with different resourcetypes", () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }, { $ref: "#/parameters/ResourceGroupParameter" }], + }, + }, + "/subscriptions/{subscriptionId}/providers/Microsoft.Bakery/cookies": { + get: { + parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }], + }, + }, + }, + parameters: { + SubscriptionIdParameter: { + name: "subscriptionId", + in: "path", + required: true, + }, + ResourceGroupParameter: { + name: "resourceGroupName", + in: "path", + required: true, + }, + ScopeParameter: { + name: "scope", + in: "path", + required: true, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) +}) -// test("NoDuplicatePathsForScopeParameter should find errors for billing scope", () => { -// const oasDoc = { -// swagger: "2.0", -// paths: { -// "/{scope}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ScopeParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/departments/{departmentId}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/DepartmentIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/enrollmentAccounts/{enrollmentAccountId}/providers/Microsoft.Bakery/breads": -// { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/EnrollmentAccountIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/billingProfiles/{billingProfileId}/providers/Microsoft.Bakery/breads": -// { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/BillingProfileIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/invoiceSections/{invoiceSectionId}/providers/Microsoft.Bakery/breads": -// { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/InvoiceSectionIdParameter" }], -// }, -// }, -// }, -// parameters: { -// ScopeParameter: { -// name: "scope", -// in: "path", -// required: true, -// }, -// BillingAccountIdParameter: { -// name: "billAcc", -// in: "path", -// required: true, -// }, -// DepartmentIdParameter: { -// name: "test", -// in: "path", -// required: true, -// }, -// EnrollmentAccountIdParameter: { -// name: "enrollmentAcc", -// in: "path", -// required: true, -// }, -// BillingProfileIdParameter: { -// name: "billProf", -// in: "path", -// required: true, -// }, -// InvoiceSectionIdParameter: { -// name: "invoiceSection", -// in: "path", -// required: true, -// }, -// }, -// } -// return linter.run(oasDoc).then((results) => { -// expect(results.length).toBe(5) -// // explicitly not using a loop here to keep the test logic basic -// const paths = Object.keys(oasDoc.paths) -// // path 1 -// expect(results[0].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[0].message).toContain(`"${paths[1]}" with explicitly defined scope`) -// expect(results[0].message).toContain(`"${paths[0]}" that has the scope parameter`) -// // path 2 -// expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[1].message).toContain(`"${paths[2]}" with explicitly defined scope`) -// expect(results[1].message).toContain(`"${paths[0]}" that has the scope parameter`) -// // path 3 -// expect(results[2].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[2].message).toContain(`"${paths[3]}" with explicitly defined scope`) -// expect(results[2].message).toContain(`"${paths[0]}" that has the scope parameter`) -// // path 4 -// expect(results[3].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[3].message).toContain(`"${paths[4]}" with explicitly defined scope`) -// expect(results[3].message).toContain(`"${paths[0]}" that has the scope parameter`) -// // path 5 -// expect(results[4].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[4].message).toContain(`"${paths[5]}" with explicitly defined scope`) -// expect(results[4].message).toContain(`"${paths[0]}" that has the scope parameter`) -// }) -// }) +test("SuggestScopeParameter should find errors for billing scope", () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/{scope}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/ScopeParameter" }], + }, + }, + "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }], + }, + }, + "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/departments/{departmentId}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/DepartmentIdParameter" }], + }, + }, + "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/enrollmentAccounts/{enrollmentAccountId}/providers/Microsoft.Bakery/breads": + { + get: { + parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/EnrollmentAccountIdParameter" }], + }, + }, + "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/billingProfiles/{billingProfileId}/providers/Microsoft.Bakery/breads": + { + get: { + parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/BillingProfileIdParameter" }], + }, + }, + "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/invoiceSections/{invoiceSectionId}/providers/Microsoft.Bakery/breads": + { + get: { + parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/InvoiceSectionIdParameter" }], + }, + }, + }, + parameters: { + ScopeParameter: { + name: "scope", + in: "path", + required: true, + }, + BillingAccountIdParameter: { + name: "billAcc", + in: "path", + required: true, + }, + DepartmentIdParameter: { + name: "test", + in: "path", + required: true, + }, + EnrollmentAccountIdParameter: { + name: "enrollmentAcc", + in: "path", + required: true, + }, + BillingProfileIdParameter: { + name: "billProf", + in: "path", + required: true, + }, + InvoiceSectionIdParameter: { + name: "invoiceSection", + in: "path", + required: true, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(5) + // explicitly not using a loop here to keep the test logic basic + //const paths = Object.keys(oasDoc.paths) + // path 1 + // expect(results[0].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") + // expect(results[0].message).toContain(`"${paths[1]}" with explicitly defined scope`) + // expect(results[0].message).toContain(`"${paths[0]}" that has the scope parameter`) + // // path 2 + // expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") + // expect(results[1].message).toContain(`"${paths[2]}" with explicitly defined scope`) + // expect(results[1].message).toContain(`"${paths[0]}" that has the scope parameter`) + // // path 3 + // expect(results[2].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") + // expect(results[2].message).toContain(`"${paths[3]}" with explicitly defined scope`) + // expect(results[2].message).toContain(`"${paths[0]}" that has the scope parameter`) + // // path 4 + // expect(results[3].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") + // expect(results[3].message).toContain(`"${paths[4]}" with explicitly defined scope`) + // expect(results[3].message).toContain(`"${paths[0]}" that has the scope parameter`) + // // path 5 + // expect(results[4].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") + // expect(results[4].message).toContain(`"${paths[5]}" with explicitly defined scope`) + // expect(results[4].message).toContain(`"${paths[0]}" that has the scope parameter`) + }) +}) // test("NoDuplicatePathsForScopeParameter should not find errors for billing scope", () => { // const oasDoc = { From 80219e228e73cc701bae3eccda721bd37088f0f1 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 12:42:24 -0700 Subject: [PATCH 08/11] Fix and add tests --- .../functions/suggest-scope-parameter.ts | 26 +- .../test/suggest-scope-parameter.test.ts | 294 +++++++----------- 2 files changed, 125 insertions(+), 195 deletions(-) diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts index 731f05d20..2d31bae57 100644 --- a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -4,26 +4,36 @@ * This function checks if the given path can be combined with other paths in the Swagger document * by introducing a scope parameter. It returns suggestions for paths that differ only in scope. */ -import _ from "lodash" // TODO: this can likely be combined with no-duplicate-paths-for-scope-parameter -// TODO: only have one error per potential scope parameter, not one per path const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { const swagger = ctx?.documentInventory?.resolved - if (path === null || typeof path !== "string" || path.length === 0 || swagger === null) { + let lowerCasePath: string + + if ( + path === null || + typeof path !== "string" || + path.length === 0 || + swagger === null || + (lowerCasePath = path.toLocaleLowerCase()).includes("{scope}") + ) { return [] } + const suffix = path.substring(path.lastIndexOf("/providers")).toLocaleLowerCase() + // Find all paths that differ only in scope - const matchingPaths = Object.keys(swagger.paths).filter( - (p: string) => !p.startsWith("{scope}") && p !== path && p.endsWith(path.substring(path.lastIndexOf("/providers"))), - ) + const matchingPaths = Object.keys(swagger.paths).filter((p: string) => { + const lower = p.toLocaleLowerCase() + return !lower.includes("{scope}") && lower !== lowerCasePath && lower.endsWith(suffix) + }) return matchingPaths.map((match: string) => { return { - message: `Path "${match}" differs from path "${path}" only in scope. These paths can be combined by defining a scope parameter.`, - path: ctx.path.concat(match), + // note: only matched path is included in the message so that Spectral can deduplicate errors + message: `Path with suffix "${suffix}" differs from another path only in scope. These paths share a suffix and can be combined by defining a scope parameter.`, + path: ctx.path, } }) } diff --git a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts index ae92a94bd..4cc8f9fa6 100644 --- a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts +++ b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts @@ -10,7 +10,7 @@ beforeAll(async () => { return linter }) -test("SuggestScopeParameter should find errors", async () => { +test("SuggestScopeParameter should find errors for subscription and resource group scopes", async () => { const oasDoc = { swagger: "2.0", paths: { @@ -40,18 +40,16 @@ test("SuggestScopeParameter should find errors", async () => { } return await linter.run(oasDoc).then((results) => { - const paths = Object.keys(oasDoc.paths) expect(results.length).toBe(2) - // path 0 + // all errors should have the same message + expect(new Set(results.map((r) => r.message)).size).toBe(1) + + // each path should have an error expect(results[0].path.join(".")).toBe( "paths./subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads", ) - expect(results[0].message).toContain(`Path "${paths[1]}" differs from path "${paths[0]}" only in scope`) - - // path 1 expect(results[1].path.join(".")).toBe("paths./subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads") - expect(results[1].message).toContain(`Path "${paths[0]}" differs from path "${paths[1]}" only in scope`) }) }) @@ -189,189 +187,111 @@ test("SuggestScopeParameter should find errors for billing scope", () => { } return linter.run(oasDoc).then((results) => { expect(results.length).toBe(5) - // explicitly not using a loop here to keep the test logic basic - //const paths = Object.keys(oasDoc.paths) - // path 1 - // expect(results[0].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") - // expect(results[0].message).toContain(`"${paths[1]}" with explicitly defined scope`) - // expect(results[0].message).toContain(`"${paths[0]}" that has the scope parameter`) - // // path 2 - // expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") - // expect(results[1].message).toContain(`"${paths[2]}" with explicitly defined scope`) - // expect(results[1].message).toContain(`"${paths[0]}" that has the scope parameter`) - // // path 3 - // expect(results[2].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") - // expect(results[2].message).toContain(`"${paths[3]}" with explicitly defined scope`) - // expect(results[2].message).toContain(`"${paths[0]}" that has the scope parameter`) - // // path 4 - // expect(results[3].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") - // expect(results[3].message).toContain(`"${paths[4]}" with explicitly defined scope`) - // expect(results[3].message).toContain(`"${paths[0]}" that has the scope parameter`) - // // path 5 - // expect(results[4].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") - // expect(results[4].message).toContain(`"${paths[5]}" with explicitly defined scope`) - // expect(results[4].message).toContain(`"${paths[0]}" that has the scope parameter`) + + // all errors should have the same message + expect(new Set(results.map((r) => r.message)).size).toBe(1) + + // each path besides the one with the scope parameter should have an error + expect(results[0].path.join(".")).toBe( + "paths.providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/Microsoft.Bakery/breads", + ) + expect(results[1].path.join(".")).toBe( + "paths.providers/Microsoft.Billing/billingAccounts/{billingAccountId}/departments/{departmentId}/providers/Microsoft.Bakery/breads", + ) + expect(results[2].path.join(".")).toBe( + "paths.providers/Microsoft.Billing/billingAccounts/{billingAccountId}/enrollmentAccounts/{enrollmentAccountId}/providers/Microsoft.Bakery/breads", + ) + expect(results[3].path.join(".")).toBe( + "paths.providers/Microsoft.Billing/billingAccounts/{billingAccountId}/billingProfiles/{billingProfileId}/providers/Microsoft.Bakery/breads", + ) + expect(results[4].path.join(".")).toBe( + "paths.providers/Microsoft.Billing/billingAccounts/{billingAccountId}/invoiceSections/{invoiceSectionId}/providers/Microsoft.Bakery/breads", + ) + }) +}) + +test("SuggestScopeParameter should not find errors for list and point get paths", () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/{scope}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/ScopeParameter" }], + }, + }, + "/{scope}/providers/Microsoft.Bakery/breads/{breadName}": { + get: { + parameters: [{ $ref: "#/parameters/ScopeParameter" }], + }, + }, + }, + parameters: { + ScopeParameter: { + name: "scope", + in: "path", + required: true, + }, + }, + } + + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) }) }) -// test("NoDuplicatePathsForScopeParameter should not find errors for billing scope", () => { -// const oasDoc = { -// swagger: "2.0", -// paths: { -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/departments/{departmentId}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/DepartmentIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/enrollmentAccounts/{enrollmentAccountId}/providers/Microsoft.Bakery/breads": -// { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/EnrollmentAccountIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/billingProfiles/{billingProfileId}/providers/Microsoft.Bakery/breads": -// { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/BillingProfileIdParameter" }], -// }, -// }, -// "providers/Microsoft.Billing/billingAccounts/{billingAccountId}/invoiceSections/{invoiceSectionId}/providers/Microsoft.Bakery/breads": -// { -// get: { -// parameters: [{ $ref: "#/parameters/BillingAccountIdParameter" }, { $ref: "#/parameters/InvoiceSectionIdParameter" }], -// }, -// }, -// }, -// parameters: { -// BillingAccountIdParameter: { -// name: "billAcc", -// in: "path", -// required: true, -// }, -// DepartmentIdParameter: { -// name: "test", -// in: "path", -// required: true, -// }, -// EnrollmentAccountIdParameter: { -// name: "enrollmentAcc", -// in: "path", -// required: true, -// }, -// BillingProfileIdParameter: { -// name: "billProf", -// in: "path", -// required: true, -// }, -// InvoiceSectionIdParameter: { -// name: "invoiceSection", -// in: "path", -// required: true, -// }, -// }, -// } -// return linter.run(oasDoc).then((results) => { -// expect(results.length).toBe(0) -// }) -// }) +test("SuggestScopeParameter should find errors for tenant level scope", async () => { + const oasDoc = { + swagger: "2.0", + paths: { + "/providers/Microsoft.Bakery/breads": { + get: { + parameters: [ + { + name: "body_param", + in: "body", + schema: { + properties: { prop: { type: "string" } }, + }, + }, + ], + }, + }, + "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }, { $ref: "#/parameters/ResourceGroupParameter" }], + }, + }, + "/subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads": { + get: { + parameters: [{ $ref: "#/parameters/SubscriptionIdParameter" }], + }, + }, + }, + parameters: { + SubscriptionIdParameter: { + name: "subscriptionId", + in: "path", + required: true, + }, + ResourceGroupParameter: { + name: "resourceGroupName", + in: "path", + required: true, + }, + }, + } -// test("NoDuplicatePathsForScopeParameter should find errors for other scopes", () => { -// const oasDoc = { -// swagger: "2.0", -// paths: { -// "/{scope}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ScopeParameter" }], -// }, -// }, -// "providers/Microsoft.Management/managementGroups/{managementGroupId}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ManagementGroupIdParameter" }], -// }, -// }, -// "providers/Microsoft.CostManagement/externalBillingAccounts/{externalBillingAccountName}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ExternalBillingAccountNameParameter" }], -// }, -// }, -// "providers/Microsoft.CostManagement/externalSubscriptions/{externalSubscriptionName}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ExternalSubscriptionNameParameter" }], -// }, -// }, -// }, -// parameters: { -// ScopeParameter: { -// name: "scope", -// in: "path", -// required: true, -// }, -// ManagementGroupIdParameter: { -// name: "billAcc", -// in: "path", -// required: true, -// }, -// ExternalBillingAccountNameParameter: { -// name: "test", -// in: "path", -// required: true, -// }, -// ExternalSubscriptionNameParameter: { -// name: "enrollmentAcc", -// in: "path", -// required: true, -// }, -// }, -// } -// return linter.run(oasDoc).then((results) => { -// expect(results.length).toBe(3) -// // explicitly not using a loop here to keep the test logic basic -// const paths = Object.keys(oasDoc.paths) -// // path 1 -// expect(results[0].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[0].message).toContain(`"${paths[1]}" with explicitly defined scope`) -// expect(results[0].message).toContain(`"${paths[0]}" that has the scope parameter`) -// // path 2 -// expect(results[1].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[1].message).toContain(`"${paths[2]}" with explicitly defined scope`) -// expect(results[1].message).toContain(`"${paths[0]}" that has the scope parameter`) -// // path 3 -// expect(results[2].path.join(".")).toBe("paths./{scope}/providers/Microsoft.Bakery/breads") -// expect(results[2].message).toContain(`"${paths[3]}" with explicitly defined scope`) -// expect(results[2].message).toContain(`"${paths[0]}" that has the scope parameter`) -// }) -// }) + return await linter.run(oasDoc).then((results) => { + expect(results.length).toBe(3) -// test("NoDuplicatePathsForScopeParameter should not find errors for list and point get paths", () => { -// const oasDoc = { -// swagger: "2.0", -// paths: { -// "/{scope}/providers/Microsoft.Bakery/breads": { -// get: { -// parameters: [{ $ref: "#/parameters/ScopeParameter" }], -// }, -// }, -// "/{scope}/providers/Microsoft.Bakery/breads/{breadName}": { -// get: { -// parameters: [{ $ref: "#/parameters/ScopeParameter" }], -// }, -// }, -// }, -// parameters: { -// ScopeParameter: { -// name: "scope", -// in: "path", -// required: true, -// }, -// }, -// } + // all errors should have the same message + expect(new Set(results.map((r) => r.message)).size).toBe(1) -// return linter.run(oasDoc).then((results) => { -// expect(results.length).toBe(0) -// }) -// }) + // each path should have an error + expect(results[0].path.join(".")).toBe("paths./providers/Microsoft.Bakery/breads") + expect(results[1].path.join(".")).toBe( + "paths./subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Bakery/breads", + ) + expect(results[2].path.join(".")).toBe("paths./subscriptions/{subscriptionId}/providers/Microsoft.Bakery/breads") + }) +}) From 1c40ee996c0a2c96e30a450bd9c2889fe63fd728 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 12:45:03 -0700 Subject: [PATCH 09/11] Remove TODO This rule should remain separate as it is a warning, not an error --- .../rulesets/src/spectral/functions/suggest-scope-parameter.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts index 2d31bae57..f57fe4402 100644 --- a/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts +++ b/packages/rulesets/src/spectral/functions/suggest-scope-parameter.ts @@ -5,7 +5,6 @@ * by introducing a scope parameter. It returns suggestions for paths that differ only in scope. */ -// TODO: this can likely be combined with no-duplicate-paths-for-scope-parameter const suggestScopeParameter = (path: any, _opts: any, ctx: any) => { const swagger = ctx?.documentInventory?.resolved From 4b0b2cd25c2765c71c0629b8d309eb1a4fd994b9 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 12:57:43 -0700 Subject: [PATCH 10/11] Clean up test code --- .../src/spectral/test/suggest-scope-parameter.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts index 4cc8f9fa6..41871d96e 100644 --- a/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts +++ b/packages/rulesets/src/spectral/test/suggest-scope-parameter.test.ts @@ -3,10 +3,8 @@ import linterForRule from "./utils" let linter: Spectral -//beforeAll(() => linterForRule("SuggestScopeParameter")) - beforeAll(async () => { - linter = await linterForRule("SuggestScopeParameter") + linter = linterForRule("SuggestScopeParameter") return linter }) @@ -39,7 +37,7 @@ test("SuggestScopeParameter should find errors for subscription and resource gro }, } - return await linter.run(oasDoc).then((results) => { + return linter.run(oasDoc).then((results) => { expect(results.length).toBe(2) // all errors should have the same message @@ -281,7 +279,7 @@ test("SuggestScopeParameter should find errors for tenant level scope", async () }, } - return await linter.run(oasDoc).then((results) => { + return linter.run(oasDoc).then((results) => { expect(results.length).toBe(3) // all errors should have the same message From 418e1b0e83e47cc8553d364da1f6b054cf99b968 Mon Sep 17 00:00:00 2001 From: Brett DeFoy Date: Tue, 28 Jan 2025 13:14:31 -0700 Subject: [PATCH 11/11] Edit change log --- packages/rulesets/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/rulesets/CHANGELOG.md b/packages/rulesets/CHANGELOG.md index 3596c6084..8346dda4f 100644 --- a/packages/rulesets/CHANGELOG.md +++ b/packages/rulesets/CHANGELOG.md @@ -5,6 +5,7 @@ ### Patches - [AllTrackedResourcesMustHaveDelete][TrackedResourcePatchOperation] Skip the api paths if it has PrivateEndpointConnectionProxy +- [SuggestScopeParameter] Added this rule to suggest using the scope parameter if paths can be combined ### Patches