diff --git a/package-lock.json b/package-lock.json index 8f81a072..489eed16 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,6 +13,9 @@ "packages/ruleset", "packages/validator" ], + "dependencies": { + "inflected": "^2.1.0" + }, "devDependencies": { "@semantic-release/changelog": "^6.0.3", "@semantic-release/git": "^10.0.1", @@ -5973,6 +5976,12 @@ "node": ">=8" } }, + "node_modules/inflected": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/inflected/-/inflected-2.1.0.tgz", + "integrity": "sha512-hAEKNxvHf2Iq3H60oMBHkB4wl5jn3TPF3+fXek/sRwAB5gP9xWs4r7aweSF95f99HFoz69pnZTcu8f0SIHV18w==", + "license": "MIT" + }, "node_modules/inflight": { "version": "1.0.6", "resolved": "https://registry.npmjs.org/inflight/-/inflight-1.0.6.tgz", @@ -15054,7 +15063,7 @@ }, "packages/ruleset": { "name": "@ibm-cloud/openapi-ruleset", - "version": "1.31.1", + "version": "1.31.2", "license": "Apache-2.0", "dependencies": { "@ibm-cloud/openapi-ruleset-utilities": "1.9.0", @@ -15116,10 +15125,10 @@ }, "packages/validator": { "name": "ibm-openapi-validator", - "version": "1.35.2", + "version": "1.35.3", "license": "Apache-2.0", "dependencies": { - "@ibm-cloud/openapi-ruleset": "1.31.1", + "@ibm-cloud/openapi-ruleset": "1.31.2", "@ibm-cloud/openapi-ruleset-utilities": "1.9.0", "@stoplight/spectral-cli": "^6.14.2", "@stoplight/spectral-core": "^1.19.4", diff --git a/package.json b/package.json index 224161e9..7cb1eb7f 100644 --- a/package.json +++ b/package.json @@ -50,5 +50,8 @@ "micromatch": "^4.0.8", "jsonpath-plus": "^10.3.0", "rollup": "2.79.2" + }, + "dependencies": { + "inflected": "^2.1.0" } } diff --git a/packages/ruleset/src/functions/operationid-naming-convention.js b/packages/ruleset/src/functions/operationid-naming-convention.js index b3b0db13..6882fd55 100644 --- a/packages/ruleset/src/functions/operationid-naming-convention.js +++ b/packages/ruleset/src/functions/operationid-naming-convention.js @@ -5,6 +5,7 @@ const { each, merge, pickBy, reduce } = require('lodash'); const { operationMethods } = require('../utils'); +const inflected = require('inflected'); module.exports = function (rootDocument) { return operationIdNamingConvention(rootDocument); @@ -58,19 +59,21 @@ function operationIdNamingConvention(resolvedSpec) { p.startsWith(op.pathKey + '/{') ); - const { checkPassed, verbs } = operationIdPassedConventionCheck( - isResourceOriented, - op['opKey'], - op.operationId, - pathEndsWithParam, - numParamRefs - ); + const { checkPassed, correctIds, operationId } = + operationIdPassedConventionCheck( + isResourceOriented, + op['opKey'], + op.operationId, + pathEndsWithParam, + numParamRefs, + op.pathKey + ); if (checkPassed === false) { errors.push({ - message: `operationIds should follow naming convention: operationId verb should be ${verbs.join( + message: `operationIds should follow naming convention: operationId should be ${correctIds.join( ' or ' - )}`, + )} but it's ${operationId} instead`, path: [...op.path, 'operationId'], }); } @@ -127,6 +130,7 @@ function operationIdNamingConvention(resolvedSpec) { * @param {string} operationId the operation's operationId * @param {boolean} pathEndsWithParam a flag that indicates whether or not the path ends with a path parameter reference * @param {number} numParamRefs the number of path parameter references in the path + * @param {string} fullPath the full path of the operation * @returns */ function operationIdPassedConventionCheck( @@ -134,13 +138,17 @@ function operationIdPassedConventionCheck( httpMethod, operationId, pathEndsWithParam, - numParamRefs + numParamRefs, + fullPath ) { // Useful for debugging. // console.log(`Debug: ${httpMethod} ${isResourceOriented} ${pathEndsWithParam} ${numParamRefs} ${operationId}`); const verbs = []; + // Verbs where pluralization can happen in the operationId based on the path. + const pluralVerbs = ['list', 'replace', 'set', 'delete', 'remove', 'unset']; + switch (httpMethod) { case 'get': if (isResourceOriented) { @@ -188,14 +196,33 @@ function operationIdPassedConventionCheck( break; } - // If we have a non-empty list of acceptable verbs, then make sure - // that the operationId starts with one of them. - if (verbs.length > 0) { - const checkPassed = verbs - .map(verb => operationId.startsWith(verb)) - .some(v => v); - return { checkPassed, verbs }; + if (verbs.length === 0) return { checkPassed: true }; + + // If we have an acceptable verb, then make sure + // that the operationId starts with that verb + // and that the rest of the operation id matches + // the path according to the naming conventions + const convertedPath = fullPath + .replace(/^\/+/, '') + .split('/') + .filter(part => !part.startsWith('{') && !part.endsWith('}')) + .filter(part => !/^v\d+$/.test(part)); + + const isPlural = pluralVerbs.some(verb => verbs.includes(verb)); + + // Singularize the words in the path according to the naming conventions. + for (let i = 0; i < convertedPath.length; i++) { + if (i !== convertedPath.length - 1 || !isPlural || pathEndsWithParam) + convertedPath[i] = inflected.singularize(convertedPath[i]); + } + + const correctIds = []; + + for (let i = 0; i < verbs.length; i++) { + const correctId = verbs[i] + '_' + convertedPath.join('_'); + if (correctId === operationId) return { checkPassed: true }; + else correctIds.push(correctId); } - return { checkPassed: true }; + return { checkPassed: false, correctIds, operationId }; } diff --git a/packages/ruleset/test/rules/operationid-naming-convention.test.js b/packages/ruleset/test/rules/operationid-naming-convention.test.js index 6b5286f0..3268b289 100644 --- a/packages/ruleset/test/rules/operationid-naming-convention.test.js +++ b/packages/ruleset/test/rules/operationid-naming-convention.test.js @@ -15,7 +15,7 @@ const rule = operationIdNamingConvention; const ruleId = 'ibm-operation-id-naming-convention'; const expectedSeverity = severityCodes.warning; const expectedMsgPrefix = - /^operationIds should follow naming convention: operationId verb should be.*$/; + /^operationIds should follow naming convention: operationId should be.*$/; describe(`Spectral rule: ${ruleId}`, () => { describe('Should not yield errors', () => { @@ -28,7 +28,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const testDocument = makeCopy(rootDocument); const newGet = makeCopy(testDocument.paths['/v1/drinks/{drink_id}'].get); - newGet.operationId = 'check_glass'; + newGet.operationId = 'check_drink_glass'; testDocument.paths['/v1/drinks/{drink_id}/glasses/{glass_id}'] = { get: newGet, }; @@ -41,7 +41,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const testDocument = makeCopy(rootDocument); const newGet = makeCopy(testDocument.paths['/v1/drinks/{drink_id}'].get); - newGet.operationId = 'get_glass'; + newGet.operationId = 'check_drink_glass'; testDocument.paths['/v1/drinks/{drink_id}/glasses/{glass_id}'] = { get: newGet, }; @@ -102,7 +102,7 @@ describe(`Spectral rule: ${ruleId}`, () => { // This should not fail as it is not considered to be "resource-oriented". testDocument.paths['/v1/drinks/{drink_id}/glasses'] = { put: { - operationId: 'replace_glasses', + operationId: 'set_drink_glasses', }, }; @@ -115,7 +115,7 @@ describe(`Spectral rule: ${ruleId}`, () => { testDocument.paths['/v1/drinks/{drink_id}/glasses'] = { put: { - operationId: 'set_drink_glass', + operationId: 'set_drink_glasses', }, }; @@ -152,7 +152,7 @@ describe(`Spectral rule: ${ruleId}`, () => { testDocument.paths['/v1/drinks/{drink_id}/glasses'] = { delete: { - operationId: 'unset_glasses', + operationId: 'unset_drink_glasses', }, }; @@ -172,7 +172,7 @@ describe(`Spectral rule: ${ruleId}`, () => { // Add another path so this API will be resource-oriented. testDocument.paths['/v1/drinks/{drink_id}/glasses/{glass_id}'] = { get: { - operationId: 'get_drink_glass', + operationId: 'check_drink_glass', }, }; @@ -205,7 +205,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*list$/); + expect(r.message).toMatch(/^.*list_drinks*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe('paths./v1/drinks.get.operationId'); }); @@ -221,7 +221,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*get$/); + expect(r.message).toMatch(/^.*get_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}.get.operationId' @@ -242,7 +242,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*get or check$/); + expect(r.message).toMatch(/^.*get_drink_glass or check_drink_glass*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}/glasses/{glass_id}.get.operationId' @@ -258,7 +258,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*create$/); + expect(r.message).toMatch(/^.*create_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe('paths./v1/drinks.post.operationId'); }); @@ -275,7 +275,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*create$/); + expect(r.message).toMatch(/^.*create_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}.post.operationId' @@ -294,7 +294,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*update$/); + expect(r.message).toMatch(/^.*update_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}.patch.operationId' @@ -313,7 +313,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*replace$/); + expect(r.message).toMatch(/^.*replace_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}.put.operationId' @@ -332,7 +332,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*replace$/); + expect(r.message).toMatch(/^.*replace_drinks*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe('paths./v1/drinks.put.operationId'); }); @@ -349,7 +349,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*replace$/); + expect(r.message).toMatch(/^.*replace_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}.put.operationId' @@ -370,7 +370,9 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*replace or set$/); + expect(r.message).toMatch( + /^.*replace_drink_glasses or set_drink_glasses*/ + ); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}/glasses.put.operationId' @@ -391,7 +393,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*replace or add$/); + expect(r.message).toMatch(/^.*replace_drink_glass or add_drink_glass*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}/glasses/{glass_id}.put.operationId' @@ -410,7 +412,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*delete$/); + expect(r.message).toMatch(/^.*delete_drink*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}.delete.operationId' @@ -431,7 +433,9 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*delete or unset$/); + expect(r.message).toMatch( + /^.*delete_drink_glasses or unset_drink_glasses*/ + ); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}/glasses.delete.operationId' @@ -452,7 +456,7 @@ describe(`Spectral rule: ${ruleId}`, () => { const r = results[0]; expect(r.code).toBe(ruleId); expect(r.message).toMatch(expectedMsgPrefix); - expect(r.message).toMatch(/^.*delete or remove$/); + expect(r.message).toMatch(/^.*delete_drink_glass or remove_drink_glass*/); expect(r.severity).toBe(expectedSeverity); expect(r.path.join('.')).toBe( 'paths./v1/drinks/{drink_id}/glasses/{glass_id}.delete.operationId' diff --git a/packages/ruleset/test/test-utils/root-document.js b/packages/ruleset/test/test-utils/root-document.js index ac6b89b9..08fb0840 100644 --- a/packages/ruleset/test/test-utils/root-document.js +++ b/packages/ruleset/test/test-utils/root-document.js @@ -170,7 +170,7 @@ module.exports = { }, '/v1/drink_menu': { get: { - operationId: 'download_menu', + operationId: 'download_drink_menu', summary: 'Download Drinks Menu', description: 'Retrieve a document containing the drinks menu.', tags: ['TestTag'], @@ -191,7 +191,7 @@ module.exports = { }, }, put: { - operationId: 'replace_menu', + operationId: 'replace_drink_menu', summary: 'Upload Drinks Menu', description: 'Publish a new Drinks Menu for public viewing.', tags: ['TestTag'], diff --git a/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml b/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml index 73f37d3c..ac6e82d0 100644 --- a/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml +++ b/packages/validator/test/cli-validator/mock-files/oas3/clean-with-tabs.yml @@ -65,7 +65,7 @@ paths: post: summary: Create a pet description: Create a pet - operationId: create_pets + operationId: create_pet tags: - pets responses: @@ -85,7 +85,7 @@ paths: get: summary: Info for a specific pet description: Get information about a specific pet - operationId: get_pet_by_id + operationId: get_pet tags: - pets parameters: diff --git a/packages/validator/test/cli-validator/mock-files/oas3/clean.yml b/packages/validator/test/cli-validator/mock-files/oas3/clean.yml index d6050dc9..b3f2e502 100644 --- a/packages/validator/test/cli-validator/mock-files/oas3/clean.yml +++ b/packages/validator/test/cli-validator/mock-files/oas3/clean.yml @@ -65,7 +65,7 @@ paths: post: summary: Create a pet description: Create a pet - operationId: create_pets + operationId: create_pet tags: - pets responses: @@ -85,7 +85,7 @@ paths: get: summary: Info for a specific pet description: Get information about a specific pet - operationId: get_pet_by_id + operationId: get_pet tags: - pets parameters: diff --git a/packages/validator/test/cli-validator/mock-files/oas31/clean.yml b/packages/validator/test/cli-validator/mock-files/oas31/clean.yml index db0a0a17..3dca2ed1 100644 --- a/packages/validator/test/cli-validator/mock-files/oas31/clean.yml +++ b/packages/validator/test/cli-validator/mock-files/oas31/clean.yml @@ -65,7 +65,7 @@ paths: post: summary: Create a pet description: Create a pet - operationId: create_pets + operationId: create_pet tags: - pets responses: @@ -85,7 +85,7 @@ paths: get: summary: Info for a specific pet description: Get information about a specific pet - operationId: get_pet_by_id + operationId: get_pet tags: - pets parameters: