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

feat: check for missing required properties #314

Merged
merged 5 commits into from
Aug 9, 2021

Conversation

barrett-schonefeld
Copy link
Contributor

Look for required properties that are not defined with the schema.

Theundefined_required_properties rule only looked in the components section, which is why it is deprecated.

The new rule enforces that required properties are defined in a given requestBody, response, and parameter schema and in any subschemas.

Related issue: #311

Barrett Schonefeld added 2 commits August 3, 2021 09:13
Check for missing required properties in a schema and any subschemas.
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #314 (740b4f3) into main (5ffb9fb) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 740b4f3 differs from pull request most recent head 48707aa. Consider uploading reports for the commit 48707aa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
- Coverage   92.01%   91.90%   -0.11%     
==========================================
  Files          73       73              
  Lines        2378     2348      -30     
  Branches      607      598       -9     
==========================================
- Hits         2188     2158      -30     
  Misses        157      157              
  Partials       33       33              
Impacted Files Coverage Δ
src/.defaultsForValidator.js 100.00% <ø> (ø)
...tic-validators/items-required-for-array-objects.js 100.00% <100.00%> (ø)

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 2e139f2...48707aa. Read the comment docs.

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Two quick questions/comments to look at before approving - code changes look good for the most part though!

given:
- $.paths[*][*][parameters][*].schema
- $.paths[*][*][parameters,responses][*].content[*].schema
- $.paths[*][*][requestBody].content[*].schema
Copy link
Member

Choose a reason for hiding this comment

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

Is this operating on the resolved API definition? I see you're not checking inside of the components section, is that because we would expect to see all schemas where they are actually used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct!

@@ -136,7 +135,8 @@ const defaults = {
*/
const deprecated = {
'no_produces_for_get': 'no_produces',
'parameters.snake_case_only': 'param_name_case_convention'
'parameters.snake_case_only': 'param_name_case_convention',
'undefined_required_properties': 'missing-required-property',
Copy link
Member

Choose a reason for hiding this comment

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

I forget how this value gets used (I think in a warning message) but it may be good to call out the fact that it's a spectral rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! The key is the old rule, and the value is the replacement rule if it exists.

I could do something simple like use missing-required-property (Spectral rule), which will result in, "Use missing-required-property (Spectral rule) instead."

That might be confusing, though, so I could update the deprecation code to produce a different message when the new rule is a Spectral rule.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine, just something to indicate that the rule isn't going to be found in the validaterc file like the old one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the note in parentheses

Copy link
Member

@dpopp07 dpopp07 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! 👍

@barrett-schonefeld barrett-schonefeld merged commit c6ebc28 into main Aug 9, 2021
@barrett-schonefeld barrett-schonefeld deleted the missing-required-props branch August 9, 2021 21:14
ibm-devx-sdk pushed a commit that referenced this pull request Sep 3, 2021
# [0.47.0](v0.46.4...v0.47.0) (2021-09-03)

### Features

* check for missing required properties ([#314](#314)) ([c6ebc28](c6ebc28))
* string schema should define enum or pattern, minLength, maxLength ([#315](#315)) ([27b7c7d](27b7c7d))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 0.47.0 🎉

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