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

adds discriminator validator to conform to openAPI 2 and 3 specifications #107

Merged
merged 6 commits into from
Oct 1, 2019
Merged

adds discriminator validator to conform to openAPI 2 and 3 specifications #107

merged 6 commits into from
Oct 1, 2019

Conversation

SaltedCaramelCoffee
Copy link
Contributor

The Swagger 2.0 spec and Open API 3 spec says 'The [discriminator] property name used MUST be defined at this schema'.
The validator currently does not flag violations of this requirement

…ifications

The Swagger 2.0 spec says 'The [discriminator] property name used MUST be defined at this schema'.
The validator currently does not flag violations of this requirement
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2019

CLA assistant check
All committers have signed the CLA.

@dpopp07 dpopp07 self-requested a review September 27, 2019 21:58
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 looks good but we should think about checking schemas in other locations, and need to decide how to handle OAS2 vs OAS3 API docs.

const errors = [];
const warnings = [];

const schemas = jsSpec.components.schemas;
Copy link
Contributor

Choose a reason for hiding this comment

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

Schemas can appear in places other than components/schemas, e.g. in request and response bodies, but for this validation it might be reasonable to just check components/schemas.

const { discriminator } = schema;

// If discriminator is not an object, error out and return
if (typeof discriminator === 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

In OpenAPI 2.0, the discriminator is just a string property containing the name of the discriminator property in the schema. So what you have done here is correct for OpenAPI 3.0 but not for OpenAPI 2.0.

There are two ways to address this: 1) use alternate paths in this validator for OAS2 vs OAS3, or 2) move this validation to OAS3, and implement a separate (but very similar) validator for OAS2 (aka Swagger).

'The discriminator property name used must be defined in this schema'
);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests!

@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   92.09%   92.32%   +0.22%     
==========================================
  Files          53       55       +2     
  Lines        1886     1941      +55     
==========================================
+ Hits         1737     1792      +55     
  Misses        149      149
Impacted Files Coverage Δ
...tion/swagger2/semantic-validators/discriminator.js 100% <100%> (ø)
...lidation/oas3/semantic-validators/discriminator.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 07c56a0...0af49b4. 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.

This looks great! 👍 The code is very well organized and easy to follow.

I left one comment on a typo -- otherwise this is good to go.

.concat([schemaName, 'discriminator', 'propertyName'])
.join('.'),
message:
'`propertName` inside discriminator object must be of type string'
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here: "propertName" -> "propertyName"

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.

Great work!

@dpopp07 dpopp07 merged commit 0ecd539 into IBM:master Oct 1, 2019
dpopp07 pushed a commit that referenced this pull request Oct 1, 2019
# [0.15.0](v0.14.0...v0.15.0) (2019-10-01)

### Features

* validate discriminator properties conform to specifications ([#107](#107)) ([0ecd539](0ecd539))
@dpopp07
Copy link
Member

dpopp07 commented Oct 1, 2019

🎉 This PR is included in version 0.15.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

4 participants