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 payload validation against asyncapi schema format #104

Merged
merged 14 commits into from
Jul 9, 2020
Merged

feat: add payload validation against asyncapi schema format #104

merged 14 commits into from
Jul 9, 2020

Conversation

derberg
Copy link
Member

@derberg derberg commented Jul 3, 2020

Description

  • add payload validation against asyncapi schema format

Related issue(s)
Fixes #75

@derberg derberg requested a review from fmvilas July 3, 2020 06:01
README.md Outdated Show resolved Hide resolved
lib/asyncapiSchemaFormatParser.js Outdated Show resolved Hide resolved
lib/asyncapiSchemaFormatParser.js Show resolved Hide resolved
lib/asyncapiSchemaFormatParser.js Outdated Show resolved Hide resolved
lib/asyncapiSchemaFormatParser.js Outdated Show resolved Hide resolved
lib/parser.js Outdated Show resolved Hide resolved
lib/parser.js Outdated Show resolved Hide resolved
test/asyncapiSchemaFormatParser_test.js Outdated Show resolved Hide resolved
test/asyncapiSchemaFormatParser_test.js Outdated Show resolved Hide resolved
@derberg
Copy link
Member Author

derberg commented Jul 6, 2020

@fmvilas ready for another round of review. I applied your suggestions and also added much smarter errors that point where the error is in the AsyncAPI document and not just the payload

fmvilas
fmvilas previously approved these changes Jul 8, 2020
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Just a suggestion. It's great you took the time to have the error point to the full path. I'm wondering now if it wouldn't be great to have both informations: fullPath and path 🤔 What do you think?

lib/asyncapiSchemaFormatParser.js Show resolved Hide resolved
errors.forEach((val) => {
val.dataPath = path + val.dataPath;
});
return errors;
Copy link
Member

Choose a reason for hiding this comment

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

I have a preference for not mutating the input parameters when possible. I know it's not a problem in this case but consider using map instead of forEach:

return errors.map((err) => ({
  ...err,
  ...{
    dataPath: `${path}${err.dataPath}`
  }
}));

Copy link
Member Author

@derberg derberg Jul 9, 2020

Choose a reason for hiding this comment

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

sure thing, changed, I do not use spread operators quite often so do not consider them much during dev, but they make a lot of sense

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 9, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member Author

derberg commented Jul 9, 2020

@fmvilas what would it be useful for? I added full path as it is super useful for editors, to highlight the exact line that has error. What would /additionalProperties path be useful for? for someone that runs a parser it is a kind of out of context

@fmvilas
Copy link
Member

fmvilas commented Jul 9, 2020

Can't think of a use case right now but thought it's cheap to add and maybe someone would find it useful.

@derberg
Copy link
Member Author

derberg commented Jul 9, 2020

@fmvilas adding is cheap, but as you said, no use case, and tbh I think at the moment that this information is completely useless, and that is why I took the effort to provide a full path

@derberg derberg merged commit 23d5d3a into asyncapi:master Jul 9, 2020
@derberg derberg deleted the additinalProps branch July 9, 2020 12:52
@asyncapi-bot
Copy link
Contributor

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

Parser inconsistently applying additionalProperties schema validation
3 participants