-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: allow create as a verb for post operations with path param #133
Conversation
Codecov Report
@@ Coverage Diff @@
## master #133 +/- ##
==========================================
+ Coverage 92.62% 92.79% +0.17%
==========================================
Files 56 56
Lines 2075 2124 +49
==========================================
+ Hits 1922 1971 +49
Misses 153 153
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to clarify the convention we want to enforce, and that will drive some changes to the code.
@@ -64,11 +64,13 @@ module.exports.validate = function({ resolvedSpec }) { | |||
// - paths with path param has a GET, a DELETE, and a POST or PUT or PATCH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operationdId convention implemented here is not correct. The rule should be:
- If the path does not end in a path parameter:
POST
should be "create" or "add"- "GET" should be "list"
- If the path ends in a path parameter:
- if the path has a
PATCH
operationPATCH
should be "update"
- else if the the path has a "POST" operation
POST
should be "update"
PUT
should be "replace"DELETE
should bedelete
- if the path has a
So two important things to note:
- You are only concerned with a path parameter at the end of the path
- You can't check operations in isolation -- you have to check all operations on the path together.
cd1e2ed
to
7ad23b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but there are a couple small changes still needed.
!operationId.match(/^update[a-zA-Z0-9_]+/m) | ||
) { | ||
checkPassed = false; | ||
verbs.push('update'); | ||
} else if (opKey === 'post') { | ||
// operationId for POST should start with "add" or "create" only if PATCH operation exist for this path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a PATCH
, I think we just don't check the operationId on POST
. We definitely don't want to recommend "add" or "create" -- that's only appropriate when there is no trailing path parameter.
verbs.push('add'); | ||
verbs.push('create'); | ||
|
||
// If PATCH operation doesn't exist for path, POST operationId should start with "update", "add" or "update" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no PATCH
operation, then we should only allow "update" for POST
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
## [0.19.1](v0.19.0...v0.19.1) (2020-02-11) ### Bug Fixes * updated code and test to reflect updated naming convention ([#133](#133)) ([c44f2fa](c44f2fa))
🎉 This PR is included in version 0.19.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is a PR in response to #131
We should allow
create
as a verb forPOST
requests with path parameters. Validator will no longer flag this as a violation.Also updated naming convention warning messages to include details about what the right operationId verb should be. The drawback is that now the
-s
summary function for openapi validator will not group all naming convention warnings together, since the summary grouping is done by grouping warnings or errors with the same message together.