Skip to content
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

Merged
merged 6 commits into from
Feb 11, 2020

Conversation

SaltedCaramelCoffee
Copy link
Contributor

This is a PR in response to #131

We should allow create as a verb for POST 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.

@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #133 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/cli-validator/utils/processConfiguration.js 90.85% <100%> (+2.53%) ⬆️
src/cli-validator/runValidator.js 93.93% <100%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4e8e07...ae8703b. Read the comment docs.

Copy link
Contributor

@mkistler mkistler left a 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.
Copy link
Contributor

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 operation
      • PATCH should be "update"
    • else if the the path has a "POST" operation
      • POST should be "update"
    • PUT should be "replace"
    • DELETE should be delete

So two important things to note:

  1. You are only concerned with a path parameter at the end of the path
  2. You can't check operations in isolation -- you have to check all operations on the path together.

Copy link
Contributor

@mkistler mkistler left a 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
Copy link
Contributor

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"
Copy link
Contributor

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.

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@SaltedCaramelCoffee SaltedCaramelCoffee merged commit c44f2fa into master Feb 11, 2020
@SaltedCaramelCoffee SaltedCaramelCoffee deleted the post-path-param-create branch February 11, 2020 18:15
dpopp07 pushed a commit that referenced this pull request Feb 11, 2020
## [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))
@dpopp07
Copy link
Member

dpopp07 commented Feb 11, 2020

🎉 This PR is included in version 0.19.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants