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

refactor: combine asyncapi validation with request body #78

Merged

Conversation

magicmatatjahu
Copy link
Member

Description

At the moment we validate AsyncAPI documents in separate middleware as well request body. That PR merge functionalities to one middleware and also introduce validation of multiple AsyncAPI documents. It's very important for PR such as #70 to validate not only asyncapi field but also another one like base or list of documents like asyncapis.

  • add new validationMiddleware
  • fix unit tests
  • update openapi.yaml

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Mar 28, 2022
openapi.yaml Show resolved Hide resolved
oneOf:
- type: string # can be in YAML format
- $ref: https://raw.githubusercontent.com/asyncapi/spec-json-schemas/master/schemas/2.0.0.json
- $ref: https://raw.githubusercontent.com/asyncapi/spec-json-schemas/master/schemas/2.1.0.json
- $ref: https://raw.githubusercontent.com/asyncapi/spec-json-schemas/master/schemas/2.2.0.json
- $ref: https://raw.githubusercontent.com/asyncapi/spec-json-schemas/master/schemas/2.3.0.json
Template:
AsyncAPIDocuments:
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this schema being used anywhere. Was this made on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not used for now, but it is needed for these two PRs #70 #69 - this is where we need to use the list of AsyncAPI documents. I don't want to add that schema in mentioned PRs, but in this :)

Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of adding things that are not used TBH. But I understand you do for helping the other two PR's so, 👍

export interface ValidationMiddlewareOptions {
path: string;
method: 'all' | 'get' | 'post' | 'put' | 'delete' | 'patch' | 'options' | 'head',
documents?: Record<string, 'single' | 'list'>;
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 needed? Can't we just base the validation on the number of documents passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean list/single? 🤔 I don't see any problems with this approach. Ok, I'll change it, thanks!

@magicmatatjahu
Copy link
Member Author

@smoya I applied changes, please check again :)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

- '2.1.0'
- '2.2.0'
- '2.3.0'
- 'latest'
Copy link
Member

@smoya smoya Mar 29, 2022

Choose a reason for hiding this comment

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

I just realized latest is a dangerous option here since at some point it could differ with what the real latest spec version is unless we sync spec releases with the update of this list and the parser-js dependency update supporting that version in server-api. Maybe this can go into a new issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I wouldn't treat it as a huge problem. We always release every package after every spec release so we don't miss it.

try {
for (const field of documents) {
const body = req.body[String(field)];
if (Array.isArray(body)) {
Copy link
Member

Choose a reason for hiding this comment

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

Much simpler IMHO, isn't?

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @magicmatatjahu !

@magicmatatjahu
Copy link
Member Author

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @magicmatatjahu! 👋🏼
This PR is not up to date with the base branch and can't be merged.
You can add comment to this PR with text: /autoupdate or /au. This way you ask our bot to perform regular updates for you. The only requirement for this to work is to enable Allow edits from maintainers option in your PR. Otherwise the only option that you have is to manually update your branch with latest version of the base branch.
Thanks 😄

@asyncapi-bot asyncapi-bot merged commit 1c0f6d0 into asyncapi:master Mar 29, 2022
@magicmatatjahu magicmatatjahu deleted the chore/support-multiple-validation branch March 29, 2022 15:44
@asyncapi-bot
Copy link
Contributor

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

3 participants