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: add warning for binary string in "application/json" or parameter #139

Merged
merged 1 commit into from
Feb 18, 2020

Conversation

barrett-schonefeld
Copy link
Contributor

@barrett-schonefeld barrett-schonefeld commented Feb 13, 2020

Changes:

  • added a findOctetSequencePaths function to handle the logic of finding schema type: string, format: binary for cases of nested arrays, objects, nested arrays of type object, objects with properties that are nested arrays, and objects with properties that are objects, and the simple case where a schema uses type: string, format: binary directly. This function takes a resolved schema and returns a list of paths to octet sequences (empty list if none found). The function accepts the path both as an array and a string and returns the path in the same format received.
  • added logic to handle application/json request bodies that use schema type: string, format: binary
  • added logic to handle application/json response bodies of type: string, format: binary
  • added logic to handle parameters of type: string, format: binary
  • removed 'binary' as a valid format for type: string parameters. parameters of type: string, format: binary will result in "type+format not well-defined" error
  • added "json_or_param_binary_string" as to .defaultsForValidator as a warning in the shared.schemas section
  • added "json_or_param_binary_string" configuration option to the README.md rules and defaults sections in the schemas section
  • changed findOctetSequencePaths to accept the path both as a string and an array.

Refactoring in responses.js:

  • moved the check for 204-response with content to be part of the other success code checks
  • added a function to get all response codes, all status codes, and all success codes

Tests:

  • added tests to ensure warnings are issued for request bodies, response bodies, and parameters with schema, type: string, format: binary
  • added complex tests to exercise combinations of nested arrays and objects that contain schema type: string, format: binary (complex tests done on response bodies)

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #139 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   92.99%   93.22%   +0.22%     
==========================================
  Files          59       60       +1     
  Lines        2156     2229      +73     
==========================================
+ Hits         2005     2078      +73     
  Misses        151      151
Impacted Files Coverage Δ
...dation/2and3/semantic-validators/parameters-ibm.js 93.02% <ø> (ø) ⬆️
src/.defaultsForValidator.js 100% <ø> (ø) ⬆️
.../validation/oas3/semantic-validators/parameters.js 100% <100%> (ø) ⬆️
.../validation/oas3/semantic-validators/operations.js 97.91% <100%> (+0.61%) ⬆️
...s/validation/oas3/semantic-validators/responses.js 100% <100%> (ø) ⬆️
src/plugins/utils/findOctetSequencePaths.js 100% <100%> (ø)

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 ab2bc90...81eacff. Read the comment docs.

@barrett-schonefeld barrett-schonefeld changed the title feat: add validator warning for binary string in "application/json" or parameter feat: add warning for binary string in "application/json" or parameter Feb 13, 2020
@barrett-schonefeld barrett-schonefeld changed the title feat: add warning for binary string in "application/json" or parameter wip: add warning for binary string in "application/json" or parameter Feb 13, 2020
@barrett-schonefeld barrett-schonefeld changed the title wip: add warning for binary string in "application/json" or parameter feat: add warning for binary string in "application/json" or parameter Feb 14, 2020
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.

This is heading in the right direction but I left some comments with suggested improvements or questions.

src/plugins/utils/hasOctetSequence.js Outdated Show resolved Hide resolved
src/plugins/utils/hasOctetSequence.js Outdated Show resolved Hide resolved
src/plugins/utils/hasOctetSequence.js Outdated Show resolved Hide resolved
src/plugins/utils/hasOctetSequence.js Outdated Show resolved Hide resolved
src/plugins/utils/hasOctetSequence.js Outdated Show resolved Hide resolved
test/plugins/validation/oas3/operations.js Outdated Show resolved Hide resolved
test/plugins/validation/oas3/parameters.js Outdated Show resolved Hide resolved
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.

There are a few minor changes to make and then potentially larger changes in how we handle the configuration situation. I also would like to see one positive test that proves the correct usage of a binary property will not create a warning.

README.md Outdated Show resolved Hide resolved
test/plugins/validation/oas3/operations.js Outdated Show resolved Hide resolved
test/plugins/validation/oas3/operations.js Outdated Show resolved Hide resolved
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.

Looking pretty good. Probably the last thing to do is discuss the config stuff. I'll think about it and get back to you

test/plugins/validation/oas3/parameters.js Outdated Show resolved Hide resolved
src/.defaultsForValidator.js Outdated Show resolved Hide resolved
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.

One little thing to address, but looks good otherwise! I'm approving but I'll wait to merge in case @mkistler wants to take one more look at it

test/plugins/validation/oas3/parameters.js Outdated Show resolved Hide resolved
@barrett-schonefeld barrett-schonefeld changed the title feat: add warning for binary string in "application/json" or parameter wip: add warning for binary string in "application/json" or parameter Feb 17, 2020
@barrett-schonefeld barrett-schonefeld changed the title wip: add warning for binary string in "application/json" or parameter feat: add warning for binary string in "application/json" or parameter Feb 18, 2020
…r parameter

- added a findOctetSequencePaths function to handle the logic of finding schema type: string, format: binary for cases of nested arrays, objects, nested arrays of type object, objects with properties that are nested arrays, and objects with properties that are objects, and the simple case where a schema uses type: string, format: binary directly. This function takes a schema object from a resolvedSpec and returns a list of paths to octet sequences (empty list if none found). The function accepts the path both as an array and a string and returns the path in the same format received.
- added logic to handle application/json request bodies that use schema type: string, format: binary
- added logic to handle application/json response bodies of type: string, format: binary
- added logic to handle parameters of type: string, format: binary
- removed 'binary' as a valid format for type: string parameters. parameters of type: string, format: binary will result in "type+format not well-defined" error
- added tests to ensure warnings are issued for request bodies, response bodies, and parameters with schema, type: string, format: binary
- added complex tests to exercise combinations of nested arrays and objects that contain schema type: string, format: binary (complex tests done on response bodies)
- added "json_or_param_binary_string" as to .defaultsForValidator as a warning in the oas3.schemas section
- added "json_or_param_binary_string" configuration option to the README.md rules and defaults sections in the oas3 schemas section
- changed findOctetSequencePaths to accept the path both as a string and an array.
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! I'll see if @mkistler wants to take one more look and then merge this morning

@dpopp07 dpopp07 dismissed mkistler’s stale review February 18, 2020 15:12

Requested changes have been addressed

@dpopp07 dpopp07 merged commit 9497333 into master Feb 18, 2020
@dpopp07 dpopp07 deleted the issue-1416 branch February 18, 2020 15:15
dpopp07 pushed a commit that referenced this pull request Feb 18, 2020
# [0.21.0](v0.20.0...v0.21.0) (2020-02-18)

### Features

* add validator warning for binary string in "application/json" or parameter ([#139](#139)) ([9497333](9497333))
@dpopp07
Copy link
Member

dpopp07 commented Feb 18, 2020

🎉 This PR is included in version 0.21.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants