feat(ibm-operationid-naming-convention): extend operationid naming check

package.json

@@ -50,5 +50,8 @@
     "micromatch": "^4.0.8",
     "jsonpath-plus": "^10.3.0",
     "rollup": "2.79.2"
+  },
+  "dependencies": {
+    "inflected": "^2.1.0"
   }
 }

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,20 +130,25 @@ 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(
   isResourceOriented,
   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 };
 }

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'

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'],

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: