feat: error for oneOf, anyOf, allOf schema that does not use array #143

barrett-schonefeld · 2020-02-19T18:55:43Z

Changes:

added default error for oneOf, anyOf, or allOf schema that is not use top-level array

Tests:

added tests when oneOf, anyOf, or allOf schemas are nested ("oneOf allOf," for example), when oneOf, anyOf, or allOf schema is used as a property in an obect, and when oneOf, anyOf, or allOf schema used as items in array
updated schema-ibm test to import the default config values and made necessary changes to test to account for unexpected errors and warnings resulting from having all default values present
added test to ensure errors not issued when compose models use an array

codecov · 2020-02-19T18:56:25Z

Codecov Report

Merging #143 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #143      +/-   ##
=========================================
+ Coverage   92.88%   92.9%   +0.02%     
=========================================
  Files          61      61              
  Lines        2109    2115       +6     
=========================================
+ Hits         1959    1965       +6     
  Misses        150     150

Impacted Files	Coverage Δ
...validation/2and3/semantic-validators/schema-ibm.js	`99.25% <100%> (+0.03%)`	⬆️

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 9f18deb...ec7c365. Read the comment docs.

dpopp07

Looks good! Just a couple minor changes

src/plugins/validation/2and3/semantic-validators/schema-ibm.js

test/cli-validator/mockFiles/oas3/complexTestComposeModel.yaml

test/cli-validator/mockFiles/oas3/composeModelProps.yaml

README.md

test/plugins/validation/2and3/schema-ibm.js

mkistler

Looks good. I recommend removing the configuration of this check altogether -- then we can avoid the need to come up with a concise but descriptive name for what is essentially an OpenAPI requirement.

README.md

mkistler · 2020-02-21T03:02:29Z

README.md

@@ -251,6 +251,7 @@ The supported rules are described below:
 | no_property_description     | Flag any schema that contains a 'property' without a `description` field.     | shared   |
 | description_mentions_json   | Flag any schema with a 'property' description that mentions the word 'JSON'.  | shared   |
 | array_of_arrays             | Flag any schema with a 'property' of type `array` with items of type `array`. | shared   |
+| compose_model_field_not_array | Flag any compose model schema that does not use an array.                   | shared   |


If we keep this configurable, I think we want a clearer name and description. "compose model" is a term from the SDK generator -- not used in OpenAPI or JSON Schema. So better to use something like "allOf_value_not_array" and "Flag any allOf schema attribute whose value is not an array".

I chose to use compose model because this PR checks that allOf, anyOf, and oneOf use arrays. In the following changes, I am going to remove the configuration option, but I want to make sure you agree with the choice to include anyOf and oneOf.

Yes -- we should check all three. I made my suggestion before I saw the code below.

src/plugins/validation/2and3/semantic-validators/schema-ibm.js

mkistler · 2020-02-21T03:06:46Z

test/plugins/validation/2and3/schema-ibm.js

-        invalid_type_format_pair: 'error'
-      }
-    };
-


All the changes above here appear to be unrelated to this feature.

Yes, while I was introducing tests to this module, I thought it would be a good idea to clean up the individual declarations of config and replace them with one import. I changed the config name to customConfig for the cases where the test uses a custom value.

^ I support these changes

…r parameter - added a findOctetSequences 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). - 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 shared.schemas section - added "json_or_param_binary_string" configuration option to the README.md rules and defaults sections in the schemas section

…r parameter - added a findOctetSequences 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). - 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 "binary_string" as to .defaultsForValidator as a warning in the shared.schemas section - added "binary_string" configuration option to the README.md rules and defaults sections in the schemas section

…r parameter - added a hasOctetSequence module 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). - 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 "binary_string" as to .defaultsForValidator as a warning in the shared.schemas section - added "binary_string" configuration option to the README.md rules and defaults sections in the schemas section

…rrors - introduced MessageCarrier class to centralize the logic/checks for adding warnings and errors. - introduced an instance of this class in each semantic validator to provide uniform warning and error reporting - added unit tests for the MessageCarrier class

