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

Integrate stoplight/spectral in parser #477

Closed
Tracked by #481
magicmatatjahu opened this issue Feb 21, 2022 · 11 comments
Closed
Tracked by #481

Integrate stoplight/spectral in parser #477

magicmatatjahu opened this issue Feb 21, 2022 · 11 comments
Labels
enhancement New feature or request

Comments

@magicmatatjahu
Copy link
Member

The whole validation/parsing process in our parser consists of 3 steps:

  • validation via AJV based on spec JSON Schema
  • custom validation(s) which cannot be done with AJV because we have to compare values between different parts of the specification, e.g. security in servers and corresponding values in components/securityRequirements. Here we also perform additional actions like applying traits etc
  • creating from validated AsyncAPI spec the models - the instance of the AsyncApiDocument class

The above operations are not complicated and are sufficient for now (however, we do not validate all edge cases from point 2, such as lack of payload's/header's example validation etc.). However, we are missing one thing that every good linter/parser/validator has, support for custom rules. I am thinking about using spectral in our parser to add such functionality. The advantage of this solution would be the complete elimination of the first two steps, because it would be done by the spectral itself and only our parser would be a wrapper for it. At the end we still need to models/intent API.

But to go in this direction we should first test spectral and see if we can support all the cases we have in AsyncAPI like:

  • validating extensions based on their schema
  • validating bindings based on version and schema
  • validating and parsing custom schema formats like Avro, Raml etc.
  • validating custom ref behaviour if we decide to change current $ref logic.

Currently spectral has in source code support for AsyncAPI with some additional rules. You can check it here https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/index.ts Here comes the problem because in order to have control over these rules we should ask the spectral people if we can extract these rules from their source code and move them to our organization under @asyncapi/spectral package.

Then, with every spec release we could add a new rules, without waiting for an update from stoplight. Additionally, such a package would have two advantages:

  • people using spectral alone would have the ability to validate AsyncAPI with the spectral CLI (only have to fetch that new rules)
  • we as an AsyncAPI organization would have the ability to use those rules in our parser during validation + support in our CLI/ServerAPI/Studio.

I'm not a fan of reinventing the wheel (unless there is a lack of functionality on the market or no feedback from the package maintainers) but we should try to reuse spectral in our parser instead of validating in our own way. Of course if Spectral will not meet our requirements then we should think about our own linter, but lets see.

What do you think about it?

cc @smoya @jonaslagoni @fmvilas @derberg

@kevinswiber
Copy link

I believe everything you aim to do will be possible if the project takes a few Spectral modules as dependencies. In terms of owning the AsyncAPI ruleset built into Spectral, that's worth mentioning on their Discord server, I think. However, rulesets are composable. AsyncAPI could host their own ruleset at a URL endpoint, and folks could use that. Here's an example of how that looks: https://github.com/kevinswiber/spectral-custom-function-example

@magicmatatjahu
Copy link
Member Author

@kevinswiber Thanks for comment! I tested spectral in my POC and as you wrote, spectral should handle all of our use cases. Also rather to host our ruleset as URL (but we should think of that) we should have @asyncapi/spectral package with that ruleset and using cdn we can handle that use case with URL :)

@smoya
Copy link
Member

smoya commented Mar 2, 2022

I'm so excited about the possibility to use Spectral (I've never used it before, but read about it's potential)!
Simplifying our parse logic, and boosting our possibilities to validate our documents into a next step where we just build validation rules is something that sounds like a big achievement.
It would be awesome if we can get a more detailed idea of what we would need to do in order to move forward (in a form of GH Issues maybe) so we can include this for the next major as well (as this also affects other possible big changes we considered such as rewriting the parser to TS).

Thanks @magicmatatjahu for taking the time 👍

@KhudaDad414
Copy link
Member

thanks, @magicmatatjahu. This looks promising. I have been playing around with spectral to see how it works with bindings, and there are a few issues that I need to mention here.

  1. spectral doesn't work with a schema containing the $ref field stoplightio/spectral#2093
  2. If we validate an object against its schema, there will be no customized message for what exactly is wrong with the object. In other words, it will just give the info of the Spectral rule that the object is being validated under, not the schema rule that the object is violating. (maybe it's just how it's supposed to work?)

@magicmatatjahu
Copy link
Member Author

@KhudaDad414 Thanks for that comment!

I know about these errors, but it won't be a big problem for us. I would opt that binding and extension schemas should be added to the parser as registry and then a spectral rule be created based on them with custom logic. We should have something like binding and extension registry (like we have with custom parsers). In this month I will try to create a draft PR with this idea, then you will see what I mean :)

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jul 15, 2022
@smoya
Copy link
Member

smoya commented Jul 18, 2022

@magicmatatjahu would you mind removing stale label?

@smoya
Copy link
Member

smoya commented Jul 27, 2022

For the record, here is the list of tasks regarding Spectral rules that should be implemented: stoplightio/spectral#2100

@derberg
Copy link
Member

derberg commented Aug 16, 2022

In reference to #481 (comment)

can you share what the new error would look like after integrating spectral? some examples? I understand you want to take 1:1 what spectral offers and this is confusing for me

From the issue description, I understand spectral goal for us is to replace custom validation - make sense 💯
But in the linked issue, you also refer to severity and governance. So do you also mean that you want to allow users to use spectral through the parser? so they provide spectral config files to our parser? I mean, is your idea to not only use spectral for our spec-related validation?

also, not clear to me how you want to replace AJV. Spectral uses AJV internally or?

@magicmatatjahu
Copy link
Member Author

@derberg

can you share what the new error would look like after integrating spectral? some examples? I understand you want to take 1:1 what spectral offers and this is confusing for me

You can look on error(s) shape here https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/__tests__/asyncapi-operation-operationId-uniqueness.test.ts#L114 next to the message, path and severity you have also range (as I remember) to determine in which place you have error - similar to our location. It is similar to our shape, but we have some variations related to the refs etc. and in spectral we have generic shape for everything.

From the issue description, I understand spectral goal for us is to replace custom validation - make sense 💯
But in the linked issue, you also refer to severity and governance. So do you also mean that you want to allow users to use spectral through the parser? so they provide spectral config files to our parser? I mean, is your idea to not only use spectral for our spec-related validation?

Yes, I wanna introduce custom config for spectral :) Of course we need to "hardcode" base rules to validate required things like whole spec based on JSON Schema, uniqueness of operationID etc, but people will have possibility to write their own rules to apply company governance.

also, not clear to me how you want to replace AJV. Spectral uses AJV internally or?

Spectral has built-in some validation function, to check if given value exists etc and one of these function is function based on AJV to validate some value/JSON on JSON Schema - check our rule to validate AsyncAPI based on our spec JSON Schema: https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/functions/asyncApi2DocumentSchema.ts#L104

Let me know if I have cleared up any doubts.

@magicmatatjahu
Copy link
Member Author

Issue is resolved in v2 ParserJS - now in https://github.com/asyncapi/parser-js/tree/next-major branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants