Skip to content

Commit

Permalink
feat: operationId should conform to naming convention (#124)
Browse files Browse the repository at this point in the history 
* added changes for checking operation id name convention

* changed test snapshots. added test for operationId name convention validator
  • Loading branch information
@SaltedCaramelCoffee
SaltedCaramelCoffee authored Jan 29, 2020
1 parent 1e9f707 commit 79ca9b8
Show file tree
Hide file tree
Showing 7 changed files with 223 additions and 20 deletions.
67 changes: 67 additions & 0 deletions src/plugins/validation/2and3/semantic-validators/operation-ids.js
View file
Original file line number Diff line number Diff line change
@@ -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');
Expand Down Expand Up @@ -30,6 +32,8 @@ module.exports.validate = function({ resolvedSpec }) {
arr.push(
merge(
{
pathKey: `${pathKey}`,
opKey: `${opKey}`,
path: `paths.${pathKey}.${opKey}`
},
op
Expand All @@ -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`
});
}
}
}
});
Expand Down
2 changes: 1 addition & 1 deletion test/cli-validator/mockFiles/clean.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion test/cli-validator/mockFiles/cleanWithTabs.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion test/cli-validator/mockFiles/oas3/clean.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
14 changes: 8 additions & 6 deletions test/cli-validator/tests/expectedOutput.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
21 changes: 12 additions & 9 deletions test/cli-validator/tests/optionHandling.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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() {
Expand Down Expand Up @@ -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
});
});
135 changes: 133 additions & 2 deletions test/plugins/validation/2and3/operation-ids.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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);
});
});

0 comments on commit 79ca9b8

Please sign in to comment.