- added error for oneOf, anyOf, or allOf schema that is not use top-level array - updated schema-ibm test to import the default config values and made necessary changes to test to account for unexpected errors and warnings resulting from having all default values present - added tests when oneOf, anyOf, or allOf schemas are nested ("oneOf allOf," for example), when oneOf, anyOf, or allOf schema is used as a property in an obect, and when oneOf, anyOf, or allOf schema used as items in array - added test to ensure errors not issued when compose models use an array

dpopp07

Looks good to me! I'll give @mkistler one more change to review before merging

mkistler

Looks good! 👍

# [0.23.0](v0.22.0...v0.23.0) (2020-02-24) ### Features * error for oneOf, anyOf, allOf schema that does not use array ([#143](#143)) ([1db6b27](1db6b27))

dpopp07 · 2020-02-24T16:46:44Z

🎉 This PR is included in version 0.23.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

barrett-schonefeld requested review from mkistler and dpopp07 February 19, 2020 18:55

barrett-schonefeld force-pushed the allOf-array branch from 83d976c to 987b863 Compare February 19, 2020 19:05

barrett-schonefeld added the ready-for-review label Feb 19, 2020

dpopp07 requested changes Feb 20, 2020

View reviewed changes

barrett-schonefeld force-pushed the allOf-array branch from 987b863 to b010206 Compare February 20, 2020 21:40

barrett-schonefeld requested a review from dpopp07 February 20, 2020 21:40

mkistler suggested changes Feb 21, 2020

View reviewed changes

barrett-schonefeld force-pushed the allOf-array branch from b010206 to 793308a Compare February 21, 2020 13:01

barrett-schonefeld changed the title ~~feat: default error for compose models schema that does not use array~~ feat: error for oneOf, anyOf, allOf schema that does not use array Feb 21, 2020

barrett-schonefeld requested a review from mkistler February 21, 2020 13:03

barrett-schonefeld force-pushed the allOf-array branch 2 times, most recently from 2a2d7a3 to 302c499 Compare February 21, 2020 18:33

Barrett Schonefeld added 5 commits February 21, 2020 12:43

barrett-schonefeld force-pushed the allOf-array branch from 302c499 to ec7c365 Compare February 21, 2020 18:43

dpopp07 approved these changes Feb 21, 2020

View reviewed changes

mkistler approved these changes Feb 23, 2020

View reviewed changes

dpopp07 merged commit 1db6b27 into master Feb 24, 2020

dpopp07 deleted the allOf-array branch February 24, 2020 15:58

dpopp07 pushed a commit that referenced this pull request Feb 24, 2020

chore(release): 0.23.0 [skip ci]

b33875a

# [0.23.0](v0.22.0...v0.23.0) (2020-02-24) ### Features * error for oneOf, anyOf, allOf schema that does not use array ([#143](#143)) ([1db6b27](1db6b27))

dpopp07 added the released label Feb 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: error for oneOf, anyOf, allOf schema that does not use array #143

feat: error for oneOf, anyOf, allOf schema that does not use array #143

barrett-schonefeld commented Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

dpopp07 left a comment

mkistler left a comment

mkistler Feb 21, 2020

barrett-schonefeld Feb 21, 2020 •

edited

Loading

mkistler Feb 23, 2020

mkistler Feb 21, 2020

barrett-schonefeld Feb 21, 2020 •

edited

Loading

dpopp07 Feb 21, 2020

dpopp07 left a comment

mkistler left a comment

dpopp07 commented Feb 24, 2020

feat: error for oneOf, anyOf, allOf schema that does not use array #143

feat: error for oneOf, anyOf, allOf schema that does not use array #143

Conversation

barrett-schonefeld commented Feb 19, 2020 • edited Loading

codecov bot commented Feb 19, 2020 • edited Loading

Codecov Report

dpopp07 left a comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

mkistler Feb 21, 2020

Choose a reason for hiding this comment

barrett-schonefeld Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

mkistler Feb 23, 2020

Choose a reason for hiding this comment

mkistler Feb 21, 2020

Choose a reason for hiding this comment

barrett-schonefeld Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

dpopp07 Feb 21, 2020

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

dpopp07 commented Feb 24, 2020

barrett-schonefeld commented Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 19, 2020 •

edited

Loading

barrett-schonefeld Feb 21, 2020 •

edited

Loading

barrett-schonefeld Feb 21, 2020 •

edited

Loading