diff --git a/src/plugins/validation/2and3/semantic-validators/operation-ids.js b/src/plugins/validation/2and3/semantic-validators/operation-ids.js index 021bc433f..f266c97c4 100644 --- a/src/plugins/validation/2and3/semantic-validators/operation-ids.js +++ b/src/plugins/validation/2and3/semantic-validators/operation-ids.js @@ -1,5 +1,7 @@ // Assertation 1: Operations must have a unique operationId. +// Assertation 2: OperationId must conform to naming conventions. + const pickBy = require('lodash/pickBy'); const reduce = require('lodash/reduce'); const merge = require('lodash/merge'); @@ -30,6 +32,8 @@ module.exports.validate = function({ resolvedSpec }) { arr.push( merge( { + pathKey: `${pathKey}`, + opKey: `${opKey}`, path: `paths.${pathKey}.${opKey}` }, op @@ -50,15 +54,78 @@ module.exports.validate = function({ resolvedSpec }) { return !!prev; }; + const operationIdPassedConventionCheck = ( + opKey, + operationId, + hasPathParam + ) => { + // Only consider paths for which + // - paths with no path param has a GET and POST path + // - paths with path param has a GET, a DELETE, and a POST or PUT or PATCH. + + let checkPassed = true; + + if (!hasPathParam) { + // operationId for GET should starts with "list" + if (opKey === 'get' && !operationId.match(/^list[a-zA-Z0-9_]+/m)) { + checkPassed = false; + } + + // operationId for POST should starts with "create" or "add" + if ( + opKey === 'post' && + !operationId.match(/^(add|create)[a-zA-Z0-9_]+/m) + ) { + checkPassed = false; + } + } else { + // operationId for GET should starts with "get" + if (opKey === 'get' && !operationId.match(/^get[a-zA-Z0-9_]+/m)) { + checkPassed = false; + } + + // operationId for DELETE should starts with "delete" + if (opKey === 'delete' && !operationId.match(/^delete[a-zA-Z0-9_]+/m)) { + checkPassed = false; + } + + // operationId for POST/PUT/PATCH should starts with "update" + if ( + ['put', 'post', 'patch'].includes(opKey) && + !operationId.match(/^update[a-zA-Z0-9_]+/m) + ) { + checkPassed = false; + } + } + return checkPassed; + }; + operations.forEach(op => { // wrap in an if, since operationIds are not required if (op.operationId) { const hasBeenSeen = tallyOperationId(op.operationId); if (hasBeenSeen) { + // Assertation 1: Operations must have a unique operationId. errors.push({ path: op.path + '.operationId', message: 'operationIds must be unique' }); + } else { + // Assertation 2: OperationId must conform to naming conventions + const regex = RegExp(/{[a-zA-Z0-9_-]+\}/m); + + const checkPassed = operationIdPassedConventionCheck( + op['opKey'], + op.operationId, + regex.test(op['pathKey']) + ); + + if (checkPassed === false) { + warnings.push({ + path: op.path + '.operationId', + message: `operationIds should follow consistent naming convention` + }); + } } } }); diff --git a/test/cli-validator/mockFiles/clean.yml b/test/cli-validator/mockFiles/clean.yml index 3594080f7..452da10cb 100644 --- a/test/cli-validator/mockFiles/clean.yml +++ b/test/cli-validator/mockFiles/clean.yml @@ -90,7 +90,7 @@ paths: - "pet" summary: "Finds Pets by status" description: "Multiple status values can be provided with comma separated strings" - operationId: "find_pets_by_status" + operationId: "list_pets_by_status" produces: - "application/xml" - "application/json" diff --git a/test/cli-validator/mockFiles/cleanWithTabs.yml b/test/cli-validator/mockFiles/cleanWithTabs.yml index 88577acfb..d4300ff19 100644 --- a/test/cli-validator/mockFiles/cleanWithTabs.yml +++ b/test/cli-validator/mockFiles/cleanWithTabs.yml @@ -90,7 +90,7 @@ paths: - "pet" summary: "Finds Pets by status" description: "Multiple status values can be provided with comma separated strings" - operationId: "find_pets_by_status" + operationId: "list_pets_by_status" produces: - "application/xml" - "application/json" diff --git a/test/cli-validator/mockFiles/oas3/clean.yml b/test/cli-validator/mockFiles/oas3/clean.yml index 127fcc640..3b8a29947 100644 --- a/test/cli-validator/mockFiles/oas3/clean.yml +++ b/test/cli-validator/mockFiles/oas3/clean.yml @@ -65,7 +65,7 @@ paths: /pets/{pet_id}: get: summary: Info for a specific pet - operationId: show_pet_by_id + operationId: get_pet_by_id tags: - pets parameters: diff --git a/test/cli-validator/tests/expectedOutput.js b/test/cli-validator/tests/expectedOutput.js index 63a6f200f..f76405375 100644 --- a/test/cli-validator/tests/expectedOutput.js +++ b/test/cli-validator/tests/expectedOutput.js @@ -97,12 +97,14 @@ describe('cli tool - test expected output - Swagger 2', function() { // warnings expect(capturedText[25].match(/\S+/g)[2]).toEqual('36'); - expect(capturedText[29].match(/\S+/g)[2]).toEqual('59'); - expect(capturedText[33].match(/\S+/g)[2]).toEqual('197'); - expect(capturedText[37].match(/\S+/g)[2]).toEqual('108'); - expect(capturedText[41].match(/\S+/g)[2]).toEqual('131'); - expect(capturedText[45].match(/\S+/g)[2]).toEqual('134'); - expect(capturedText[49].match(/\S+/g)[2]).toEqual('126'); + expect(capturedText[29].match(/\S+/g)[2]).toEqual('87'); + expect(capturedText[33].match(/\S+/g)[2]).toEqual('36'); + expect(capturedText[37].match(/\S+/g)[2]).toEqual('59'); + expect(capturedText[41].match(/\S+/g)[2]).toEqual('197'); + expect(capturedText[45].match(/\S+/g)[2]).toEqual('108'); + expect(capturedText[49].match(/\S+/g)[2]).toEqual('131'); + expect(capturedText[53].match(/\S+/g)[2]).toEqual('134'); + expect(capturedText[57].match(/\S+/g)[2]).toEqual('126'); }); it('should return exit code of 0 if there are only warnings', async function() { diff --git a/test/cli-validator/tests/optionHandling.js b/test/cli-validator/tests/optionHandling.js index 49a8df73e..fa56ca3bc 100644 --- a/test/cli-validator/tests/optionHandling.js +++ b/test/cli-validator/tests/optionHandling.js @@ -150,7 +150,7 @@ describe('cli tool - test option handling', function() { // totals expect(capturedText[statsSection + 1].match(/\S+/g)[5]).toEqual('5'); - expect(capturedText[statsSection + 2].match(/\S+/g)[5]).toEqual('7'); + expect(capturedText[statsSection + 2].match(/\S+/g)[5]).toEqual('9'); // errors expect(capturedText[statsSection + 4].match(/\S+/g)[0]).toEqual('2'); @@ -167,22 +167,25 @@ describe('cli tool - test option handling', function() { // warnings expect(capturedText[statsSection + 10].match(/\S+/g)[0]).toEqual('2'); - expect(capturedText[statsSection + 10].match(/\S+/g)[1]).toEqual('(29%)'); + expect(capturedText[statsSection + 10].match(/\S+/g)[1]).toEqual('(22%)'); - expect(capturedText[statsSection + 11].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 11].match(/\S+/g)[1]).toEqual('(14%)'); + expect(capturedText[statsSection + 11].match(/\S+/g)[0]).toEqual('2'); + expect(capturedText[statsSection + 11].match(/\S+/g)[1]).toEqual('(22%)'); expect(capturedText[statsSection + 12].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 12].match(/\S+/g)[1]).toEqual('(14%)'); + expect(capturedText[statsSection + 12].match(/\S+/g)[1]).toEqual('(11%)'); expect(capturedText[statsSection + 13].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 13].match(/\S+/g)[1]).toEqual('(14%)'); + expect(capturedText[statsSection + 13].match(/\S+/g)[1]).toEqual('(11%)'); expect(capturedText[statsSection + 14].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 14].match(/\S+/g)[1]).toEqual('(14%)'); + expect(capturedText[statsSection + 14].match(/\S+/g)[1]).toEqual('(11%)'); expect(capturedText[statsSection + 15].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 15].match(/\S+/g)[1]).toEqual('(14%)'); + expect(capturedText[statsSection + 15].match(/\S+/g)[1]).toEqual('(11%)'); + + expect(capturedText[statsSection + 16].match(/\S+/g)[0]).toEqual('1'); + expect(capturedText[statsSection + 16].match(/\S+/g)[1]).toEqual('(11%)'); }); it('should not print statistics report by default', async function() { @@ -307,7 +310,7 @@ describe('cli tool - test option handling', function() { } } }); - expect(warningCount).toEqual(1); // without the config this value is 5 + expect(warningCount).toEqual(3); // without the config this value is 5 expect(errorCount).toEqual(3); // without the config this value is 0 }); }); diff --git a/test/plugins/validation/2and3/operation-ids.js b/test/plugins/validation/2and3/operation-ids.js index cefcd5bb1..1b5520db4 100644 --- a/test/plugins/validation/2and3/operation-ids.js +++ b/test/plugins/validation/2and3/operation-ids.js @@ -25,7 +25,7 @@ describe('validation plugin - semantic - operation-ids', function() { expect(res.errors.length).toEqual(1); expect(res.errors[0].path).toEqual('paths./coolPath.get.operationId'); expect(res.errors[0].message).toEqual('operationIds must be unique'); - expect(res.warnings.length).toEqual(0); + expect(res.warnings.length).toEqual(1); }); it('should complain about a repeated operationId in a different path', function() { @@ -54,7 +54,7 @@ describe('validation plugin - semantic - operation-ids', function() { expect(res.errors.length).toEqual(1); expect(res.errors[0].path).toEqual('paths./greatPath.put.operationId'); expect(res.errors[0].message).toEqual('operationIds must be unique'); - expect(res.warnings.length).toEqual(0); + expect(res.warnings.length).toEqual(1); }); it('should complain about a repeated operationId in a shared path item', async function() { @@ -95,6 +95,137 @@ describe('validation plugin - semantic - operation-ids', function() { expect(res.errors.length).toEqual(1); expect(res.errors[0].path).toEqual('paths./greatPath.get.operationId'); expect(res.errors[0].message).toEqual('operationIds must be unique'); + expect(res.warnings.length).toEqual(1); + }); + + it('should complain about operationId naming convention', async function() { + const spec = { + paths: { + '/books': { + get: { + operationId: 'getBooks' + }, + post: { + operationId: 'changeBooks' + } + }, + '/coffee': { + get: { + operationId: 'get books' + }, + post: { + operationId: 'change_books' + } + }, + '/books/{id}': { + parameters: [ + { + name: 'id', + in: 'path' + } + ], + get: { + operationId: 'listBooks' + }, + delete: { + operationId: 'removeBooks' + }, + post: { + operationId: 'changeBooks1' + }, + put: { + operationId: 'changeBooks2' + }, + patch: { + operationId: 'changeBooks3' + } + } + } + }; + + const resolvedSpec = await resolver.dereference(spec); + const res = validate({ resolvedSpec }); + + expect(res.errors.length).toEqual(0); + expect(res.warnings.length).toEqual(9); + expect(res.warnings[0].path).toEqual('paths./books.get.operationId'); + expect(res.warnings[0].message).toEqual( + 'operationIds should follow consistent naming convention' + ); + }); + + it('should not complain about operationId naming convention', async function() { + const spec = { + paths: { + '/books': { + get: { + operationId: 'listBooks' + }, + post: { + operationId: 'addBooks' + } + }, + '/coffee': { + get: { + operationId: 'list_coffee' + }, + post: { + operationId: 'add_coffee' + } + }, + '/books/{id}': { + parameters: [ + { + name: 'id', + in: 'path' + } + ], + get: { + operationId: 'getBook' + }, + delete: { + operationId: 'deleteBook' + }, + post: { + operationId: 'updateBook1' + }, + put: { + operationId: 'updateBook2' + }, + patch: { + operationId: 'updateBook3' + } + }, + '/coffee/{id}': { + parameters: [ + { + name: 'id', + in: 'path' + } + ], + get: { + operationId: 'get_coffee' + }, + delete: { + operationId: 'delete_book' + }, + post: { + operationId: 'update_book_1' + }, + put: { + operationId: 'update_book_2' + }, + patch: { + operationId: 'update_book_3' + } + } + } + }; + + const resolvedSpec = await resolver.dereference(spec); + const res = validate({ resolvedSpec }); + + expect(res.errors.length).toEqual(0); expect(res.warnings.length).toEqual(0); }); });